[PATCH] Add experimental support to receive PAC via GSSAPI

[Sumit Bose]

Hi,

this series of patches adds support to receive a windows PAC via GSSAPI
and to create a user based on the data in the PAC. This is useful
because in an environment with lots of trust relationships between AD
server it might be quite time consuming to find out about all the group
memberships of a domain user by querying the domain controllers. But the
PAC contains all information about group memberships of the
corresponding user.

The general idea is to add the user, if it doesn't exist in the cache,
to the cache of the corresponding domain (see thread about sub-domains,
currently this patch add the user to the local domain for simplicity)
and to add all group memberships (currently not implemented). If one of
the groups cannot be found in the cache a dummy entry with all data
needed to resolve this group quickly is added to the cache.

Currently there are a couple of loose end, e.g.
- groups and group memberships are not handled
- PAC is not validated
- missing sub-domains
- no real SID to uid/gid mapping

etc, but I like to start the discussion about the code and the general
direction as soon as possible. Currently sssd with these patches can
only be build on rawhide, because of the dependencies to the samba4
package (libndr-krb5pac).

Patch 0007 contains a little example that demonstrates that the pac
responder can also be used to add user and groups based on other input,
e.g. it can be used as a backend for the sss_* utilities. This would
allow a much better control about which user is allowed to do what kind
of operation. Currently only the root user can add and modify user and
group entries with the sss_* tools.

I have used 'pac' as a part of names here because this was the original
target, but I would be happy to change it to a more generic keyword if
anyone has a good suggestion.

bye,
Sumit

[Jan Zelený]

>  Hi,
>  
>  this series of patches adds support to receive a windows PAC via GSSAPI
>  and to create a user based on the data in the PAC. This is useful
>  because in an environment with lots of trust relationships between AD
>  server it might be quite time consuming to find out about all the group
>  memberships of a domain user by querying the domain controllers. But the
>  PAC contains all information about group memberships of the
>  corresponding user.
>  
>  The general idea is to add the user, if it doesn't exist in the cache,
>  to the cache of the corresponding domain (see thread about sub-domains,
>  currently this patch add the user to the local domain for simplicity)
>  and to add all group memberships (currently not implemented). If one of
>  the groups cannot be found in the cache a dummy entry with all data
>  needed to resolve this group quickly is added to the cache.
>  
>  Currently there are a couple of loose end, e.g.
>  - groups and group memberships are not handled
>  - PAC is not validated
>  - missing sub-domains
>  - no real SID to uid/gid mapping
>  
>  etc, but I like to start the discussion about the code and the general
>  direction as soon as possible. Currently sssd with these patches can
>  only be build on rawhide, because of the dependencies to the samba4
>  package (libndr-krb5pac).
>  
>  Patch 0007 contains a little example that demonstrates that the pac
>  responder can also be used to add user and groups based on other input,
>  e.g. it can be used as a backend for the sss_* utilities. This would
>  allow a much better control about which user is allowed to do what kind
>  of operation. Currently only the root user can add and modify user and
>  group entries with the sss_* tools.
>  
>  I have used 'pac' as a part of names here because this was the original
>  target, but I would be happy to change it to a more generic keyword if
>  anyone has a good suggestion.
>  
>  bye,
>  Sumit 

Hi,

I'm sending some comments based on code review (I didn't actually try the 
code). I skipped all autotools parts, since it's not my area of expertise and 
it still makes my eyes bleed.

0002
- pac_get_config() seems redundant
- what is the pac_ctx structure for? Is it there for some future
  extensibility? So far it seems to be redundant

0004
- would it be possible to wrap those unbelievably long and lines in
  pac_add_pac_user() in some kind of macro or function?
- the block on lines 149-156 doesn't make any sense (should the last line
  be deleted)

0005
- as we discussed on IRC, functions sssdpac_externalize() and
  sssdpac_internalize() might be redundant. If they are mandatory, please
  try to find out if it's ok just to place a return in them, it would be better
  I think
- the sssdpac_size() function is mandatory?
- can you just briefly explain, what does the flag AD_USAGE_TGS_REQ in
  sssdpac_flags() mean. It's little confusing for me

0007
- as I pointed out before, I'm against implementing functions which won't be
  used
- considering we wanted to push the patch anyway, what is the purpose of
  pac_cmd_handler() function? I mean why don't we call pac_putpwent() and
  other similar functions directly?


Thanks
Jan

[Sumit Bose]

On Tue, Jun 07, 2011 at 02:14:56PM +0200, Jan Zelený wrote:
>  > Hi,
>  > 
>  > this series of patches adds support to receive a windows PAC via GSSAPI
>  > and to create a user based on the data in the PAC. This is useful
>  > because in an environment with lots of trust relationships between AD
>  > server it might be quite time consuming to find out about all the group
>  > memberships of a domain user by querying the domain controllers. But the
>  > PAC contains all information about group memberships of the
>  > corresponding user.
>  > 
>  > The general idea is to add the user, if it doesn't exist in the cache,
>  > to the cache of the corresponding domain (see thread about sub-domains,
>  > currently this patch add the user to the local domain for simplicity)
>  > and to add all group memberships (currently not implemented). If one of
>  > the groups cannot be found in the cache a dummy entry with all data
>  > needed to resolve this group quickly is added to the cache.
>  > 
>  > Currently there are a couple of loose end, e.g.
>  > - groups and group memberships are not handled
>  > - PAC is not validated
>  > - missing sub-domains
>  > - no real SID to uid/gid mapping
>  > 
>  > etc, but I like to start the discussion about the code and the general
>  > direction as soon as possible. Currently sssd with these patches can
>  > only be build on rawhide, because of the dependencies to the samba4
>  > package (libndr-krb5pac).
>  > 
>  > Patch 0007 contains a little example that demonstrates that the pac
>  > responder can also be used to add user and groups based on other input,
>  > e.g. it can be used as a backend for the sss_* utilities. This would
>  > allow a much better control about which user is allowed to do what kind
>  > of operation. Currently only the root user can add and modify user and
>  > group entries with the sss_* tools.
>  > 
>  > I have used 'pac' as a part of names here because this was the original
>  > target, but I would be happy to change it to a more generic keyword if
>  > anyone has a good suggestion.
>  > 
>  > bye,
>  > Sumit
>  
>  
>  Hi,
>  
>  I'm sending some comments based on code review (I didn't actually try the 
>  code). I skipped all autotools parts, since it's not my area of expertise and 
>  it still makes my eyes bleed.
>  
>  0002
>  - pac_get_config() seems redundant 
removed, we can easily add it if we start using config options

>  - what is the pac_ctx structure for? Is it there for some future
>    extensibility? So far it seems to be redundant 
yes, there is a similar context in the pam and nss responders and I
expect that we will have more data in the context in future.

>  
>  0004
>  - would it be possible to wrap those unbelievably long and lines in
>    pac_add_pac_user() in some kind of macro or function?
>  - the block on lines 149-156 doesn't make any sense (should the last line
>    be deleted) 
I have restructured this file a bit and hope it makes the intention more
clear now.

>  
>  0005
>  - as we discussed on IRC, functions sssdpac_externalize() and
>    sssdpac_internalize() might be redundant. If they are mandatory, please
>    try to find out if it's ok just to place a return in them, it would be better
>    I think
>  - the sssdpac_size() function is mandatory? 
all three are mandatory and have to return to data, because all three
are used as a substitute is the copy call, which is optional, is not
available. See src/lib/krb5/krb/authdata.c in the MIT sources starting
at line 960 for details.

>  - can you just briefly explain, what does the flag AD_USAGE_TGS_REQ
> in
>    sssdpac_flags() mean. It's little confusing for me 
ah, thanks, AD_USAGE_TGS_REQ is wrong here. I have changed it to
'AD_USAGE_KDC_ISSUED | AD_INFORMATIONAL' which means that we are
interested in data issued by the KDC and the caller must not fail if an
error occurs. With AD_USAGE_TGS_REQ the client can add data in the TGS
request. See http://k5wiki.kerberos.org/wiki/Projects/VerifyAuthData for
some more information.

>  
>  0007
>  - as I pointed out before, I'm against implementing functions which won't be
>    used
>  - considering we wanted to push the patch anyway, what is the purpose of
>    pac_cmd_handler() function? I mean why don't we call pac_putpwent() and
>    other similar functions directly?
>   
I dropped the patch completely for now but I keep it in my tree. If
someone finds a good use case I can update it and send it again.

>  
>  Thanks
>  Jan 
Thank you very much for the review.

bye,
Sumit


>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

Sumit Bose <sbose(a)redhat.com&gt; wrote:
>  On Tue, Jun 07, 2011 at 02:14:56PM +0200, Jan Zelený wrote:
>  > > Hi,
>  > > 
>  > > this series of patches adds support to receive a windows PAC via GSSAPI
>  > > and to create a user based on the data in the PAC. This is useful
>  > > because in an environment with lots of trust relationships between AD
>  > > server it might be quite time consuming to find out about all the group
>  > > memberships of a domain user by querying the domain controllers. But
>  > > the PAC contains all information about group memberships of the
>  > > corresponding user.
>  > > 
>  > > The general idea is to add the user, if it doesn't exist in the cache,
>  > > to the cache of the corresponding domain (see thread about sub-domains,
>  > > currently this patch add the user to the local domain for simplicity)
>  > > and to add all group memberships (currently not implemented). If one of
>  > > the groups cannot be found in the cache a dummy entry with all data
>  > > needed to resolve this group quickly is added to the cache.
>  > > 
>  > > Currently there are a couple of loose end, e.g.
>  > > - groups and group memberships are not handled
>  > > - PAC is not validated
>  > > - missing sub-domains
>  > > - no real SID to uid/gid mapping
>  > > 
>  > > etc, but I like to start the discussion about the code and the general
>  > > direction as soon as possible. Currently sssd with these patches can
>  > > only be build on rawhide, because of the dependencies to the samba4
>  > > package (libndr-krb5pac).
>  > > 
>  > > Patch 0007 contains a little example that demonstrates that the pac
>  > > responder can also be used to add user and groups based on other input,
>  > > e.g. it can be used as a backend for the sss_* utilities. This would
>  > > allow a much better control about which user is allowed to do what kind
>  > > of operation. Currently only the root user can add and modify user and
>  > > group entries with the sss_* tools.
>  > > 
>  > > I have used 'pac' as a part of names here because this was the
> original
>  > > target, but I would be happy to change it to a more generic keyword if
>  > > anyone has a good suggestion.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > Hi,
>  > 
>  > I'm sending some comments based on code review (I didn't actually try the
>  > code). I skipped all autotools parts, since it's not my area of expertise
>  > and it still makes my eyes bleed.
>  > 
>  > 0002
>  > - pac_get_config() seems redundant
>  
>  removed, we can easily add it if we start using config options
>  
>  > - what is the pac_ctx structure for? Is it there for some future
>  > 
>  >   extensibility? So far it seems to be redundant
>  
>  yes, there is a similar context in the pam and nss responders and I
>  expect that we will have more data in the context in future.
>  
>  > 0004
>  > - would it be possible to wrap those unbelievably long and lines in
>  > 
>  >   pac_add_pac_user() in some kind of macro or function?
>  > 
>  > - the block on lines 149-156 doesn't make any sense (should the last line
>  > 
>  >   be deleted)
>  
>  I have restructured this file a bit and hope it makes the intention more
>  clear now.
>  
>  > 0005
>  > - as we discussed on IRC, functions sssdpac_externalize() and
>  > 
>  >   sssdpac_internalize() might be redundant. If they are mandatory, please
>  >   try to find out if it's ok just to place a return in them, it would be
>  >   better I think
>  > 
>  > - the sssdpac_size() function is mandatory?
>  
>  all three are mandatory and have to return to data, because all three
>  are used as a substitute is the copy call, which is optional, is not
>  available. See src/lib/krb5/krb/authdata.c in the MIT sources starting
>  at line 960 for details.
>  
>  > - can you just briefly explain, what does the flag AD_USAGE_TGS_REQ in
>  > 
>  >   sssdpac_flags() mean. It's little confusing for me
>  
>  ah, thanks, AD_USAGE_TGS_REQ is wrong here. I have changed it to
>  'AD_USAGE_KDC_ISSUED | AD_INFORMATIONAL' which means that we are
>  interested in data issued by the KDC and the caller must not fail if an
>  error occurs. With AD_USAGE_TGS_REQ the client can add data in the TGS
>  request. See http://k5wiki.kerberos.org/wiki/Projects/VerifyAuthData for
>  some more information.
>  
>  > 0007
>  > - as I pointed out before, I'm against implementing functions which won't
>  > be
>  > 
>  >   used
>  > 
>  > - considering we wanted to push the patch anyway, what is the purpose of
>  > 
>  >   pac_cmd_handler() function? I mean why don't we call pac_putpwent() and
>  >   other similar functions directly?
>  
>  I dropped the patch completely for now but I keep it in my tree. If
>  someone finds a good use case I can update it and send it again.
>  
>  > Thanks
>  > Jan
>  
>  Thank you very much for the review.
>  
>  bye,
>  Sumit 

All patches look much better now (the only issue I found was in patch 4 on 
lines 103 and 104 - you have two returns there), although I didn't have time 
review them very closely nor test them. If noone does the close review in the 
next two weeks, could you please sent me some tips how to set up testing 
environment? I'd like to do the full-scale review as soon as I'm back from 
vacation.

Thanks
Jan