In order to support the AD Domain\User style and the more usual kerberos user@realm style, sssd needs per domain re_expression and full_name_format options.
Attached is a rough patch implementing per domain qualified user names.
When discussing it on IRC we came up with the following plan: In order to prevent conflicts between the regular expressions for different domains, we parse with a domains regular expression and then check that the resulting domain matches that domain's name.
It's not clear that we should support 'null-domains' in these regular expressions and sss_parse_name_for_domains(). There's a TODO in the patch to sort this out. It may be that we choose to have callers of sss_parse_name_for_domains() which can accept unqualified user domains use the full input string when parsing into a qualified name fails.
In other words, sss_parse_name_for_domains() would not support returning a NULL *domain.
The global re_expression and full_name_format options remain as defaults for the domains.
This patch is especially important for Samba integration. Samba only allows Domain\User format, with the exception that the slash can be replaced with another character.
Cheers,
Stef
On Mon, 2012-04-23 at 20:09 +0200, Stef Walter wrote:
In order to support the AD Domain\User style and the more usual kerberos user@realm style, sssd needs per domain re_expression and full_name_format options.
Attached is a rough patch implementing per domain qualified user names.
When discussing it on IRC we came up with the following plan: In order to prevent conflicts between the regular expressions for different domains, we parse with a domains regular expression and then check that the resulting domain matches that domain's name.
It's not clear that we should support 'null-domains' in these regular expressions and sss_parse_name_for_domains(). There's a TODO in the patch to sort this out. It may be that we choose to have callers of sss_parse_name_for_domains() which can accept unqualified user domains use the full input string when parsing into a qualified name fails.
In other words, sss_parse_name_for_domains() would not support returning a NULL *domain.
The global re_expression and full_name_format options remain as defaults for the domains.
This patch is especially important for Samba integration. Samba only allows Domain\User format, with the exception that the slash can be replaced with another character.
Doesn't this end up running potentially the same regex over and over for each domain we have configured ? Wouldn't it make sense to detect how many different regexes we actually have (in the default case just one, the same for all domains) and just run them once ? Then we can sort out which of the domains using that regex is being addressed ... or is there something I am missing ?
Simo.
On 04/23/2012 09:00 PM, Simo Sorce wrote:
Doesn't this end up running potentially the same regex over and over for each domain we have configured ? Wouldn't it make sense to detect how many different regexes we actually have (in the default case just one, the same for all domains) and just run them once ? Then we can sort out which of the domains using that regex is being addressed ... or is there something I am missing ?
Yes that could be a nice optimization ... if we are in fact matching more than a handful of regexes (ie: configured domains).
But that brings to mind another related point. How are other 'trusted' domains in the forest handled, and does this patch break them? Is there somewhere in SSSD that handles arbitrary domain names, and figures out which configured domain they go to. How does that work?
For example let's say I have a domain 'ad.thewalter.lan' configured in sssd which is part of a trust relationship with 'other.thewalter.lan'. 'other.thewalter.lan' is not a configured sssd domain. How do we resolve user names for other.thewalter.lan?
If this is correctly handled, then I guess I missed consideration of this in my patch. Pointers to the where this logic lives in sssd would be nice.
Cheers,
Stef
On Tue, Apr 24, 2012 at 08:36:32AM +0200, Stef Walter wrote:
On 04/23/2012 09:00 PM, Simo Sorce wrote:
Doesn't this end up running potentially the same regex over and over for each domain we have configured ? Wouldn't it make sense to detect how many different regexes we actually have (in the default case just one, the same for all domains) and just run them once ? Then we can sort out which of the domains using that regex is being addressed ... or is there something I am missing ?
Yes that could be a nice optimization ... if we are in fact matching more than a handful of regexes (ie: configured domains).
But that brings to mind another related point. How are other 'trusted' domains in the forest handled, and does this patch break them? Is there somewhere in SSSD that handles arbitrary domain names, and figures out which configured domain they go to. How does that work?
For example let's say I have a domain 'ad.thewalter.lan' configured in sssd which is part of a trust relationship with 'other.thewalter.lan'. 'other.thewalter.lan' is not a configured sssd domain. How do we resolve user names for other.thewalter.lan?
If this is correctly handled, then I guess I missed consideration of this in my patch. Pointers to the where this logic lives in sssd would be nice.
Hi Stef, the trusted domain appear as subdomains in SSSD. The subdomains support is currently on the list on review: https://fedorahosted.org/pipermail/sssd-devel/2012-March/009069.html
The design document is here: https://fedorahosted.org/sssd/wiki/DesignDocs/SubDomains
I think that patch #4 in the patch set shows what you need - there is a "struct sss_domain_info **subdomains;" array that lists the sub domains the main domain trusts.
On 04/24/2012 11:43 AM, Jakub Hrozek wrote:
On Tue, Apr 24, 2012 at 08:36:32AM +0200, Stef Walter wrote:
On 04/23/2012 09:00 PM, Simo Sorce wrote:
Doesn't this end up running potentially the same regex over and over for each domain we have configured ? Wouldn't it make sense to detect how many different regexes we actually have (in the default case just one, the same for all domains) and just run them once ? Then we can sort out which of the domains using that regex is being addressed ... or is there something I am missing ?
Yes that could be a nice optimization ... if we are in fact matching more than a handful of regexes (ie: configured domains).
But that brings to mind another related point. How are other 'trusted' domains in the forest handled, and does this patch break them? Is there somewhere in SSSD that handles arbitrary domain names, and figures out which configured domain they go to. How does that work?
For example let's say I have a domain 'ad.thewalter.lan' configured in sssd which is part of a trust relationship with 'other.thewalter.lan'. 'other.thewalter.lan' is not a configured sssd domain. How do we resolve user names for other.thewalter.lan?
If this is correctly handled, then I guess I missed consideration of this in my patch. Pointers to the where this logic lives in sssd would be nice.
Hi Stef, the trusted domain appear as subdomains in SSSD. The subdomains support is currently on the list on review:
Integrated subdomain support into the patch.
Along the lines of what Simo said, all subdomains use the regular expression of the parent domain and we only match the regular expression once for those cases.
Thanks for the other reviews too. Updated patch attached.
I'd appreciate help testing this. I don't know how to reach all the code paths involved.
Cheers,
Stef
On Tue, 2012-05-08 at 07:59 +0200, Stef Walter wrote:
Integrated subdomain support into the patch.
Along the lines of what Simo said, all subdomains use the regular expression of the parent domain and we only match the regular expression once for those cases.
Thanks for the other reviews too. Updated patch attached.
I'd appreciate help testing this. I don't know how to reach all the code paths involved.
Thanks for the patch, you're making great progress here.
A few issues remain:
sss_parse_name_for_domains(): Please create a tmp_ctx and use that for allocating dmatch and nmatch, instead of allocating them on memctx. You're effectively leaking memory in a way that valgrind cannot detect (they're allocated and attached to memctx, but unreachable). Please use the tmp_ctx and steal the results onto memctx just prior to returning from the function. Also make sure that all exit points free tmp_ctx.
Please prefer to use the 'bool' type for only_name_seen and only_name_mismatch and 'true' or 'false' as values, instead of zero and one. It just reads easier.
I think only_name_seen is probably unnecessary. You should be able to just check whether only_name is NULL. You have a guarantee that nmatch will never be NULL if code == EOK after sss_parse_name. Well, assuming no bugs in sss_parse_name, but it's not your responsibility in sss_parse_name_for_domains() to work around such bugs. You can simplify the mismatch testing considerably with this in mind.
Actually, thinking more on this, you should have a guarantee that if dmatch is NULL and code is EOK, nmatch will always be the same as orig. Again, if it is not, that would be a bug in sss_parse_name().
So I think we can pretty handily simplify this down.
Sorry for the delay. Rebased onto master.
On 05/10/2012 02:12 AM, Stephen Gallagher wrote:
Thanks for the patch, you're making great progress here.
A few issues remain:
sss_parse_name_for_domains(): Please create a tmp_ctx and use that for allocating dmatch and nmatch, instead of allocating them on memctx. You're effectively leaking memory in a way that valgrind cannot detect (they're allocated and attached to memctx, but unreachable). Please use the tmp_ctx and steal the results onto memctx just prior to returning from the function. Also make sure that all exit points free tmp_ctx.
Fixed in the attached patch.
Please prefer to use the 'bool' type for only_name_seen and only_name_mismatch and 'true' or 'false' as values, instead of zero and one. It just reads easier.
Makes sense.
I think only_name_seen is probably unnecessary. You should be able to just check whether only_name is NULL. You have a guarantee that nmatch will never be NULL if code == EOK after sss_parse_name. Well, assuming no bugs in sss_parse_name, but it's not your responsibility in sss_parse_name_for_domains() to work around such bugs. You can simplify the mismatch testing considerably with this in mind.
Actually, thinking more on this, you should have a guarantee that if dmatch is NULL and code is EOK, nmatch will always be the same as orig. Again, if it is not, that would be a bug in sss_parse_name().
But I don't think that's the case. A regular expression can easily produce a name but no domain.
Cheers,
Stef
On Mon, 2012-06-11 at 17:14 +0200, Stef Walter wrote:
Sorry for the delay. Rebased onto master.
On 05/10/2012 02:12 AM, Stephen Gallagher wrote:
Thanks for the patch, you're making great progress here.
A few issues remain:
sss_parse_name_for_domains(): Please create a tmp_ctx and use that for allocating dmatch and nmatch, instead of allocating them on memctx. You're effectively leaking memory in a way that valgrind cannot detect (they're allocated and attached to memctx, but unreachable). Please use the tmp_ctx and steal the results onto memctx just prior to returning from the function. Also make sure that all exit points free tmp_ctx.
Fixed in the attached patch.
Please prefer to use the 'bool' type for only_name_seen and only_name_mismatch and 'true' or 'false' as values, instead of zero and one. It just reads easier.
Makes sense.
I think only_name_seen is probably unnecessary. You should be able to just check whether only_name is NULL. You have a guarantee that nmatch will never be NULL if code == EOK after sss_parse_name. Well, assuming no bugs in sss_parse_name, but it's not your responsibility in sss_parse_name_for_domains() to work around such bugs. You can simplify the mismatch testing considerably with this in mind.
Actually, thinking more on this, you should have a guarantee that if dmatch is NULL and code is EOK, nmatch will always be the same as orig. Again, if it is not, that would be a bug in sss_parse_name().
But I don't think that's the case. A regular expression can easily produce a name but no domain.
Stef and I discussed this on IRC. We came to the agreement that it's valid (if ugly) for a regular expression to modify the username (such as substituting underscores for spaces), so we do need to handle this case. So with that in mind, ack to this patch.
Thanks very much!
On Mon, 2012-06-11 at 12:45 -0400, Stephen Gallagher wrote:
On Mon, 2012-06-11 at 17:14 +0200, Stef Walter wrote:
Sorry for the delay. Rebased onto master.
On 05/10/2012 02:12 AM, Stephen Gallagher wrote:
Thanks for the patch, you're making great progress here.
A few issues remain:
sss_parse_name_for_domains(): Please create a tmp_ctx and use that for allocating dmatch and nmatch, instead of allocating them on memctx. You're effectively leaking memory in a way that valgrind cannot detect (they're allocated and attached to memctx, but unreachable). Please use the tmp_ctx and steal the results onto memctx just prior to returning from the function. Also make sure that all exit points free tmp_ctx.
Fixed in the attached patch.
Please prefer to use the 'bool' type for only_name_seen and only_name_mismatch and 'true' or 'false' as values, instead of zero and one. It just reads easier.
Makes sense.
I think only_name_seen is probably unnecessary. You should be able to just check whether only_name is NULL. You have a guarantee that nmatch will never be NULL if code == EOK after sss_parse_name. Well, assuming no bugs in sss_parse_name, but it's not your responsibility in sss_parse_name_for_domains() to work around such bugs. You can simplify the mismatch testing considerably with this in mind.
Actually, thinking more on this, you should have a guarantee that if dmatch is NULL and code is EOK, nmatch will always be the same as orig. Again, if it is not, that would be a bug in sss_parse_name().
But I don't think that's the case. A regular expression can easily produce a name but no domain.
Stef and I discussed this on IRC. We came to the agreement that it's valid (if ugly) for a regular expression to modify the username (such as substituting underscores for spaces), so we do need to handle this case. So with that in mind, ack to this patch.
Thanks very much!
Pushed to master.
On Mon, Apr 23, 2012 at 08:09:17PM +0200, Stef Walter wrote:
In order to support the AD Domain\User style and the more usual kerberos user@realm style, sssd needs per domain re_expression and full_name_format options.
Attached is a rough patch implementing per domain qualified user names.
When discussing it on IRC we came up with the following plan: In order to prevent conflicts between the regular expressions for different domains, we parse with a domains regular expression and then check that the resulting domain matches that domain's name.
It's not clear that we should support 'null-domains' in these regular expressions and sss_parse_name_for_domains(). There's a TODO in the patch to sort this out. It may be that we choose to have callers of sss_parse_name_for_domains() which can accept unqualified user domains use the full input string when parsing into a qualified name fails.
In other words, sss_parse_name_for_domains() would not support returning a NULL *domain.
I'm not sure if I understand the comment in the code: + ...and change callers use orig directly as a user name, + * if caller can continue without a domain? + */
Why should the caller have a problem with the NULL domain?
The global re_expression and full_name_format options remain as defaults for the domains.
This patch is especially important for Samba integration. Samba only allows Domain\User format, with the exception that the slash can be replaced with another character.
Cheers,
Stef
I only have some nitpicks: - can you also rename CONFDB_MONITOR_NAME_REGEX and CONFDB_MONITOR_FULL_NAME_FORMAT (maybe just drop the MONITOR_ part). The name is a little confusing now that the option can be used outside the [sssd] section
- some places in the patch use the old numeric DEBUG levels: + DEBUG(4, ("name '%s' did not match any domain's expression\n", orig)); Can you change them to the new SSSDBG_ macros?
sss_process_init(): + /* TODO: Should we do this in confdb_get_domains? */ + ret = sss_names_init(rctx->cdb, rctx->cdb, dom->name, &dom->names);
I think that sss_process_init is OK.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stef Walter -
Stephen Gallagher