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).
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 will file a tracter ticket for AD expired, disabled users
in master branch(future sssd-1.14)
Simo.
--
Simo Sorce * Red Hat, Inc * New York