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 will file a tracter ticket for AD expired, disabled users
> in master branch(future sssd-1.14)
LS