Patch to handle sid compression for consideration ...

[Richard Sharpe]

On Tue, Jun 17, 2014 at 7:30 AM, Simo <simo at samba.org> wrote:
> On Mon, 2014-06-16 at 23:37 -0700, Jeremy Allison wrote:
>> On Mon, Jun 16, 2014 at 10:36:12PM -0700, Jeremy Allison wrote:
>> >
>> > Yep - in that case IMHO the first patch in the series
>> > should be to change the make_session_info_krb5() interface
>> > to use a passed in struct netr_SamInfo3 *, as you noticed
>> > it doesn't use the full struct PAC_LOGON_INFO * at all.
>> >
>> > In fact - I think:
>> >
>> > Patch #1 should be to change:
>> >
>> > make_server_info_info3() to use a const struct netr_SamInfo3 *info3,
>> > as it only reads from info3, never modifies.
>> >
>> > Patch #2 should be to change make_session_info_krb5() to
>> > to take a const struct netr_SamInfo3 *info3, not the
>> > full struct PAC_LOGON_INFO *.
>> >
>> > Patch #3 changes auth3_generate_session_info_pac() to
>> > make a info3 copy from logon_info->info3, and then
>> > does the merge resource sids, caches the info3 copy
>> > using netsamlogon_cache_store(), and then simply passes
>> > that copied info3 as a const pointer to make_session_info_krb5()
>> > in place of the existing struct PAC_LOGON_INFO *logon_info pointer.
>> >
>> > Patch #4 changes winbindd_pam_auth_pac_send() to
>> > make a info3 copy from logon_info->info3, and
>> > calls the netsamlogon_cache_store() with the copy.
>> >
>> > Does that work as a patchset for you ?
>> >
>> > I can make those changes for you to test
>> > later on this week if I get the chance
>> > whilst I'm at the plugfest, or I'm happy
>> > to review these changes if you want to
>> > code 'em up !!
>>
>> Ok, couldn't get to sleep :-). Here is the patchset.
>> Ended up being 5 patches.
>>
>> Compiles but isn't tested. If you can test it
>> that would help greatly !
>>
>> Simo & Andrew, does this address your concerns ?
>> Rchard, does it do what you need ?
>
> I like your plan very much in general.
>
> However I see that you repeat the same 3 calls in an "if (logon_info)"
> wrapped code block in 2 places. That calls for a utility function
> instead, or we risk someone changing it in one place but not another in
> the future. I would like something that does
> make_info3_from_pac() or similar that takes PAC_LOGON_INFO and returns
> netr_SamInfo3.
>
> So then the common code becomes:
> if (logon_info) {
>         status = make_info3_from_pac(tmp_ctx, logon_info, &info3);
>         if (!NT_STATUS_IS_OK(status)) ...
>         netsamlogon_cache_store(ntuser, info3);
> }
>
> Alternatively:
> If (logon_info) {
>         status = netsamlogon_cache_from_pac(logon_info);
>         if (!NT_STATUS_IS_OK(status)) ...
> }
>
> And just put all in this function.

Right. That was one of my concerns originally as well, although I
didn't articulate it.

If no one else gets to it today, I will probably do the following tonight:

1. Add the additional const Volker suggested, and
2. Add the helper function Simo suggested,
3. Resubmit the patch set

Anyone running 3.6 who wants SID compression support can go with the
patches already sent I guess (although it will change a little as the
calling location in smbd is different.) Our concern here is the
functionality with the cleanest code for the master branch.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)