On Mon, 2016-03-07 at 18:40 +0100, Lukas Slebodnik wrote: > On (07/03/16 11:29), Simo Sorce wrote: > >On Mon, 2016-03-07 at 16:58 +0100, Lukas Slebodnik wrote: > >> On (04/03/16 16:42), Simo Sorce wrote: > >> >On Fri, 2016-03-04 at 21:27 +0100, Lukas Slebodnik wrote: > >> >> On (02/03/16 10:02), Simo Sorce wrote: > >> >> >On Wed, 2016-03-02 at 15:34 +0100, Lukas Slebodnik wrote: > >> >> >> On (01/03/16 18:28), Simo Sorce wrote: > >> >> >> >On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > >> >> >> >> On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > >> >> >> >> > On (01/03/16 12:05), Simo Sorce wrote: > >> >> >> >> > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> >> >> >> > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> >> >> >> > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> >> >> >> > >> >>Expired != Disabled > >> >> >> >> > >> >>this change is intentional. > >> >> >> >> > >> >> > >> >> >> >> > >> >Yes, but explain it to Active directory :-) > >> >> >> >> > >> > > >> >> >> >> > >> >Attached is patch with workaround/hack > >> >> >> >> > >> >regression with expired AD users. > >> >> >> >> > >> > > >> >> >> >> > >> ENOPATCH > >> >> >> >> > >> > >> >> >> >> > >> LS > >> >> >> >> > > > >> >> >> >> > >I think a better approach is to return the KRBKDC error from > >> >> >> >> > >the child > >> >> >> >> > >without mapping (or with an intermediate mapping) and have the > >> >> >> >> > >IPA and > >> >> >> >> > >AD providers map it on their own. > >> >> >> >> > > > >> >> >> >> > It's not related to mapping KRBKDC error codes to internal > >> >> >> >> > error code. > >> >> >> >> > The main problem is that AD return the same error code for > >> >> >> >> > expired > >> >> >> >> > and disabled user. And ad provider used generic krb5 functions. > >> >> >> >> > > >> >> >> >> > BTW the same issue would be with id_provider ldap + > >> >> >> >> > auth_provider = krb5 with AD :-( > >> >> >> >> > I'm not sure how your proposal would help. > >> >> >> >> > >> >> >> >> I think AD returns additional information in edata, maybe we can > >> >> >> >> use > >> >> >> >> that to do the proper mapping in the generic krb5 code. > >> >> >> >> > >> >> >> >> Absence of AD specific edata would indicate MIT mapping, presence > >> >> >> >> would > >> >> >> >> allow us to use that additional data to figure out the correct > >> >> >> >> mapping. > >> >> >> >> > >> >> >> >> Simo. > >> >> >> >> > >> >> >> > > >> >> >> >See MS-KILE[1] 2.2.1, I bet the two conditions returns two different > >> >> >> >windows Style errors in etext (not edata, sorry). > >> >> >> > > >> >> >> >[1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > >> >> >> > > >> >> >> Interesting idea and it seems to work. > >> >> >> The main difference was in time and last octet string. > >> >> >> * response for expired user [2] > >> >> >> the last octet string in ASN: > >> >> >> 930100C00000000001000000 > >> >> > > >> >> >This is error C0000193 aka NT_STATUS_ACCOUNT_EXPIRED NT_STATUS > >> >> >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l494 > >> >> > > >> >> >> * response for disabled user [3] > >> >> >> the last octet string in ASN: > >> >> >> 720000C00000000001000000T > >> >> > > >> >> >This is error C0000072, aka NT_STATUS_ACCOUNT_DISABLED NT_STATUS > >> >> >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l211 > >> >> >> > >> >> >> The only question is how to get etext from krb5 response. > >> >> >> I do not want to implement ASN.1 parser. > >> >> > > >> >> >We use asn1c in FreeIPA if we need to generate a parser, but I do not > >> >> >think we need to get that far. > >> >> >The main issue is whether the krb5 fucntions let us get access to the > >> >> >etext data at all, it seem buried pretty low in the internal of krb5. > >> >> > > >> >> It took me a while to find out why my patch broke password change > >> >> then I realized I have unused argument in_tkt_service > >> >> in my version of krb5_get_init_creds_password. > >> >> We might enable gcc warning Wunused-argument :-) > >> >> > >> >> Please review attached patch. > >> > > >> >There is a difference between using krb5_get_init_creds_password() and > >> >the function you created in that the krb5 native API actually retries by > >> >locating a KDC master server if the initial attempt fails and the KDC > >> >that was contacted is not a master KDC. > >> > > >> >Is that functionality we want to preserve ? > >> > > >> There were variuos responses on IRC. > >> 06:43 < ab> lslebodn: I think it is better to have the same logic > >> 06:43 < ab> lslebodn: the difference is going to hurt us in hard to debug > >> cases > >> 06:45 < sbose> lslebodn, I think it is ok to drop this addtional lookup for > >> the master KDC. AD does not support this notion at all and > >> if > >> I understand it correctly IPA adds the related DNS SRV > >> records > >> but does treats them the same as the default ones. There > >> might > >> be only one item to consider. SSSD's krb5 locator plugin > >> will > >> return the address of krb5_kpasswd if there is a > >> seperate one defined in sssd.conf if the address of > >> 06:45 < sbose> the master KDC is requested. A setup which relies on this as > >> a cheap fallback method might break by this change. > >> Unfortunately it looks like there is no public API in > >> libkrb5 > >> which allows to specify if the master KDC should be used or > >> not, > >> so there is no chance to add this feature back. > >> 06:47 < lslebodn> sbose: I could call original function fot this case > >> 06:53 < sbose> lslebodn, yes, but this would break the expired/disabled > >> detection if krb5_kpasswd is used with the AD provider. If > >> we > >> want to keep the old behavior I think it would be better to > >> add an option to switch between > >> krb5_get_init_creds_password() > >> and sss_krb5_get_init_creds_password() and use > >> sss_krb5_get_init_creds_password() as new default. > >> 07:57 * RootWyrm_ awakens. > >> 08:05 < RootWyrm_> lslebodn: oh, I see what he's asking. Not to be contrary > >> for being contrary, but personally I would prefer to see > >> retry retained. In slow replication scenarios (e.g. > >> remote offices) a retry may be necessary for valid > >> reasons. > >> > >> I might to agree that it would be good to preserve old behaviour. > > > >I might agree for pure MIT environments it may be helpful, but in the > >AD/IPA cases I think the retry is a no-op because every server is > >considered a master (or no masters exist at all in DNS SRV records). > > > Yes but change is done in krb5_child which is common > for all cases (AD, IPA, pure MIT) > > >Also th check for the master may cause synchronous DNS lookups that can > >cause delays even when there is no reason to get that data (bacause the > >call was successful). > > > >So the more I think of it the more I am in favor of using your code. > > > :-) > > >At most we can try to call the original code again with some specific > >error conditions that causes a retry in the original code, but I am not > >convinced we should. > >In that case it does not matter that we can't interpret the AD specific > >error codes, because if those errors are returned by our own call we > >will never fallback to the original call and we expect different errors > >in that case. > > > >> I thought it would be a simpler solution which could be backported to > >> stable branch sssd-1-13. But it's going to be more complicated :-) > > > >We may want to put this in master only for now, indeed. > > > >> Because your patch introduced error code which is used to fix ticket #2839. > >> > >> I think it would be better to rever yourt patch in downstream and > >> backport just an error code e.g. > >> https://git.fedorahosted.org/cgit/sssd.git/commit/?id=af717c5b022d5c28141333fc02d5d9e1f322505c > > > >Why revert the patch ?If you do so the other half of setups will get an > >error, either way some setup is broken. > > > I need to elaborate here. > Your patch is correct and was pushed only to master > but we backported your patch to downstream (1.13). > I meant revert/remove your patch in downstream (1.13). > and push just error code af717c5b022d5c28141333fc02d5d9e1f322505c > to downstream.
I'll let you and Jakub decide what goes into the stable and downstream releases. > >> I will file a tracter ticket for AD expired, disabled users > >> in master branch(future sssd-1.14) > > LS -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
