Patch to handle sid compression for consideration ...

Jeremy Allison jra at samba.org
Mon Jun 16 23:36:12 MDT 2014


On Mon, Jun 16, 2014 at 09:39:21PM -0700, Richard Sharpe wrote:
> 
> Saying that you feel uncomfortable about it is not a good argument and
> is very passive aggressive, IMO.

Yeah, not the best way of phrasing that I agree.

> Perhaps what I should do is return a new logon_info structure with the
> merged SIDs from the merge function, and use the original structure as
> the talloc context.

Personally I'd just use the existing copy_netr_SamInfo3()
function and make a copy just before merging the sids, and
keep the netsamlogon_cache_store() call separate.

So your change :

-               netsamlogon_cache_store(ntuser, &logon_info->info3);
+               status = merge_resource_sids_and_cache(ntuser, logon_info);

Would then look something like:

	struct netr_SamInfo3 *info3_copy = copy_netr_SamInfo3(ctx, &logon_info->info3);
	if (info3_copy == NULL) {
		// error...
	}
	status = merge_resource_sids(ntuser, logon_info, &info3_copy);
	if (!NT_STATUS_IS_OK(status)) {
		// error....
	}
	netsamlogon_cache_store(ntuser, &info3_copy);
	TALLOC_FREE(info3_copy);

That keeps everything at the same level, and avoids
multi-use functions like merge_resource_sids_and_cache()
which are doing two functionally different things in
one function.

> In the smbd path, I will have to arrange to pass the new logon_info
> structure to the function that creates the session context
> (make_session_info_krb5).
> 
> Unfortunately I cannot just return an info3 structure without also
> changing make_session_info_krb5, although the only thing that
> make_session_info_krb5 uses is the info3 structure in the logon_info,
> so a case could be made to make that change in the interests of
> reducing the unnecessary info being passed around.

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 !!

Cheers,

	Jeremy.


More information about the samba-technical mailing list