[SSSD] Support for subdomains - stage 1

Stephen Gallagher sgallagh at redhat.com
Fri Mar 23 19:53:57 UTC 2012


Finishing my review.



On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote:
...
> #0007
> Basic infrastructure for handling get_subdomains requests in
> providers.

One additional point about this patch. I don't like that you're forcing
the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer
that we add a subdomain_provider option, which defaults to being the
same as the ID provider. I prefer to maintain the distinction in case we
ever decide to have alternate mechanisms for determining the subdomains
(in IPA or AD).

> 
> #0008
> Add the infrastructure in IPA provider which fetches a list of handled 
> subdomains from server and stores in in sysdb.
> 

Nack.

Please don't use variables named 'state' if they're not the state for a
tevent_req. It gets confusing. Please rename 'struct subdomains_state'
to 'struct ipa_subdomains_ctx' and call the variable something like
ipa_sd_ctx.


ipa_subdomains_get_conn_done() should follow the multiple search base
pattern, not really because we expect to support multiple bases but
because we *should* support filters if they've been added. It's possible
that a deployment might choose to filter out certain domains from a
particular client using this method.

You have the wrong debug message if new_domain_list[c] fails to be
allocated in ipa_subdomains_parse_results(). It lists talloc_array.

I'd prefer if you added an ipa_subdomain_map and used that for
determining the attribute names to use in
ipa_subdomains_parse_results(). Mostly for consistency.


> #0009
> Currently the connections to the data provider use the same name as the 
> domain. With sub-domains the name of the sub-domain cannot be used to find the 
> right data provider connection, hence we store the name of the connection in a 
> new member.
> 

Ack

> #0010
> Support for subdomain name given to provider when calling get_account_info
> 

Nack (minor)

Some whitespace issues in be_get_account_info (leading tabs) around the
req->domain allocation.

> #0011
> New ID-related config options for subdomains, these have to be present because 
> IPA provider doesn't provide these values and defaults need to be implemented. 
> Having defaults on the responder level didn't seem right since the policy 
> might differ for each domain.
> 

Nack.

I don't think this really makes sense at all. In most cases, users will
prefer to use the value on the LDAP server. If they choose to override
it, they'll do so through the existing override options (in the case of
override_homedir, it already has %d available anyway.

We definitely don't need separate handling for shells. I can *kind of*
see a value if you wanted to have only subdomains have a non-default
location. I'm not sure I like that though. I feel like it's probably
more complexity than we need.


> #0012
> The routine expand_homedir_template is now used on more place, therefore it 
> was convenient to move it to util code.
> 

Ack

> #0013
> s2n extended LDAP operation - this is used to ask IPA server for entities from 
> supported subdomains
> 


Nack.

This code really needs comments. s2n_encode_request() and
s2n_response_to_attrs() are incomprehensible.

Please make ipa_s2n_exop_send() and  ipa_s2n_exop_recv() static.


> #0014
> Extend IPA ID provider toi support fetching information for entities within 
> subdomains
> 

Ack

> #0015
> Support for routing PAM request from reponder to the right provider. Similar 
> case as patches #3-#6 in NSS responder but much more simple.
> 

Ack

> 
> #0016
> Basic support (or rather un-support) for subdomains in auth providers. 
> Basically it's designed to acknowledge that the request is not for the main 
> domain but for subdomain and in that case it supports only SSS_PAM_ACCT_MGMT 
> and returns error otherwise. Note that similar thing as the IPA backend change 
> in last hunk is probably not necessary for krb5 backend since when using it, 
> there will be no subdomains for that domain.
> 

Ack

> #0017
> Support providing sysdb context and domain info per request, not per backend 
> context. This is necessary for the next patches.
> 

Ack

> #0018
> Accept be_req instead of be_ctx in access providers. Together with the 
> previous patch, this is needed for the next one.
> 

Ack

> #0019
> Detect if the authorization request came for a user in subdomain and if yes, 
> replace the original backend-wide sysdb context and domain info with their 
> subdomain specific replacements.
> 

Ack

> #0020
> Use request-wide sysdb context and domain info instead of their backend-wide 
> counterparts in HBAC.
> 

Ack

> 
> That's all folks, have fun reviewing :)
> Thanks and bye,
> Jan and Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120323/49d7d7ad/attachment.sig>


More information about the sssd-devel mailing list