Patch to handle sid compression for consideration ...

[Simo]

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.

Besides that it looks the right way to go.

Simo.