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@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@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@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@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
