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@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/myser...@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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
