[SSSD] [PATCH] AD: Add initial gpo-smb implementation.

Sumit Bose sbose at redhat.com
Tue Jun 24 10:03:57 UTC 2014


On Mon, Jun 23, 2014 at 08:52:53PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Jun 19, 2014 at 03:06:04PM -0400, Yassir Elley wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > > > On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
> > > > > 
> > > > > 
> > > > > ----- Original Message -----
> > > > > > On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > ----- Original Message -----
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ----- Original Message -----
> > > > > > > > > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > * You suggested using the name of the DC that SSSD is
> > > > > > > > > > currently
> > > > > > > > > > connected
> > > > > > > > > > to in the smb uri (rather than the domain.name, which will
> > > > > > > > > > require
> > > > > > > > > > libsmbclient to perform a DNS resolution). Is there an easy
> > > > > > > > > > way
> > > > > > > > > > to
> > > > > > > > > > get
> > > > > > > > > > the
> > > > > > > > > > name of the DC that SSSD is currently connected to? I am
> > > > > > > > > > having
> > > > > > > > > > trouble
> > > > > > > > > > finding it.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > In struct ad_gpo_access_state you have a member struct
> > > > > > > > > sdap_id_conn_ctx
> > > > > > > > > *conn. conn->service->uri is the LDAP uri for the current
> > > > > > > > > connection.
> > > > > > > > > You can use calls from OpenLDAP or ldb to split it into
> > > > > > > > > components,
> > > > > > > > > picj
> > > > > > > > > the hostname and create the smb uri.
> > > > > > > > > 
> > > > > > > > > In general the uri should always be available since you read
> > > > > > > > > the
> > > > > > > > > GPO
> > > > > > > > > data from LDAP before doing the smb operations. Nevertheless
> > > > > > > > > you
> > > > > > > > > can
> > > > > > > > > call be_resolve_server_send() to make sure it is set, see e.g.
> > > > > > > > > auth_get_server() how to use it.
> > > > > > > > > 
> > > > > > > > > HTH
> > > > > > > > > 
> > > > > > > > > bye,
> > > > > > > > > Sumit
> > > > > > > > > _______________________________________________
> > > > > > > > > sssd-devel mailing list
> > > > > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I have attached a revised patch that modifies the smb uri to use
> > > > > > > > the
> > > > > > > > server
> > > > > > > > name rather than the domain name.
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > Yassir.
> > > > > > > > _______________________________________________
> > > > > > > > sssd-devel mailing list
> > > > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > > > 
> > > > > > > 
> > > > > > > Oops. Forgot to attach the patch.
> > > > > > > 
> > > > > > > Yassir.
> > > > > > 
> > > > > > Thank you, the patch is working as expected and now uses the hostname
> > > > > > to
> > > > > > connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP
> > > > > > to
> > > > > > split the url and take the hostname from the lud_host member of
> > > > > > typedef
> > > > > > struct ldap_url_desc. The LDAP url can contain port numbers which
> > > > > > would
> > > > > > currently cause troubles with your scheme.
> > > > > >
> > > > > > As a general comment, please try to split your patches into smaller
> > > > > > units. This would help to review them especially to compare multiple
> > > > > > versions of a patch.
> > > > > 
> > > > > I thought you had previously indicated a preference for a single patch
> > > > > (with the understanding that this makes it more difficult for the
> > > > > reviewer). Perhaps I misunderstood. In any case, I have attached two
> > > > > patches to this email: the previous patch, and a new patch to address
> > > > > your
> > > > > comment about using ldap_url_parse().
> > > > > 
> > > > > > 
> > > > > > I have not looked at the child code in details yet, but I would like
> > > > > > to
> > > > > > suggest a change in the workflow. I think the child should only
> > > > > > download
> > > > > > the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/
> > > > > > and
> > > > > > then the backend will read an process it. This way you already have
> > > > > > the
> > > > > > file available in the offline case. When calling the child the
> > > > > > backend
> > > > > > should provide the smb url and a location to store the result. The
> > > > > > child
> > > > > > can e.g. return a checksum for the file which the backend can save
> > > > > > together with the download time in the sysdb cache in a subtree below
> > > > > > cn=custom (grep sysdb.h for 'custom' to find the related sysdb
> > > > > > calls).
> > > > > > With the download time it would be even possible to specific cache
> > > > > > lifetime during which the gpo file will not be downloaded again to
> > > > > > save
> > > > > > bandwidth. But this should be optional.
> > > > > > 
> > > > > > What do you think?
> > > > > > 
> > > > > > bye,
> > > > > > Sumit
> > > > > > _______________________________________________
> > > > > > sssd-devel mailing list
> > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > 
> > > > > 
> > > > > With regard to your suggestion to store the policy data in a local
> > > > > file (and store the file's metadata in the cache), I think it would be
> > > > > preferrable to have the child parse the file, and store the relevant
> > > > > data/metadata directly to the cache. I believe this is how IPA HBAC
> > > > > currently works (i.e. storing to cache, rather than to file). This
> > > > 
> > > > yes, but the original IPA HBAC data comes from a LDAP tree so storing it
> > > > to the cache which has LDAP semantics as well is natural.
> > > > 
> > > > > would allow the file parsing to be done once (when it is written by
> > > > > the child), rather than the file parsing being done every time the
> > > > > policy is processed (when it is read by the backend). Additionally,
> > > > 
> > > > That's a good point. Maybe be can do both, the child just reads and
> > > > saves the file and the backend parses it once and save the access
> > > > control data to the cache so that it has to parse the file again only
> > > > when a new version is read by the child.
> > > > 
> > > > > keep in mind that we are interested in only a subset of the stanzas
> > > > > (i.e. "Privilege Rights" stanza) in the Security Settings policy file
> > > > > (GptTmpl.inf). In other words, only a small portion of the file
> > > > > contains data that is relevant for us.
> > > > 
> > > > yes, nevertheless I'd like to keep the file around. E.g. it would help
> > > > to make debugging easier because we do not have to tell users how to
> > > > download the file from the server to check to content. Additionally
> > > > other application might be interested in the content as well (they can
> > > > even ensure that the version of the file is recent by calling
> > > > pam_acct_mgmt() which would trigger the access provider in the SSSD
> > > > backend, I agree that this is not the nicest solution :-).
> > > > 
> > > > > 
> > > > > For this initial patch, I am having the child return the allowed_sids
> > > > > and denied_sids to the backend for further processing. However, in
> > > > > subsequent patches, I am planning on having the child store the
> > > > > relevant policy data to the cache, and then have the backend retrieve
> > > > > the relevant policy data from the cache. In other words, the proposed
> > > > > logic for future patches is:
> > > > > * if we are online, the backend calls the child (which stores the
> > > > > parsed
> > > > > data to cache)
> > > > > * regardless of whether we are online or offline, the backend retrieves
> > > > > the
> > > > > parsed data from the cache and makes a policy decision.
> > > > 
> > > > Please do not read/write form/to the cache from the child. So far we
> > > > tried to limit the child processes to only do what is blocking and
> > > > cannot be one in an asynchronous ways. Currently this are mostly
> > > > Kerberos related operations like getting TGTs or updating DNS with
> > > > GSS-TSIG.
> > > 
> > > You cite two reasons to save the policy data as a local file:
> > > 1) to have the local file available as an artifact for debugging and/or
> > > consumption by other applications.
> > > 2) to keep processing in the child to the bare minimum (i.e. the child
> > > shouldn't cache the policy data)
> > > 
> > > The first reason doesn't seem very compelling IMO. It is unclear to me why
> > > being able to consult the policy file (which includes lots of extraneous
> > > policy data) would be better than simply relying on the debug logs (which
> > > contain only the relevant policy data). Also, I am not aware of any
> > > applications (other than the sssd-ad provider) that are planning on using
> > > this policy data. Do you have particular application(s) in mind? The
> > > second reason is more compelling. However, if we have the child retrieve
> > > the data, parse the data, and return the parsed data to the backend, then
> > > the child would not need to interact with the cache (i.e. only the backend
> > > would interact with the cache).
> > > 
> > > The reason I am pushing back on the suggestion to store the data to a file
> > > is that it adds complexity without clear benefits IMO (unless I am missing
> > > something). For example, if we agree that the backend should parse the
> > > data and store it to cache anyway (at least when there is a new version),
> > > why not save the step of writing to a local file, by having the child
> > > parse the data, and having the backend store the data to cache.
> > > Additionally, exposing the *entire* contents of the policy file for
> > > debugging purposes, when we are only using a few fields, seems like it
> > > would only confuse users.
> > 
> > What about the third one about other applications. Since SSSD is already
> > doing the work of figuring out which GPOs apply to the host and loading
> > the data why not making the data available to others?
> 
> OK. I agree that this is compelling enough to use a file-based approach.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > With regard to reducing unnecessary downloads, I am planning on
> > > > > implementing the logic suggested by [MS-GPOL] of comparing the cached
> > > > > gpo version against the current gpo version (by downloading only the
> > > > > GPT.INI file). If the gpo versions are the same, there is no need to
> > > > > download the policy file. Again, this is intended for a subsequent
> > > > > patch.
> > > > 
> > > > I think the term 'bandwidth' was not chosen well in my comment. I'm not
> > > > much concerned by the actual download itself but the general costs
> > > > including creating the connection, doing Kerberos authentication etc.
> > > > Please note that Windows client might stay connected to the sysvol and
> > > > might have some clever mechanism to make it cheap to check GPT.INI every
> > > > time. I think from our point of view just reading GPT.INI or reading
> > > > GptTmpl.inf shouldn't make much difference in the general case. So I
> > > > still would suggest to introduce an SSSD specific timeout. But I won't
> > > > mind checking GPT.INI as well. So the version of the policy should be
> > > > written to the cache as well and send to the child so that it first can
> > > > download GPT.INI and check if the version changed. If the version has
> > > > not changed it should indicate this to the backend so that it know it
> > > > does not has to read and parse the policy file (if we agree that the
> > > > backend should do this :-).
> > > 
> > > In addition to the GPT.INI check, I agree that adding an SSSD specific
> > > timeout could be useful. Additionally, the backend should probably send
> > > *all* the guid-filtered GPOs to the child at once (so that the child can
> > > create a single smb connection, and re-use it for retrieving all the
> > > relevant GPT.INI and GptTmpl.inf files). What do you think?
> > > 
> 
> I think my concern about performance could just be based on my current design of the gpo_child. Currently, for *each* smb_uri, I fork/exec a new gpo_child process, send it the smb_uri, have it create an smb connection, perform the smb retrieval, return some data, and then exit the gpo_child process. If gpo-ldap determines that multiple gpos are applicable, the current design could be problematic, as it requires multiple gpo_child processes to be spawned and killed (i.e. one gpo_child process per smb_uri). I am only using this design b/c that is how the ldap_child and krb5_child seem to operate.
> 
> Perhaps a better approach would be: to continue to call the gpo_child separately for each smb_uri, but to only launch a single gpo_child process to handle all the gpos related to a single policy application. This would do away with spawning multiple processes; it would also allow the single gpo_child to create and maintain a single SMB connection, at least for the duration of a single policy application. Does this fit in with the design philosophy of minimalist child processes? Or would it be preferrable to just stay with launching a new child process for each smb_uri?
> 

yes, all needed policy files should be loaded in a single run by a
single process.

> >
> >
> 
> On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?

I think it's ok. I'm not sure if I can finish the review today, if not
I'll be done tomorrow.

bye,
Sumit

> 
> Regards,
> Yassir.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list