Reliably looking up user's group membership SIDs

[Isaac Boukris]

On Mon, Mar 12, 2018 at 1:24 PM, Stefan Metzmacher <metze at samba.org> wrote:
> Hi Issac,
>
>> $ git diff
>> diff --git a/src/lib/krb5/krb/pac.c b/src/lib/krb5/krb/pac.c
>> index 0eb19e6bb..38d25dc37 100644
>> --- a/src/lib/krb5/krb/pac.c
>> +++ b/src/lib/krb5/krb/pac.c
>> @@ -413,6 +413,7 @@ k5_pac_validate_client(krb5_context context,
>>      krb5_ui_2 pac_princname_length;
>>      int64_t pac_nt_authtime;
>>      krb5_principal pac_principal;
>> +    int enterprise = 0;
>>
>>      ret = k5_pac_locate_buffer(context, pac, KRB5_PAC_CLIENT_INFO,
>>                                 &client_info);
>> @@ -440,8 +441,13 @@ k5_pac_validate_client(krb5_context context,
>>      if (ret != 0)
>>          return ret;
>>
>> +    if (principal->type == KRB5_NT_ENTERPRISE_PRINCIPAL)
>> +       enterprise = 1;
>> +
>>      ret = krb5_parse_name_flags(context, pac_princname,
>> -                                KRB5_PRINCIPAL_PARSE_NO_REALM, &pac_principal);
>> +                                KRB5_PRINCIPAL_PARSE_NO_REALM | enterprise ?
>> +                                KRB5_PRINCIPAL_PARSE_ENTERPRISE : 0,
>> +                                &pac_principal);
>>      if (ret != 0) {
>>          free(pac_princname);
>>
>>
>> With this fix, it now works ok even for a user from child domain with
>> the same UPN suffix (abc).
>> What do you think of this patch? Did you encounter this issue, other issues?
>
> I prepared the attached patch (but not yet tested it).


Works fine too.

> Also see the discussion on krbdev at mit.edu.
> http://mailman.mit.edu/pipermail/krbdev/2017-August/012806.html


Ah :)

> Can you follow up on that discussion with MIT and get this fixed in any
> of the possible ways?


Sure, I'll do a PR with your patch and reference this thread.
I'm not sure I could add a test in MIT, as I can't find a way to get
the returned pac-principal with an '@' sign.

> In addition it would be good to get the bug in Heimdal fixed, so that
> we can at least have some minimal tests in Samba's autobuild.

Indeed.