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.
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.