Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 25 Sep 2013 23:05:48 +0200 Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
As discussed on the phone with Sumit, the way the map is selected is wrong and would only allow members who are added using the SFU dialog to be searched.
On Thu, Sep 26, 2013 at 04:01:17PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 25 Sep 2013 23:05:48 +0200 Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
As discussed on the phone with Sumit, the way the map is selected is wrong and would only allow members who are added using the SFU dialog to be searched.
New patches are attached, rebased on top of Sumit's capath patches. The patch with the map is added, instead there is a mini-patch that documents that POSIX attributes must be replicated to GC for trusted users with POSIX attributes to be reachable.
On Thu, Sep 26, 2013 at 10:16:50PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 04:01:17PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 25 Sep 2013 23:05:48 +0200 Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
As discussed on the phone with Sumit, the way the map is selected is wrong and would only allow members who are added using the SFU dialog to be searched.
New patches are attached, rebased on top of Sumit's capath patches. The patch with the map is added, instead there is a mini-patch that documents that POSIX attributes must be replicated to GC for trusted users with POSIX attributes to be reachable.
Patches are working as expected.
....
- if (use_id_mapping) {
/* When mapping IDs or looking for SIDs, we don't want to limit* ourselves to groups with a GID value. But there must be a SID to map* from.*/state->base_filter = talloc_asprintf_append(state->base_filter,"(%s=*))",opts->group_map[SDAP_AT_GROUP_OBJECTSID].name);- } else {
/* When not ID-mapping, make sure there is a non-NULL UID */state->base_filter = talloc_asprintf_append(state->base_filter,"(&(%s=*)(!(%s=0))))",opts->group_map[SDAP_AT_GROUP_GID].name,opts->group_map[SDAP_AT_GROUP_GID].name);- }
- if (!state->base_filter) {
talloc_zfree(req);return NULL;- }
I think calling talloc_zfree(req) and returning NULL in a *_send function is old style. Since it is used in other parts of the patched calls as well I think it is ok.
But that brings me to two questions. First, is this really old style or we should recommend to only return in *_send if the request itself cannot be allocated and call tevent_req_error() and tevent_req_post() for any other errors. If it is old style shall we open a ticket to refactor *_send calls using the old style?
Nevertheless ACK to the patches.
bye, Sumit
On Fri, Sep 27, 2013 at 02:55:58PM +0200, Sumit Bose wrote:
On Thu, Sep 26, 2013 at 10:16:50PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 04:01:17PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 25 Sep 2013 23:05:48 +0200 Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
As discussed on the phone with Sumit, the way the map is selected is wrong and would only allow members who are added using the SFU dialog to be searched.
New patches are attached, rebased on top of Sumit's capath patches. The patch with the map is added, instead there is a mini-patch that documents that POSIX attributes must be replicated to GC for trusted users with POSIX attributes to be reachable.
Patches are working as expected.
....
- if (use_id_mapping) {
/* When mapping IDs or looking for SIDs, we don't want to limit* ourselves to groups with a GID value. But there must be a SID to map* from.*/state->base_filter = talloc_asprintf_append(state->base_filter,"(%s=*))",opts->group_map[SDAP_AT_GROUP_OBJECTSID].name);- } else {
/* When not ID-mapping, make sure there is a non-NULL UID */state->base_filter = talloc_asprintf_append(state->base_filter,"(&(%s=*)(!(%s=0))))",opts->group_map[SDAP_AT_GROUP_GID].name,opts->group_map[SDAP_AT_GROUP_GID].name);- }
- if (!state->base_filter) {
talloc_zfree(req);return NULL;- }
I think calling talloc_zfree(req) and returning NULL in a *_send function is old style. Since it is used in other parts of the patched calls as well I think it is ok.
Yes, I wanted to keep the same style in the function.
The other thing I was wondering about was whether to include a unified function to construct the filter. Currently the same or very similar filter is used for user requests and group requests (just s/UID/GID). But then I didn't want to touch more than needed this close to a release.
But that brings me to two questions. First, is this really old style or we should recommend to only return in *_send if the request itself cannot be allocated and call tevent_req_error() and tevent_req_post() for any other errors.
Yes, I think it's much better to end the request with tevent_req_error. The caller then knows more precisely what went wrong.
If it is old style shall we open a ticket to refactor *_send calls using the old style?
Sure, feel free to open such ticket. But I'm not sure how realistic it would be to mass-convert all _send() functions... I wonder if instead we should be converting one function at a time (with a separate patch) when we touch such function? But then we'd never convert the stable parts of the SSSD...
On Fri, Sep 27, 2013 at 03:48:16PM +0200, Jakub Hrozek wrote:
On Fri, Sep 27, 2013 at 02:55:58PM +0200, Sumit Bose wrote:
On Thu, Sep 26, 2013 at 10:16:50PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 04:01:17PM +0200, Jakub Hrozek wrote:
On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2070 where subdomain users have POSIX attributes and the SSSD honors them.
So far I only tested with AD provider, not with trust environment, but I wanted to sent the patches for review anyway.
The patches mostly amend the search filters and bases which presumed strictly ID mapping in AD domains before. I also implemented Sumit's idea on how to handle the SFU attributes as a new map. I think it's the right thing to do, because if SFU is configured on the server, then the POSIX clients should honour it if ID mapping is off.
One question I have is whether we should explicitly document that the POSIX attributes must be replicated in the man page?
From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 25 Sep 2013 23:05:48 +0200 Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
As discussed on the phone with Sumit, the way the map is selected is wrong and would only allow members who are added using the SFU dialog to be searched.
New patches are attached, rebased on top of Sumit's capath patches. The patch with the map is added, instead there is a mini-patch that documents that POSIX attributes must be replicated to GC for trusted users with POSIX attributes to be reachable.
Patches are working as expected.
....
- if (use_id_mapping) {
/* When mapping IDs or looking for SIDs, we don't want to limit* ourselves to groups with a GID value. But there must be a SID to map* from.*/state->base_filter = talloc_asprintf_append(state->base_filter,"(%s=*))",opts->group_map[SDAP_AT_GROUP_OBJECTSID].name);- } else {
/* When not ID-mapping, make sure there is a non-NULL UID */state->base_filter = talloc_asprintf_append(state->base_filter,"(&(%s=*)(!(%s=0))))",opts->group_map[SDAP_AT_GROUP_GID].name,opts->group_map[SDAP_AT_GROUP_GID].name);- }
- if (!state->base_filter) {
talloc_zfree(req);return NULL;- }
I think calling talloc_zfree(req) and returning NULL in a *_send function is old style. Since it is used in other parts of the patched calls as well I think it is ok.
Yes, I wanted to keep the same style in the function.
The other thing I was wondering about was whether to include a unified function to construct the filter. Currently the same or very similar filter is used for user requests and group requests (just s/UID/GID). But then I didn't want to touch more than needed this close to a release.
But that brings me to two questions. First, is this really old style or we should recommend to only return in *_send if the request itself cannot be allocated and call tevent_req_error() and tevent_req_post() for any other errors.
Yes, I think it's much better to end the request with tevent_req_error. The caller then knows more precisely what went wrong.
If it is old style shall we open a ticket to refactor *_send calls using the old style?
Sure, feel free to open such ticket. But I'm not sure how realistic it would be to mass-convert all _send() functions... I wonder if instead we should be converting one function at a time (with a separate patch) when we touch such function? But then we'd never convert the stable parts of the SSSD...
Thank you for the review, pushed to master and sssd-1-11
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Sumit Bose