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