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@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(a)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.