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