[SSSD] [PATCH] LDAP: Make it possible to use full principal in ldap_sasl_authid again

Stephen Gallagher sgallagh at redhat.com
Tue Nov 20 15:12:25 UTC 2012


On 11/19/2012 11:49 AM, Jakub Hrozek wrote:
> On Mon, Nov 19, 2012 at 10:54:43AM -0500, Stephen Gallagher wrote:
>> On Mon 19 Nov 2012 10:35:43 AM EST, Jakub Hrozek wrote:
>>> On Mon, Nov 19, 2012 at 02:29:59PM +0100, Pavel Březina wrote:
>>>> On 11/19/2012 11:53 AM, Jakub Hrozek wrote:
>>>>> The behaviour of ldap_sasl_authid was changed in
>>>>> e81a816cddab4a62f263d1a0274d5d3f101e8e0f so that it no longer accepted full
>>>>> principal, but only hostname. The realm was read from the (undocumented)
>>>>> ldap_sasl_realm option.
>>>>>
>>>>> Moreover the "hostname" had to be specified in exactly the format that would
>>>>> match the keytab later, ie if the first match was host/hostname, the option
>>>>> would have to be specified as host/hostname, otherwise the user got an error.
>>>>>
>>>>> I also found out that the principal selection is only used in IPA and AD
>>>>> providers, but I'll file a separate ticket to track that. Right now, I'm
>>>>> mostly interested in fixing the regression.
>>>>>
>>>>> The attached patched modify that behaviour:
>>>>>
>>>>> [PATCH 1/3] MAN: document the ldap_sasl_realm option
>>>>> The option was completely undocumented.
>>>>>
>>>>> [PATCH 2/3] LDAP: Provide a common sdap_set_sasl_options init function
>>>>> The AD and IPA initialization functions shared the same code. This patch
>>>>> moves the code into a common initialization function.
>>>>> [PATCH 3/3] LDAP: Make it possible to use full principal in ldap_sasl_authid again
>>>>> When the guessing of principals was introduced, we changed the existing
>>>>> behaviour and instead of allowing both host/hostname at REALM and
>>>>> host/hostname, only the latter worked and the realm was always appended
>>>> >from the (then undocumented) ldap_sasl_realm option.
>>>>>
>>>>> This patch changes the behaviour to be more backwards-compatible -- if
>>>>> the ldap_sasl_authid contains the @-sign, then the ldap_sasl_realm is
>>>>> not always appended.
>>>>>
>>>>> The strict requirement of checking the requested authid/realm against
>>>>> the one found in keytab was also kind of relaxed because it didn't
>>>>> really work. For example when hostname was requested and the keytab
>>>>> matched host/hostname first, the code blew up.
>>>>>
>>>>> https://fedorahosted.org/sssd/ticket/1635
>>>>
>>>> Hi,
>>>>
>>>>> -    if ((primary_requested && strcmp(desired_primary, sasl_primary) != 0) ||
>>>>> -        (realm_requested && strcmp(desired_realm, sasl_realm) != 0)) {
>>>>> -        DEBUG(SSSDBG_CRIT_FAILURE,
>>>>> -               ("Configured SASL auth ID/realm not found in keytab.\n"));
>>>>> -        ret = ENOENT;
>>>>> -        goto done;
>>>>> +    if (primary_requested && strcmp(desired_primary, sasl_primary) != 0) {
>>>>> +        DEBUG(SSSDBG_MINOR_FAILURE,
>>>>> +               ("Configured SASL auth ID not found in keytab. "
>>>>> +                "Requested %s, found %s\n", desired_primary, sasl_primary));
>>>>> +    }
>>>>> +
>>>>> +    if (realm_requested && strcmp(desired_realm, sasl_realm) != 0) {
>>>>> +        DEBUG(SSSDBG_MINOR_FAILURE,
>>>>> +               ("Configured SASL realm not found in keytab. "
>>>>> +                "Requested %s, found %s\n", desired_realm, sasl_realm));
>>>>>      }
>>>>
>>>> Nack.
>>>>
>>>> I agree with splitting the condition but can you do it in patch #2?
>>>
>>> Done.
>>>
>>
>> Sorry, I wish I had seen this request earlier. Please separate this
>> into its own patch. Patches that move functions from one location to
>> a common location should do that and nothing else. Moving and
>> changing at the same time makes it difficult to review or examine
>> later.
>>
>>>>
>>>> Also you are not interpreting it as error any longer. Is it intentional?
>>>
>>> The check is too restrictive as the select_principal_from_keytab can
>>> return something else than you requested right now.
>>>
>>> Consider that you query for host/myserver at EXAMPLE.COM, then the
>>> select_principal_from_keytab function will return "myserver" in primary
>>> and "EXAMPLE.COM" in realm. So you'd need to add logic to also break
>>> down the principal to get rid of the host/ part. I think the heuristics
>>> would simply get too complex.
>>>
>>> select_principal_from_keytab will error out anyway if there's no
>>> suitable principal at all.
>>
>>
>> Yes, this is the correct behavior, though as I said above, please
>> move functional changes into a separate patch from the
>> de-duplication patch.
>
> Thank you for the review. A new patchset is attached with the single
> change in a separate patch.
>

I acked this yesterday, but mistakenly sent the Ack only to Jakub. 
Correcting this on the list for posterity.



More information about the sssd-devel mailing list