Hi,
this series of patches contains improvements for the PAC responder related to the support of UIDs and GIDs managed by AD.
The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. The original idea in the ticket was to modify an existing user entry instead of deleting and recreating it. But since the PAC does not contain any useful information which would improve the entry I decided to not touch existing user entries at all and only update the group memberships.
Please find details about the other patches in the commit messages.
bye, Sumit
On Fri, Aug 16, 2013 at 06:25:02PM +0200, Sumit Bose wrote:
Hi,
this series of patches contains improvements for the PAC responder related to the support of UIDs and GIDs managed by AD.
The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. The original idea in the ticket was to modify an existing user entry instead of deleting and recreating it. But since the PAC does not contain any useful information which would improve the entry I decided to not touch existing user entries at all and only update the group memberships.
Please find details about the other patches in the commit messages.
bye, Sumit
Hi,
The patches work well with one addition I sent to the list separately. I don't have any change requests, just some questions before the patches are applied, see below:
From 1e54d2061d72d2f72c30d3fced9b43d4ec369a28 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 1 Aug 2013 12:40:24 +0200 Subject: [PATCH 1/6] PAC: if user entry already exists keep it
Currently the PAC responder deletes a user entry and recreates it if some attributes seems to be different.
Two of the attributes where the home directory and the shell of the user. Those two attributes are not available from the PAC but where generates by the PAC responder. The corresponding ID provider might have better means to determine those attributes, e.g. read them from LDAP, so we shouldn't change them here.
The third attribute is the user name. Since the PAC responder does lookups only based on the UID we can wait until the ID provider updates the entry.
The current logic doesn't detect if any of the extra attributes we put into the "_attrs" variable of get_pwd_from_pac() changed. I think this can only apply to UPN as the alias is derived from the name and the SID should never change. Can this be a problem?
Also maybe we should lowercase the alias explicitly as we do in the IPA extdom handler code. But this patch is good as-is, I think, if there are any issues, they can be solved separately.
From: Sumit Bose sbose@redhat.com Date: Tue, 6 Aug 2013 11:10:10 +0200 Subject: [PATCH 2/6] PAC: do not create users with missing GID
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 12:35:12 +0200 Subject: [PATCH 3/6] PAC: handle non-POSIX groups in cache
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 14:09:42 +0200 Subject: [PATCH 4/6] PAC: read user DN instead of constructing it
ACK.
From 966390e5436d2eed565774adddf33f6ce3f85b24 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 16:56:06 +0200 Subject: [PATCH 5/6] PAC: do not fail if a single group cannot be added/removed
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 18:29:48 +0200 Subject: [PATCH 6/6] PAC: use SID instead of GID to search for groups
ACK
On Mon, Aug 26, 2013 at 11:02:56AM +0200, Jakub Hrozek wrote:
On Fri, Aug 16, 2013 at 06:25:02PM +0200, Sumit Bose wrote:
Hi,
this series of patches contains improvements for the PAC responder related to the support of UIDs and GIDs managed by AD.
The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. The original idea in the ticket was to modify an existing user entry instead of deleting and recreating it. But since the PAC does not contain any useful information which would improve the entry I decided to not touch existing user entries at all and only update the group memberships.
Please find details about the other patches in the commit messages.
bye, Sumit
Hi,
The patches work well with one addition I sent to the list separately. I don't have any change requests, just some questions before the patches are applied, see below:
From 1e54d2061d72d2f72c30d3fced9b43d4ec369a28 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 1 Aug 2013 12:40:24 +0200 Subject: [PATCH 1/6] PAC: if user entry already exists keep it
Currently the PAC responder deletes a user entry and recreates it if some attributes seems to be different.
Two of the attributes where the home directory and the shell of the user. Those two attributes are not available from the PAC but where generates by the PAC responder. The corresponding ID provider might have better means to determine those attributes, e.g. read them from LDAP, so we shouldn't change them here.
The third attribute is the user name. Since the PAC responder does lookups only based on the UID we can wait until the ID provider updates the entry.
The current logic doesn't detect if any of the extra attributes we put into the "_attrs" variable of get_pwd_from_pac() changed. I think this can only apply to UPN as the alias is derived from the name and the SID should never change. Can this be a problem?
I think not. If it is the generated UPN, i.e. username + realm, they are the same and if the UPN in the cache was extracted from the Kerberos ticket during password authentication it would be the more correct one.
Also maybe we should lowercase the alias explicitly as we do in the IPA extdom handler code. But this patch is good as-is, I think, if there are any issues, they can be solved separately.
I agree this is a good idea. Maybe we should consider to introduce a new sysdb call like sysdb_attrs_add_lower_case_string()? Would you mind to open a suitable ticket?
Thank you for the review.
bye, Sumit
From: Sumit Bose sbose@redhat.com Date: Tue, 6 Aug 2013 11:10:10 +0200 Subject: [PATCH 2/6] PAC: do not create users with missing GID
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 12:35:12 +0200 Subject: [PATCH 3/6] PAC: handle non-POSIX groups in cache
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 14:09:42 +0200 Subject: [PATCH 4/6] PAC: read user DN instead of constructing it
ACK.
From 966390e5436d2eed565774adddf33f6ce3f85b24 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 16:56:06 +0200 Subject: [PATCH 5/6] PAC: do not fail if a single group cannot be added/removed
ACK
From: Sumit Bose sbose@redhat.com Date: Thu, 8 Aug 2013 18:29:48 +0200 Subject: [PATCH 6/6] PAC: use SID instead of GID to search for groups
ACK
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Aug 26, 2013 at 11:26:41AM +0200, Sumit Bose wrote:
On Mon, Aug 26, 2013 at 11:02:56AM +0200, Jakub Hrozek wrote:
On Fri, Aug 16, 2013 at 06:25:02PM +0200, Sumit Bose wrote:
Hi,
this series of patches contains improvements for the PAC responder related to the support of UIDs and GIDs managed by AD.
The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. The original idea in the ticket was to modify an existing user entry instead of deleting and recreating it. But since the PAC does not contain any useful information which would improve the entry I decided to not touch existing user entries at all and only update the group memberships.
Please find details about the other patches in the commit messages.
bye, Sumit
Hi,
The patches work well with one addition I sent to the list separately. I don't have any change requests, just some questions before the patches are applied, see below:
From 1e54d2061d72d2f72c30d3fced9b43d4ec369a28 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 1 Aug 2013 12:40:24 +0200 Subject: [PATCH 1/6] PAC: if user entry already exists keep it
Currently the PAC responder deletes a user entry and recreates it if some attributes seems to be different.
Two of the attributes where the home directory and the shell of the user. Those two attributes are not available from the PAC but where generates by the PAC responder. The corresponding ID provider might have better means to determine those attributes, e.g. read them from LDAP, so we shouldn't change them here.
The third attribute is the user name. Since the PAC responder does lookups only based on the UID we can wait until the ID provider updates the entry.
The current logic doesn't detect if any of the extra attributes we put into the "_attrs" variable of get_pwd_from_pac() changed. I think this can only apply to UPN as the alias is derived from the name and the SID should never change. Can this be a problem?
I think not. If it is the generated UPN, i.e. username + realm, they are the same and if the UPN in the cache was extracted from the Kerberos ticket during password authentication it would be the more correct one.
Also maybe we should lowercase the alias explicitly as we do in the IPA extdom handler code. But this patch is good as-is, I think, if there are any issues, they can be solved separately.
I agree this is a good idea. Maybe we should consider to introduce a new sysdb call like sysdb_attrs_add_lower_case_string()? Would you mind to open a suitable ticket?
Thank you for the review.
Sure: https://fedorahosted.org/sssd/ticket/2056
ACK to the patch #1.
On Mon, Aug 26, 2013 at 11:41:02AM +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 11:26:41AM +0200, Sumit Bose wrote:
On Mon, Aug 26, 2013 at 11:02:56AM +0200, Jakub Hrozek wrote:
On Fri, Aug 16, 2013 at 06:25:02PM +0200, Sumit Bose wrote:
Hi,
this series of patches contains improvements for the PAC responder related to the support of UIDs and GIDs managed by AD.
The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. The original idea in the ticket was to modify an existing user entry instead of deleting and recreating it. But since the PAC does not contain any useful information which would improve the entry I decided to not touch existing user entries at all and only update the group memberships.
Please find details about the other patches in the commit messages.
bye, Sumit
Hi,
The patches work well with one addition I sent to the list separately. I don't have any change requests, just some questions before the patches are applied, see below:
From 1e54d2061d72d2f72c30d3fced9b43d4ec369a28 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Thu, 1 Aug 2013 12:40:24 +0200 Subject: [PATCH 1/6] PAC: if user entry already exists keep it
Currently the PAC responder deletes a user entry and recreates it if some attributes seems to be different.
Two of the attributes where the home directory and the shell of the user. Those two attributes are not available from the PAC but where generates by the PAC responder. The corresponding ID provider might have better means to determine those attributes, e.g. read them from LDAP, so we shouldn't change them here.
The third attribute is the user name. Since the PAC responder does lookups only based on the UID we can wait until the ID provider updates the entry.
The current logic doesn't detect if any of the extra attributes we put into the "_attrs" variable of get_pwd_from_pac() changed. I think this can only apply to UPN as the alias is derived from the name and the SID should never change. Can this be a problem?
I think not. If it is the generated UPN, i.e. username + realm, they are the same and if the UPN in the cache was extracted from the Kerberos ticket during password authentication it would be the more correct one.
Also maybe we should lowercase the alias explicitly as we do in the IPA extdom handler code. But this patch is good as-is, I think, if there are any issues, they can be solved separately.
I agree this is a good idea. Maybe we should consider to introduce a new sysdb call like sysdb_attrs_add_lower_case_string()? Would you mind to open a suitable ticket?
Thank you for the review.
Sure: https://fedorahosted.org/sssd/ticket/2056
ACK to the patch #1.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Sumit Bose