On Tue, Nov 20, 2012 at 10:12:25AM -0500, Stephen Gallagher wrote: > 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.
I pushed the patches yesterday to master and sssd-1-9. I didn't notice the ack went only to my Inbox either. Sorry. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
