Patch to handle sid compression for consideration ...

[Richard Sharpe]

On Mon, Jun 16, 2014 at 6:35 PM, Andrew Bartlett <abartlet at samba.org> wrote:
> On Mon, 2014-06-16 at 08:37 -0700, Richard Sharpe wrote:
>> On Mon, Jun 16, 2014 at 7:56 AM, Simo <simo at samba.org> wrote:
>> > On Mon, 2014-06-16 at 07:45 -0700, Richard Sharpe wrote:
>> >> Hi folks,
>> >>
>> >> Attached is a cleaned up patch to handle sid compression in master.
>> >>
>> >> I have implemented Volker's suggestion to not mess with SID internals
>> >> and use compose_sid instead.
>> >>
>> >> I have also moved things around a bit to fit better into the existing
>> >> code and not expose too many internal functions.
>> >>
>> >> Finally, I added a call to winbindd_pam where needed.
>> >>
>> >> This is a first pass at handling SID compression, and if we find we
>> >> need to handle resource SIDs differently depending on which domain
>> >> they are in, then the function merge_resource_sids_and_cache can
>> >> absorb that code.
>> >>
>> >> Review please, and if acceptable, please push.
>> >
>> > Wouldn't it be better to copy info3 first and then modify the copy ?
>>
>> Sure, I could do that. What advantage do you see? We actually need the
>> modified info3 later when we create the token, at least in one path.
>
> Richard,
>
> A number of us are uncomfortable with this being modified in-place.
> Constant input data should stay constant.

Saying that you feel uncomfortable about it is not a good argument and
is very passive aggressive, IMO.

A better argument is that in the future we may, in fact, need access
to the original structure, and an in-place modification makes that
hard. It also makes it harder for newbies to understand what is
happening.

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.

In the winbindd path we can simply discard the return value and the
memory will be cleaned up when the original talloc context is
freed/deleted.

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.

> Please make the copy.
>
> Andrew Bartlett
>
> --
> Andrew Bartlett
> http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba
>
>
>
>



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