[SSSD] Re: [SSSD-users] Re: Announcing SSSD 2.2.1
On 9/12/19 2:59 PM, Timo Aaltonen wrote: On 29.8.2019 15.38, Michal Židek wrote: I am sorry I did not include all translation files into the tarball for this release. I will do another minor release that will update the translation as well. Please disregard the 2.2.1 version. Sorry for the inconvenience. 2.2.1 is still the latest, should there be a 2.2.2 by now? Just uploaded the tarball for 2.2.2 to the releases folder https://releases.pagure.org/SSSD/sssd/ will send the announcement soon with release notes. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [SSSD-users] Announcing SSSD 2.2.1
I am sorry I did not include all translation files into the tarball for this release. I will do another minor release that will update the translation as well. Please disregard the 2.2.1 version. Sorry for the inconvenience. Michal On 8/28/19 10:07 AM, Michal Židek wrote: == SSSD 2.2.1 === The SSSD team is proud to announce the release of version 2.2.1 of the System Security Services Daemon. The tarball can be downloaded from: https://releases.pagure.org/SSSD/sssd/ RPM packages will be made available for Fedora shortly. Feedback Please provide comments, bugs and other feedback via the sssd-devel or sssd-users mailing lists: https://lists.fedorahosted.org/mailman/listinfo/sssd-devel https://lists.fedorahosted.org/mailman/listinfo/sssd-users Highlights -- New features * New options were added which allow sssd-kcm to handle bigger data. See manual pages for ``max_ccaches``, ``max_uid_caches`` and ``max_ccache_size``. * SSSD can now automatically refresh cached user data from subdomains in IPA/AD trust. Notable bug fixes ^ * Fixed issue with SSSD hanging when connecting to non-responsive server with ldaps:// * SSSD is now restarted by systemd after crashes. * Fixed refression when dyndns_update was set to True and dyndns_refresh_interval was not set or set to 0 then DNS records were not updated at all. * Fixed issue when ``default_domain_suffix`` was used with ``id_provider = files`` and caused all results from files domain to be fully qualified. * Fixed issue with sudo rules not being visible on OpenLDAP servers * Fixed crash with ``auth_provider = proxy`` that prevented logins Packaging Changes - None Documentation Changes - A new option ``dns_resolver_server_timeout`` was added A new option ``max_ccaches`` was added A new option ``max_uid_ccaches`` was added A new option ``max_ccache_size`` was added A new option ``ocsp_dgst`` was added Tickets Fixed - * `2878 <https://pagure.io/SSSD/sssd/issue/2878>`_ - sssd failover does not work on connecting to non-responsive ldaps:// server * `3217 <https://pagure.io/SSSD/sssd/issue/3217>`_ - Conflicting default timeout values * `3386 <https://pagure.io/SSSD/sssd/issue/3386>`_ - sssd-kcm cannot handle big tickets * `3489 <https://pagure.io/SSSD/sssd/issue/3489>`_ - p11_child should work wit openssl1.0+ * `3685 <https://pagure.io/SSSD/sssd/issue/3685>`_ - KCM: Default to a new back end that would write to the secrets database directly * `3833 <https://pagure.io/SSSD/sssd/issue/3833>`_ - port to pcre2 * `3894 <https://pagure.io/SSSD/sssd/issue/3894>`_ - multihost tests: ldb-tools is needed for multihost tests * `3905 <https://pagure.io/SSSD/sssd/issue/3905>`_ - SSSD doesn't clear cache entries for IDs below min_id. * `4012 <https://pagure.io/SSSD/sssd/issue/4012>`_ - SSSD is not refreshing cached user data for the ipa sub-domain in a IPA/AD trust * `4026 <https://pagure.io/SSSD/sssd/issue/4026>`_ - EVP_PKEY_new_raw_private_key() was only added in OpenSSL 1.1.1 * `4028 <https://pagure.io/SSSD/sssd/issue/4028>`_ - sssd-kcm calls sssd-genconf which triggers nscd warning * `4037 <https://pagure.io/SSSD/sssd/issue/4037>`_ - Logins fail after upgrade to 2.2.0 * `4040 <https://pagure.io/SSSD/sssd/issue/4040>`_ - Reasonable to Restart sssd on crashes? * `4046 <https://pagure.io/SSSD/sssd/issue/4046>`_ - sudo: incorrect usn value for openldap * `4047 <https://pagure.io/SSSD/sssd/issue/4047>`_ - dyndns_update = True is no longer not enough to get the IP address of the machine updated in IPA upon sssd.service startup * `4050 <https://pagure.io/SSSD/sssd/issue/4050>`_ - nss_cmd_endservent resets the wrong index * `4052 <https://pagure.io/SSSD/sssd/issue/4052>`_ - sssd config option "default_domain_suffix" should not cause the files domain entries to be qualified * `3931 <https://pagure.io/SSSD/sssd/issue/3931>`_ - proxy provider is not working with enumerate=true when trying to fetch all groups * `4043 <https://pagure.io/SSSD/sssd/issue/4043>`_ - Typo in systemd.m4 prevents detection of systemd.pc * `3978 <https://pagure.io/SSSD/sssd/issue/3978>`_ - UPN negative cache does not use values from 'filter_users' config option * `4032 <https://pagure.io/SSSD/sssd/issue/4032>`_ - p11_child::do_ocsp() function implementation is not FIPS140 compliant * `4039 <https://pagure.io/SSSD/sssd/issue/4039>`_ - p11_child::sign_data() function implementation is not FIPS140 compliant * `4056 <https://pagure.io/SSSD/sssd/issue/4056>`_ - permission denied on logs when running sssd as non-root user * `4024 <https://pagure.io/SSSD/sssd/issue/4024>`_ - Non FIPS140 compliant usage of PRNG * `2854 <https://pagure.io/
[SSSD] Announcing SSSD 2.2.1
//pagure.io/SSSD/sssd/issue/4024>`_ - Non FIPS140 compliant usage of PRNG * `4026 <https://pagure.io/SSSD/sssd/issue/4026>`_ - EVP_PKEY_new_raw_private_key() was only added in OpenSSL 1.1.1 Detailed changelog -- Alex Rodin (1): tests/cmocka/test_dyndns.c: Switching from tevent_loop_once() to tevent_loop_wait() Alexey Tikhonov (14): util/crypto/libcrypto: changed sss_hmac_sha1() util/crypto/libcrypto: changed sss_hmac_sha1() util/secrets: memory leaks are fixed util/crypto/nss/nss_nite: params sanitization crypto/libcrypto/crypto_nite: HMAC calculation changed util/find_uid.c: fixed debug message util/find_uid.c: fixed race condition bug util/crypto: removed erroneous declaration util/crypto/sss_crypto.c: cleanup of includes util/crypto: generate_csprng_buffer() changed util/crypto: added sss_rand() crypto/libcrypto/crypto_nite.c: memory leak fixed FIPS140 compliant usage of PRNG crypto/nss: some nss_ctx_init() params made const Jakub Hrozek (34): Updating the version for the 2.2.1 release TESTS: Install expect to drive password-change modifications TESTS: Also add LDAP password when creating users TESTS: Test changing LDAP password with extended operation and modification TEST: Add a multihost test for not returning / for an empty home dir MONITOR: Don't check for the nscd socket while regenerating configuration SYSDB: Add sysdb_search_with_ts_attr BE: search with sysdb_search_with_ts_attr BE: Enable refresh for multiple domains BE: Make be_refresh_ctx_init set up the periodical task, too BE/LDAP: Call be_refresh_ctx_init() in the provider libraries, not in back end BE: Pass in attribute to look up with instead of hardcoding SYSDB_NAME BE: Change be_refresh_ctx_init to return errno and set be_ctx->refresh_ctx BE/LDAP: Split out a helper function from sdap_refresh for later reuse BE: Pass in filter_type when creating the refresh account request BE: Send refresh requests in batches BE: Extend be_ptask_create() with control when to schedule next run after success BE: Schedule the refresh interval from the finish time of the last run AD: Implement background refresh for AD domains IPA: Implement background refresh for IPA domains BE/IPA/AD/LDAP: Add inigroups refresh support BE/IPA/AD/LDAP: Initialize the refresh callback from a list to reduce logic duplication IPA/AD/SDAP/BE: Generate refresh callbacks with a macro MAN: Amend the documentation for the background refresh DP/SYSDB: Move the code to set initgrExpireTimestamp to a reusable function IPA/AD/LDAP: Increase the initgrExpireTimestamp after finishing refresh request MAN: Get rid of sssd-secrets reference MAN: Document that it is enough to systemctl restart sssd-kcm.service lately SECRETS: Use different option names from secrets and KCM for quota options SECRETS: Don't limit the global number of ccaches KCM: Pass confdb context to the ccache db initialization KCM: Configurable quotas for the secdb ccache back end TESTS: Add tests for the configurable quotas Don't qualify users from files domain when default_domain_suffix is set Jakub Jelen (1): pam_sss: Add missing colon to the PIN prompt Lukas Slebodnik (1): PROXY: Return data in output parameter if everything is OK Michal Židek (2): TESTS: ldb-tools and sssd-tools are required for multihost tests Update the translations for the 2.2.1 release Niranjan M.R (1): TESTS: Test kvno correctly displays vesion numbers of principals Pavel Březina (11): ci: disable timeout ci: switch to new tooling and remove 'Read trusted files' stage ci: rebase pull request on the target branch ci: print node on which the test is being run sudo: use proper datetime for default modifyTimestamp value systemd: add Restart=on-failure to sssd.service man: fix description of dns_resolver_op_timeout man: fix description of dns_resolver_timeout failover: add dns_resolver_server_timeout option failover: change default timeouts config: add dns_resolver_op_timeout to option list Sam Morris (1): build: fix detection of systemd.pc Samuel Cabrero (1): nss: Fix command 'endservent' resetting wrong struct member Sumit Bose (10): negcache: add fq-usernames of know domains to all UPN neg-caches p11_child: prefer better digest function if card supports it p11_child: fix a memory leak and other memory mangement issues pam: make sure p11_child.log has the right permissions ssh: make sure p11_child.log has the right permissions BE: make sure child log files have the right permissions utils: remove unused prototype (cert_to_ssh_key) utils: move parse_
[SSSD] Re: Question about memory mapped cache
On 11/08/2017 11:28 AM, Sumit Bose wrote: Hi, while trying to prepare some diagrams to illustrate how the memory mapped cache works I realized that we might not use the payload factor as expected. In sss_mmap_cache_init() we set a payload depending of the type of cached data, e.g. for passwd data 4 * MC_SLOT_SIZE or for group data 3 * MC_SLOT_SIZE. This value is used to calculate the data table size: #define MC_DT_SIZE(elems, payload) ( (elems) * (payload) ) mc_ctx->dt_size = MC_DT_SIZE(n_elem, payload); since this does not change the slot size there is now room for e.g (n_elem * 4) slots for passwd data, which makes sense since we assume that a typical passwd entry will occupy 4 slot and we want the cache to be able to store n_elem passwd entries. But the memory for the tree table is calculated only with n_elem #define MC_FT_SIZE(elems) ( (elems) / 8 ) mc_ctx->ft_size = MC_FT_SIZE(n_elem); This means that although we allocated memory for e.g. (4 * n_elem) slots we can only handle n_elem slots because there are only n_elem bits in the free table. As a result 3/4 of the allocate memory for the data is unused and entries in the cache are overwritten earlier than needed. Do you agree with this or did I miss something in the remaining part of the code which makes sure the memory allocated for the data table can be used completely? If not I would suggest to make the payload only a factor which then can be used to allocate the memory for the data table and the free table. bye, Sumit Hi Sumit, I looked into it and I think you are correct. Will you file a ticket? Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Announcing ding-libs 0.6.1
A new version of ding-libs (0.6.1) was released today! ding-libs, or "Ding is not GLib" is a a set of helpful libraries used by projects such as SSSD or gss-proxy. The tarball can be downloaded from: https://releases.pagure.org/SSSD/ding-libs/ MD5 sum is: 141ffba92d7703b7efc2595971305de7 == Highlights == * libini: Length of values in INI files is no longer limited to PATH_MAX. The current limit is the amount of memory getline is able to allocate. == Note for distribution packagers == * API and ABI is backward compatible with last release (0.6.0) == Detailed Changelog == Alexander Scheel (8): Fix build with TRACE_LEVEL Document use of basic regex in ini_config_augment INI: Fix ini_config parsing SEGVs INI: Tests for section/key name collisions INI: Prevent null return_cfg during augment INI: Add INI_MS_DETECT merge notifications INI: Extend INI_MS_DETECT to be non-exclusive INI: Test INI_MS_DETECT non-exclusive behavior Lukas Slebodnik (10): BUILD: Fix linking of ini_augment_ut_check INI: Fix usage of buiddir in ini_augment_ut_check INI: Fix memory leaks in unit test test_ini_augment_empty_dir DHASH: Suppress gcc7 warning INI: Fix warning Walloc-size-larger-than Do not define _GNU_SOURCE COLLECTION: Remove unused macros INI: Fix doxygen comment for ini_errobj_create COLLECTION: Fix misused comma DHASH: Do not use c99 structure initialisation Michal Židek (9): ini_augment: Use full path when reporting pattern mismatch DHASH: Add check based unit test GIT: Add commit template INI: Unit test for augmentation with empty dir INI: do not use readdir_r INI: Allow longer values then PATH_MAX INI: Add test for long values Bump version info Update versions before 0.6.1 release Philip Prindeville (1): DHASH: Add new key type HASH_KEY_CONST_STRING ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/23/2017 03:26 PM, Jakub Hrozek wrote: On Tue, Aug 22, 2017 at 03:21:14PM +0200, Michal Židek wrote: On 08/22/2017 12:31 PM, Jakub Hrozek wrote: On Tue, Aug 22, 2017 at 11:40:39AM +0200, Michal Židek wrote: On 08/22/2017 11:21 AM, Michal Židek wrote: On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Also there is the question if we want to include users from all trusted domains... I think what could be done relatively easily is to feed some tool with SIDs/fqdns of users/groups and make the list just for them/their members. Would that be something we want? Where would these come from? If the GPOs that perhaps. The SIDs or names (probably just names) would be provided by the user of the tool. I imagine somethink like this: $ sssctl access-check --users f...@ad.com b...@ad.com f...@ad.comALLOWED b...@ad.test DENIED $ sssctl access-check --groups linuxus...@ad.com linuxspec...@ad.com linuxus...@ad.com: f...@ad.comALLOWED b...@ad.comDENIED linuxspec...@ad.com: sp...@ad.com ALLOWED sp...@ad.com ALLOWED sp...@ad.com ALLOWED I'm not sure this will be enough. The request specifically asks for per-machine report, not per-user. We would get that with the above version if there is a group that contains all users. I think it is better if the admin provides somehow the scope of users that are going to be checked rather then us (SSSD) trying to figure this out. Also if we do not need to get the resulting list fast, then I think this would be easy to implement. We could enhance it later with --verbose to print detailed information (like what GPO allowed/denied the access). As I said, I think this would be good debugging tool :) , but you are right that we need to align the effort with the RFE requester's expectations. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/22/2017 12:31 PM, Jakub Hrozek wrote: On Tue, Aug 22, 2017 at 11:40:39AM +0200, Michal Židek wrote: On 08/22/2017 11:21 AM, Michal Židek wrote: On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Also there is the question if we want to include users from all trusted domains... I think what could be done relatively easily is to feed some tool with SIDs/fqdns of users/groups and make the list just for them/their members. Would that be something we want? Where would these come from? If the GPOs that perhaps. The SIDs or names (probably just names) would be provided by the user of the tool. I imagine somethink like this: $ sssctl access-check --users f...@ad.com b...@ad.com f...@ad.comALLOWED b...@ad.test DENIED $ sssctl access-check --groups linuxus...@ad.com linuxspec...@ad.com linuxus...@ad.com: f...@ad.comALLOWED b...@ad.comDENIED linuxspec...@ad.com: sp...@ad.com ALLOWED sp...@ad.com ALLOWED sp...@ad.com ALLOWED ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/22/2017 12:43 PM, Michal Židek wrote: On 08/22/2017 11:38 AM, Jakub Hrozek wrote: On Tue, Aug 22, 2017 at 11:21:43AM +0200, Michal Židek wrote: On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The point of this thread is to come up with the design :-) I would prefer to see what we can do currently, do a first draft of the design and then pass it on to users and customers who requested the feature and see if they are OK with this. The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. Hmm, I was going to say just allowed, but now I remember that we also can use GPOs to deny access, right? Yes, we can have rules to specifically deny access and if I am not mistaken they have higher priority (IIRC if the user is allowed in some rule and denied in other rule then he is denied access). With GPOs there is also the issue with security filtering. A GPO can be filtered out for some users or groups (the GPO can be made relevent only if specified users and groups are logging in) in that case it does not matter what is in the GPO. I mean, for the user who is not in the security filter of some GPO, it does not matter what is in the GPO, because it is ignored when this user logs in. This can get more complicated, because that means different users see different subset of GPOs, if we merge all GPOs and make attestation report based on that, we may actually end up with list that is not relevant at all. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. So, here is what I'm thinking for IPA and I initially thought we could take a similar approach for AD. For IPA, we fetch all the HBAC rules which apply for this host, either directly or via a host group. The rules contain the user name or a group. Usernames are easy and the report could just print them. For groups, unfortunately we don't have other mean that to call getgrnam. GPOs do not contain any names, only SIDs, and those can be both user SIDs and group SIDs. So the report would look like this: for rule in cached_hbac_rules: for user in rule.users: print "$user is allowed" for group in rule.groups: print "Members of $group are allowed. That includes: " group_members = getgrnam($group) for member in $group_members: print "$member" In the worst case scenario, we can really fall back to create a tool for AD provider that gets list of names or groups and does the GPO evaluation as if those people were logging in. This would give the most accurate results and could also be used as debugging tool for GPO processing. But it would be very slow to do for all users in the AD, but given the nature of this tool the database can be split into parts and not everything needs to be done at once (for example admin decides to create the ALLOW/DENY list for just users from few groups (linux_users for example)). I /though/ that we fetch all the GPOs and we could print a similar report as the pseudocode above, just unrolling the SIDs in the GPOs. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. This could be an option, too. But as I said the security filter could make this problematic. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. See above, the RFE is a bit fuzzy, we need to see what we can do first. Sure, thanks for explanation. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/22/2017 11:38 AM, Jakub Hrozek wrote: On Tue, Aug 22, 2017 at 11:21:43AM +0200, Michal Židek wrote: On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The point of this thread is to come up with the design :-) I would prefer to see what we can do currently, do a first draft of the design and then pass it on to users and customers who requested the feature and see if they are OK with this. The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. Hmm, I was going to say just allowed, but now I remember that we also can use GPOs to deny access, right? Yes, we can have rules to specifically deny access and if I am not mistaken they have higher priority (IIRC if the user is allowed in some rule and denied in other rule then he is denied access). With GPOs there is also the issue with security filtering. A GPO can be filtered out for some users or groups (the GPO can be made relevent only if specified users and groups are logging in) in that case it does not matter what is in the GPO. This can get more complicated, because that means different users see different subset of GPOs, if we merge all GPOs and make attestation report based on that, we may actually end up with list that is not relevant at all. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. So, here is what I'm thinking for IPA and I initially thought we could take a similar approach for AD. For IPA, we fetch all the HBAC rules which apply for this host, either directly or via a host group. The rules contain the user name or a group. Usernames are easy and the report could just print them. For groups, unfortunately we don't have other mean that to call getgrnam. GPOs do not contain any names, only SIDs, and those can be both user SIDs and group SIDs. So the report would look like this: for rule in cached_hbac_rules: for user in rule.users: print "$user is allowed" for group in rule.groups: print "Members of $group are allowed. That includes: " group_members = getgrnam($group) for member in $group_members: print "$member" In the worst case scenario, we can really fall back to create a tool for AD provider that gets list of names or groups and does the GPO evaluation as if those people were logging in. This would give the most accurate results and could also be used as debugging tool for GPO processing. But it would be very slow to do for all users in the AD, but given the nature of this tool the database can be split into parts and not everything needs to be done at once (for example admin decides to create the ALLOW/DENY list for just users from few groups (linux_users for example)). I /though/ that we fetch all the GPOs and we could print a similar report as the pseudocode above, just unrolling the SIDs in the GPOs. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. This could be an option, too. But as I said the security filter could make this problematic. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. See above, the RFE is a bit fuzzy, we need to see what we can do first. Sure, thanks for explanation. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/22/2017 11:21 AM, Michal Židek wrote: On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Also there is the question if we want to include users from all trusted domains... I think what could be done relatively easily is to feed some tool with SIDs/fqdns of users/groups and make the list just for them/their members. Would that be something we want? Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about access control reporting with id_provider=ad
On 08/21/2017 02:27 PM, Jakub Hrozek wrote: Hi Michal and sssd-devel, one of the RFEs that keeps coming up for SSSD is to provide a sort of an 'attestation report' for SSSD. Mostly the request is about printing who can access this client machine. I know that we fetch all the HBAC rules for a client with the IPA provider, but Michal, you mentioned that it's problematic do to something similar for the AD provider. Could you elaborate why? Would it be possible to extend the AD access provider to fetch all GPOs that match this client? I am not sure how that attestation should look like. Could you point me to an design page if we have some? The way I understood it is that we want list of ALL users in AD with label ALLOWED or DENIED. I am not sure if this possible to do without basically enumerating all users in AD and do the GPO evaluation for every single one of them. If we just want to print the access control related rules in GPO in some nice format, then it would be possible without the enumeration. My point is that making the ALLOW/DENY list could take a lot of time even if we use cached GPOs. That was my main concern. But again, maybe I misunderstood the RFE. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Shall we freeze the development till #3463 is solved?
On 08/08/2017 04:51 PM, Fabiano Fidêncio wrote: People, There's a test, part of our internal CI, recurrently failing in the past few weeks: === FAILURES === _ test_add_remove_user _ Traceback (most recent call last): File "/var/lib/jenkins/workspace/ci/label/debian_testing/src/tests/intg/test_enumeration.py", line 418, in test_add_remove_user ent.assert_passwd(ent.contains_only()) File "/var/lib/jenkins/workspace/ci/label/debian_testing/src/tests/intg/ent.py", line 345, in assert_passwd assert not d, d AssertionError: list mismatch: unexpected users found: [{'dir': '/home/user', 'gecos': '2001', 'gid': 2000, 'name': 'user', 'passwd': '*', 'shell': '/bin/bash', 'uid': 2001}] 1 failed, 226 passed in 976.19 seconds There's already an open issue for this case: https://pagure.io/SSSD/sssd/issue/3463 in order to track the issue. So, as the subject says, shall we officially stop pushing patches till we have this issue solved? Best Regards, I would prefer to normally continue development with the currently open PRs and issues planned for the next release and make the CI issue blocker for the next release. This IMO is less disruptive, while also making the severity of the CI issue high. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: WIP design page: Subdomain configuration
On 04/07/2017 08:51 AM, Jakub Hrozek wrote: On Mon, Jan 16, 2017 at 03:35:11PM +0100, Michal Židek wrote: Hi, I started working on the design page for subdomain configuration in server mode. It is located here: https://fedorahosted.org/sssd/wiki/DesignDocs/SubdomConf The implementation details and how to debug sections will be added later. For now, the design page is short but should at least set the proper expectations for the feature. Please tell me if you think something is unclear. I will add more to the page soon. Hi, I moved the design page to pagure docs: https://docs.pagure.org/SSSD.sssd/design_pages/subdomain_configuration.html and did a little cleanup (I added short implementation section and expanded the examples in the how to test section, mostly) Please let me know (or send a PR) if there's anything wrong or missing. Thank you for moving the page. I sent one tiny PR to fix a typo I noticed. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Question about ipa_domain option
My bad. It works as expected. I forgot to rename the domain domains option. Sorry for spam :) On 04/05/2017 02:46 PM, Michal Židek wrote: On 04/05/2017 02:32 PM, Jakub Hrozek wrote: On Wed, Apr 05, 2017 at 02:19:20PM +0200, Michal Židek wrote: Hello! When I create a [domain/IPADOMAIN] section and then use the ipa_domain option to use ipadomain.test as a domain name (instead of IPADOMAIN) then SSSD is not able to connect to the ipadomain.test properly. I wonder if someone uses the ipa_domain option and if it makes sense to fix or not. It does not seem to be very useful option to me. But maybe I am using the option wrong. Does anyone know what is the use case for the ipa_domain option (why was it added)? I thought that authconfig used to set domain name to 'default' and at least that's the primary use-case I can think of. But renaming the domain works fine for me, what kind of issues were you seeing in the logs? It looks like SSSD is trying to access wrong section on multiple places. Which also prevents SSSD from finding some important options for example: https://paste.fedoraproject.org/paste/Z~wePyB8s5~dnXpaW7o~nV5M1UNdIGYhyRLivL9gydE= If I rename the section name from [donmain/IPADOMAIN] to [domain/ipadomain.test] everything works as expected. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Question about ipa_domain option
Hello! When I create a [domain/IPADOMAIN] section and then use the ipa_domain option to use ipadomain.test as a domain name (instead of IPADOMAIN) then SSSD is not able to connect to the ipadomain.test properly. I wonder if someone uses the ipa_domain option and if it makes sense to fix or not. It does not seem to be very useful option to me. But maybe I am using the option wrong. Does anyone know what is the use case for the ipa_domain option (why was it added)? Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: https://pagure.io/SSSD/sssd/issue/3158 {where to find source code ini_rules_check(), may be sss_util.c}
On 03/18/2017 02:39 PM, amit kumar wrote: Hello, sssctl config-check does not show what file contains the problem ./src/util/sss_ini.c:578:ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, *errobj*); *errobj=*> This contains errorObjects which are thrown on screen. # grep -rwn . -e "ini_rules_check" Binary file ./src/util/.libs/libsss_util_la-sss_ini.o matches ./src/util/sss_ini.c:578:ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); ./src/util/sss_ini.c:581: "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); Binary file ./x86_64/src/util/.libs/libsss_util_la-sss_ini.o matches Binary file ./x86_64/.libs/*libsss_util.so* matches<=Query[where to find libsss_util.so source code (may be sss_util.c)] Binary file ./x86_64/.libs/libsss_util.soT matches Binary file ./.libs/libsss_util.so matches Binary file ./.libs/libsss_util.soT matches I know libsss_util.so is created during reconfig & chmake, as it creates x86_64. Functions with ini_ prefix are from ding-libs (libini part to be more precise). Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: sssd-1.14.3 milestone cleanup
On 02/02/2017 11:36 AM, Jakub Hrozek wrote: On Wed, Jan 11, 2017 at 06:52:32PM +0100, Lukas Slebodnik wrote: On (11/01/17 16:31), Jakub Hrozek wrote: Hi, despite new development happening in the sssd-1-15 branch (aka master), there are still too many tickets in the 1.14.3 milestone. The tickets should be moved out to current milestones unless someone is really working on them. These are: * https://fedorahosted.org/sssd/ticket/3063 - add an integration test for the configuration include directories - would it be enough to have two users in two search bases and drop a snippet with the second base, then try to resolve a user? If yes, this is a one-hour effort, any takers? If not, move to CI milestone - Lukas said there would be issue with integration test would be how to detect whether libini_config suports it (rhel6 does not support it). Therefore I suggest we move the ticket to CI milestone. * https://fedorahosted.org/sssd/ticket/3085 - looks fixed in https://git.fedorahosted.org/cgit/sssd.git/commit/?id=11540d9efb85b9ed0341e8a1fc97fc078c6ce418 OK to close? - Lukas already added a +1 last week on our meeting, so I'll probably close the ticket. yes, but Michal might check wheter the commit is good enough for him. This one was already closed. I actually sent the patch that closed the ticket as a reaction to this thread, but forgot to respond after. * https://fedorahosted.org/sssd/ticket/3197 - add a line to sssd-ad man page on how does the POSIX attrs in GC work - Someone suggested reverting the logic for POSIX-attrs-in-GC lately, but I forgot the details, does anyone remember? Otherwise this is a 5-minute patch, so I suggest just closing it. * https://fedorahosted.org/sssd/ticket/3208 - Need detailed information about config-check option - what is this ticket about? Do we need it? I suggest we just close it I think that issue is with mninimal manual page. There is just following paragraph AVAILABLE COMMANDS To list all available commands run sssctl without any parameters. To print help for selected command run sssctl COMMAND --help. We might do the same as in dnf SYNOPSIS sssctl [options] [...] Available commands: * domain-list * domain-status and then describe Commands in details. Michal, do you have an opinon here? If this is something that can be fixed quickly (several hours max), then let's just get the ticket out of our radar. The original idea was to keep the manual page minimal and move most of the help in the --help output for each command. The reason was to not have the same info on two places (man page and help). Now, we do have the part with minimal man page finished :D , but the --help does not contain enough help for most commands. I do not know how much time will it take to write the missing documentation, from what I see, each command is missing something like general description in the --help output, so it does not seem to be that much work. I can do it after I finish the SELinux bugs, it should not take much time. * https://fedorahosted.org/sssd/ticket/3222 - sssd still showing ipa user after removed from last group - unless anyone is actively working on the ticket, just move to patches welcome This ticket was caught as part of testing Web_App_Authentication. @see related BZ. Do we want to ignore such bugs? It might affect integration effort. I'm just worried that noone is working on the bug and that we don't even know how to fix it. * https://fedorahosted.org/sssd/ticket/2554 - Update spec file according to updated guidelines - Unless anyone would like to clean up our reference upstream specfile, I suggest we close the ticket There are still parts which need to be updated in spec file. I sent some patches from time to time to decrease them. Because it's not a priority. We might move it to "patches welcome" bucket OK, moved. * https://fedorahosted.org/sssd/ticket/3113 - Please move sudo_timed option to sssd-sudo man page - 5 minutes patch and George is unlikely to send a patch, any takers? * https://fedorahosted.org/sssd/ticket/3074 - Move timestamp cache to tmpfs * https://fedorahosted.org/sssd/ticket/3097 - Measure the difference between tmpfs database and NOSYNC database - tmpfs provides very little benefit, close with an explanation Could you provide numbers/data? It would be good to know what is a "very little benefit" But I'm fine with closing #3074 of course you are right. I re-run the tests and added measurements into #3097 (now I think 3097 can be closed because the purpose was only to add the measurements) and with what the benchmarks show is quite conslusive in the sense that #3074 can be closed or moved into patches welcome. Please let me know if you
[SSSD] Re: WIP design page: Subdomain configuration
On 01/17/2017 11:15 AM, Jakub Hrozek wrote: On Mon, Jan 16, 2017 at 03:35:11PM +0100, Michal Židek wrote: Hi, I started working on the design page for subdomain configuration in server mode. It is located here: https://fedorahosted.org/sssd/wiki/DesignDocs/SubdomConf The implementation details and how to debug sections will be added later. For now, the design page is short but should at least set the proper expectations for the feature. Please tell me if you think something is unclear. I will add more to the page soon. My main comment is this: ``` his feature will only be available in server mode (SSSD installed on IPA server). ``` Why not make the feature available always? At the very least there are many users who would appreciate if they could set the list of ad_servers per subdomain. The ticket is only talking about the server mode, which is why I think I got the wrong impression we want it only for the server mode. I guess you are right there is no point in limiting ourselves only to the server mode and there may be valid use cases for non server mode as well. I will change the design page. Thanks for catching this Jakub! ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] WIP design page: Subdomain configuration
Hi, I started working on the design page for subdomain configuration in server mode. It is located here: https://fedorahosted.org/sssd/wiki/DesignDocs/SubdomConf The implementation details and how to debug sections will be added later. For now, the design page is short but should at least set the proper expectations for the feature. Please tell me if you think something is unclear. I will add more to the page soon. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: RFC: Configuration of trusted domain (a.k.a. subdomain) in sssd.conf
On 01/03/2017 04:51 PM, Jakub Hrozek wrote: On Tue, Jan 03, 2017 at 04:46:25PM +0100, Michal Židek wrote: Hi, for IPA provider, we plan to add the ability to configure trusted domains (currently AD domains) in a similar way the main domain is configured in sssd.conf. If ipadomain.test is the main IPA domain and addomain.test is the AD domain and there is IPA-AD trust extablished between the two, I think the way to configure addomain.test specific option could be: [domain/ipadomain.com] # the usual main domain configuration [domain/ipadomain.com/addomain.com] # configuration of trusted domain I like the general format of the configuration. So, the main domain would be used as prefix in the section name where the trusted domain / subdomain is configured. I wanted to ask what other developers think about the format. Note that not all options that are available in the main domain will be available for the subdomain. Of course, options like id_provider will not be overridable. Maybe we can extend the dp_opts array to indicate if the option is possible to be overriden? That is an implementation detail. My main concern was that we often talk about "domain section of sssd.conf" and this looks like a domain section, but a lot of what was said about domain section (on list, forums, etc.) does not apply to this section and that concerned me a little. But OTOH I do not think the confusion would be big and of course it will be explained in the man page together with list of options that are available for this "subdomain" section. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] RFC: Configuration of trusted domain (a.k.a. subdomain) in sssd.conf
Hi, for IPA provider, we plan to add the ability to configure trusted domains (currently AD domains) in a similar way the main domain is configured in sssd.conf. If ipadomain.test is the main IPA domain and addomain.test is the AD domain and there is IPA-AD trust extablished between the two, I think the way to configure addomain.test specific option could be: [domain/ipadomain.com] # the usual main domain configuration [domain/ipadomain.com/addomain.com] # configuration of trusted domain So, the main domain would be used as prefix in the section name where the trusted domain / subdomain is configured. I wanted to ask what other developers think about the format. Note that not all options that are available in the main domain will be available for the subdomain. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: trac cleanup of the 1.14 backlog milestone
On 12/06/2016 11:56 AM, Jakub Hrozek wrote: Hi, I checked the 1.14 backlog milestone. I think most of the tickets can be just moved to "Future releases" except for a couple where I was quite confident the ticket can be just closed (and I just closed them), except for these: https://fedorahosted.org/sssd/ticket/1400 - [RFE] In memory fast cache should support negative caching +1 for Patches welcome https://fedorahosted.org/sssd/ticket/2497 - When machine gets IPA-enrolled to different IPA with the same domain name and the same users, caches prevent correct operation This should at least be documented as a known issue before we put it to Patches welcome milestone. Maybe putting it to the troubleshooting wiki page? https://fedorahosted.org/sssd/ticket/2967 - Intermittent attribute retrieval failure from AD Not sure about this. Lukas owns the issue, maybe he will know more. The three above should IMO be moved to "Patches welcome". Next on my cleanup todo list is 1.14.3 milestone - IMO only bug fixes should be there, the other tickets would just be overlooked since we are not developing new features or new tests for 1.14. Then it's finally time to start drafting the 1.15 milestone.. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Nested netgroups with IPA provider
On 11/10/2016 03:18 PM, Michal Židek wrote: On 11/10/2016 02:13 PM, Lukas Slebodnik wrote: On (10/11/16 13:57), Michal Židek wrote: On 11/10/2016 12:29 PM, Jakub Hrozek wrote: On Thu, Nov 10, 2016 at 10:49:55AM +0100, Michal Židek wrote: Hi, this is continuation of discussion about pull request 51 and associated tickets. For context, see: https://github.com/SSSD/sssd/pull/59 https://fedorahosted.org/sssd/ticket/3159 https://fedorahosted.org/sssd/ticket/3116 The FreeIPA UQE guys added upstream test for this issue because we do not have upstream CI tests in SSSD with IPA provider yet and this bug is not present in the plain LDAP. We use hash tables to store members of netgroups while processing netgroups (and creating the netgroup triples). The netgroup names are lowercased before they are stored in the hash table. The reason for this normalization is unknown to me. I also don't know the reason behind this normalization. There is a comment above the code that lowercases the DN that says: 646 /* Transform the DN to lower case. 647 * this is important, as the member/memberof attributes 648 * have the value also in lower-case 649 */ But I really have no idea what this means. We're using SYSDB_NETGROUP_MEMBER, but we're storing names in that attribute and the IPA code lowercases original DNs, so I'm quite confused about it. FreeIPA only creates lowercased netgroup names, so lowercasing only affects the attribute name (that is stored as prefix to the netgroup name in the hash table, and maybe it can happen that the attribute name can be stored in different cases at some point, which would explain why we lower case it, however I was not able to confirm if this is the case). I really think this is about the original DN values. This is what we enter: (gdb) p key.str "ipauniqueid=026e49e8-969d-11e6-a2f4-525400f6cf82,cn=ng,cn=alt,dc=ipa,dc=test" When we read the hash table, we do not lowercase the keys, so the nested netgroups are not found and this is the reason why the bug appears. The patch in PR 51 lower cases the keys before reading the hash table and the bug does not appear after that. Lukas thinks however that this is not good approach, because there should be no need for the lower casing in the first place. Patch that removes the lower casing before adding the keys to htable should also fix the issue. I did not send the patch with this approach, because I was not sure why the lowercasing happens in the first place and I know that lowercasing is not harmful for the IPA netgroups, so I find it safer to use the approach in the PR 51 especially while we do not have good code coverage for IPA provider, however as I mentioned in the PR 51, I am looking for opinions on this. I'm fine with removing the lowercasing if this moves fixing the issue forward. I just quickly tried it without the lowercasing and it does work for me. awesome LS The patch for master is in attachment. Michal Lukas wanted a new PR, so here it is: https://github.com/SSSD/sssd/pull/78 Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Nested netgroups with IPA provider
On 11/10/2016 02:13 PM, Lukas Slebodnik wrote: On (10/11/16 13:57), Michal Židek wrote: On 11/10/2016 12:29 PM, Jakub Hrozek wrote: On Thu, Nov 10, 2016 at 10:49:55AM +0100, Michal Židek wrote: Hi, this is continuation of discussion about pull request 51 and associated tickets. For context, see: https://github.com/SSSD/sssd/pull/59 https://fedorahosted.org/sssd/ticket/3159 https://fedorahosted.org/sssd/ticket/3116 The FreeIPA UQE guys added upstream test for this issue because we do not have upstream CI tests in SSSD with IPA provider yet and this bug is not present in the plain LDAP. We use hash tables to store members of netgroups while processing netgroups (and creating the netgroup triples). The netgroup names are lowercased before they are stored in the hash table. The reason for this normalization is unknown to me. I also don't know the reason behind this normalization. There is a comment above the code that lowercases the DN that says: 646 /* Transform the DN to lower case. 647 * this is important, as the member/memberof attributes 648 * have the value also in lower-case 649 */ But I really have no idea what this means. We're using SYSDB_NETGROUP_MEMBER, but we're storing names in that attribute and the IPA code lowercases original DNs, so I'm quite confused about it. FreeIPA only creates lowercased netgroup names, so lowercasing only affects the attribute name (that is stored as prefix to the netgroup name in the hash table, and maybe it can happen that the attribute name can be stored in different cases at some point, which would explain why we lower case it, however I was not able to confirm if this is the case). I really think this is about the original DN values. This is what we enter: (gdb) p key.str "ipauniqueid=026e49e8-969d-11e6-a2f4-525400f6cf82,cn=ng,cn=alt,dc=ipa,dc=test" When we read the hash table, we do not lowercase the keys, so the nested netgroups are not found and this is the reason why the bug appears. The patch in PR 51 lower cases the keys before reading the hash table and the bug does not appear after that. Lukas thinks however that this is not good approach, because there should be no need for the lower casing in the first place. Patch that removes the lower casing before adding the keys to htable should also fix the issue. I did not send the patch with this approach, because I was not sure why the lowercasing happens in the first place and I know that lowercasing is not harmful for the IPA netgroups, so I find it safer to use the approach in the PR 51 especially while we do not have good code coverage for IPA provider, however as I mentioned in the PR 51, I am looking for opinions on this. I'm fine with removing the lowercasing if this moves fixing the issue forward. I just quickly tried it without the lowercasing and it does work for me. awesome LS The patch for master is in attachment. Michal >From e70366ed2d5c4eb4b08237bd78f5fbced95fefec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Thu, 10 Nov 2016 15:04:57 +0100 Subject: [PATCH] ipa: Nested netgroups do not work We lowercase the keys to the hash table used to store netgroups but do not lowercase it when reading the table. This results in nested netgroups not being found when they should and the processing fails. The lowercasing does not seem to be necessary anymore (not sure if it ever was) so we can skip it. Resolves: https://fedorahosted.org/sssd/ticket/3159 --- src/providers/ipa/ipa_netgroups.c | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/providers/ipa/ipa_netgroups.c b/src/providers/ipa/ipa_netgroups.c index a19e5e0..17b11af 100644 --- a/src/providers/ipa/ipa_netgroups.c +++ b/src/providers/ipa/ipa_netgroups.c @@ -563,7 +563,6 @@ static void ipa_netgr_members_process(struct tevent_req *subreq) size_t count; int ret, i; const char *orig_dn; -char *orig_dn_lower; hash_table_t *table; hash_key_t key; hash_value_t value; @@ -638,20 +637,12 @@ static void ipa_netgr_members_process(struct tevent_req *subreq) goto fail; } -orig_dn_lower = talloc_strdup(table, orig_dn); -if (orig_dn_lower == NULL) { +key.str = talloc_strdup(table, orig_dn); +if (key.str == NULL) { ret = ENOMEM; goto fail; } -/* Transform the DN to lower case. - * this is important, as the member/memberof attributes - * have the value also in lower-case - */ -key.str = orig_dn_lower; -while (*orig_dn_lower != '\0') { -*orig_dn_lower = tolower(*orig_dn_lower); -orig_dn_lower++; -} + value.ptr = entities[i]; ret = hash_enter(table, , ); if (ret != HASH_SUCCESS) { -- 2.5.5 ___ sssd-devel mailing
[SSSD] Re: Nested netgroups with IPA provider
On 11/10/2016 12:29 PM, Jakub Hrozek wrote: On Thu, Nov 10, 2016 at 10:49:55AM +0100, Michal Židek wrote: Hi, this is continuation of discussion about pull request 51 and associated tickets. For context, see: https://github.com/SSSD/sssd/pull/59 https://fedorahosted.org/sssd/ticket/3159 https://fedorahosted.org/sssd/ticket/3116 The FreeIPA UQE guys added upstream test for this issue because we do not have upstream CI tests in SSSD with IPA provider yet and this bug is not present in the plain LDAP. We use hash tables to store members of netgroups while processing netgroups (and creating the netgroup triples). The netgroup names are lowercased before they are stored in the hash table. The reason for this normalization is unknown to me. I also don't know the reason behind this normalization. There is a comment above the code that lowercases the DN that says: 646 /* Transform the DN to lower case. 647 * this is important, as the member/memberof attributes 648 * have the value also in lower-case 649 */ But I really have no idea what this means. We're using SYSDB_NETGROUP_MEMBER, but we're storing names in that attribute and the IPA code lowercases original DNs, so I'm quite confused about it. FreeIPA only creates lowercased netgroup names, so lowercasing only affects the attribute name (that is stored as prefix to the netgroup name in the hash table, and maybe it can happen that the attribute name can be stored in different cases at some point, which would explain why we lower case it, however I was not able to confirm if this is the case). I really think this is about the original DN values. This is what we enter: (gdb) p key.str "ipauniqueid=026e49e8-969d-11e6-a2f4-525400f6cf82,cn=ng,cn=alt,dc=ipa,dc=test" When we read the hash table, we do not lowercase the keys, so the nested netgroups are not found and this is the reason why the bug appears. The patch in PR 51 lower cases the keys before reading the hash table and the bug does not appear after that. Lukas thinks however that this is not good approach, because there should be no need for the lower casing in the first place. Patch that removes the lower casing before adding the keys to htable should also fix the issue. I did not send the patch with this approach, because I was not sure why the lowercasing happens in the first place and I know that lowercasing is not harmful for the IPA netgroups, so I find it safer to use the approach in the PR 51 especially while we do not have good code coverage for IPA provider, however as I mentioned in the PR 51, I am looking for opinions on this. I'm fine with removing the lowercasing if this moves fixing the issue forward. I just quickly tried it without the lowercasing and it does work for me. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Nested netgroups with IPA provider
Hi, this is continuation of discussion about pull request 51 and associated tickets. For context, see: https://github.com/SSSD/sssd/pull/59 https://fedorahosted.org/sssd/ticket/3159 https://fedorahosted.org/sssd/ticket/3116 The FreeIPA UQE guys added upstream test for this issue because we do not have upstream CI tests in SSSD with IPA provider yet and this bug is not present in the plain LDAP. We use hash tables to store members of netgroups while processing netgroups (and creating the netgroup triples). The netgroup names are lowercased before they are stored in the hash table. The reason for this normalization is unknown to me. FreeIPA only creates lowercased netgroup names, so lowercasing only affects the attribute name (that is stored as prefix to the netgroup name in the hash table, and maybe it can happen that the attribute name can be stored in different cases at some point, which would explain why we lower case it, however I was not able to confirm if this is the case). When we read the hash table, we do not lowercase the keys, so the nested netgroups are not found and this is the reason why the bug appears. The patch in PR 51 lower cases the keys before reading the hash table and the bug does not appear after that. Lukas thinks however that this is not good approach, because there should be no need for the lower casing in the first place. Patch that removes the lower casing before adding the keys to htable should also fix the issue. I did not send the patch with this approach, because I was not sure why the lowercasing happens in the first place and I know that lowercasing is not harmful for the IPA netgroups, so I find it safer to use the approach in the PR 51 especially while we do not have good code coverage for IPA provider, however as I mentioned in the PR 51, I am looking for opinions on this. Thanks, Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Should we use VMs or containers for (some) tests?
On 11/01/2016 02:06 PM, Nikolai Kondrashov wrote: On 11/01/2016 01:29 PM, Michal Židek wrote: Btw, I wanted to ask if the code for the new infrastructure is going to reside in the contrib directory in SSSD repo or somewhere else. Does someone have any preferences? I have a preference for it to be in the SSSD repo, unless it's way too different/big and doesn't integrate well. Nick I would like to keep it in SSSD repo too. However I would like it to have the possibility to consume not just the SSSD code in the same repo, but also any SSSD srpm. This way, we would be able to run tests from master for older SSSD versions without the need to backport the code, which is sometimes not very easy task. Of course this can be done even if the tests are in the same repo as SSSD. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Should we use VMs or containers for (some) tests?
On 11/01/2016 11:57 AM, Nikolai Kondrashov wrote: On 11/01/2016 11:36 AM, Jakub Hrozek wrote: On Tue, Nov 01, 2016 at 10:31:20AM +0200, Nikolai Kondrashov wrote: Hi Jakub, On 10/27/2016 05:20 PM, Jakub Hrozek wrote: I'm currently working on integration tests for the 'files' provider and during this work I started to feel we are pushing the boundaries around our test infrastructure already quite a bit. When SSSD talks over network to a server, then we're more or less okay, but for some parts of SSSD, like the files provider, we have to mock a lot of pieces and the end result is that we are testing something that resembles the target system, but probably has its own bugs. Additionally, we can't run some tests at all (anything against IPA) and I suspect we'll be running into this sort of a problem even more in the future. So I'm interested in hearing what are other's thoughts on exploring how to run some of our tests in a privileged environment, either in a VM or in a container? Our current tests have the big advantage of being able to provision a test locally in a screen session, but maybe something similar would be possible by e.g. running a screen in a container and attaching to its tty.. And for "simple" tests like LDAP provider we could keep the current infrastructure around. I agree that the current test setup is limiting, can be difficult to work with, and bring its own issues. However I believe we haven't employed it fully yet, and I'm not sure how spending time on another test system compares to just writing more tests (e.g. for Samba and just general tests). Yes, many tests can still continue exploring the wrapped scenario. I think I will be perfectly happy writing tests for the KCM service using a cwrapped KDC. If your tests talks to a server, then as long as the server is running and you're talking to it over socket_wrapper, you're fine. But to give a concrete example, here are the issues I ran into with the files provider tests: - in production, the files provider dlopens libnss_files.so and calls its functions. I had to wrap a different module for tests that calls nss_wrapped functions instead to reach nss_wrapper's passwd and groups. This is just additional work, but I wouldn't have to do it in a VM/container. - more importantly, one part of testing the files provider is detecting the changes the admin does to /etc/passwd and /etc/groups. In tests, I wrote a simple module that modifies nss_wrappers' passwd and group files, but then I need to take care so that my module changes the files using the same modifications (create a new file and replace the old one with it, not modify the old file in-place) otherwise the files provider was receiving different inotify callbacks. In a container or a VM, I would just call the shadow-utils binaries. Other examples of things we can't test easily with wrappers are D-Bus system bus interfaces (these wouldn't be too hard to wrap, though, 'just' additional work) and anything that involves an IPA server. Yes, I agree, a VM or a container can help with many scenarios. Regardless, I was casually thinking about this for a while and have this to say. I think being able to run tests locally, on your machine, easily is very important. It is also important to let outside developers run those tests with minimal setup. Yes, I really like being able to spin up a screen session and work on my test there. But what I'm worried about is that (see the examples about files provider) by writings wrappers and helpers for my tests so that they run in a cwrapped environment is actually testing something different than what the real system would do. Yes. If we can do containers and VMs as portably, easy and fast to run, that would be better. The Docker container registry helps with that a lot: we can just publish the containers we built and pull/refresh them automatically with each run, and so could outside developers. At least as far as I understand it. I don't think there is a similar service for VM images, and I don't think we want to build our own. There is: https://atlas.hashicorp.com/boxes/search but of course, this would require the system running the test to have vagrant installed. Well, as long as it's present in all major distros and doesn't require much manual setup, we should be fine. Looking at how Cockpit runs their test, they simply publish qcow2 disk images on Internet and fetch them from there.. That's nice. I was thinking about something like this. How would we manage maintenance and updates, though? The images will probably be quite big and we'll need to implement some caching scheme. The latter part is easy, though, as long as clocks are more-or-less in sync. Even though Docker is still limiting, it is less limiting than the current setup. (Replying to Michal also here..) In my view, whether we use containers or VMs should be a detail as
[SSSD] Re: Should we use VMs or containers for (some) tests?
Hi, On 10/27/2016 04:20 PM, Jakub Hrozek wrote: Hi, I'm currently working on integration tests for the 'files' provider and during this work I started to feel we are pushing the boundaries around our test infrastructure already quite a bit. When SSSD talks over network to a server, then we're more or less okay, but for some parts of SSSD, like the files provider, we have to mock a lot of pieces and the end result is that we are testing something that resembles the target system, but probably has its own bugs. Additionally, we can't run some tests at all (anything against IPA) and I suspect we'll be running into this sort of a problem even more in the future. I agree that we are hitting the limits of current "tier" and we should start putting together infrastructure for a new one. So I'm interested in hearing what are other's thoughts on exploring how to run some of our tests in a privileged environment, either in a VM or in a container? IMO we should go for normal VMs. It will be the easiest way and also most accurately represents the real world scenarios. I am afraid that if build the test infrastructure with containers we will drift again too far away from the scenarios we want to simulate. Our current tests have the big advantage of being able to provision a test locally in a screen session, but maybe something similar would be possible by e.g. running a screen in a container and attaching to its tty.. And for "simple" tests like LDAP provider we could keep the current infrastructure around. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Milestone names
On 10/07/2016 12:20 PM, Jakub Hrozek wrote: Hi, for better or worse, our milestone and release planning is not great. We normally decide on what we want to work on for the next release and release new versions based on Fedora or RHEL releases (mostly because there is normally no other driver..if there are other projects or distributions who would like us to release on a different schedule, please just speak up!). The result is that our milestones don't really reflect the reality -- we have a huge milestone called "1.16" that is not really going to become 1.16, it's rather a set of tickets we'd like to work on "sometimes in the future". In addition, we have milestones that track bug fixing in released versions, currently 1.13.5 and 1.14.2 and the version currently under development, which is 1.15 at the moment. What I propose to make it clearer to outsiders what the expectations are is simply the following: - rename the current 1.16 bucket to "Future releases", maybe with a more explanatory note, like "Future releases - not planned for a particular date" - triage the milestones like 1.17 or 2.0, move tickets from there either to deferred or just close them - rename the "Deferred" milestone to "Patches welcome". I think this would make it clearer a bit in terms of what the sssd upstream is working on now and what we are working on next.. Comments? Thoughts? Ack. We can do the 1.17 and 2.0 (and maybe other milestones) triage on the next devel meeting. Or we can make a special BJ call for this. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
On 10/05/2016 04:48 PM, Petr Cech wrote: On 10/05/2016 04:39 PM, Michal Židek wrote: On 10/05/2016 04:30 PM, Petr Cech wrote: On 10/05/2016 04:18 PM, Michal Židek wrote: On 10/05/2016 03:47 PM, Philip Prindeville wrote: On Oct 5, 2016, at 7:18 AM, Michal Židek <mzi...@redhat.com> wrote: Hello Michal, I comment two things online. I agree with the comments. New test attached. 0002-DHASH-Add-check-based-unit-test.patch From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Wed, 5 Oct 2016 13:10:35 +0200 Subject: [PATCH 2/2] DHASH: Add check based unit test --- LGTM. ACK. Regards Pushed to ding-libs master: * d713c1f979ba3e046a6dd53f5262ae0cdbe53bc4 * f085b9b26f6e00cec4a0acf6896843436234ca4f Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
On 10/05/2016 05:43 PM, Jakub Hrozek wrote: On Wed, Oct 05, 2016 at 07:42:23AM -0600, Philip Prindeville wrote: On Oct 5, 2016, at 5:45 AM, Michal Židek <mzi...@redhat.com> wrote: ACK to the code from Philip. I amended the commit message to meet our style. I would like to push this together with at least some sanity tests. See the second patch. I am looking for someone from SSSD developers to review the tests. Michal PS: Btw. Philip, I am interested for what project are you using ding-libs. The project was an SSSD provider (proxy) for Tacacs+ accounting, authentication, and authorization. Wow, I'm intrigued and I think all of us want to help! Is that an in-house project or do you plan on open sourcing it? If the latter, I wonder if we could help with review, writing more docs or just about anything? I agree that this looks interesting. It would be good if the discussion about it took place here on the sssd devel-list, and maybe we could give a hand with something. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
On 10/05/2016 04:30 PM, Petr Cech wrote: On 10/05/2016 04:18 PM, Michal Židek wrote: On 10/05/2016 03:47 PM, Philip Prindeville wrote: On Oct 5, 2016, at 7:18 AM, Michal Židek <mzi...@redhat.com> wrote: I forgot to attach the patches. Again the first one is acked by me, the second needs a review. Michal Thanks for writing those tests. Minor comment, dhash_ut_check.c and the existing checks don’t have any negative tests, such as attempting to delete a non-existent key, or deleting an already deleted key… or entering a key which is already present. -Philip Good point. I added some delete operations to these tests. However these are just some sanity tests to cover the code that was changed. My intention was not to test everything here. New tests are attached. Michal Hello Michal, I comment two things online. I agree with the comments. New test attached. 0002-DHASH-Add-check-based-unit-test.patch From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Wed, 5 Oct 2016 13:10:35 +0200 Subject: [PATCH 2/2] DHASH: Add check based unit test --- Makefile.am| 14 dhash/dhash_ut_check.c | 210 + 2 files changed, 224 insertions(+) create mode 100644 dhash/dhash_ut_check.c diff --git a/Makefile.am b/Makefile.am index 65528a8..ca9710e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,12 +114,26 @@ libdhash_la_LDFLAGS = \ check_PROGRAMS += dhash_test dhash_example TESTS += dhash_test dhash_example +if HAVE_CHECK +check_PROGRAMS += dhash_ut_check +TESTS += dhash_ut_check +endif + + dhash_test_SOURCES = dhash/examples/dhash_test.c dhash_test_LDADD = libdhash.la dhash_example_SOURCES = dhash/examples/dhash_example.c dhash_example_LDADD = libdhash.la +dhash_ut_check_SOURCES = dhash/dhash_ut_check.c +dhash_ut_chech_CFLAGS = $(AM_CFLAGS) \ +$(CHECK_CFLAGS) \ +$(NULL) +dhash_ut_check_LDADD = libdhash.la \ + $(CHECK_LIBS) \ + $(NULL) + dist_examples_DATA += \ dhash/examples/dhash_test.c \ dhash/examples/dhash_example.c diff --git a/dhash/dhash_ut_check.c b/dhash/dhash_ut_check.c new file mode 100644 index 000..b4b36fa --- /dev/null +++ b/dhash/dhash_ut_check.c @@ -0,0 +1,210 @@ +/* +Authors: +Michal Zidek <mzi...@redhat.com> + +Copyright (C) 2016 Red Hat + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License +along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include "config.h" + +#include +#include +#include IMO, this is unnecessary. +#include +#include + +/* #define TRACE_LEVEL 7 */ +#define TRACE_HOME +#include "dhash.h" +#include "path_utils.h" IMO, this is unnecessary. + +#define HTABLE_SIZE 128 + +int verbose = 0; + +/* There must be no warnings generated during this test + * without having to cast the key value. */ +START_TEST(test_key_const_string) +{ +hash_table_t *htable; +int ret; +hash_value_t ret_val; +hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1}; +hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2}; +hash_key_t key = {.type = HASH_KEY_CONST_STRING, .c_str = "constant"}; + +ret = hash_create(HTABLE_SIZE, , NULL, NULL); +fail_unless(ret == 0); + +/* The table is empty, lookup should return error */ +ret = hash_lookup(htable, , _val); +fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND); + +/* Deleting with non-existing key should return error */ +ret = hash_delete(htable, ); +fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND); + +ret = hash_enter(htable, , _val1); +fail_unless(ret == 0); + +hash_lookup(htable, , _val); +fail_unless(ret == 0); +fail_unless(ret_val.i == 1); + +/* Overwrite the entry */ +ret = hash_enter(htable, , _val2); +fail_unless(ret == 0); + +hash_lookup(htable, , _val); +fail_unless(ret == 0); +fail_unless(ret_val.i == 2); + +ret = hash_delete(htable, ); +fail_unless(ret == 0); + +/* Delete again with the same key */ +ret = hash_delete(htable, ); +fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND); + +ret = hash_destroy(htable); +fail_unless(ret == 0); +} +END_TEST + +START_TEST(test_key_string) +{ +
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
On 10/05/2016 03:47 PM, Philip Prindeville wrote: On Oct 5, 2016, at 7:18 AM, Michal Židek <mzi...@redhat.com> wrote: I forgot to attach the patches. Again the first one is acked by me, the second needs a review. Michal Thanks for writing those tests. Minor comment, dhash_ut_check.c and the existing checks don’t have any negative tests, such as attempting to delete a non-existent key, or deleting an already deleted key… or entering a key which is already present. -Philip Good point. I added some delete operations to these tests. However these are just some sanity tests to cover the code that was changed. My intention was not to test everything here. New tests are attached. Michal >From 14618f9d49d784283894e228382a2a608d1a649e Mon Sep 17 00:00:00 2001 From: Philip Prindeville <phil...@fedoraproject.org> Date: Mon, 3 Oct 2016 15:19:58 +0200 Subject: [PATCH 1/2] DHASH: Add new key type HASH_KEY_CONST_STRING Add constant string as new key type. This key type is alternative to string key type and avoids unnecessary casts that may look dangerous (discarding const). --- dhash/dhash.c | 29 + dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..98439e8 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table) while (p != NULL) { q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); -if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +if (p->entry.key.type == HASH_KEY_STRING +|| p->entry.key.type == HASH_KEY_CONST_STRING) { +/* Internally we do not use constant memory for keys + * in hash table elements. */ +hfree(table, p->entry.key.str); } hfree(table, (char *)p); p = q; @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) element->entry.key.ul = key->ul; break; case HASH_KEY_STRING: -len = strlen(key->str)+1; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str) + 1; element->entry.key.str = halloc(table, len); if (element->entry.key.str == NULL) { hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); break; } @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) return error; } } -if (element->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)element->entry.key.str); +if (element->entry.key.type == HASH_KEY_STRING +|| element->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, element->entry.key.str); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dh
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
I forgot to attach the patches. Again the first one is acked by me, the second needs a review. Michal On 10/05/2016 01:45 PM, Michal Židek wrote: ACK to the code from Philip. I amended the commit message to meet our style. I would like to push this together with at least some sanity tests. See the second patch. I am looking for someone from SSSD developers to review the tests. Michal PS: Btw. Philip, I am interested for what project are you using ding-libs. On 09/30/2016 11:55 PM, Philip Prindeville wrote: From: Philip Prindeville <phil...@fedoraproject.org> Add c_str as a "const char *" to the hash_key_t. Redux per comments from Michal Zidek. --- dhash/dhash.c | 29 + dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..98439e8 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table) while (p != NULL) { q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); -if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +if (p->entry.key.type == HASH_KEY_STRING +|| p->entry.key.type == HASH_KEY_CONST_STRING) { +/* Internally we do not use constant memory for keys + * in hash table elements. */ +hfree(table, p->entry.key.str); } hfree(table, (char *)p); p = q; @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) element->entry.key.ul = key->ul; break; case HASH_KEY_STRING: -len = strlen(key->str)+1; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str) + 1; element->entry.key.str = halloc(table, len); if (element->entry.key.str == NULL) { hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); break; } @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) return error; } } -if (element->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)element->entry.key.str); +if (element->entry.key.type == HASH_KEY_STRING +|| element->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, element->entry.key.str); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t; typedef enum { HASH_KEY_STRING, -HASH_KEY_ULONG +HASH_KEY_ULONG, +HASH_KEY_CONST_STRING } hash_key_enum; typedef enum @@ -137,6 +138,7 @@ typedef struct hash_key_t { hash_key_enum type; union { char *str; +const char *c_str; unsigne
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
ACK to the code from Philip. I amended the commit message to meet our style. I would like to push this together with at least some sanity tests. See the second patch. I am looking for someone from SSSD developers to review the tests. Michal PS: Btw. Philip, I am interested for what project are you using ding-libs. On 09/30/2016 11:55 PM, Philip Prindeville wrote: From: Philip PrindevilleAdd c_str as a "const char *" to the hash_key_t. Redux per comments from Michal Zidek. --- dhash/dhash.c | 29 + dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..98439e8 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table) while (p != NULL) { q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); -if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +if (p->entry.key.type == HASH_KEY_STRING +|| p->entry.key.type == HASH_KEY_CONST_STRING) { +/* Internally we do not use constant memory for keys + * in hash table elements. */ +hfree(table, p->entry.key.str); } hfree(table, (char *)p); p = q; @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) element->entry.key.ul = key->ul; break; case HASH_KEY_STRING: -len = strlen(key->str)+1; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str) + 1; element->entry.key.str = halloc(table, len); if (element->entry.key.str == NULL) { hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); break; } @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) return error; } } -if (element->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)element->entry.key.str); +if (element->entry.key.type == HASH_KEY_STRING +|| element->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, element->entry.key.str); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t; typedef enum { HASH_KEY_STRING, -HASH_KEY_ULONG +HASH_KEY_ULONG, +HASH_KEY_CONST_STRING } hash_key_enum; typedef enum @@ -137,6 +138,7 @@ typedef struct hash_key_t { hash_key_enum type; union { char *str; +const char *c_str; unsigned long ul; }; } hash_key_t; diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c index 303e9f8..7613e23 100644 --- a/dhash/examples/dhash_test.c +++ b/dhash/examples/dhash_test.c @@ -64,6 +64,9 @@ static char *key_string(hash_key_t
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
Hello Philip, please read the comments inline. I also attached git diff to demonstrate what I mean to this email. From: Philip PrindevilleAdd c_str as a "const char *" to the hash_key_t. --- dhash/dhash.c | 35 ++- dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..5f9f631 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -44,6 +44,7 @@ #include #include #include +#include Why did you include stdint? #include #include "dhash.h" @@ -70,6 +71,9 @@ } \ } while(0) +#define discard_const(ptr) ((void *)((intptr_t)(ptr))) +#define discard_const_p(type, ptr) ((type *)discard_const(ptr)) + I do not think it is good to use the discard_const macro for this case. See further comments below. /*/ /** Internal Type Definitions / /*/ @@ -176,7 +180,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +188,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +221,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +254,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -671,7 +683,9 @@ int hash_destroy(hash_table_t *table) q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +hfree(table, p->entry.key.str); +} else if (p->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, discard_const(p->entry.key.c_str)); } hfree(table, (char *)p); p = q; @@ -978,7 +992,16 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); +break; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str)+1; +element->entry.key.c_str = halloc(table, len); +if (element->entry.key.c_str == NULL) { +hfree(table, element); +return HASH_ERROR_NO_MEMORY; +} +memcpy(discard_const(element->entry.key.c_str), key->c_str, len); break; We do an internal copy here. We do not need to special case the HASH_KEY_CONST_STRING. Internally we can always access the key in the hash table with key.str and the key from the user with key.c_str no matter if the user chose constant or non constant key. We will only modify the internal part during free operation and do not touch the key from user. } @@ -1071,7 +1094,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) } } if (element->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)element->entry.key.str); +hfree(table, element->entry.key.str); +} else if (element->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, discard_const(element->entry.key.c_str)); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
Hi! Ding-libs originates in SSSD project and shares a lot of SSSD infrastructure. Mailing list for ding-libs and SSSD is the same, You can subscribe to the mailing list here: https://lists.fedorahosted.org/admin/lists/sssd-devel.lists.fedorahosted.org/ Ding-libs and SSSD also share the ticketing system and wiki (trac): https://fedorahosted.org/sssd/ In order to subscribe to the mailing list or login to the trac instance, you will need a FAS account. This account is used to login to most of fedorahosted services (including for example copr). Michal On 09/29/2016 07:43 PM, Philip Prindeville wrote: Is there a ding-libs website? I wanted to subscribe to the mailing list and read the archives but fedorahosted.org only seems to point to the git repo… nothing about a ding-libs general website, etc. On Sep 29, 2016, at 4:50 AM, Michal Židek <mzi...@redhat.com> wrote: Moving this to sssd-devel list. So that other developers can see the patch and review process. I will start the review after today's meeting. Michal (Lukas, Stephen, I put you two to CC only because you were also original recipients of the mail, I will not CC you further) On 09/28/2016 03:00 AM, Philip Prindeville wrote: From: Philip Prindeville <phil...@fedoraproject.org> Add c_str as a "const char *" to the hash_key_t. --- dhash/dhash.c | 35 ++- dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..5f9f631 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -44,6 +44,7 @@ #include #include #include +#include #include #include "dhash.h" @@ -70,6 +71,9 @@ } \ } while(0) +#define discard_const(ptr) ((void *)((intptr_t)(ptr))) +#define discard_const_p(type, ptr) ((type *)discard_const(ptr)) + /*/ /** Internal Type Definitions / /*/ @@ -176,7 +180,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +188,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +221,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +254,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -671,7 +683,9 @@ int hash_destroy(hash_table_t *table) q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +hfree(table, p->entry.key.str); +} else if (p->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, discard_const(p->entry.key.c_str)); } hfree(table, (char *)p); p = q; @@ -978,7 +992,16 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); +break; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str)+1; +element->entry.key.c_str = halloc(table, len); +if (element->entry.key.c_str == NULL) { +hfree(table, element); +return HASH_ERROR_NO_MEMORY; +} +memcpy(discard_const(element->
[SSSD] Re: [PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys
Moving this to sssd-devel list. So that other developers can see the patch and review process. I will start the review after today's meeting. Michal (Lukas, Stephen, I put you two to CC only because you were also original recipients of the mail, I will not CC you further) On 09/28/2016 03:00 AM, Philip Prindeville wrote: From: Philip PrindevilleAdd c_str as a "const char *" to the hash_key_t. --- dhash/dhash.c | 35 ++- dhash/dhash.h | 4 +++- dhash/examples/dhash_test.c | 3 +++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/dhash/dhash.c b/dhash/dhash.c index 45ee0cf..5f9f631 100644 --- a/dhash/dhash.c +++ b/dhash/dhash.c @@ -44,6 +44,7 @@ #include #include #include +#include #include #include "dhash.h" @@ -70,6 +71,9 @@ } \ } while(0) +#define discard_const(ptr) ((void *)((intptr_t)(ptr))) +#define discard_const_p(type, ptr) ((type *)discard_const(ptr)) + /*/ /** Internal Type Definitions / /*/ @@ -176,7 +180,7 @@ static void sys_free_wrapper(void *ptr, void *pvt) static address_t convert_key(hash_key_t *key) { address_t h; -unsigned char *k; +const unsigned char *k; switch(key->type) { case HASH_KEY_ULONG: @@ -184,7 +188,12 @@ static address_t convert_key(hash_key_t *key) break; case HASH_KEY_STRING: /* Convert string to integer */ -for (h = 0, k = (unsigned char *) key->str; *k; k++) +for (h = 0, k = (const unsigned char *) key->str; *k; k++) +h = h * PRIME_1 ^ (*k - ' '); +break; +case HASH_KEY_CONST_STRING: +/* Convert string to integer */ +for (h = 0, k = (const unsigned char *) key->c_str; *k; k++) h = h * PRIME_1 ^ (*k - ' '); break; default: @@ -212,6 +221,7 @@ static bool is_valid_key_type(hash_key_enum key_type) switch(key_type) { case HASH_KEY_ULONG: case HASH_KEY_STRING: +case HASH_KEY_CONST_STRING: return true; default: return false; @@ -244,6 +254,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b) return (a->ul == b->ul); case HASH_KEY_STRING: return (strcmp(a->str, b->str) == 0); +case HASH_KEY_CONST_STRING: +return (strcmp(a->c_str, b->c_str) == 0); } return false; } @@ -671,7 +683,9 @@ int hash_destroy(hash_table_t *table) q = p->next; hdelete_callback(table, HASH_TABLE_DESTROY, >entry); if (p->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)p->entry.key.str); +hfree(table, p->entry.key.str); +} else if (p->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, discard_const(p->entry.key.c_str)); } hfree(table, (char *)p); p = q; @@ -978,7 +992,16 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value) hfree(table, element); return HASH_ERROR_NO_MEMORY; } -memcpy((void *)element->entry.key.str, key->str, len); +memcpy(element->entry.key.str, key->str, len); +break; +case HASH_KEY_CONST_STRING: +len = strlen(key->c_str)+1; +element->entry.key.c_str = halloc(table, len); +if (element->entry.key.c_str == NULL) { +hfree(table, element); +return HASH_ERROR_NO_MEMORY; +} +memcpy(discard_const(element->entry.key.c_str), key->c_str, len); break; } @@ -1071,7 +1094,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key) } } if (element->entry.key.type == HASH_KEY_STRING) { -hfree(table, (char *)element->entry.key.str); +hfree(table, element->entry.key.str); +} else if (element->entry.key.type == HASH_KEY_CONST_STRING) { +hfree(table, discard_const(element->entry.key.c_str)); } hfree(table, element); return HASH_SUCCESS; diff --git a/dhash/dhash.h b/dhash/dhash.h index baa0d6a..c36ab79 100644 --- a/dhash/dhash.h +++ b/dhash/dhash.h @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t; typedef enum { HASH_KEY_STRING, -HASH_KEY_ULONG +HASH_KEY_ULONG, +HASH_KEY_CONST_STRING } hash_key_enum; typedef enum @@ -137,6 +138,7 @@ typedef struct hash_key_t { hash_key_enum type; union { char *str; +
[SSSD] Re: [PATCH] SSSDConfig: Do not fail with nonexisting domains/services
On 09/22/2016 05:36 PM, Lukas Slebodnik wrote: On (22/09/16 17:30), Michal Židek wrote: On 09/22/2016 10:07 AM, Lukas Slebodnik wrote: On (21/09/16 14:20), Lukas Slebodnik wrote: ehlo, Almost oneliner i there would not be a unit test :-) LS Bump LS The python tests fails in distcheck: http://sssd-ci.duckdns.org/logs/job/53/88/summary.html I forgot to add new testconfig to tarball. Updated patch is attached. LS Conditional ACK if this CI passes: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/5389/ Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH] SSSDConfig: Do not fail with nonexisting domains/services
On 09/22/2016 10:07 AM, Lukas Slebodnik wrote: On (21/09/16 14:20), Lukas Slebodnik wrote: ehlo, Almost oneliner i there would not be a unit test :-) LS Bump LS The python tests fails in distcheck: http://sssd-ci.duckdns.org/logs/job/53/88/summary.html Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH] SPEC: Rename python packages using macro %python_provide
On 09/22/2016 10:23 AM, Michal Židek wrote: On 09/22/2016 10:08 AM, Lukas Slebodnik wrote: On (15/09/16 20:44), Lukas Slebodnik wrote: On (15/09/16 14:08), Lukas Slebodnik wrote: On (05/07/16 13:39), Lukas Slebodnik wrote: ehlo, SSSD python packages were renamed in fedora few months ago. python-* -> python2-* But we didn't rename packages in upstream spec file and therefore upgrade from fedora 24 -> sssd master is not possible. Attached patch shoudl fix the issue. BTW here are provides and obsoletes for current master sh$ rpm -qp --provides python-libipa_hbac-1.13.92-0.fc24.x86_64.rpm libipa_hbac-python = 1.13.92-0.fc24 python-libipa_hbac = 1.13.92-0.fc24 python-libipa_hbac(x86-64) = 1.13.92-0.fc24 sh$ rpm -qp --obsoletes python-libipa_hbac-1.13.92-0.fc24.x86_64.rpm libipa_hbac-python < 1.12.90 and after renaming sh$ rpm -qp --provides python2-libipa_hbac-1.13.92-0.el6.x86_64.rpm libipa_hbac-python = 1.13.92-0.el6 python-libipa_hbac = 1.13.92-0.el6 python2-libipa_hbac = 1.13.92-0.el6 python2-libipa_hbac(x86-64) = 1.13.92-0.el6 sh$ rpm -qp --obsoletes python2-libipa_hbac-1.13.92-0.el6.x86_64.rpm libipa_hbac-python < 1.12.90 python-libipa_hbac < 1.13.92-0.el6 Attached is an updated patch. http://sssd-ci.duckdns.org/logs-test/job/4/51/summary.html I sent wrong patch in previous mail. LS From 60f504d1123a3458a85797b7063cfee8202e5cf4 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik <lsleb...@redhat.com> Date: Wed, 14 Sep 2016 14:31:29 +0200 Subject: [PATCH] SPEC: Rename python packages using macro %python_provide Fedora and epel contains macro %python_provide for simpler renaming of python packages. It will generate correct provides and obsoletes. Bump LS Works for me. Waiting for the CI to give formal ack. Michal ACK. CI: http://sssd-ci.duckdns.org/logs/job/53/85/summary.html Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH] SPEC: Rename python packages using macro %python_provide
On 09/22/2016 10:08 AM, Lukas Slebodnik wrote: On (15/09/16 20:44), Lukas Slebodnik wrote: On (15/09/16 14:08), Lukas Slebodnik wrote: On (05/07/16 13:39), Lukas Slebodnik wrote: ehlo, SSSD python packages were renamed in fedora few months ago. python-* -> python2-* But we didn't rename packages in upstream spec file and therefore upgrade from fedora 24 -> sssd master is not possible. Attached patch shoudl fix the issue. BTW here are provides and obsoletes for current master sh$ rpm -qp --provides python-libipa_hbac-1.13.92-0.fc24.x86_64.rpm libipa_hbac-python = 1.13.92-0.fc24 python-libipa_hbac = 1.13.92-0.fc24 python-libipa_hbac(x86-64) = 1.13.92-0.fc24 sh$ rpm -qp --obsoletes python-libipa_hbac-1.13.92-0.fc24.x86_64.rpm libipa_hbac-python < 1.12.90 and after renaming sh$ rpm -qp --provides python2-libipa_hbac-1.13.92-0.el6.x86_64.rpm libipa_hbac-python = 1.13.92-0.el6 python-libipa_hbac = 1.13.92-0.el6 python2-libipa_hbac = 1.13.92-0.el6 python2-libipa_hbac(x86-64) = 1.13.92-0.el6 sh$ rpm -qp --obsoletes python2-libipa_hbac-1.13.92-0.el6.x86_64.rpm libipa_hbac-python < 1.12.90 python-libipa_hbac < 1.13.92-0.el6 Attached is an updated patch. http://sssd-ci.duckdns.org/logs-test/job/4/51/summary.html I sent wrong patch in previous mail. LS From 60f504d1123a3458a85797b7063cfee8202e5cf4 Mon Sep 17 00:00:00 2001 From: Lukas SlebodnikDate: Wed, 14 Sep 2016 14:31:29 +0200 Subject: [PATCH] SPEC: Rename python packages using macro %python_provide Fedora and epel contains macro %python_provide for simpler renaming of python packages. It will generate correct provides and obsoletes. Bump LS Works for me. Waiting for the CI to give formal ack. Michal ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: fedorahosted.org sunset
On 09/16/2016 01:19 PM, Jakub Hrozek wrote: On Fri, Sep 16, 2016 at 12:50:54PM +0200, Jakub Hrozek wrote: The first step imo is -- define what exactly we miss from pagure's tracker. For me it's: - milestones Apparently, pagure has a creative way to deal with milestones: https://docs.pagure.org/pagure/usage/roadmap.html This maps our current processes pretty well. New tickets have no roadmap tags, so they are "Unplanned" (NEEDS_TRIAGE), and adding them to a certain milestone is a matter of setting a tag. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: fedorahosted.org sunset
On 09/16/2016 11:09 AM, Jakub Hrozek wrote: Hi, fedorahosted.org is being decomissioned: https://lists.fedoraproject.org/archives/list/annou...@lists.fedoraproject.org/thread/RLL3LFUPLYMAUKGZ5B3O64XKJXBT24KZ/ so we need to find a new home for SSSD.. I wanted to ask: 1) anyone from the core development team who is interested in finding a new home to raise a hand, we need someone to "own" this work 2) anyone from outside or inside the team who has an opinion to voice it :) so far we haven't even thought about the requirements in too much detail, though.. In the meantime I just wrote up what fields we use from Trac and therefore what info we need to keep in the new system: https://fedorahosted.org/sssd/wiki/ticket_fields ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org I would love to see SSSD on Pagure. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] ini_augment: Use full path when reporting pattern mismatch
On 09/13/2016 04:53 PM, Lukas Slebodnik wrote: On (01/09/16 17:35), Michal Židek wrote: On 09/01/2016 05:26 PM, Dmitri Pal wrote: Hello, I do not like either of the versions of the patch. It is OK to use path_concat instead of snprintf. The whole point of not using it was to simplify the code and not have to check yet another error clause. But using path_concat is fine. The thing that I do not like is that in the current code it is done in both branches of the if clause. I think we should move the path_concat call above the |/* Match names */ match = ini_aug_match_name(entry->d_name, ra_regex);| Logic would be: - create the full path - check errors - call the function above - use full name in both clauses as needed That approach IMO would be cleaner. -- Thank you, Dmitri Pal Engineering Director, Identity Management and Platform Security Red Hat, Inc. Indeed. New patch is attached. Michal From 5079c97723e2865b41daa4bebe6b43e0e6685fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Thu, 1 Sep 2016 12:59:07 +0200 Subject: [PATCH] ini_augment: Use full path when reporting pattern mismatch We used full path name when reporting access check failures but only write filename when reporting on pattern mismatch. This inconsistency does not look good when the messages are used in sssctl config-check. Resolves: https://fedorahosted.org/sssd/ticket/3166 --- ini/ini.d/merge.validator | 16 ini/ini_augment.c | 15 ++- 2 files changed, 18 insertions(+), 13 deletions(-) ACK LS ding-libs master: commit 72ca1a3552fdc33b7fa7e832ecfc235dd685a7e7 Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] ini_augment: Use full path when reporting pattern mismatch
On 09/01/2016 05:26 PM, Dmitri Pal wrote: Hello, I do not like either of the versions of the patch. It is OK to use path_concat instead of snprintf. The whole point of not using it was to simplify the code and not have to check yet another error clause. But using path_concat is fine. The thing that I do not like is that in the current code it is done in both branches of the if clause. I think we should move the path_concat call above the |/* Match names */ match = ini_aug_match_name(entry->d_name, ra_regex);| Logic would be: - create the full path - check errors - call the function above - use full name in both clauses as needed That approach IMO would be cleaner. -- Thank you, Dmitri Pal Engineering Director, Identity Management and Platform Security Red Hat, Inc. Indeed. New patch is attached. Michal >From 5079c97723e2865b41daa4bebe6b43e0e6685fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Thu, 1 Sep 2016 12:59:07 +0200 Subject: [PATCH] ini_augment: Use full path when reporting pattern mismatch We used full path name when reporting access check failures but only write filename when reporting on pattern mismatch. This inconsistency does not look good when the messages are used in sssctl config-check. Resolves: https://fedorahosted.org/sssd/ticket/3166 --- ini/ini.d/merge.validator | 16 ini/ini_augment.c | 15 ++- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/ini/ini.d/merge.validator b/ini/ini.d/merge.validator index dc6d6aa..1defe8e 100644 --- a/ini/ini.d/merge.validator +++ b/ini/ini.d/merge.validator @@ -1,11 +1,11 @@ -File merge.validator did not match provided patterns. Skipping. -File real8.conf did not match provided patterns. Skipping. -File new_line.conf did not match provided patterns. Skipping. -File real32be.conf did not match provided patterns. Skipping. -File real32le.conf did not match provided patterns. Skipping. -File real16be.conf did not match provided patterns. Skipping. -File real16le.conf did not match provided patterns. Skipping. -File foo.conf.in did not match provided patterns. Skipping. +File %s%s/merge.validator did not match provided patterns. Skipping. +File %s%s/real8.conf did not match provided patterns. Skipping. +File %s%s/new_line.conf did not match provided patterns. Skipping. +File %s%s/real32be.conf did not match provided patterns. Skipping. +File %s%s/real32le.conf did not match provided patterns. Skipping. +File %s%s/real16be.conf did not match provided patterns. Skipping. +File %s%s/real16le.conf did not match provided patterns. Skipping. +File %s%s/foo.conf.in did not match provided patterns. Skipping. Errors detected while parsing: %s%s/comment.conf. Error (9) on line 22: Invalid space character at the beginning of the line. Error (9) on line 24: Invalid space character at the beginning of the line. diff --git a/ini/ini_augment.c b/ini/ini_augment.c index ea3d3da..81ff50b 100644 --- a/ini/ini_augment.c +++ b/ini/ini_augment.c @@ -437,13 +437,18 @@ static int ini_aug_construct_list(char *dirname , INI_PARENT_DIR, sizeof(INI_PARENT_DIR)) == 0)) continue; +error = path_concat(fullname, PATH_MAX, dirname, entry->d_name); +if (error != EOK) { +TRACE_ERROR_NUMBER("path_concat failed.", ret); +ref_array_destroy(ra_regex); +closedir(dir); +free(entry); +return error; +} + /* Match names */ match = ini_aug_match_name(entry->d_name, ra_regex); - if (match) { - -snprintf(fullname, PATH_MAX, "%s/%s", dirname, entry->d_name); - if(ini_check_file_perm(fullname, check_perm, ra_err)) { /* Dup name and add to the array */ @@ -473,7 +478,7 @@ static int ini_aug_construct_list(char *dirname , ini_aug_add_string(ra_err, "File %s did not match provided patterns." " Skipping.", - entry->d_name); + fullname); } } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] ini_augment: Use full path when reporting pattern mismatch
Hi, see the attached ding-libs patch for ticket #3166. This is how sssctl config-check prints the merging issues without this patch: Messages generated during configuration merging: 2 File blaa did not match provided patterns. Skipping. File /etc/sssd/conf.d/blaa.conf did not pass access check. Skipping. And with the patch: Messages generated during configuration merging: 2 File /etc/sssd/conf.d/blaa did not match provided patterns. Skipping. File /etc/sssd/conf.d/blaa.conf did not pass access check. Skipping. Michal >From 85f54a943d65bd5e9c7863877288ddede8496154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Thu, 1 Sep 2016 12:59:07 +0200 Subject: [PATCH] ini_augment: Use full path when reporting pattern mismatch We used full path name when reporting access check failures but only write filename when reporting on pattern mismatch. This inconsistency does not look good when the messages are used in sssctl config-check. Resolves: https://fedorahosted.org/sssd/ticket/3166 --- ini/ini.d/merge.validator | 16 ini/ini_augment.c | 3 ++- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/ini/ini.d/merge.validator b/ini/ini.d/merge.validator index dc6d6aa..1defe8e 100644 --- a/ini/ini.d/merge.validator +++ b/ini/ini.d/merge.validator @@ -1,11 +1,11 @@ -File merge.validator did not match provided patterns. Skipping. -File real8.conf did not match provided patterns. Skipping. -File new_line.conf did not match provided patterns. Skipping. -File real32be.conf did not match provided patterns. Skipping. -File real32le.conf did not match provided patterns. Skipping. -File real16be.conf did not match provided patterns. Skipping. -File real16le.conf did not match provided patterns. Skipping. -File foo.conf.in did not match provided patterns. Skipping. +File %s%s/merge.validator did not match provided patterns. Skipping. +File %s%s/real8.conf did not match provided patterns. Skipping. +File %s%s/new_line.conf did not match provided patterns. Skipping. +File %s%s/real32be.conf did not match provided patterns. Skipping. +File %s%s/real32le.conf did not match provided patterns. Skipping. +File %s%s/real16be.conf did not match provided patterns. Skipping. +File %s%s/real16le.conf did not match provided patterns. Skipping. +File %s%s/foo.conf.in did not match provided patterns. Skipping. Errors detected while parsing: %s%s/comment.conf. Error (9) on line 22: Invalid space character at the beginning of the line. Error (9) on line 24: Invalid space character at the beginning of the line. diff --git a/ini/ini_augment.c b/ini/ini_augment.c index ea3d3da..cced53b 100644 --- a/ini/ini_augment.c +++ b/ini/ini_augment.c @@ -470,10 +470,11 @@ static int ini_aug_construct_list(char *dirname , } } else { +snprintf(fullname, PATH_MAX, "%s/%s", dirname, entry->d_name); ini_aug_add_string(ra_err, "File %s did not match provided patterns." " Skipping.", - entry->d_name); + fullname); } } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] GPO: Cat vals with same key from different GPOs
On 09/01/2016 10:31 AM, Jakub Hrozek wrote: On Thu, Sep 01, 2016 at 09:56:33AM +0200, Michal Židek wrote: On 08/31/2016 07:49 PM, Stephen Gallagher wrote: On 08/31/2016 01:24 PM, Simo Sorce wrote: On Wed, 2016-08-31 at 17:41 +0200, Michal Židek wrote: Hi, here is patch for ticket #3161. See more in the ticket description. I was thinking why we originally replaced the lists and I think it comes from confusion on how we handle the same keys in single GPO ini file, however that is handled by libini not by SSSD. Sorry to come to this late, but do you have a documentation reference that says that merging is the correct behavior ? Merging is not (nor has it ever been) the correct behavior. The way that AD GPOs work is with a strict hierarchy. I'd have to double-check the code to remember which order it is, but I *think* it's: Domain is overridden by site which is overridden by individual machine. Basically, if I have a GPO on domain EXAMPLE.COM that include SeInteractiveLogonRight=foo and another GPO on machine client.example.com that has SeInteractiveLogonRight="bar, baz", then "bar, baz" completely replaces "foo" for that machine. It does not merge in any way. This is the reason that we switched to storing this information in the LDB cache; we were able to do the processing the same way Windows does with the registry, by having each iteration through the hierarchy replace the previous one. Again, no merging should happen here. I forgot a lot about how multiple GPOs are supposed to be merged but I seem to recall there may be a policy that actually controls how merging is done. CCing Günther who has worked around GPO processing a few years ago. Simo. Thank you everyone for the input. So either we have a different bug or the configuration I am working with is wrong. Does anyone know, how is the precedence of GPOs (that are defined on the same level, for example for domain.test) determined? I guess the tree structure of LDB is not helping much here. When I look in the Group policy management window at the GPOs, it looks like the ones I created later have higher precedence (are applied later). But I am not sure if this is a coincidence. Michal, I think it would help if you describe in detail how the affected GPO configuration looks like. Simplified it looks like this, addomain.test has 2 GPOs: - precedence: Name: Location: - 1 Default Domain Policy addomain.test 2 my-special-gpo addomain.test Both are enabled and enforced. Both contain lists of SIDs that are allowed to login remotely (group SIDs). Let's say Default Domain Policy contains SID of group1 and my-special-gpo contains SID of group2. Now userA is logging into machine, both GPOs are downloaded by SSSD and applicable for the userA. Should the userA be allowed to login if he is member of grpoup1 or group2? Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] GPO: Cat vals with same key from different GPOs
On 08/31/2016 07:49 PM, Stephen Gallagher wrote: On 08/31/2016 01:24 PM, Simo Sorce wrote: On Wed, 2016-08-31 at 17:41 +0200, Michal Židek wrote: Hi, here is patch for ticket #3161. See more in the ticket description. I was thinking why we originally replaced the lists and I think it comes from confusion on how we handle the same keys in single GPO ini file, however that is handled by libini not by SSSD. Sorry to come to this late, but do you have a documentation reference that says that merging is the correct behavior ? Merging is not (nor has it ever been) the correct behavior. The way that AD GPOs work is with a strict hierarchy. I'd have to double-check the code to remember which order it is, but I *think* it's: Domain is overridden by site which is overridden by individual machine. Basically, if I have a GPO on domain EXAMPLE.COM that include SeInteractiveLogonRight=foo and another GPO on machine client.example.com that has SeInteractiveLogonRight="bar, baz", then "bar, baz" completely replaces "foo" for that machine. It does not merge in any way. This is the reason that we switched to storing this information in the LDB cache; we were able to do the processing the same way Windows does with the registry, by having each iteration through the hierarchy replace the previous one. Again, no merging should happen here. I forgot a lot about how multiple GPOs are supposed to be merged but I seem to recall there may be a policy that actually controls how merging is done. CCing Günther who has worked around GPO processing a few years ago. Simo. Thank you everyone for the input. So either we have a different bug or the configuration I am working with is wrong. Does anyone know, how is the precedence of GPOs (that are defined on the same level, for example for domain.test) determined? I guess the tree structure of LDB is not helping much here. When I look in the Group policy management window at the GPOs, it looks like the ones I created later have higher precedence (are applied later). But I am not sure if this is a coincidence. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] GPO: Cat vals with same key from different GPOs
Hi, here is patch for ticket #3161. See more in the ticket description. I was thinking why we originally replaced the lists and I think it comes from confusion on how we handle the same keys in single GPO ini file, however that is handled by libini not by SSSD. Michal >From f1a54ec427fe109c8c166e76615eef2d4d83433b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Tue, 30 Aug 2016 20:03:36 +0200 Subject: [PATCH] GPO: Cat vals with same key from different GPOs Concatenate values from different GPO files that have the same key. This allows to specify GPO deny and allow rules across several GPO files. Previously we replaced values with the same key which caused access denials even to users that were supposed login. Moreover, if the deny rules were specified across several GPO files, this bug could allow users to log into machines they were not supposed to, because we did not create the whole list of denied SIDs and only worked with the part specified in the last processed GPO file. Resolves: https://fedorahosted.org/sssd/ticket/3161 --- src/db/sysdb_gpo.c | 31 +++ src/tests/sysdb-tests.c | 14 ++ 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/db/sysdb_gpo.c b/src/db/sysdb_gpo.c index e5af91b..583ba86 100644 --- a/src/db/sysdb_gpo.c +++ b/src/db/sysdb_gpo.c @@ -391,6 +391,7 @@ sysdb_gpo_store_gpo_result_setting(struct sss_domain_info *domain, size_t count; bool in_transaction = false; TALLOC_CTX *tmp_ctx; +const char *old_value = NULL; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) return ENOMEM; @@ -479,22 +480,20 @@ sysdb_gpo_store_gpo_result_setting(struct sss_domain_info *domain, goto done; } -lret = ldb_msg_add_fmt(update_msg, ini_key, "%s", ini_value); -if (lret != LDB_SUCCESS) { -ret = sysdb_error_to_errno(lret); -goto done; -} -} else { -/* If the value is NULL, we need to remove it from the cache */ -DEBUG(SSSDBG_TRACE_FUNC, "Removing setting: key [%s]\n", ini_key); - -/* Update the policy setting */ -lret = ldb_msg_add_empty(update_msg, ini_key, - LDB_FLAG_MOD_DELETE, - NULL); -if (lret != LDB_SUCCESS) { -ret = sysdb_error_to_errno(lret); -goto done; +old_value = ldb_msg_find_attr_as_string(msgs[0], ini_key, NULL); +if (old_value) { +/* Concatenate the old and new values */ +lret = ldb_msg_add_fmt(update_msg, ini_key, "%s,%s", old_value, ini_value); +if (lret != LDB_SUCCESS) { +ret = sysdb_error_to_errno(lret); +goto done; +} +} else { +lret = ldb_msg_add_fmt(update_msg, ini_key, "%s", ini_value); +if (lret != LDB_SUCCESS) { +ret = sysdb_error_to_errno(lret); +goto done; +} } } diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index d145001..72a8dd0 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -6341,7 +6341,7 @@ START_TEST(test_gpo_result) ck_assert_int_eq(ret, EOK); fail_unless(value == NULL); -/* Updating replaces the original value */ +/* Updating concatenates the original and the new value */ ret = sysdb_gpo_store_gpo_result_setting(test_ctx->domain, allow_key, "allow_val2"); ck_assert_int_eq(ret, EOK); @@ -6349,17 +6349,7 @@ START_TEST(test_gpo_result) ret = sysdb_gpo_get_gpo_result_setting(test_ctx, test_ctx->domain, allow_key, ); ck_assert_int_eq(ret, EOK); -ck_assert_str_eq(value, "allow_val2"); - -/* NULL removes the value completely */ -ret = sysdb_gpo_store_gpo_result_setting(test_ctx->domain, - allow_key, NULL); -ck_assert_int_eq(ret, EOK); - -ret = sysdb_gpo_get_gpo_result_setting(test_ctx, test_ctx->domain, - allow_key, ); -ck_assert_int_eq(ret, EOK); -fail_unless(value == NULL); +ck_assert_str_eq(value, "allow_val1,allow_val2"); /* Delete the result */ ret = sysdb_gpo_delete_gpo_result_object(test_ctx, test_ctx->domain); -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH 2/2] sdap: Skip exact duplicates when extending maps
On 08/10/2016 08:36 PM, Lukas Slebodnik wrote: On (10/08/16 17:41), Michal Židek wrote: Hi, see the attached patch. I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr). When we try to add entry to the map that already exists in the map, then without this patch we will fail. With this patch, we only fail if the newly added extension would redefine already existing entry in the map. Otherwise it is just skipped without a failure (we just skip adding what is already there). I created simple CI test for this (first patch). Michal From 932745aaadd7c2a73e817bfd685fbe1aaa462cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Wed, 10 Aug 2016 16:40:45 +0200 Subject: [PATCH 1/2] CI: Test extra attributes duplicate Regresion test for ticket #3120 Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/tests/intg/ldap_test.py | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 84f7f2d..b8315e5 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -292,6 +292,28 @@ def sanity_rfc2307_bis(request, ldap_conn): return None +@pytest.fixture +def just_start_ldap(request, ldap_conn): +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) +create_ldap_fixture(request, ldap_conn, ent_list) + +conf = \ +format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \ +unindent("""\ +[domain/LDAP] +ldap_user_extra_attrs = mail +""").format(**locals()) + +create_conf_fixture(request, conf) +create_sssd_cleanup(request) +return None + + +def test_extra_attribute_already_exists(ldap_conn, just_start_ldap): +if subprocess.call(["sssd", "-D", "-f"]) != 0: +raise Exception("sssd start failed") + + def test_regression_ticket2163(ldap_conn, simple_rfc2307): ent.assert_passwd_by_name( 'usr\\001', -- 2.5.0 From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail. We should only fail if we try to change purpose of already used sysdb attribute. Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/providers/ldap/sdap.c | 41 +++-- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; } -static bool is_sysdb_duplicate(struct sdap_attr_map *map, - int num_entries, - const char *sysdb_attr) +/* _already_in_map is set to true if the attribute + * already exists in the map and is used for the same + * LDAP attribute. + * + * _conflicts_with_map is set to true if the attribute + * already exists in map, but is used for different + * LDAP attribute. + * */ +static void check_duplicate(struct sdap_attr_map *map, +int num_entries, +const char *sysdb_attr, +const char *ldap_attr, +bool *_already_in_map, +bool *_conflicts_with_map) { This function has 3 output boolean argumets: It would be better to return enum instead of adding new parametrs. LS Ok, attached is version with enum. Michal >From 932745aaadd7c2a73e817bfd685fbe1aaa462cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Wed, 10 Aug 2016 16:40:45 +0200 Subject: [PATCH 1/2] CI: Test extra attributes duplicate Regresion test for ticket #3120 Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/tests/intg/ldap_test.py | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 84f7f2d..b8315e5 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -292,6 +292,28 @@ def sanity_rfc2307_bis(request, ldap_conn): return None +@pytest.fixture +def just_start_ldap(request, ldap_conn): +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) +create_ldap_fixture(request, ldap_conn, ent_list) + +conf = \ +format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \ +unindent("""\ +[domain/LDAP] +ldap_user_extra_attrs = mail +""").format(**locals()) + +create_conf_fixture
[SSSD] [PATCH 2/2] sdap: Skip exact duplicates when extending maps
Hi, see the attached patch. I modified the detection of duplicates when extending the maps (sysdb_attr:ldap_attr). When we try to add entry to the map that already exists in the map, then without this patch we will fail. With this patch, we only fail if the newly added extension would redefine already existing entry in the map. Otherwise it is just skipped without a failure (we just skip adding what is already there). I created simple CI test for this (first patch). Michal >From 932745aaadd7c2a73e817bfd685fbe1aaa462cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Wed, 10 Aug 2016 16:40:45 +0200 Subject: [PATCH 1/2] CI: Test extra attributes duplicate Regresion test for ticket #3120 Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/tests/intg/ldap_test.py | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 84f7f2d..b8315e5 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -292,6 +292,28 @@ def sanity_rfc2307_bis(request, ldap_conn): return None +@pytest.fixture +def just_start_ldap(request, ldap_conn): +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) +create_ldap_fixture(request, ldap_conn, ent_list) + +conf = \ +format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \ +unindent("""\ +[domain/LDAP] +ldap_user_extra_attrs = mail +""").format(**locals()) + +create_conf_fixture(request, conf) +create_sssd_cleanup(request) +return None + + +def test_extra_attribute_already_exists(ldap_conn, just_start_ldap): +if subprocess.call(["sssd", "-D", "-f"]) != 0: +raise Exception("sssd start failed") + + def test_regression_ticket2163(ldap_conn, simple_rfc2307): ent.assert_passwd_by_name( 'usr\\001', -- 2.5.0 >From 5a2ef2a98e483701603a42bc50e9a11d8ee651ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Wed, 10 Aug 2016 15:41:34 +0200 Subject: [PATCH 2/2] sdap: Skip exact duplicates when extending maps When extending map with entry that already exists in the map in the exacty same form, then there is no need to fail. We should only fail if we try to change purpose of already used sysdb attribute. Resolves: https://fedorahosted.org/sssd/ticket/3120 --- src/providers/ldap/sdap.c | 41 +++-- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 97b8f12..e1cf70f 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -122,19 +122,39 @@ static errno_t split_extra_attr(TALLOC_CTX *mem_ctx, return EOK; } -static bool is_sysdb_duplicate(struct sdap_attr_map *map, - int num_entries, - const char *sysdb_attr) +/* _already_in_map is set to true if the attribute + * already exists in the map and is used for the same + * LDAP attribute. + * + * _conflicts_with_map is set to true if the attribute + * already exists in map, but is used for different + * LDAP attribute. + * */ +static void check_duplicate(struct sdap_attr_map *map, +int num_entries, +const char *sysdb_attr, +const char *ldap_attr, +bool *_already_in_map, +bool *_conflicts_with_map) { int i; for (i = 0; i < num_entries; i++) { if (strcmp(map[i].sys_name, sysdb_attr) == 0) { -return true; +if (strcmp(map[i].name, ldap_attr) == 0) { +*_already_in_map = true; +*_conflicts_with_map = false; +return; +} else { +*_already_in_map = false; +*_conflicts_with_map = true; +return; +} } } -return false; +*_already_in_map = false; +*_conflicts_with_map = false; } int sdap_extend_map(TALLOC_CTX *memctx, @@ -150,6 +170,8 @@ int sdap_extend_map(TALLOC_CTX *memctx, char *ldap_attr; char *sysdb_attr; errno_t ret; +bool already_in_map; +bool conflicts_with_map; if (extra_attrs == NULL) { DEBUG(SSSDBG_FUNC_DATA, "No extra attributes\n"); @@ -174,7 +196,14 @@ int sdap_extend_map(TALLOC_CTX *memctx, continue; } -if (is_sysdb_duplicate(map, num_entries, sysdb_attr)) { +check_duplicate(map, num_entries, sysdb_attr, ldap_attr, +_in_map, _with_map); +if (already_in_map) { +DEBUG(SSSDBG_TRACE_FUNC, + "Attribute %s (%s in LDAP) is already in map.\n", + sysdb_attr, ldap_attr); +continue; +} else if (conflicts_with_map) { DEBUG(SSSDBG_FATAL_FAILURE,
[SSSD] Re: [PATCH] gpo: gPCMachineExtensionNames with just whitespaces
On 08/10/2016 11:35 AM, Jakub Hrozek wrote: On Wed, Aug 10, 2016 at 12:02:18PM +0300, Alexander Bokovoy wrote: On Tue, 09 Aug 2016, Michal Židek wrote: Summary for Alexander (in CC): - Regarding processing GPOs on the client. - If groupPolicyContainer in AD has attribute gPCMachineExtensionNames that contains only whitespaces, SSSD fails to process GPOs and denies access to users - if the gPCMachineExtensionNames is missing, it is Ok and SSSD skips such GPO (because we are only interested in Machine extensions) - We have customer that has thousands of GPOs stored in AD and some of them have just ' ' (space) in the gPCMachineExtensionNames attribute. The AD administrators say that they created the GPOs using the GUI provided by AD. - Treating the gPCMachineExtensionNames with just whitespaces the same way as if the gpcMachineExtensionNames was missing completely fixed the issue for the customer. - Now, it would be good to support the fix with some links to documentation. - I believe we should go with that fix, but could not find any documentation that would explicitly say something about just whitespaces in the gpcMachineExtensionNames - Gunter could also not find documentation that would say something about just whitespaces in that attribute, but believes that we should use the fix and skip such attributes. Alexander, can you try to find something in the MSDN documentation, that would support our fix? If not, then just what is your opinion? You should use MS-GPOL spec. 2.2.4 'GPO Search' section says that when processing gPCMachineExtensionNames, "Group Policy processing terminates at the first out of sequence." Since ' ' (space only) does not fall into defined syntax for gPCMachineExtensionNames, this Group Policy processing is stopped and its CSE GUIDs are set to 'empty list'. Because of the 3.2.5.1.10 'Extension Protocol Sequences' language The Group Policy client MUST evaluate the subset of the abstract element Filtered GPO list separately for each Group Policy extension by including in the subset only those GPOs whose gPCUserExtensionNames (for user policy mode) or gPCMachineExtensionNames (for computer policy mode) attributes contain CSE GUID that correspond to the Group Policy extension. If the CSE GUID corresponding to the Group Policy extension is present in Extension List, it is invoked using the Implementation Identifier field. Applicability is determined as specified in section 3.2.1.5. The Group Policy Registry Extension MUST always execute first. All other applicable Group Policy extensions in the Extension List MUST be loaded and executed in Extension List order. A failure in any Group Policy extension sequence MUST NOT affect the execution of other Group Policy extensions. - I think we can practically treat wrong content of gPCMachineExtensionNames (and gPCUserExtensionNames) as inability of the GPO to pass through the Filtered GPO list. Thus, the GPO would be ignored. Michal, if you add Alexander's response into the commit message, I will push the patch. I copied the entire comment into the commit message. New patch is attached. Michal >From fc0927d0a8dbce10505e09316878aa0e11b042fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 29 Jul 2016 16:09:16 +0200 Subject: [PATCH] gpo: gPCMachineExtensionNames with just whitespaces Resolves: https://fedorahosted.org/sssd/ticket/3114 We failed GPO procesing if the gPCMachineExtensionNames attribute contained just whitespaces. This coused failures in some server settings. Comment from Alexander Bokovoy quoting: You should use MS-GPOL spec. 2.2.4 'GPO Search' section says that when processing gPCMachineExtensionNames, "Group Policy processing terminates at the first out of sequence." Since ' ' (space only) does not fall into defined syntax for gPCMachineExtensionNames, this Group Policy processing is stopped and its CSE GUIDs are set to 'empty list'. Because of the 3.2.5.1.10 'Extension Protocol Sequences' language The Group Policy client MUST evaluate the subset of the abstract element Filtered GPO list separately for each Group Policy extension by including in the subset only those GPOs whose gPCUserExtensionNames (for user policy mode) or gPCMachineExtensionNames (for computer policy mode) attributes contain CSE GUID that correspond to the Group Policy extension. If the CSE GUID corresponding to the Group Policy extension is present in Extension List, it is invoked using the Implementation Identifier field. Applicability is determined as specified in section 3.2.1.5. The Group Policy Registry Extension MUST always execute first. All other applicable Group Policy extensions in the Extension List MUST be loa
[SSSD] Re: [PATCH] gpo: gPCMachineExtensionNames with just whitespaces
Summary for Alexander (in CC): - Regarding processing GPOs on the client. - If groupPolicyContainer in AD has attribute gPCMachineExtensionNames that contains only whitespaces, SSSD fails to process GPOs and denies access to users - if the gPCMachineExtensionNames is missing, it is Ok and SSSD skips such GPO (because we are only interested in Machine extensions) - We have customer that has thousands of GPOs stored in AD and some of them have just ' ' (space) in the gPCMachineExtensionNames attribute. The AD administrators say that they created the GPOs using the GUI provided by AD. - Treating the gPCMachineExtensionNames with just whitespaces the same way as if the gpcMachineExtensionNames was missing completely fixed the issue for the customer. - Now, it would be good to support the fix with some links to documentation. - I believe we should go with that fix, but could not find any documentation that would explicitly say something about just whitespaces in the gpcMachineExtensionNames - Gunter could also not find documentation that would say something about just whitespaces in that attribute, but believes that we should use the fix and skip such attributes. Alexander, can you try to find something in the MSDN documentation, that would support our fix? If not, then just what is your opinion? Thanks (the original conversation is below - does not include Gunter because that was on IRC). On 08/09/2016 11:50 AM, Lukas Slebodnik wrote: On (09/08/16 11:48), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 05:40:44PM +0200, Michal Židek wrote: Hi, the attached patch fixes: https://fedorahosted.org/sssd/ticket/3114 We have a user that can not login with enforced GPO because of this. I do not think it is a common issue, I could not create groupPolicyContainer with gPCMachineExtensionNames containing only whitespaces, but you can create it with a script and the blank value is not an error so we should handle it properly (and maybe thre is a way in the GUI maze to create such GPOs as well, I just could not find it). I was able to reproduce the same "sssd log path" the user has and this patch fixed the issue. If the user tested the patch, then I would say we should accept it. Did you ask someone from the Samba team if this is the right thing to do? I asked Gunter and he said that we should ignore this attribute if it contains just whitespaces. Btw. I can confirm that this fixed the issue for the customer. He is now hitting this: https://fedorahosted.org/sssd/ticket/2751 which is already fixed in master. It would be good to add link to the MSDN documentation. Try to ask ab. He is quite familiar with the documentation. I tried to find some MSDN documentation, but nothing explicitly mentioned if the attribute can contain just whitespaces or not. Gunter could not find anything either. However if the attribute is missing completely, it is a valid GPO (we already ignore such GPOs). So having it containing just whitespaces is not too distant from that. I can ask Alexander if he can find something in the documentation though. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] tools: Add missing gettext macro
Hi, see the attached simple patch. I sent it as part of the commands renaming patchset, but forgot to include it in the last iteration. Michal >From 3dbbe08b50ca1ffeae4945cb3ea4b263d1d91305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Fri, 5 Aug 2016 12:55:00 +0200 Subject: [PATCH] tools: Add missing gettext macro The message in SSS_TOOL_DELIMITER should be transalted. --- src/tools/common/sss_tools.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h index 41c8b10..6d24642 100644 --- a/src/tools/common/sss_tools.h +++ b/src/tools/common/sss_tools.h @@ -47,7 +47,7 @@ typedef errno_t #define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn} #define SSS_TOOL_COMMAND_NOMSG(cmd, err, fn) {cmd, NULL, err, fn} -#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL} +#define SSS_TOOL_DELIMITER(message) {"", _(message), 0, NULL} #define SSS_TOOL_LAST {NULL, NULL, 0, NULL} struct sss_route_cmd { -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Generic help for cache-upgrade and config-check
On 08/05/2016 12:01 PM, Pavel Březina wrote: On 07/26/2016 04:43 PM, Michal Židek wrote: Hi! Attached is patch for ticket: https://fedorahosted.org/sssd/ticket/3086 This patch applies on top of the patches from thread: [SSSD] [PATCH] sssctl: Consistent commands naming Michal Hi, I believe you can use NULL instead of options. Yes. New patch attached. Michal >From befa994975dee5bd34e45d068b4bcb2860330154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 26 Jul 2016 16:35:55 +0200 Subject: [PATCH] sssctl: Generic help for cache-upgrade and config-check sssctl COMMAND --help should print at least generic help, even if the command does not accept any command specific options. Resolves: https://fedorahosted.org/sssd/ticket/3086 --- src/tools/sssctl/sssctl_config.c | 6 ++ src/tools/sssctl/sssctl_data.c | 6 ++ 2 files changed, 12 insertions(+) diff --git a/src/tools/sssctl/sssctl_config.c b/src/tools/sssctl/sssctl_config.c index a66d774..630df3c 100644 --- a/src/tools/sssctl/sssctl_config.c +++ b/src/tools/sssctl/sssctl_config.c @@ -47,6 +47,12 @@ errno_t sssctl_config_check(struct sss_cmdline *cmdline, char **strs = NULL; TALLOC_CTX *tmp_ctx = NULL; +ret = sss_tool_popt(cmdline, NULL, SSS_TOOL_OPT_OPTIONAL, NULL, NULL); +if (ret != EOK) { +DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n"); +return ret; +} + tmp_ctx = talloc_new(NULL); init_data = sss_ini_initdata_init(tmp_ctx); if (!init_data) { diff --git a/src/tools/sssctl/sssctl_data.c b/src/tools/sssctl/sssctl_data.c index a26ddd8..72823ab 100644 --- a/src/tools/sssctl/sssctl_data.c +++ b/src/tools/sssctl/sssctl_data.c @@ -266,6 +266,12 @@ errno_t sssctl_cache_upgrade(struct sss_cmdline *cmdline, struct sysdb_upgrade_ctx db_up_ctx; errno_t ret; +ret = sss_tool_popt(cmdline, NULL, SSS_TOOL_OPT_OPTIONAL, NULL, NULL); +if (ret != EOK) { +DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n"); +return ret; +} + if (sss_deamon_running()) { return ERR_SSSD_RUNNING; } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 08/05/2016 12:30 PM, Lukas Slebodnik wrote: On (04/08/16 16:21), Michal Židek wrote: Hi, As was requested on devel meeting, I removed the compatibility with old commands. New patch attached. Michal From 0c18c75e2b6a2e29f95b0e477369b5db766afbdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 25 Jul 2016 13:50:13 +0200 Subject: [PATCH] sssctl: Consistent commands naming Fixes: https://fedorahosted.org/sssd/ticket/3087 Use TOPIC-ACTION pattern for sssctl command names. It would be good to be consistent in the content of comit message. We already have a prefered template. It would be good if you could use it. @see https://git.fedorahosted.org/cgit/sssd.git/commit/?id=3d9edb4c510028def2df41aa7b0ce705b197e6fc LS Yes, sorry for that. I will also update our contribute wiki page to reflect this. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
Hi, As was requested on devel meeting, I removed the compatibility with old commands. New patch attached. Michal >From 0c18c75e2b6a2e29f95b0e477369b5db766afbdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Mon, 25 Jul 2016 13:50:13 +0200 Subject: [PATCH] sssctl: Consistent commands naming Fixes: https://fedorahosted.org/sssd/ticket/3087 Use TOPIC-ACTION pattern for sssctl command names. --- src/tools/common/sss_tools.h | 2 ++ src/tools/sss_override.c | 26 +++ src/tools/sssctl/sssctl.c | 22 ++-- src/tools/sssctl/sssctl.h | 43 +++ src/tools/sssctl/sssctl_cache.c | 18 src/tools/sssctl/sssctl_data.c| 16 +++ src/tools/sssctl/sssctl_domains.c | 6 +++--- src/tools/sssctl/sssctl_logs.c| 4 ++-- 8 files changed, 69 insertions(+), 68 deletions(-) diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h index a9ebabe..41c8b10 100644 --- a/src/tools/common/sss_tools.h +++ b/src/tools/common/sss_tools.h @@ -46,7 +46,9 @@ typedef errno_t void *pvt); #define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn} +#define SSS_TOOL_COMMAND_NOMSG(cmd, err, fn) {cmd, NULL, err, fn} #define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL} +#define SSS_TOOL_LAST {NULL, NULL, 0, NULL} struct sss_route_cmd { const char *command; diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c index 45a28fd..d41da52 100644 --- a/src/tools/sss_override.c +++ b/src/tools/sss_override.c @@ -1913,19 +1913,19 @@ static int override_group_export(struct sss_cmdline *cmdline, int main(int argc, const char **argv) { struct sss_route_cmd commands[] = { -{"user-add", NULL, 0, override_user_add}, -{"user-del", NULL, 0, override_user_del}, -{"user-find", NULL, 0, override_user_find}, -{"user-show", NULL, 0, override_user_show}, -{"user-import", NULL, 0, override_user_import}, -{"user-export", NULL, 0, override_user_export}, -{"group-add", NULL, 0, override_group_add}, -{"group-del", NULL, 0, override_group_del}, -{"group-find", NULL, 0, override_group_find}, -{"group-show", NULL, 0, override_group_show}, -{"group-import", NULL, 0, override_group_import}, -{"group-export", NULL, 0, override_group_export}, -{NULL, NULL, 0, NULL} +SSS_TOOL_COMMAND_NOMSG("user-add", 0, override_user_add), +SSS_TOOL_COMMAND_NOMSG("user-del", 0, override_user_del), +SSS_TOOL_COMMAND_NOMSG("user-find", 0, override_user_find), +SSS_TOOL_COMMAND_NOMSG("user-show", 0, override_user_show), +SSS_TOOL_COMMAND_NOMSG("user-import", 0, override_user_import), +SSS_TOOL_COMMAND_NOMSG("user-export", 0, override_user_export), +SSS_TOOL_COMMAND_NOMSG("group-add", 0, override_group_add), +SSS_TOOL_COMMAND_NOMSG("group-del", 0, override_group_del), +SSS_TOOL_COMMAND_NOMSG("group-find", 0, override_group_find), +SSS_TOOL_COMMAND_NOMSG("group-show", 0, override_group_show), +SSS_TOOL_COMMAND_NOMSG("group-import", 0, override_group_import), +SSS_TOOL_COMMAND_NOMSG("group-export", 0, override_group_export), +SSS_TOOL_LAST }; return sss_tool_main(argc, argv, commands, NULL); diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c index 86656f1..20ea26f 100644 --- a/src/tools/sssctl/sssctl.c +++ b/src/tools/sssctl/sssctl.c @@ -257,25 +257,25 @@ int main(int argc, const char **argv) { struct sss_route_cmd commands[] = { SSS_TOOL_DELIMITER("SSSD Status:"), -SSS_TOOL_COMMAND("list-domains", "List available domains", 0, sssctl_list_domains), +SSS_TOOL_COMMAND("domain-list", "List available domains", 0, sssctl_domain_list), SSS_TOOL_COMMAND("domain-status", "Print information about domain", 0, sssctl_domain_status), SSS_TOOL_DELIMITER("Information about cached content:"), -SSS_TOOL_COMMAND("user", "Information about cached user", 0, sssctl_user), -SSS_TOOL_COMMAND("group", "Information about cached group", 0, sssctl_group), -SSS_TOOL_COMMAND("netgroup", "Information about cached netgroup", 0, sssctl_netgroup), +SSS_TOOL_COMMAND("user-show", "Information about cached user", 0, sssctl_user_show), +SSS_TOOL_COMMAND("group-show", "Information about cached group", 0, sssctl_group_show), +SSS_TOOL_COMMAND("netgroup-show", "Information about cached netgroup", 0, sssctl_netgroup_show), SSS_TOOL_DELIMITER("Local data tools:"), -SSS_TOOL_COMMAND("backup-local-data", "Backup local data", 0, sssctl_backup_local_data), -SSS_TOOL_COMMAND("restore-local-data", "Restore local data from backup", 0, sssctl_restore_local_data), -SSS_TOOL_COMMAND("remove-cache", "Backup local data and remove cached
[SSSD] Re: [PATCH] Change debug level of config error msgs
On 08/04/2016 12:53 PM, Lukas Slebodnik wrote: On (04/08/16 12:13), Michal Židek wrote: On 07/27/2016 03:13 PM, Petr Cech wrote: On 07/27/2016 03:05 PM, Petr Cech wrote: On 07/27/2016 02:32 PM, Michal Židek wrote: Hi, I believe that this patch makes pinpointing of config errors a little easier. Especially when using sssctl tool that currently refuses to start a command when there are syntax errors in sssd.conf, but by default it does not print problematic line number. Compare: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:05:39:185114 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:05:39:185192 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:05:39:185229 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:05:39:185255 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:05:39:185288 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error and: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:22:51:096949 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:22:51:097173 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:22:51:097490 2016) [sssd] [sss_ini_config_print_errors] (0x0010): Error (2) on line 10: No closing bracket. (Wed Jul 27 14:22:51:097946 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:22:51:098452 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:22:51:098651 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error Patch is attached. Michal Hi Michal, thanks for your patch. It looks good to me. => LGTM I think I should run tests only locally for such simple patch. Please, wait a moment. CI locally passed. => ACK Regards This was already acked. Can we push it? man sssd.conf says: 0, 0x0010: Fatal failures. Anything that would prevent SSSD from starting up or causes it to cease running. Are you sure that validation warnings match this descriptions? LS This is not validation warning, but syntax error. And yes it prevents SSSD from starting. Try to put debug_level Without equal sign in sssd.conf (for example in [sssd] section). Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Change debug level of config error msgs
On 07/27/2016 03:13 PM, Petr Cech wrote: On 07/27/2016 03:05 PM, Petr Cech wrote: On 07/27/2016 02:32 PM, Michal Židek wrote: Hi, I believe that this patch makes pinpointing of config errors a little easier. Especially when using sssctl tool that currently refuses to start a command when there are syntax errors in sssd.conf, but by default it does not print problematic line number. Compare: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:05:39:185114 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:05:39:185192 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:05:39:185229 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:05:39:185255 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:05:39:185288 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error and: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:22:51:096949 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:22:51:097173 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:22:51:097490 2016) [sssd] [sss_ini_config_print_errors] (0x0010): Error (2) on line 10: No closing bracket. (Wed Jul 27 14:22:51:097946 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:22:51:098452 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:22:51:098651 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error Patch is attached. Michal Hi Michal, thanks for your patch. It looks good to me. => LGTM I think I should run tests only locally for such simple patch. Please, wait a moment. CI locally passed. => ACK Regards This was already acked. Can we push it? Thanks, Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 08/02/2016 06:04 PM, Michal Židek wrote: On 07/29/2016 09:38 PM, Lukas Slebodnik wrote: On (29/07/16 15:54), Michal Židek wrote: On 07/29/2016 03:23 PM, Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 03:06:47PM +0200, Lukas Slebodnik wrote: On (29/07/16 14:27), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 02:09:02PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:59), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:49:41PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:44), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:07:56PM +0200, Lukas Slebodnik wrote: Others who? :-) non developers (The person who requested this change; I assume this change was not requested by developers) It was (and btw I agree with the change, consistent naming is important as I wish I raised this concern when I reviewed the patches in the first place..) I was expecting an answer for keeping backward compatibility with unused feature. At this point it would be only compatibility for rawhide users and anyone who compiled sssd from source or anyone who was alrady using the 1.14.0 tarball. Which is not many people, but still. So if you want to keep old versions then we document obsoleted version. At least with help "Deprecated alias for (new-name)" e.g. [root@host ~]# sssctl Usage: sssctl COMMAND COMMAND-ARGS Available commands: SSSD Status: * list-domains Deprecated alias for (domain-list) * domain-listList available domains * domain-status Print information about domain The idea of hidding options is really terrible. a) it's not documented anywhere that it's deprecated b) users might wonder why it works. Fine by me, but additioanlly, what about printing the deprecation warning when a user runs that command? If we really want to insist on "backward compatibility" with unused feature then it will be good addition to the the updated help output. The point of hiding the option is to make it less discoverable. I know what is point of hiding the option but it isn't good from user point of view if we want to keep backward compatibility. Backward compatible changes are usually well documented and not hiden Please update design page with renamed commands and also with deprecated commands. OK, but this is something for the author of the patch to do :) Ok, I will update the design page, but I am little lost in this thread. I will do this: 1. update the design page yes + ask for blessing/review from the requester of the change 2. wrap the old command names in function that prints warning about the command being deprecated and that it will be removed in future versions. 3. Will list the old commands in the sssctl usage with note that they are deprecated. Do all agree with the above? If we insinst on keeping backward comaptibility with unused feature then yes for 2) and 3) I would personally just rename them but I'm fine with 2) and 3) It would be much simpler. LS CCing Marc. Marc could you give us your opinion/blessing to this change? Long story short, we though the commands will be easier to understand if they follow similar pattern as the TOPIC-ACTION that ipa tool uses. This will help us create more consistent command names as we add more functionality to the sssctl tool. Using the obsolete commands is still possible and prints message like this: # sssctl fetch-logs out.tgz Command 'fetch-logs' is deprecated and may be removed in future versions. Use 'logs-fetch' instead. We can remove the obsolete commands in some later milestone o get rid of the code. The usage now looks like this: # sssctl --help Usage: sssctl COMMAND COMMAND-ARGS Available commands: SSSD Status: * domain-list List available domains * domain-status Print information about domain Information about cached content: * user-show Information about cached user * group-show Information about cached group * netgroup-show Information about cached netgroup Local data tools: * client-data-backup Backup local data * client-data-restore Restore local data from backup * cache-removeBackup local data and remove cached content * cache-upgrade Perform cache upgrade Log files tools: * logs-remove Remove existing SSSD log files * logs-fetch Archive SSSD log files in tarball Configuration files tools: * config-checkPerform static analysis of SSSD configuration Deprecated commands that may be removed in the future: * list-domainsObsolete alias for domain-list * userObsolete alias for user-show * group Obsolete alias for group-show * netgroupObsolete alias for netgroup-show * backup-local-data Obsolete alias for client-data-backup * restore-local-data Obsolete alias for client-data-restore * remove-cacheObsolete alias for cache-remove * upgrade-cache Obso
[SSSD] Re: [PATCH] config: Some fixes to schema
On 08/04/2016 11:40 AM, Jakub Hrozek wrote: On Thu, Aug 04, 2016 at 11:35:30AM +0200, Michal Židek wrote: On 07/12/2016 06:38 PM, Lukas Slebodnik wrote: On (12/07/16 15:59), Michal Židek wrote: On 07/12/2016 03:36 PM, Lukas Slebodnik wrote: On (12/07/16 15:16), Michal Židek wrote: +# secrets responder +option = provider + I think you need to also update "rule/allowed_sections" maybe you could run tour tool "sssctl config-check" before sending patches :-) What a useful tool it turned out to be :) And there are another related question to this topic. Should we add undocumented option to the list? We already have "command" in schema. Should we add other as well? IMHO, no. LS So far we only added options that we expect users to use. Options that are for developers are not added to the schema for now. Michal From 42a3038b68452cf92b2f87ae0875f4e3b8b1f051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/3] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. Also remove unused macro CONFDB_SERVICE_TIMEOUT. --- ACK From cacd9f84e702c2aa7f5c41d0d257eb5ce8c77a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:34:03 +0200 Subject: [PATCH 2/3] config: Add config_file_version to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 --- src/config/SSSDConfigTest.py | 1 + src/config/cfg_rules.ini | 1 + src/config/etc/sssd.api.conf | 1 + 3 files changed, 3 insertions(+) ACK From f292235689986eae02fec9a91fb8af151b553eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 12 Jul 2016 15:05:16 +0200 Subject: [PATCH 3/3] config: Allow 'secrets' section Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow the 'secrets' section in config file schema. --- src/config/SSSDConfigTest.py | 6 -- src/config/cfg_rules.ini | 22 ++ src/config/etc/sssd.api.conf | 4 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 332d870..4748ecb 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -1351,7 +1351,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'autofs', 'ssh', 'pac', -'ifp'] +'ifp', +'secrets'] for section in control_list: self.assertTrue(sssdconfig.has_section(section), "Section [%s] missing" % @@ -1444,7 +1445,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'autofs', 'ssh', 'pac', -'ifp'] +'ifp', +'secrets'] service_list = sssdconfig.list_services() for service in control_list: self.assertTrue(service in service_list, diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 635c078..cab25fc 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -8,6 +8,7 @@ section = autofs section = ssh section = pac section = ifp +section_re = ^secrets/.*$ section_re = ^domain/.*$ [rule/allowed_sssd_options] @@ -224,6 +225,27 @@ option = diag_cmd option = allowed_uids option = user_attributes +[rule/allowed_secrets_options] +validator = ini_allowed_options +section_re = ^secrets/.*$ + +option = timeout +option = debug +option = debug_level +option = debug_timestamps +option = debug_microseconds +option = debug_to_files +option = command +option = reconnection_retries +option = fd_limit +option = client_idle_timeout +option = force_timeout +option = description +option = diag_cmd + +# secrets responder +option = provider + There are some options which you didn't include (e.g. forward_headers) @see grep confdb_get -A 5 src/responder/secrets/* grep proxy_get_config_string -A 5 src/responder/secrets/* LS It looks like secret uses different set of options completely. Also there is the service/program specific configuration of secrets that should work similar to domains (like [secret/apache] or something like that). I will not sent this patch until I have better understanding of how secrets service work and how it can be configured. I think we should make a ticket for the documentation/design page enhancement and make the schema change as a subtask for it. We already have: https://fedorahosted.org/sssd/ticket/3053 I would like to work on that next week.. Ah, great. I will add the need for schema update in the comments of that ticket and close the one about fixes to initial schema. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] config: Some fixes to schema
On 07/12/2016 06:38 PM, Lukas Slebodnik wrote: On (12/07/16 15:59), Michal Židek wrote: On 07/12/2016 03:36 PM, Lukas Slebodnik wrote: On (12/07/16 15:16), Michal Židek wrote: +# secrets responder +option = provider + I think you need to also update "rule/allowed_sections" maybe you could run tour tool "sssctl config-check" before sending patches :-) What a useful tool it turned out to be :) And there are another related question to this topic. Should we add undocumented option to the list? We already have "command" in schema. Should we add other as well? IMHO, no. LS So far we only added options that we expect users to use. Options that are for developers are not added to the schema for now. Michal From 42a3038b68452cf92b2f87ae0875f4e3b8b1f051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/3] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. Also remove unused macro CONFDB_SERVICE_TIMEOUT. --- ACK From cacd9f84e702c2aa7f5c41d0d257eb5ce8c77a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:34:03 +0200 Subject: [PATCH 2/3] config: Add config_file_version to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 --- src/config/SSSDConfigTest.py | 1 + src/config/cfg_rules.ini | 1 + src/config/etc/sssd.api.conf | 1 + 3 files changed, 3 insertions(+) ACK From f292235689986eae02fec9a91fb8af151b553eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 12 Jul 2016 15:05:16 +0200 Subject: [PATCH 3/3] config: Allow 'secrets' section Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow the 'secrets' section in config file schema. --- src/config/SSSDConfigTest.py | 6 -- src/config/cfg_rules.ini | 22 ++ src/config/etc/sssd.api.conf | 4 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 332d870..4748ecb 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -1351,7 +1351,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'autofs', 'ssh', 'pac', -'ifp'] +'ifp', +'secrets'] for section in control_list: self.assertTrue(sssdconfig.has_section(section), "Section [%s] missing" % @@ -1444,7 +1445,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'autofs', 'ssh', 'pac', -'ifp'] +'ifp', +'secrets'] service_list = sssdconfig.list_services() for service in control_list: self.assertTrue(service in service_list, diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 635c078..cab25fc 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -8,6 +8,7 @@ section = autofs section = ssh section = pac section = ifp +section_re = ^secrets/.*$ section_re = ^domain/.*$ [rule/allowed_sssd_options] @@ -224,6 +225,27 @@ option = diag_cmd option = allowed_uids option = user_attributes +[rule/allowed_secrets_options] +validator = ini_allowed_options +section_re = ^secrets/.*$ + +option = timeout +option = debug +option = debug_level +option = debug_timestamps +option = debug_microseconds +option = debug_to_files +option = command +option = reconnection_retries +option = fd_limit +option = client_idle_timeout +option = force_timeout +option = description +option = diag_cmd + +# secrets responder +option = provider + There are some options which you didn't include (e.g. forward_headers) @see grep confdb_get -A 5 src/responder/secrets/* grep proxy_get_config_string -A 5 src/responder/secrets/* LS It looks like secret uses different set of options completely. Also there is the service/program specific configuration of secrets that should work similar to domains (like [secret/apache] or something like that). I will not sent this patch until I have better understanding of how secrets service work and how it can be configured. I think we should make a ticket for the documentation/design page enhancement and make the schema change as a subtask for it. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] LDAP: Fixing of removing netgroup from cache
Two nitpicks, see inline. On 07/22/2016 02:34 PM, Petr Cech wrote: +static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx, + struct sysdb_attrs *attrs, + const char *ext_key, + char ***_missing) ^ Coding style. Remove the space between function name and "(". Do not forget to align the parameters after that. +{ +bool is_present = false; +size_t size = 0; +size_t ret; + +for (int i = 0; i < attrs->num; i++) { +if (strcmp(ext_key, attrs->a[i].name) == 0) { +is_present = true; +} +size++; +} + +if (is_present == false) { +ret = add_string_to_list(attrs, ext_key, _missing); +if (ret != EOK) { +goto fail; +} +} + +ret = EOK; + +fail: Please change the label name to "done". According to our new coding style, the code that follows label "fail" is only executed when failure occurs. I know we do not follow this everywhere, but I would like to be consistent in new code. +return ret; +} + Other than that it looks good to me. Also it would be good to add a CI tests for this. I do not want to postpone this patch before release, so you can do it later as part of this ticket: https://fedorahosted.org/sssd/ticket/3119 So either send a patch with CI test now or assign the above ticket to yourself and do it when there is more time. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/29/2016 09:38 PM, Lukas Slebodnik wrote: On (29/07/16 15:54), Michal Židek wrote: On 07/29/2016 03:23 PM, Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 03:06:47PM +0200, Lukas Slebodnik wrote: On (29/07/16 14:27), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 02:09:02PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:59), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:49:41PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:44), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:07:56PM +0200, Lukas Slebodnik wrote: Others who? :-) non developers (The person who requested this change; I assume this change was not requested by developers) It was (and btw I agree with the change, consistent naming is important as I wish I raised this concern when I reviewed the patches in the first place..) I was expecting an answer for keeping backward compatibility with unused feature. At this point it would be only compatibility for rawhide users and anyone who compiled sssd from source or anyone who was alrady using the 1.14.0 tarball. Which is not many people, but still. So if you want to keep old versions then we document obsoleted version. At least with help "Deprecated alias for (new-name)" e.g. [root@host ~]# sssctl Usage: sssctl COMMAND COMMAND-ARGS Available commands: SSSD Status: * list-domains Deprecated alias for (domain-list) * domain-listList available domains * domain-status Print information about domain The idea of hidding options is really terrible. a) it's not documented anywhere that it's deprecated b) users might wonder why it works. Fine by me, but additioanlly, what about printing the deprecation warning when a user runs that command? If we really want to insist on "backward compatibility" with unused feature then it will be good addition to the the updated help output. The point of hiding the option is to make it less discoverable. I know what is point of hiding the option but it isn't good from user point of view if we want to keep backward compatibility. Backward compatible changes are usually well documented and not hiden Please update design page with renamed commands and also with deprecated commands. OK, but this is something for the author of the patch to do :) Ok, I will update the design page, but I am little lost in this thread. I will do this: 1. update the design page yes + ask for blessing/review from the requester of the change 2. wrap the old command names in function that prints warning about the command being deprecated and that it will be removed in future versions. 3. Will list the old commands in the sssctl usage with note that they are deprecated. Do all agree with the above? If we insinst on keeping backward comaptibility with unused feature then yes for 2) and 3) I would personally just rename them but I'm fine with 2) and 3) It would be much simpler. LS CCing Marc. Marc could you give us your opinion/blessing to this change? Long story short, we though the commands will be easier to understand if they follow similar pattern as the TOPIC-ACTION that ipa tool uses. This will help us create more consistent command names as we add more functionality to the sssctl tool. Using the obsolete commands is still possible and prints message like this: # sssctl fetch-logs out.tgz Command 'fetch-logs' is deprecated and may be removed in future versions. Use 'logs-fetch' instead. We can remove the obsolete commands in some later milestone o get rid of the code. The usage now looks like this: # sssctl --help Usage: sssctl COMMAND COMMAND-ARGS Available commands: SSSD Status: * domain-listList available domains * domain-status Print information about domain Information about cached content: * user-show Information about cached user * group-show Information about cached group * netgroup-show Information about cached netgroup Local data tools: * client-data-backup Backup local data * client-data-restoreRestore local data from backup * cache-remove Backup local data and remove cached content * cache-upgrade Perform cache upgrade Log files tools: * logs-removeRemove existing SSSD log files * logs-fetch Archive SSSD log files in tarball Configuration files tools: * config-check Perform static analysis of SSSD configuration Deprecated commands that may be removed in the future: * list-domains Obsolete alias for domain-list * user Obsolete alias for user-show * group Obsolete alias for group-show * netgroup Obsolete alias for netgroup-show * backup-local-data Obsolete alias for client-data-backup * restore-local-data Obsolete alias for client-data-restore * remove-cache Obsolete alias for cache-remove * upgrade-cache Obsolete alias for cache-upgrade * remove-logsObsolete a
[SSSD] Re: [PATCH] ipa_netgroups: Lowercase key to htable
On 08/02/2016 03:46 PM, Lukas Slebodnik wrote: On (02/08/16 13:50), Petr Cech wrote: On 08/02/2016 12:41 PM, Petr Cech wrote: On 08/02/2016 11:09 AM, Michal Židek wrote: Hi! When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider. Attached patch fixes one of them. I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117 Reviewing this is not priority for this week, but I already had the patch and wanted to put the list. Thanks, Michal 0001-ipa_netgroups-Lowercase-key-to-htable.patch From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable Fixes: https://fedorahosted.org/sssd/ticket/3116 We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN. --- Hi Michal, it looks good to me (aka LGTM). I am waiting for CI. CI passed: http://sssd-ci.duckdns.org/logs/job/50/67/summary.html ACK And for test... I think you might file a ticket. I'm sorry I have a different opinion. Michal tried to resolve nested netgroup membership which should have been fixed in #2275 This patch is just a fix to the partial problem. a) it does not fix a bug It does fix one. b) it does not have an integration test c) id does not have an unit test ^ Yes we do not have upstream tests for netgroups. We should implement them. Many people consider a code without code coverage as a legacy code. We need to avoid such situations and moreover we need to avoid possible regressions in future. Therefore nested netgroup membership in IPA need to be fixed (and not just a partially). And we should have a test for this bug. I don't might which king of test. But IMHO the simplest would be to write an integration test to freeipa project. => Conditional NACK untill resolvimng previously mentioned issues. LS The purpose of this patch was to fix one particular bug in code that I found when looking into the netgroups. It fixes one issue with netgroups, not all of them. I sent it to list and filed the tickets so that I do not need to keep the information about it in my head. It does not have high priority (now) and I am OK if the two tickets are solved together later. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] ipa_netgroups: Lowercase key to htable
Hi! When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider. Attached patch fixes one of them. I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117 Reviewing this is not priority for this week, but I already had the patch and wanted to put the list. Thanks, Michal >From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable Fixes: https://fedorahosted.org/sssd/ticket/3116 We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN. --- src/providers/ipa/ipa_netgroups.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_netgroups.c b/src/providers/ipa/ipa_netgroups.c index a19e5e0..38b4b5d 100644 --- a/src/providers/ipa/ipa_netgroups.c +++ b/src/providers/ipa/ipa_netgroups.c @@ -888,7 +888,13 @@ static int ipa_netgr_process_all(struct ipa_get_netgroups_state *state) j = 0; if (ret == EOK) { for (j = 0; members[j]; j++) { -key.str = discard_const(members[j]); +/* Lower case the search key (we do this also when storing + * the entries in the hash table). */ +key.str = talloc_strdup(state, discard_const(members[j])); +for (int p = 0; key.str[p] != '\0'; p++) { +key.str[p] = tolower(key.str[p]); +} + ret = hash_lookup(state->new_netgroups, , ); if (ret != HASH_SUCCESS) { ret = ENOENT; -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] gpo: gPCMachineExtensionNames with just whitespaces
Hi, the attached patch fixes: https://fedorahosted.org/sssd/ticket/3114 We have a user that can not login with enforced GPO because of this. I do not think it is a common issue, I could not create groupPolicyContainer with gPCMachineExtensionNames containing only whitespaces, but you can create it with a script and the blank value is not an error so we should handle it properly (and maybe thre is a way in the GUI maze to create such GPOs as well, I just could not find it). I was able to reproduce the same "sssd log path" the user has and this patch fixed the issue. Thanks. Michal >From b3e413e930aca78157fa137cee84af32a4262155 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Fri, 29 Jul 2016 16:09:16 +0200 Subject: [PATCH] gpo: gPCMachineExtensionNames with just whitespaces Fixes: https://fedorahosted.org/sssd/ticket/3114 We failed GPO procesing if the gPCMachineExtensionNames attribute contained just whitespaces. This coused failures in some server settings. --- src/providers/ad/ad_gpo.c | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index f609d28..63c68ce 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -3765,6 +3765,24 @@ done: } } +static bool machine_ext_names_is_blank(char *attr_value) +{ +char *ptr; + +if (attr_value == NULL) { +return true; +} + +ptr = attr_value; +for (; *ptr != '\0'; ptr++) { +if (!isspace(*ptr)) { +return false; +} +} + +return true; +} + static errno_t ad_gpo_sd_process_attrs(struct tevent_req *req, char *smb_host, @@ -3880,7 +3898,8 @@ ad_gpo_sd_process_attrs(struct tevent_req *req, goto done; } -if ((ret == ENOENT) || (el->num_values == 0)) { +if ((ret == ENOENT) || (el->num_values == 0) +|| machine_ext_names_is_blank((char *) el[0].values[0].data)) { /* * if gpo has no machine_ext_names (which is perfectly valid: it could * have only user_ext_names, for example), we continue to next gpo -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/29/2016 03:23 PM, Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 03:06:47PM +0200, Lukas Slebodnik wrote: On (29/07/16 14:27), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 02:09:02PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:59), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:49:41PM +0200, Lukas Slebodnik wrote: On (29/07/16 13:44), Jakub Hrozek wrote: On Fri, Jul 29, 2016 at 01:07:56PM +0200, Lukas Slebodnik wrote: Others who? :-) non developers (The person who requested this change; I assume this change was not requested by developers) It was (and btw I agree with the change, consistent naming is important as I wish I raised this concern when I reviewed the patches in the first place..) I was expecting an answer for keeping backward compatibility with unused feature. At this point it would be only compatibility for rawhide users and anyone who compiled sssd from source or anyone who was alrady using the 1.14.0 tarball. Which is not many people, but still. So if you want to keep old versions then we document obsoleted version. At least with help "Deprecated alias for (new-name)" e.g. [root@host ~]# sssctl Usage: sssctl COMMAND COMMAND-ARGS Available commands: SSSD Status: * list-domains Deprecated alias for (domain-list) * domain-listList available domains * domain-status Print information about domain The idea of hidding options is really terrible. a) it's not documented anywhere that it's deprecated b) users might wonder why it works. Fine by me, but additioanlly, what about printing the deprecation warning when a user runs that command? If we really want to insist on "backward compatibility" with unused feature then it will be good addition to the the updated help output. The point of hiding the option is to make it less discoverable. I know what is point of hiding the option but it isn't good from user point of view if we want to keep backward compatibility. Backward compatible changes are usually well documented and not hiden Please update design page with renamed commands and also with deprecated commands. OK, but this is something for the author of the patch to do :) Ok, I will update the design page, but I am little lost in this thread. I will do this: 1. update the design page 2. wrap the old command names in function that prints warning about the command being deprecated and that it will be removed in future versions. 3. Will list the old commands in the sssctl usage with note that they are deprecated. Do all agree with the above? Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/28/2016 02:11 PM, Michal Židek wrote: On 07/28/2016 01:57 PM, Jakub Hrozek wrote: On Thu, Jul 28, 2016 at 01:51:40PM +0200, Pavel Březina wrote: On 07/28/2016 01:38 PM, Jakub Hrozek wrote: On Thu, Jul 28, 2016 at 12:23:24PM +0200, Michal Židek wrote: On 07/28/2016 10:00 AM, Pavel Březina wrote: On 07/27/2016 03:28 PM, Michal Židek wrote: On 07/27/2016 11:09 AM, Jakub Hrozek wrote: On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina wrote: On 07/26/2016 04:19 PM, Michal Židek wrote: On 07/26/2016 01:19 PM, Pavel Březina wrote: On 07/25/2016 02:12 PM, Michal Židek wrote: Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal Hi, ok, it looks like a good idea. When touching the code, can you also convert sss_override command to use the macro instead? And I think it may be nice to also add a macro for command sentinel i.e. for {NULL, ...}. I'm not very fond of renaming local-data-* to cache-* so it doesn't imply that we backup the whole cache content. We only backup and restore data that are local to the client and not present in LDAP. Currently only local overrides, but it may include local users and groups in the future. When we have the files provider there would be a cache as well. Moreover, we store secrets now. The restore command backs up all *.ldb files, right? This is how I understood it at first, but the current backup and restore only work for local overrides. But as Pavel mentioned, it may work for local users and groups in the future (id_provider=local). My original confusion was that I also thought it backs-up and restores all ldb files, which is not the case. No, the intention is to backup only data that are not stored on server and would be lost when the cache is removed. In the time, only local overrides were local. If secrets creates local data, the tool should be modified. Yes, but users and groups from local domain would also be lost if the local data is deleted. So I though we want to backup them as well in the future versions (I mean the local provider, not the files provider). Well, probably not in the first version, but definitely in a couple of months we want to add the capability to set extended attributes to the files provider which we'll want to back up as well. But I also think we will add the additional data into a new directory, just like we did with the secrets database. Btw. do we want to merge sss_override tool into sssctl? Because if the local-data-* commands work currently with overrides only, then we could make a new topic 'overrides' and add commands like I would like to merge all tools into sssctl unless there is a strong reason to keep them separate (the local domain tools should be kept separate IMO). The reason is simply discoverability, if there is a new sssd release, the admin would just run sssctl and if there are any new tools, their topics would be displayed. (Also I think the code duplication would be reduced as a side-effect). sssctl overrides-backup sssctl overrides-restore and later also all the functionality of sss_overrides sssctl overrides-user-add sssctl overrides-user-del Maybe, but later. etc. This way we could avoid confusion between local-data and cache. If secrets will also create some local data, we will add topic 'secrets' to deal with that separately. sssctl secrets-backup sssctl secrets-restore I think I got lost in the thread :-) What is the benefit of having more backup/restore commands than one that backs up or removes of value under the /var/lib/sss/ structure? So far I can only think of cache being more intuitive to admins than local-data. How about client-data instead of local-data? SGTM (sounds good to me :-)) SGTM too :) I will send version with client-data after the meeting. I forgot to update this after the meeting. New patch is attached. Michal >From 73c3e97013274ff644aa630547494fd79b1854c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 25 Jul 2016 13:50:13 +0200 Subject: [PATCH 1/3] sssctl: Consistent commands naming Fixes: https://fedorahosted.org/sssd/ticket/3087 Use TOPIC-ACTION pattern for sssctl command names. --- src/tools/common/sss_tools.c | 4 src/tools/common/sss_tools.h | 8 ++-- src/tools/sss_override.c | 26 src/tools/sssctl/sssctl.c | 33 -- src/tools/sssctl/sssctl.h | 42 +++ src/tools/sssctl/sssctl_cache.c | 18 - src/tools/sssctl/sssctl_data.c| 16 +++ src/tools/sssctl/sssctl_domains.c | 6 +++--- src/tools/sssctl/sssctl_logs.c| 4 ++-- 9 files changed, 88 insertions(+), 69
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/28/2016 01:57 PM, Jakub Hrozek wrote: On Thu, Jul 28, 2016 at 01:51:40PM +0200, Pavel Březina wrote: On 07/28/2016 01:38 PM, Jakub Hrozek wrote: On Thu, Jul 28, 2016 at 12:23:24PM +0200, Michal Židek wrote: On 07/28/2016 10:00 AM, Pavel Březina wrote: On 07/27/2016 03:28 PM, Michal Židek wrote: On 07/27/2016 11:09 AM, Jakub Hrozek wrote: On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina wrote: On 07/26/2016 04:19 PM, Michal Židek wrote: On 07/26/2016 01:19 PM, Pavel Březina wrote: On 07/25/2016 02:12 PM, Michal Židek wrote: Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal Hi, ok, it looks like a good idea. When touching the code, can you also convert sss_override command to use the macro instead? And I think it may be nice to also add a macro for command sentinel i.e. for {NULL, ...}. I'm not very fond of renaming local-data-* to cache-* so it doesn't imply that we backup the whole cache content. We only backup and restore data that are local to the client and not present in LDAP. Currently only local overrides, but it may include local users and groups in the future. When we have the files provider there would be a cache as well. Moreover, we store secrets now. The restore command backs up all *.ldb files, right? This is how I understood it at first, but the current backup and restore only work for local overrides. But as Pavel mentioned, it may work for local users and groups in the future (id_provider=local). My original confusion was that I also thought it backs-up and restores all ldb files, which is not the case. No, the intention is to backup only data that are not stored on server and would be lost when the cache is removed. In the time, only local overrides were local. If secrets creates local data, the tool should be modified. Yes, but users and groups from local domain would also be lost if the local data is deleted. So I though we want to backup them as well in the future versions (I mean the local provider, not the files provider). Well, probably not in the first version, but definitely in a couple of months we want to add the capability to set extended attributes to the files provider which we'll want to back up as well. But I also think we will add the additional data into a new directory, just like we did with the secrets database. Btw. do we want to merge sss_override tool into sssctl? Because if the local-data-* commands work currently with overrides only, then we could make a new topic 'overrides' and add commands like I would like to merge all tools into sssctl unless there is a strong reason to keep them separate (the local domain tools should be kept separate IMO). The reason is simply discoverability, if there is a new sssd release, the admin would just run sssctl and if there are any new tools, their topics would be displayed. (Also I think the code duplication would be reduced as a side-effect). sssctl overrides-backup sssctl overrides-restore and later also all the functionality of sss_overrides sssctl overrides-user-add sssctl overrides-user-del Maybe, but later. etc. This way we could avoid confusion between local-data and cache. If secrets will also create some local data, we will add topic 'secrets' to deal with that separately. sssctl secrets-backup sssctl secrets-restore I think I got lost in the thread :-) What is the benefit of having more backup/restore commands than one that backs up or removes of value under the /var/lib/sss/ structure? So far I can only think of cache being more intuitive to admins than local-data. How about client-data instead of local-data? SGTM (sounds good to me :-)) SGTM too :) I will send version with client-data after the meeting. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/28/2016 10:00 AM, Pavel Březina wrote: On 07/27/2016 03:28 PM, Michal Židek wrote: On 07/27/2016 11:09 AM, Jakub Hrozek wrote: On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina wrote: On 07/26/2016 04:19 PM, Michal Židek wrote: On 07/26/2016 01:19 PM, Pavel Březina wrote: On 07/25/2016 02:12 PM, Michal Židek wrote: Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal Hi, ok, it looks like a good idea. When touching the code, can you also convert sss_override command to use the macro instead? And I think it may be nice to also add a macro for command sentinel i.e. for {NULL, ...}. I'm not very fond of renaming local-data-* to cache-* so it doesn't imply that we backup the whole cache content. We only backup and restore data that are local to the client and not present in LDAP. Currently only local overrides, but it may include local users and groups in the future. When we have the files provider there would be a cache as well. Moreover, we store secrets now. The restore command backs up all *.ldb files, right? This is how I understood it at first, but the current backup and restore only work for local overrides. But as Pavel mentioned, it may work for local users and groups in the future (id_provider=local). My original confusion was that I also thought it backs-up and restores all ldb files, which is not the case. No, the intention is to backup only data that are not stored on server and would be lost when the cache is removed. In the time, only local overrides were local. If secrets creates local data, the tool should be modified. Yes, but users and groups from local domain would also be lost if the local data is deleted. So I though we want to backup them as well in the future versions (I mean the local provider, not the files provider). Btw. do we want to merge sss_override tool into sssctl? Because if the local-data-* commands work currently with overrides only, then we could make a new topic 'overrides' and add commands like sssctl overrides-backup sssctl overrides-restore and later also all the functionality of sss_overrides sssctl overrides-user-add sssctl overrides-user-del etc. This way we could avoid confusion between local-data and cache. If secrets will also create some local data, we will add topic 'secrets' to deal with that separately. sssctl secrets-backup sssctl secrets-restore We can then keep the topic 'local' for the local provider related tools (if we want to merge them at some point). Like sssctl local-user-add (to replace sss_useradd) sssctl local-user-del (to replace sss_userdel) etc. What do you think? * cache-backup Backup local data * cache-restore Restore local data from backup That was confusion on my side. I thought local data means entire cache. In that case I would propose topic name "local" for actions that work with data that is not present on remote server. local-backup local-restore What do you think? New patch is attached. I also added patch for missing gettext macro (did not want to hide this change in the first patch). Michal I'd still stick with local-data-backup and local-data-restore, what do others thing? But otherwise ack. I'm not sure, on one hand the admins would think 'cache' but on the other hand, if we save and restore all cache data, the we operate on more than the cache.. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/27/2016 11:09 AM, Jakub Hrozek wrote: On Wed, Jul 27, 2016 at 11:03:34AM +0200, Pavel Březina wrote: On 07/26/2016 04:19 PM, Michal Židek wrote: On 07/26/2016 01:19 PM, Pavel Březina wrote: On 07/25/2016 02:12 PM, Michal Židek wrote: Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal Hi, ok, it looks like a good idea. When touching the code, can you also convert sss_override command to use the macro instead? And I think it may be nice to also add a macro for command sentinel i.e. for {NULL, ...}. I'm not very fond of renaming local-data-* to cache-* so it doesn't imply that we backup the whole cache content. We only backup and restore data that are local to the client and not present in LDAP. Currently only local overrides, but it may include local users and groups in the future. When we have the files provider there would be a cache as well. Moreover, we store secrets now. The restore command backs up all *.ldb files, right? This is how I understood it at first, but the current backup and restore only work for local overrides. But as Pavel mentioned, it may work for local users and groups in the future (id_provider=local). My original confusion was that I also thought it backs-up and restores all ldb files, which is not the case. * cache-backup Backup local data * cache-restore Restore local data from backup That was confusion on my side. I thought local data means entire cache. In that case I would propose topic name "local" for actions that work with data that is not present on remote server. local-backup local-restore What do you think? New patch is attached. I also added patch for missing gettext macro (did not want to hide this change in the first patch). Michal I'd still stick with local-data-backup and local-data-restore, what do others thing? But otherwise ack. I'm not sure, on one hand the admins would think 'cache' but on the other hand, if we save and restore all cache data, the we operate on more than the cache.. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] Change debug level of config error msgs
Hi, I believe that this patch makes pinpointing of config errors a little easier. Especially when using sssctl tool that currently refuses to start a command when there are syntax errors in sssd.conf, but by default it does not print problematic line number. Compare: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:05:39:185114 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:05:39:185192 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:05:39:185229 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:05:39:185255 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:05:39:185288 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error and: ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header (Wed Jul 27 14:22:51:096949 2016) [sssd] [sss_ini_get_config] (0x0010): Failed to parse configuration. Error 5. (Wed Jul 27 14:22:51:097173 2016) [sssd] [sss_ini_get_config] (0x0010): Errors detected while parsing: /etc/sssd/sssd.conf (Wed Jul 27 14:22:51:097490 2016) [sssd] [sss_ini_config_print_errors] (0x0010): Error (2) on line 10: No closing bracket. (Wed Jul 27 14:22:51:097946 2016) [sssd] [confdb_init_db] (0x0010): Failed to load configuration (Wed Jul 27 14:22:51:098452 2016) [sssd] [confdb_setup] (0x0010): ConfDB initialization has failed [5]: Input/output error (Wed Jul 27 14:22:51:098651 2016) [sssd] [sss_tool_confdb_init] (0x0010): Unable to setup ConfDB [5]: Input/output error Patch is attached. Michal >From 85f45ddb9b0c270c684ee6eae3da0baa14a3c1de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Wed, 27 Jul 2016 14:14:33 +0200 Subject: [PATCH] Change debug level of config error msgs Syntax errors in configuration files prevent SSSD or sssctl to start completely. It would be good to display these errors by default with the highest level. --- src/util/sss_ini.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index d9bc46a..e56006c 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -182,7 +182,7 @@ int sss_ini_get_mtime(struct sss_ini_initdata *init_data, /* Print ini_config errors */ -void sss_ini_config_print_errors(char **error_list) +static void sss_ini_config_print_errors(char **error_list) { #ifdef HAVE_LIBINI_CONFIG_V1 unsigned count = 0; @@ -192,7 +192,7 @@ void sss_ini_config_print_errors(char **error_list) } while (error_list[count]) { -DEBUG(SSSDBG_CRIT_FAILURE, "%s\n", error_list[count]); +DEBUG(SSSDBG_FATAL_FAILURE, "%s\n", error_list[count]); count++; } #endif -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] sssctl: Generic help for cache-upgrade and config-check
Hi! Attached is patch for ticket: https://fedorahosted.org/sssd/ticket/3086 This patch applies on top of the patches from thread: [SSSD] [PATCH] sssctl: Consistent commands naming Michal >From c7d59835f5d05d6972e29f0d0bc974bd00155189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Tue, 26 Jul 2016 16:35:55 +0200 Subject: [PATCH] sssctl: Generic help for cache-upgrade and config-check Fixes: https://fedorahosted.org/sssd/ticket/3086 sssctl COMMAND --help should print at least generic help, even if the command does not accept any command specific options. --- src/tools/sssctl/sssctl_config.c | 11 +++ src/tools/sssctl/sssctl_data.c | 11 +++ 2 files changed, 22 insertions(+) diff --git a/src/tools/sssctl/sssctl_config.c b/src/tools/sssctl/sssctl_config.c index a66d774..05054bc 100644 --- a/src/tools/sssctl/sssctl_config.c +++ b/src/tools/sssctl/sssctl_config.c @@ -47,6 +47,17 @@ errno_t sssctl_config_check(struct sss_cmdline *cmdline, char **strs = NULL; TALLOC_CTX *tmp_ctx = NULL; +/* Parse command line. */ +struct poptOption options[] = { +POPT_TABLEEND +}; + +ret = sss_tool_popt(cmdline, options, SSS_TOOL_OPT_OPTIONAL, NULL, NULL); +if (ret != EOK) { +DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n"); +return ret; +} + tmp_ctx = talloc_new(NULL); init_data = sss_ini_initdata_init(tmp_ctx); if (!init_data) { diff --git a/src/tools/sssctl/sssctl_data.c b/src/tools/sssctl/sssctl_data.c index 7a0f1b2..c4f09fd 100644 --- a/src/tools/sssctl/sssctl_data.c +++ b/src/tools/sssctl/sssctl_data.c @@ -266,6 +266,17 @@ errno_t sssctl_cache_upgrade(struct sss_cmdline *cmdline, struct sysdb_upgrade_ctx db_up_ctx; errno_t ret; +/* Parse command line. */ +struct poptOption options[] = { +POPT_TABLEEND +}; + +ret = sss_tool_popt(cmdline, options, SSS_TOOL_OPT_OPTIONAL, NULL, NULL); +if (ret != EOK) { +DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n"); +return ret; +} + if (sss_deamon_running()) { return ERR_SSSD_RUNNING; } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Consistent commands naming
On 07/26/2016 01:19 PM, Pavel Březina wrote: On 07/25/2016 02:12 PM, Michal Židek wrote: Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal Hi, ok, it looks like a good idea. When touching the code, can you also convert sss_override command to use the macro instead? And I think it may be nice to also add a macro for command sentinel i.e. for {NULL, ...}. I'm not very fond of renaming local-data-* to cache-* so it doesn't imply that we backup the whole cache content. We only backup and restore data that are local to the client and not present in LDAP. Currently only local overrides, but it may include local users and groups in the future. * cache-backup Backup local data * cache-restore Restore local data from backup That was confusion on my side. I thought local data means entire cache. In that case I would propose topic name "local" for actions that work with data that is not present on remote server. local-backup local-restore What do you think? New patch is attached. I also added patch for missing gettext macro (did not want to hide this change in the first patch). Michal >From e5ccd4ceaeb8a185374f859a8e769f822a32426e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 25 Jul 2016 13:50:13 +0200 Subject: [PATCH 1/2] sssctl: Consistent commands naming Fixes: https://fedorahosted.org/sssd/ticket/3087 Use TOPIC-ACTION pattern for sssctl command names. --- src/tools/common/sss_tools.c | 4 src/tools/common/sss_tools.h | 8 +-- src/tools/sss_override.c | 26 +++ src/tools/sssctl/sssctl.c | 33 +++-- src/tools/sssctl/sssctl.h | 44 +++ src/tools/sssctl/sssctl_cache.c | 18 src/tools/sssctl/sssctl_data.c| 16 +++--- src/tools/sssctl/sssctl_domains.c | 6 +++--- src/tools/sssctl/sssctl_logs.c| 4 ++-- 9 files changed, 89 insertions(+), 70 deletions(-) diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 686b53a..4977358 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -283,6 +283,10 @@ void sss_tool_usage(const char *tool_name, struct sss_route_cmd *commands) min_len = sss_tool_max_length(commands); for (i = 0; commands[i].command != NULL; i++) { +if (commands[i].hidden) { +continue; +} + if (sss_tool_is_delimiter([i])) { fprintf(stderr, "\n%s\n", commands[i].description); continue; diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h index a9ebabe..b567ea4 100644 --- a/src/tools/common/sss_tools.h +++ b/src/tools/common/sss_tools.h @@ -45,14 +45,18 @@ typedef errno_t struct sss_tool_ctx *tool_ctx, void *pvt); -#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn} -#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL} +#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn, false} +#define SSS_TOOL_COMMAND_NOMSG(cmd, err, fn) {cmd, NULL, err, fn, false} +#define SSS_TOOL_COMMAND_OBSOLETE(cmd, err, fn) {cmd, NULL, err, fn, true} +#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL, false} +#define SSS_TOOL_LAST {NULL, NULL, 0, NULL, false} struct sss_route_cmd { const char *command; const char *description; errno_t handles_init_err; sss_route_fn fn; +bool hidden; /* Do not show this command in sssctl usage */ }; void sss_tool_usage(const char *tool_name, diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c index 45a28fd..d41da52 100644 --- a/src/tools/sss_override.c +++ b/src/tools/sss_override.c @@ -1913,19 +1913,19 @@ static int override_group_export(struct sss_cmdline *cmdline, int main(int argc, const char **argv) { struct sss_route_cmd commands[] = { -{"user-add", NULL, 0, override_user_add}, -{"user-del", NULL, 0, override_user_del}, -{"user-find", NULL, 0, override_user_find}, -{"user-show", NULL, 0, override_user_show}, -{"user-import", NULL, 0, override_user_import}, -{"user-export", NULL, 0, override_user_export}, -{"group-add", NULL, 0, override_group_add}, -{"group-del", NULL, 0, override_group_del}, -{"group-find", NULL, 0, override_group_find}, -{"group-show", NULL, 0, override_group_show}, -{"group-import", NULL, 0, override_group_import}, -{"group-export", NULL, 0
[SSSD] [PATCH] sssctl: Consistent commands naming
Hi, this patches makes the sssctl commands more similar to ipa tool commands. I also think this pattern makes it easier to remember the commands. Note that in the future, there will be more user-* group-* and netgroup-* commands (like seed for user, list of all etc.) Comments are welcome. Michal >From 52c7142c5fd301ce5203a29b8308ce7f4aabd8df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Mon, 25 Jul 2016 13:50:13 +0200 Subject: [PATCH] sssctl: Consistent commands naming Fixes: https://fedorahosted.org/sssd/ticket/3087 Use TOPIC-ACTION pattern for sssctl command names. --- src/tools/common/sss_tools.c | 4 src/tools/common/sss_tools.h | 6 -- src/tools/sss_override.c | 26 +++ src/tools/sssctl/sssctl.c | 33 +++-- src/tools/sssctl/sssctl.h | 44 +++ src/tools/sssctl/sssctl_cache.c | 18 src/tools/sssctl/sssctl_data.c| 16 +++--- src/tools/sssctl/sssctl_domains.c | 6 +++--- src/tools/sssctl/sssctl_logs.c| 4 ++-- 9 files changed, 87 insertions(+), 70 deletions(-) diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c index 686b53a..4977358 100644 --- a/src/tools/common/sss_tools.c +++ b/src/tools/common/sss_tools.c @@ -283,6 +283,10 @@ void sss_tool_usage(const char *tool_name, struct sss_route_cmd *commands) min_len = sss_tool_max_length(commands); for (i = 0; commands[i].command != NULL; i++) { +if (commands[i].hidden) { +continue; +} + if (sss_tool_is_delimiter([i])) { fprintf(stderr, "\n%s\n", commands[i].description); continue; diff --git a/src/tools/common/sss_tools.h b/src/tools/common/sss_tools.h index a9ebabe..8c2872c 100644 --- a/src/tools/common/sss_tools.h +++ b/src/tools/common/sss_tools.h @@ -45,14 +45,16 @@ typedef errno_t struct sss_tool_ctx *tool_ctx, void *pvt); -#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn} -#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL} +#define SSS_TOOL_COMMAND(cmd, msg, err, fn) {cmd, _(msg), err, fn, false} +#define SSS_TOOL_COMMAND_OBSOLETE(cmd, err, fn) {cmd, NULL, err, fn, true} +#define SSS_TOOL_DELIMITER(message) {"", (message), 0, NULL, false} struct sss_route_cmd { const char *command; const char *description; errno_t handles_init_err; sss_route_fn fn; +bool hidden; /* Do not show this command in sssctl usage */ }; void sss_tool_usage(const char *tool_name, diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c index 45a28fd..197ec9a 100644 --- a/src/tools/sss_override.c +++ b/src/tools/sss_override.c @@ -1913,19 +1913,19 @@ static int override_group_export(struct sss_cmdline *cmdline, int main(int argc, const char **argv) { struct sss_route_cmd commands[] = { -{"user-add", NULL, 0, override_user_add}, -{"user-del", NULL, 0, override_user_del}, -{"user-find", NULL, 0, override_user_find}, -{"user-show", NULL, 0, override_user_show}, -{"user-import", NULL, 0, override_user_import}, -{"user-export", NULL, 0, override_user_export}, -{"group-add", NULL, 0, override_group_add}, -{"group-del", NULL, 0, override_group_del}, -{"group-find", NULL, 0, override_group_find}, -{"group-show", NULL, 0, override_group_show}, -{"group-import", NULL, 0, override_group_import}, -{"group-export", NULL, 0, override_group_export}, -{NULL, NULL, 0, NULL} +{"user-add", NULL, 0, override_user_add, false}, +{"user-del", NULL, 0, override_user_del, false}, +{"user-find", NULL, 0, override_user_find, false}, +{"user-show", NULL, 0, override_user_show, false}, +{"user-import", NULL, 0, override_user_import, false}, +{"user-export", NULL, 0, override_user_export, false}, +{"group-add", NULL, 0, override_group_add, false}, +{"group-del", NULL, 0, override_group_del, false}, +{"group-find", NULL, 0, override_group_find, false}, +{"group-show", NULL, 0, override_group_show, false}, +{"group-import", NULL, 0, override_group_import, false}, +{"group-export", NULL, 0, override_group_export, false}, +{NULL, NULL, 0, NULL, false} }; return sss_tool_main(argc, argv, commands, NULL); diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c index 86656f1..4e6ae97 100644 --- a/src/tools/sssctl/sssctl.c +++ b/src/tools/sssctl/sssctl.c @@ -257,25 +257,36 @@ int main(int argc, const char **argv) { struct sss_route_cmd commands[] = { SSS_TOOL_DELIMITER("SSSD Status:"), -SSS_TOOL_COMMAND("list-domains", "List available domains", 0, sssctl_list_domains), +SSS_TOOL_COMMAND("domain-list", "List available domains", 0,
[SSSD] Re: [PATCH] rename be_acct_req to dp_id_data
On 07/14/2016 12:19 PM, Pavel Březina wrote: To follow other names... ACK. CI passed. http://sssd-ci.duckdns.org/logs/job/49/80/summary.html Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Rethinking debug levels
On 07/14/2016 10:12 AM, Pavel Březina wrote: We recently focus on providing lots of tracing messages in new code that helps us follow the code, however this makes the debug logs quite big and we usually focus only on a specific area or task while debugging. A year ago we discussed what we can do to improve our debugging capabilities but it never got into action since we hit the long standing requirement to revisit all debug messages and their levels. 1) Originally, we had only numeric debug levels. The problem here was that those numbers were assigned relatively randomly per developer decision. 2) My first task when I joined the team was to change the levels from numeric to a bit mask. This seemed good at first as it allows to select specific messages that we want to see but it was never polished into a usable feature for two main reasons -- it required us to revisit all existing messages and it turned out it actually doesn't make much sense to disable any lower level since if you want to see tracing message you always want to see error etc. So in the end the bit mask is used in the same way as numeric representation (if used at all) it is just harder to read. My needs when debugging some issue are very simple -- be able to see as small log as possible but having all errors and tracing messages. The problem usually lies in a small area of code and I don't need to see other parts of code that hinders these messages. As a developer I would like the change into new format be seamless so it does not require manual changes into current code. And I would also like to have smaller number of choices. What I propose is this: have a numeric debug level and a bit mask representing an sssd component. *Debug level* 0 = Fatal failures, important messages. Always enabled. For cases that prevents SSSD from starting and for very important messages. 1 = Errors, for any error that we can recover from. 2 = Warnings, for unexpected situations were we can actually continue with the operation. (E.g. when processing bunch of objects in a for cycle and continue on error). 3 = Data, for user input, configuration. 4 = Trace, for tracing code flow. 5 = Verbose, for things that are very rarely required and would just put lots of things into logs (e.g. verbose information about sudo rules). *Component* An SSSD module as separated from the rest as possible. For example: all, failover, sudo, id, subdomains, ldb, ... lots of possibilities. SSSD needs to be started or configured with two options --debug-leve=number, --component=bitmask (default all). Messages with level <= numeric and component & mask are printed. Ideally components can be separated also on makefile level into static libraries that we will link with. This would help with unit testing those parts since we wouldn't have to maintain source file list in unit test definitions but link with a library instead. It would also speed up compilation. This library could have -DSSS_COMPONENT_NAME=$name and we wouldn't even have to put it in DEBUG macro. Separating on makefile level sounds nice. I like the idea of having SSSD split into several libraries. As you mentioned this would speed up the compilation. Also as part of this effort we could clean up the mess in our source file dependencies. But this effort could take more time, maybe we could use something else in the meantime. Ideally something that will be forward compatible with the solution you proposed. What about having the #define SSS_COMPONENT_NAME at the beginning of .c files? We could do this before the split into libraries. When the libraries are made the the #define SSS_COMPONENT_NAME can be removed, because it will be defined in the makefile. I see the work as following: 1. Make DEBUG macro work with SSS_COMPONENT_NAME bitmask defined (as well as undefined - current behavior) 2. Make list of components that we want to be defined. 3. Work on splitting the code into libraries 3a) #defining the component names in .c files (after this we can already use the new debug levels functionality), but we should not yet document the new behavior. 3b) When all are agreed on how the code is split into the components, we can modify the Makefile.am (to actually make the libraries), if not, we can update list from point 2. or rethink the 3a point until developers come to an agreement. 3c) We can remove the #defines from .c files 4. Document the new behavior so that other people can use it. Thoughts? +1 for opening up this discussion. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] sdap: Fix ldap_rfc_2307_fallback_to_local_users
Hi, see the attached simple patch for ticket: https://fedorahosted.org/sssd/ticket/3045 The patch is missing a CI test. I will add one (hopefully later tomorrow) after I take a look at one bugzilla which has currently higher priority. If someone writes a test for this until then, I will gladly review it :) The reproducer is simple: 1. have ldap with RFC2307 schema with group that contains user from /etc/passwd (for example local_user) 2. run 'id local_user' 3. the ldap group should be among the displayed groups Michal >From c324ca57d5bed4ad2a290d819ad84349d45cc669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Wed, 13 Jul 2016 20:02:47 +0200 Subject: [PATCH] sdap: Fix ldap_rfc_2307_fallback_to_local_users Fixes: https://fedorahosted.org/sssd/ticket/3045 We wrongly tried to store empty user attributes instead of the local user attributes with ldap_rfc_2307_fallback_to_local_users set to true. This gave us bad initgroups results and caused segfaults. --- src/providers/ldap/sdap_async_initgroups.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c index d14563c..17593f0 100644 --- a/src/providers/ldap/sdap_async_initgroups.c +++ b/src/providers/ldap/sdap_async_initgroups.c @@ -2893,6 +2893,9 @@ static void sdap_get_initgr_user(struct tevent_req *subreq) (dp_opt_get_bool(state->opts->basic, SDAP_RFC2307_FALLBACK_TO_LOCAL_USERS) == true)) { ret = sdap_fallback_local_user(state, state->shortname, -1, _attrs); +if (ret == EOK) { +state->orig_user = usr_attrs[0]; +} } else { ret = ENOENT; } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] config: Some fixes to schema
On 07/12/2016 03:36 PM, Lukas Slebodnik wrote: On (12/07/16 15:16), Michal Židek wrote: On 07/12/2016 01:28 PM, Lukas Slebodnik wrote: On (11/07/16 07:44), Michal Zidek wrote: Ok, I split the patches (one per option). Michal From 4c11e6cfcfee3cad801d513d75e136e4bd3bd598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/4] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. --- src/config/cfg_rules.ini | 7 +++ src/config/etc/sssd.api.conf | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) Almost ACK I realized that there is an unused macro CONFDB_SERVICE_TIMEOUT Could you remove it in this patch? This macro is not used since commit 31d97bce8f113276bf73c7d4349f720cd5edbcb8 (3+ years) From 851e274f5a8067f10b2fd29acc6a3bfc8da49cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:11:41 +0200 Subject: [PATCH 2/4] config: override_space is monitor's option Fixes: https://fedorahosted.org/sssd/ticket/3068 We read override_space from [sssd] not [nss] section. --- ACK From c478a9440bb50c56c6004da806c0cdf8e9bbcc56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:23:40 +0200 Subject: [PATCH 3/4] config: Fix user_attributes Fixes: https://fedorahosted.org/sssd/ticket/3068 Option user_attributes is also available in NSS responder, but not in PAC responder. --- ACK From ee4449c7b5c6154bfb079725e62874948c42124d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:34:03 +0200 Subject: [PATCH 4/4] config: Add config_file_version to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 --- src/config/cfg_rules.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 5c8d05a..635c078 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -39,6 +39,7 @@ option = user option = default_domain_suffix option = certificate_verification option = override_space +option = config_file_version [rule/allowed_nss_options] validator = ini_allowed_options -- 1.8.3.1 Python API schema is not generated yet therefore we should add this option also to src/config/etc/sssd.api.conf. It was probably removed with the change from default 1 -> 2 BTW. We need to also allow section + add default options for "secrets" service. So you will need to modify the 1st patch. I will push acked patches after CI. LS Sending the patches that were not acked + patch that adds the 'secrets' service. Michal From 42a3038b68452cf92b2f87ae0875f4e3b8b1f051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/3] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. Also remove unused macro CONFDB_SERVICE_TIMEOUT. ACK From cacd9f84e702c2aa7f5c41d0d257eb5ce8c77a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Mon, 11 Jul 2016 13:34:03 +0200 Subject: [PATCH 2/3] config: Add config_file_version to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 --- src/config/SSSDConfigTest.py | 1 + src/config/cfg_rules.ini | 1 + src/config/etc/sssd.api.conf | 1 + 3 files changed, 3 insertions(+) diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 5fa9bce..332d870 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -289,6 +289,7 @@ class SSSDConfigTestSSSDService(unittest.TestCase): options = service.list_options() control_list = [ +'config_file_version', 'services', 'domains', 'timeout', diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 5c8d05a..635c078 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -39,6 +39,7 @@ option = user option = default_domain_suffix option = certificate_verification option = override_space +option = config_file_version [rule/allowed_nss_options] validator = ini_allowed_options diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index e4011a3..737f0e1 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -19,6 +19,7 @@ diag_cmd = str, None, false [sssd] # Monitor service +config_file_version = int, None, false services = list, str, true, nss, pam domains = list, str, true sbus_timeout = int, None, false -- 2.5.0 LGTM. I haven't tested yet. From 27964d4eca57972512f145033b253d0dbf29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 12 Jul 2016
[SSSD] Re: [PATCH] dyndns: Add checks for NULL
On 07/12/2016 01:36 PM, Michal Židek wrote: On 07/12/2016 01:15 PM, Pavel Březina wrote: On 07/12/2016 12:34 PM, Michal Židek wrote: state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL); LGTM but maybe we should place the check before this line? Not sure... I only added checks for the line with strcmp (which is where it segfaulted). If I moved it up where you suggest, there would be the question why I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx. But if you prefer having it there I can do it, but will also add a comment that the checks are relevant for the strcmp line. Michal Ok, I moved it as Pavel suggested and added a comment that should make it clear why the checks are there and also amended the commit message as Jakub suggested. Michal >From 1339f8b24e9dceb4d71010df831fb177df500b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Tue, 12 Jul 2016 12:11:18 +0200 Subject: [PATCH] dyndns: Add checks for NULL Fixes: https://fedorahosted.org/sssd/ticket/3076 We segfaulted in this area once. This patch makes the code more defensive and adds some DEBUG messages. Normally the structures are filled in online and/or resolve callbacks. --- src/providers/ipa/ipa_dyndns.c | 20 1 file changed, 20 insertions(+) diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c index 7217c61..dc91077 100644 --- a/src/providers/ipa/ipa_dyndns.c +++ b/src/providers/ipa/ipa_dyndns.c @@ -162,6 +162,26 @@ ipa_dyndns_update_send(struct ipa_options *ctx) } state->ipa_ctx = ctx; +/* The following three checks are here to prevent SEGFAULT + * from ticket #3076. */ +if (ctx->service == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "service structure not initialized\n"); +ret = EINVAL; +goto done; +} + +if (ctx->service->sdap == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "sdap structure not initialized\n"); +ret = EINVAL; +goto done; +} + +if (ctx->service->sdap->uri == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "LDAP uri not set\n"); +ret = EINVAL; +goto done; +} + if (ctx->dyndns_ctx->last_refresh + 60 > time(NULL) || ctx->dyndns_ctx->timer_in_progress) { DEBUG(SSSDBG_FUNC_DATA, "Last periodic update ran recently or timer " -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] config: Some fixes to schema
On 07/12/2016 01:28 PM, Lukas Slebodnik wrote: On (11/07/16 07:44), Michal Zidek wrote: Ok, I split the patches (one per option). Michal From 4c11e6cfcfee3cad801d513d75e136e4bd3bd598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/4] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. --- src/config/cfg_rules.ini | 7 +++ src/config/etc/sssd.api.conf | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) Almost ACK I realized that there is an unused macro CONFDB_SERVICE_TIMEOUT Could you remove it in this patch? This macro is not used since commit 31d97bce8f113276bf73c7d4349f720cd5edbcb8 (3+ years) From 851e274f5a8067f10b2fd29acc6a3bfc8da49cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Mon, 11 Jul 2016 13:11:41 +0200 Subject: [PATCH 2/4] config: override_space is monitor's option Fixes: https://fedorahosted.org/sssd/ticket/3068 We read override_space from [sssd] not [nss] section. --- ACK From c478a9440bb50c56c6004da806c0cdf8e9bbcc56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Mon, 11 Jul 2016 13:23:40 +0200 Subject: [PATCH 3/4] config: Fix user_attributes Fixes: https://fedorahosted.org/sssd/ticket/3068 Option user_attributes is also available in NSS responder, but not in PAC responder. --- ACK From ee4449c7b5c6154bfb079725e62874948c42124d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Mon, 11 Jul 2016 13:34:03 +0200 Subject: [PATCH 4/4] config: Add config_file_version to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 --- src/config/cfg_rules.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 5c8d05a..635c078 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -39,6 +39,7 @@ option = user option = default_domain_suffix option = certificate_verification option = override_space +option = config_file_version [rule/allowed_nss_options] validator = ini_allowed_options -- 1.8.3.1 Python API schema is not generated yet therefore we should add this option also to src/config/etc/sssd.api.conf. It was probably removed with the change from default 1 -> 2 BTW. We need to also allow section + add default options for "secrets" service. So you will need to modify the 1st patch. I will push acked patches after CI. LS Sending the patches that were not acked + patch that adds the 'secrets' service. Michal >From 42a3038b68452cf92b2f87ae0875f4e3b8b1f051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Mon, 11 Jul 2016 13:03:28 +0200 Subject: [PATCH 1/3] config: Allow timeout for all sevices Fixes: https://fedorahosted.org/sssd/ticket/3068 Allow option "timeout" for all sevices. Also remove unused macro CONFDB_SERVICE_TIMEOUT. --- src/confdb/confdb.h | 1 - src/config/cfg_rules.ini | 7 +++ src/config/etc/sssd.api.conf | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 54b1cbc..cc8f66f 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -58,7 +58,6 @@ #define CONFDB_SERVICE_DEBUG_TIMESTAMPS "debug_timestamps" #define CONFDB_SERVICE_DEBUG_MICROSECONDS "debug_microseconds" #define CONFDB_SERVICE_DEBUG_TO_FILES "debug_to_files" -#define CONFDB_SERVICE_TIMEOUT "timeout" #define CONFDB_SERVICE_FORCE_TIMEOUT "force_timeout" #define CONFDB_SERVICE_RECON_RETRIES "reconnection_retries" #define CONFDB_SERVICE_FD_LIMIT "fd_limit" diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 85a15be..5c8d05a 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -44,6 +44,7 @@ option = override_space validator = ini_allowed_options section_re = ^nss$ +option = timeout option = debug option = debug_level option = debug_timestamps @@ -82,6 +83,7 @@ option = memcache_timeout validator = ini_allowed_options section_re = ^pam$ +option = timeout option = debug option = debug_level option = debug_timestamps @@ -115,6 +117,7 @@ option = p11_child_timeout validator = ini_allowed_options section_re = ^sudo$ +option = timeout option = debug option = debug_level option = debug_timestamps @@ -136,6 +139,7 @@ option = sudo_inverse_order validator = ini_allowed_options section_re = ^autofs$ +option = timeout option = debug option = debug_level option = debug_timestamps @@ -156,6 +160,7 @@ option = autofs_negative_timeout validator = ini_allowed_options section_re = ^ssh$ +option = timeout option = debug option = debug_level option = debug_timestamps @@ -178,6 +183,7 @@ option = ca_db validator = ini_allowed_options section_re = ^pac$ +option = timeout option = debug option = debug_level option = debug_timestamps @@
[SSSD] Re: [PATCH] dyndns: Add checks for NULL
On 07/12/2016 01:15 PM, Pavel Březina wrote: On 07/12/2016 12:34 PM, Michal Židek wrote: state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL); LGTM but maybe we should place the check before this line? Not sure... I only added checks for the line with strcmp (which is where it segfaulted). If I moved it up where you suggest, there would be the question why I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx. But if you prefer having it there I can do it, but will also add a comment that the checks are relevant for the strcmp line. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] dyndns: Add checks for NULL
Hi! The attached simple patch just makes the code more defensive. We do not know the real cause for the segfault and we do not have a reproducer. Michal >From fffc09ee7de730df5700cac61fbf71d43de473a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Tue, 12 Jul 2016 12:11:18 +0200 Subject: [PATCH] dyndns: Add checks for NULL Fixes: https://fedorahosted.org/sssd/ticket/3076 We segfaulted in this area once. This patch makes the code more defensive and adds some DEBUG messages. --- src/providers/ipa/ipa_dyndns.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c index 7217c61..1d5d7a0 100644 --- a/src/providers/ipa/ipa_dyndns.c +++ b/src/providers/ipa/ipa_dyndns.c @@ -172,6 +172,24 @@ ipa_dyndns_update_send(struct ipa_options *ctx) } state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL); +if (ctx->service == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "service structure not initialized\n"); +ret = EINVAL; +goto done; +} + +if (ctx->service->sdap == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "sdap structure not initialized\n"); +ret = EINVAL; +goto done; +} + +if (ctx->service->sdap->uri == NULL) { +DEBUG(SSSDBG_CRIT_FAILURE, "LDAP uri not set\n"); +ret = EINVAL; +goto done; +} + if (strncmp(ctx->service->sdap->uri, "ldap://;, 7) != 0) { DEBUG(SSSDBG_CRIT_FAILURE, "Unexpected format of LDAP URI.\n"); -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] config: Some fixes to schema
On 07/08/2016 12:56 PM, Lukas Slebodnik wrote: On (08/07/16 12:17), Michal Židek wrote: From 66419775a94768efe8c98ce6e8bbfa4743107eae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 8 Jul 2016 11:32:50 +0200 Subject: [PATCH] config: Some fixes to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 Option "timeout" must be allowed for all responders. Option "user_attributes" is also available in NSS responder. Option override_space is read from monitor section, not NSS. --- src/config/cfg_rules.ini | 10 +- src/config/etc/sssd.api.conf | 10 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index 9114659..03e5a72 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -28,9 +28,12 @@ krb5_rcache_dir = str, None, false user = str, None, false default_domain_suffix = str, None, false certificate_verification = str, None, false +override_space = str, None, false // snip [ifp] # InfoPipe responder +timeout = int, None, false IMHO, it would be better to add it to the section "[service]" The same as debug_level. We would avoid some duplication LS new patch attached. There was one more error with the PAC responder and user_attributes option. Michal >From 3f1466274d70e3e275b53bab99549c3e0d4f2a30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 8 Jul 2016 11:32:50 +0200 Subject: [PATCH] config: Some fixes to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 Option "timeout" must be allowed for all responders. Option "user_attributes" is also available in NSS responder. Option override_space is read from monitor section, not NSS. --- src/config/SSSDConfig/__init__.py.in | 3 ++- src/config/SSSDConfigTest.py | 3 ++- src/config/cfg_rules.ini | 11 +-- src/config/etc/sssd.api.conf | 6 +++--- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index 52af138..ea29000 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -63,6 +63,7 @@ option_strings = { 'default_domain_suffix' : _('Domain to add to names without a domain component.'), 'user' : _('The user to drop privileges to'), 'certificate_verification' : _('Tune certificate verification'), +'override_space': _('All spaces in group or user names will be replaced with this character'), # [nss] 'enum_cache_timeout' : _('Enumeration cache timeout length (seconds)'), @@ -81,7 +82,7 @@ option_strings = { 'shell_fallback' : _('If a shell stored in central directory is allowed but not available, use this fallback'), 'default_shell': _('Shell to use if the provider does not list one'), 'memcache_timeout': _('How long will be in-memory cache records valid'), -'override_space': _('All spaces in group or user names will be replaced with this character'), +'user_attributes': _('List of user attributes the NSS is allowed to publish'), # [pam] 'offline_credentials_expiration' : _('How long to allow cached logins between online logins (days)'), diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 6ec3023..5fa9bce 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -310,7 +310,8 @@ class SSSDConfigTestSSSDService(unittest.TestCase): 'client_idle_timeout', 'diag_cmd', 'description', -'certificate_verification'] +'certificate_verification', +'override_space'] self.assertTrue(type(options) == dict, "Options should be a dictionary") diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index d738ddf..bcc3d2e 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -38,6 +38,7 @@ option = krb5_rcache_dir option = user option = default_domain_suffix option = certificate_verification +option = override_space [rule/allowed_nss_options] validator = ini_allowed_options @@ -57,6 +58,8 @@ option = description option = diag_cmd # Name service +option = timeout +option = user_attributes option = enum_cache_timeout option = entry_cache_nowait_percentage option = entry_negative_timeout @@ -75,7 +78,6 @@ option = shell_fallback option = default_shell option = get_domains_timeout option = memcache_timeout -option = override_space [rule/allowed_pam_options] validator = ini_allowed_options @@ -95,6 +97,7 @@ option = description option = diag_cmd # Authentication service +option = timeout option = offline_credentials_expiration option = offline_failed_login_attempts option = offline_failed_login_delay @@ -128,6 +131,7 @@ option = description option
[SSSD] Re: [PATCH] config: Some fixes to schema
On 07/08/2016 12:07 PM, Lukas Slebodnik wrote: On (08/07/16 12:03), Michal Židek wrote: Hi, attached is patch for ticket https://fedorahosted.org/sssd/ticket/3068 The ticket also talks about allowing options for negative cache timeouts in all responders, but I did not do that. We do indeed initialize negative cache in all responders, but we always read the timeouts from NSS section. Also in the man pages, we only document these options for NSS. So it is not problem with the schema. I do agree that this is not ideal, but should we fix it? I do not think it is worth the time, but if someone thinks otherwise, please open a ticket. Michal From 2c41d136c7d43ad089510cc9fb3ae5e870400791 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 8 Jul 2016 11:32:50 +0200 Subject: [PATCH] config: Some fixes to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 Option "timeout" must be allowed for all responders. Option "user_attributes" is also available in NSS responder. Option override_space is read from monitor section, not NSS. --- src/config/cfg_rules.ini | 10 +- This file was created from files in src/config/etc/ Therefore there are missing options in some sections. Please update these files as well because we still cannot autogenerate them. LS Thanks for noticing this. New patch attached. Michal >From 66419775a94768efe8c98ce6e8bbfa4743107eae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 8 Jul 2016 11:32:50 +0200 Subject: [PATCH] config: Some fixes to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 Option "timeout" must be allowed for all responders. Option "user_attributes" is also available in NSS responder. Option override_space is read from monitor section, not NSS. --- src/config/cfg_rules.ini | 10 +- src/config/etc/sssd.api.conf | 10 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index d738ddf..6e8258e 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -38,6 +38,7 @@ option = krb5_rcache_dir option = user option = default_domain_suffix option = certificate_verification +option = override_space [rule/allowed_nss_options] validator = ini_allowed_options @@ -57,6 +58,8 @@ option = description option = diag_cmd # Name service +option = timeout +option = user_attributes option = enum_cache_timeout option = entry_cache_nowait_percentage option = entry_negative_timeout @@ -75,7 +78,6 @@ option = shell_fallback option = default_shell option = get_domains_timeout option = memcache_timeout -option = override_space [rule/allowed_pam_options] validator = ini_allowed_options @@ -95,6 +97,7 @@ option = description option = diag_cmd # Authentication service +option = timeout option = offline_credentials_expiration option = offline_failed_login_attempts option = offline_failed_login_delay @@ -128,6 +131,7 @@ option = description option = diag_cmd # sudo service +option = timeout option = sudo_timed option = sudo_inverse_order @@ -149,6 +153,7 @@ option = description option = diag_cmd # autofs service +option = timeout option = autofs_negative_timeout [rule/allowed_ssh_options] @@ -169,6 +174,7 @@ option = description option = diag_cmd # ssh service +option = timeout option = ssh_hash_known_hosts option = ssh_known_hosts_timeout option = ca_db @@ -191,6 +197,7 @@ option = description option = diag_cmd # PAC responder +option = timeout option = allowed_uids option = user_attributes option = pac_lifetime @@ -213,6 +220,7 @@ option = description option = diag_cmd # InfoPipe responder +option = timeout option = allowed_uids option = user_attributes diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index 9114659..03e5a72 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -28,9 +28,12 @@ krb5_rcache_dir = str, None, false user = str, None, false default_domain_suffix = str, None, false certificate_verification = str, None, false +override_space = str, None, false [nss] # Name service +timeout = int, None, false +user_attributes = str, None, false enum_cache_timeout = int, None, false entry_cache_nowait_percentage = int, None, false entry_negative_timeout = int, None, false @@ -49,10 +52,10 @@ shell_fallback = str, None, false default_shell = str, None, false get_domains_timeout = int, None, false memcache_timeout = int, None, false -override_space = str, None, false [pam] # Authentication service +timeout = int, None, false offline_credentials_expiration = int, None, false offline_failed_login_attempts = int, None, false offline_failed_login_delay = int, None, false @@ -70,27 +73,32 @@ p11_child_timeout = int, None, false [sudo] # sudo service +timeout = int, None, false sudo_timed = bo
[SSSD] [PATCH] config: Some fixes to schema
Hi, attached is patch for ticket https://fedorahosted.org/sssd/ticket/3068 The ticket also talks about allowing options for negative cache timeouts in all responders, but I did not do that. We do indeed initialize negative cache in all responders, but we always read the timeouts from NSS section. Also in the man pages, we only document these options for NSS. So it is not problem with the schema. I do agree that this is not ideal, but should we fix it? I do not think it is worth the time, but if someone thinks otherwise, please open a ticket. Michal >From 2c41d136c7d43ad089510cc9fb3ae5e870400791 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Fri, 8 Jul 2016 11:32:50 +0200 Subject: [PATCH] config: Some fixes to schema Fixes: https://fedorahosted.org/sssd/ticket/3068 Option "timeout" must be allowed for all responders. Option "user_attributes" is also available in NSS responder. Option override_space is read from monitor section, not NSS. --- src/config/cfg_rules.ini | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index d738ddf..6e8258e 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -38,6 +38,7 @@ option = krb5_rcache_dir option = user option = default_domain_suffix option = certificate_verification +option = override_space [rule/allowed_nss_options] validator = ini_allowed_options @@ -57,6 +58,8 @@ option = description option = diag_cmd # Name service +option = timeout +option = user_attributes option = enum_cache_timeout option = entry_cache_nowait_percentage option = entry_negative_timeout @@ -75,7 +78,6 @@ option = shell_fallback option = default_shell option = get_domains_timeout option = memcache_timeout -option = override_space [rule/allowed_pam_options] validator = ini_allowed_options @@ -95,6 +97,7 @@ option = description option = diag_cmd # Authentication service +option = timeout option = offline_credentials_expiration option = offline_failed_login_attempts option = offline_failed_login_delay @@ -128,6 +131,7 @@ option = description option = diag_cmd # sudo service +option = timeout option = sudo_timed option = sudo_inverse_order @@ -149,6 +153,7 @@ option = description option = diag_cmd # autofs service +option = timeout option = autofs_negative_timeout [rule/allowed_ssh_options] @@ -169,6 +174,7 @@ option = description option = diag_cmd # ssh service +option = timeout option = ssh_hash_known_hosts option = ssh_known_hosts_timeout option = ca_db @@ -191,6 +197,7 @@ option = description option = diag_cmd # PAC responder +option = timeout option = allowed_uids option = user_attributes option = pac_lifetime @@ -213,6 +220,7 @@ option = description option = diag_cmd # InfoPipe responder +option = timeout option = allowed_uids option = user_attributes -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: config-check access check report
On 07/07/2016 04:03 PM, Michal Židek wrote: Hi, Pavel requested this small change to sssctl config-check command. Michal This email got lost yesterday. Sending again. Michal >From 07bb03731ef425fecf01cbaefc533c28d4875151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Thu, 7 Jul 2016 15:43:11 +0200 Subject: [PATCH] sssctl: config-check access check report Improve output when access check error is detected by sssctl config-check command. --- src/tools/sssctl/sssctl_config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/sssctl/sssctl_config.c b/src/tools/sssctl/sssctl_config.c index fc13582..4f6dbcd 100644 --- a/src/tools/sssctl/sssctl_config.c +++ b/src/tools/sssctl/sssctl_config.c @@ -68,7 +68,8 @@ errno_t sssctl_config_check(struct sss_cmdline *cmdline, /* Check the file permissions */ ret = sss_ini_config_access_check(init_data); if (ret != EOK) { -printf(_("Access check on sssd.conf file failed.\n")); +printf(_("File ownership and permissions check failed. " + "Expected root:root and 0600.\n")); ret = EPERM; goto done; } -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: manual page
On 07/07/2016 07:12 PM, Michal Židek wrote: On 07/07/2016 06:45 PM, Michal Židek wrote: The man page looks good to me with exception for one detail (see inline) On 07/04/2016 12:45 PM, Pavel Březina wrote: + +COMMON OPTIONS + +Those options are available with all commands. + + + + +--debug +LEVEL + +http://www.w3.org/2001/XInclude; href="include/debug_levels.xml" /> This include contains some irrelevant information for sssctl that could be confusing for users. But I do not think we should block the patch, I will create a ticket to track this as a man page bug. + + + + +http://www.w3.org/2001/XInclude; href="include/seealso.xml" /> + The attached patch is the same as Pavel's, with some whitespaces removed (git complaint about them). Otherwise LGTM ( == ACK, because we do not have much time and I think it is better than nothing). Michal The mailman is very slow today for some mails. Sending again with the hope it will arrive sooner. Michal Third time... ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Add config-check command
On 07/07/2016 02:15 PM, Pavel Březina wrote: On 07/07/2016 01:54 PM, Michal Židek wrote: On 07/07/2016 01:20 PM, Lukas Slebodnik wrote: On (07/07/16 12:37), Michal Židek wrote: On 07/04/2016 05:27 PM, Michal Židek wrote: On 07/01/2016 05:35 PM, Lukas Slebodnik wrote: On (01/07/16 17:26), Michal Židek wrote: Hello! This patch adds new command config-check for sssctl tool. The output looks like this: Issues identified by validators: 3 [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for typos. [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section 'sssd'. Check for typos. [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in section 'sssd'. Check for typos. Messages generated during configuration merging: 4 File conf.dd did not match provided patterns. Skipping. Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf. Error (5) on line 5: Equal sign is missing. Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. Skipping. Used configuration snippet files: 1 /etc/sssd/conf.d/snip-good.conf Currently it can only be used by root and checks only configuration in default path. Michal From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 15:10:30 +0200 Subject: [PATCH 1/4] sss_ini: Small refacoring of sss_ini_call_validators Separate logic to fill errobj so that the errors can be printed by the caller. --- src/util/sss_ini.c | 49 - src/util/sss_ini.h | 12 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index b4dbb07..78f5490 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, { #ifdef HAVE_LIBINI_CONFIG_V1_3 int ret; -struct ini_cfgobj *rules_cfgobj = NULL; struct ini_errobj *errobj = NULL; ret = ini_errobj_create(); @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, goto done; } -ret = ini_rules_read_from_file(rules_path, _cfgobj); +ret = sss_ini_call_validators_errobj(data, + rules_path, + errobj); if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); -goto done; -} - -ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); -if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to get errors from validators.\n"); goto done; } @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, ini_errobj_next(errobj); } +ret = EOK; + done: -if (rules_cfgobj) ini_config_destroy(rules_cfgobj); ini_errobj_destroy(); - return ret; #else DEBUG(SSSDBG_TRACE_FUNC, @@ -590,3 +584,32 @@ done: return EOK; #endif /* HAVE_LIBINI_CONFIG_V1_3 */ } + +#ifdef HAVE_LIBINI_CONFIG_V1_3 +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj) +{ +int ret; +struct ini_cfgobj *rules_cfgobj = NULL; + +ret = ini_rules_read_from_file(rules_path, _cfgobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); +goto done; +} + +ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +goto done; +} + +done: +if (rules_cfgobj) ini_config_destroy(rules_cfgobj); + +return ret; +} +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h index 7734bab..77943d6 100644 --- a/src/util/sss_ini.h +++ b/src/util/sss_ini.h @@ -27,6 +27,10 @@ #ifndef __SSS_INI_H__ #define __SSS_INI_H__ +#ifdef HAVE_LIBINI_CONFIG_V1_3 +#include +#endif + /* Structure declarations */ /* INI data structure */ @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx, int sss_ini_call_validators(struct sss_ini_initdata *data, const char *rules_path); +#ifdef HAVE_LIBINI_CONFIG_V1_3 +/* Get errors from validators with ini_errobj */ +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj); +#endif /* HAVE_LIBINI_CONFIG_V1
[SSSD] Re: [PATCH] sssctl: Add config-check command
On 07/07/2016 01:54 PM, Michal Židek wrote: On 07/07/2016 01:20 PM, Lukas Slebodnik wrote: On (07/07/16 12:37), Michal Židek wrote: On 07/04/2016 05:27 PM, Michal Židek wrote: On 07/01/2016 05:35 PM, Lukas Slebodnik wrote: On (01/07/16 17:26), Michal Židek wrote: Hello! This patch adds new command config-check for sssctl tool. The output looks like this: Issues identified by validators: 3 [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for typos. [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section 'sssd'. Check for typos. [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in section 'sssd'. Check for typos. Messages generated during configuration merging: 4 File conf.dd did not match provided patterns. Skipping. Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf. Error (5) on line 5: Equal sign is missing. Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. Skipping. Used configuration snippet files: 1 /etc/sssd/conf.d/snip-good.conf Currently it can only be used by root and checks only configuration in default path. Michal From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 15:10:30 +0200 Subject: [PATCH 1/4] sss_ini: Small refacoring of sss_ini_call_validators Separate logic to fill errobj so that the errors can be printed by the caller. --- src/util/sss_ini.c | 49 - src/util/sss_ini.h | 12 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index b4dbb07..78f5490 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, { #ifdef HAVE_LIBINI_CONFIG_V1_3 int ret; -struct ini_cfgobj *rules_cfgobj = NULL; struct ini_errobj *errobj = NULL; ret = ini_errobj_create(); @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, goto done; } -ret = ini_rules_read_from_file(rules_path, _cfgobj); +ret = sss_ini_call_validators_errobj(data, + rules_path, + errobj); if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); -goto done; -} - -ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); -if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to get errors from validators.\n"); goto done; } @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, ini_errobj_next(errobj); } +ret = EOK; + done: -if (rules_cfgobj) ini_config_destroy(rules_cfgobj); ini_errobj_destroy(); - return ret; #else DEBUG(SSSDBG_TRACE_FUNC, @@ -590,3 +584,32 @@ done: return EOK; #endif /* HAVE_LIBINI_CONFIG_V1_3 */ } + +#ifdef HAVE_LIBINI_CONFIG_V1_3 +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj) +{ +int ret; +struct ini_cfgobj *rules_cfgobj = NULL; + +ret = ini_rules_read_from_file(rules_path, _cfgobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); +goto done; +} + +ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +goto done; +} + +done: +if (rules_cfgobj) ini_config_destroy(rules_cfgobj); + +return ret; +} +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h index 7734bab..77943d6 100644 --- a/src/util/sss_ini.h +++ b/src/util/sss_ini.h @@ -27,6 +27,10 @@ #ifndef __SSS_INI_H__ #define __SSS_INI_H__ +#ifdef HAVE_LIBINI_CONFIG_V1_3 +#include +#endif + /* Structure declarations */ /* INI data structure */ @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx, int sss_ini_call_validators(struct sss_ini_initdata *data, const char *rules_path); +#ifdef HAVE_LIBINI_CONFIG_V1_3 +/* Get errors from validators with ini_errobj */ +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj); +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ + NACK src/util/sss_ini.h must not contai
[SSSD] Re: [PATCH] sssctl: Add config-check command
On 07/07/2016 01:20 PM, Lukas Slebodnik wrote: On (07/07/16 12:37), Michal Židek wrote: On 07/04/2016 05:27 PM, Michal Židek wrote: On 07/01/2016 05:35 PM, Lukas Slebodnik wrote: On (01/07/16 17:26), Michal Židek wrote: Hello! This patch adds new command config-check for sssctl tool. The output looks like this: Issues identified by validators: 3 [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for typos. [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section 'sssd'. Check for typos. [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in section 'sssd'. Check for typos. Messages generated during configuration merging: 4 File conf.dd did not match provided patterns. Skipping. Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf. Error (5) on line 5: Equal sign is missing. Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. Skipping. Used configuration snippet files: 1 /etc/sssd/conf.d/snip-good.conf Currently it can only be used by root and checks only configuration in default path. Michal From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 15:10:30 +0200 Subject: [PATCH 1/4] sss_ini: Small refacoring of sss_ini_call_validators Separate logic to fill errobj so that the errors can be printed by the caller. --- src/util/sss_ini.c | 49 - src/util/sss_ini.h | 12 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index b4dbb07..78f5490 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, { #ifdef HAVE_LIBINI_CONFIG_V1_3 int ret; -struct ini_cfgobj *rules_cfgobj = NULL; struct ini_errobj *errobj = NULL; ret = ini_errobj_create(); @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, goto done; } -ret = ini_rules_read_from_file(rules_path, _cfgobj); +ret = sss_ini_call_validators_errobj(data, + rules_path, + errobj); if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); -goto done; -} - -ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); -if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to get errors from validators.\n"); goto done; } @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, ini_errobj_next(errobj); } +ret = EOK; + done: -if (rules_cfgobj) ini_config_destroy(rules_cfgobj); ini_errobj_destroy(); - return ret; #else DEBUG(SSSDBG_TRACE_FUNC, @@ -590,3 +584,32 @@ done: return EOK; #endif /* HAVE_LIBINI_CONFIG_V1_3 */ } + +#ifdef HAVE_LIBINI_CONFIG_V1_3 +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj) +{ +int ret; +struct ini_cfgobj *rules_cfgobj = NULL; + +ret = ini_rules_read_from_file(rules_path, _cfgobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); +goto done; +} + +ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +goto done; +} + +done: +if (rules_cfgobj) ini_config_destroy(rules_cfgobj); + +return ret; +} +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h index 7734bab..77943d6 100644 --- a/src/util/sss_ini.h +++ b/src/util/sss_ini.h @@ -27,6 +27,10 @@ #ifndef __SSS_INI_H__ #define __SSS_INI_H__ +#ifdef HAVE_LIBINI_CONFIG_V1_3 +#include +#endif + /* Structure declarations */ /* INI data structure */ @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx, int sss_ini_call_validators(struct sss_ini_initdata *data, const char *rules_path); +#ifdef HAVE_LIBINI_CONFIG_V1_3 +/* Get errors from validators with ini_errobj */ +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj); +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ + NACK src/util/sss_ini.h must not contain conditional build. That's the pu
[SSSD] Re: [PATCH] sssctl: Add config-check command
On 07/04/2016 05:27 PM, Michal Židek wrote: On 07/01/2016 05:35 PM, Lukas Slebodnik wrote: On (01/07/16 17:26), Michal Židek wrote: Hello! This patch adds new command config-check for sssctl tool. The output looks like this: Issues identified by validators: 3 [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for typos. [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section 'sssd'. Check for typos. [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in section 'sssd'. Check for typos. Messages generated during configuration merging: 4 File conf.dd did not match provided patterns. Skipping. Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf. Error (5) on line 5: Equal sign is missing. Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. Skipping. Used configuration snippet files: 1 /etc/sssd/conf.d/snip-good.conf Currently it can only be used by root and checks only configuration in default path. Michal From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 15:10:30 +0200 Subject: [PATCH 1/4] sss_ini: Small refacoring of sss_ini_call_validators Separate logic to fill errobj so that the errors can be printed by the caller. --- src/util/sss_ini.c | 49 - src/util/sss_ini.h | 12 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index b4dbb07..78f5490 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, { #ifdef HAVE_LIBINI_CONFIG_V1_3 int ret; -struct ini_cfgobj *rules_cfgobj = NULL; struct ini_errobj *errobj = NULL; ret = ini_errobj_create(); @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, goto done; } -ret = ini_rules_read_from_file(rules_path, _cfgobj); +ret = sss_ini_call_validators_errobj(data, + rules_path, + errobj); if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); -goto done; -} - -ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); -if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to get errors from validators.\n"); goto done; } @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, ini_errobj_next(errobj); } +ret = EOK; + done: -if (rules_cfgobj) ini_config_destroy(rules_cfgobj); ini_errobj_destroy(); - return ret; #else DEBUG(SSSDBG_TRACE_FUNC, @@ -590,3 +584,32 @@ done: return EOK; #endif /* HAVE_LIBINI_CONFIG_V1_3 */ } + +#ifdef HAVE_LIBINI_CONFIG_V1_3 +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj) +{ +int ret; +struct ini_cfgobj *rules_cfgobj = NULL; + +ret = ini_rules_read_from_file(rules_path, _cfgobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); +goto done; +} + +ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +goto done; +} + +done: +if (rules_cfgobj) ini_config_destroy(rules_cfgobj); + +return ret; +} +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h index 7734bab..77943d6 100644 --- a/src/util/sss_ini.h +++ b/src/util/sss_ini.h @@ -27,6 +27,10 @@ #ifndef __SSS_INI_H__ #define __SSS_INI_H__ +#ifdef HAVE_LIBINI_CONFIG_V1_3 +#include +#endif + /* Structure declarations */ /* INI data structure */ @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx, int sss_ini_call_validators(struct sss_ini_initdata *data, const char *rules_path); +#ifdef HAVE_LIBINI_CONFIG_V1_3 +/* Get errors from validators with ini_errobj */ +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj); +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ + NACK src/util/sss_ini.h must not contain conditional build. That's the putpose of implementation file. Header file shoudl contain just a prototypes becuase there might be missing prototypes
[SSSD] Re: [PATCH] MAN: Config file merging
On 07/01/2016 10:34 PM, Dan Lavu wrote: I couldn't apply your patch to my repo, so I just modified the text in your patch. I have a question though, will this be the new default behavior for sssd 1.4.x onwards in Fedora and RHEL? Also I think the feature, being called "merging" is not very accurate, I think it should be 'include additional configuration files' or 'read additional configuration files', merging suggests that it will merge the configuration files into one file... thanks to Git. ;) Dan Hi Dan, thank you for the new wording. I agree that calling the feature config file merging could be confusing, so I used the verb "include" instead of "merge" and changed the section name. I also removed the SSSD version reference as Lukas suggested. Otherwise the wording should be the same as you proposed. To your question. Yes, this is going to be default behavior with all new SSSD versions compiled with new libini. See the attached patch. Michal >From 2b3fa8d8723d78b1da72b956cad3e980f8463ea0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=Date: Fri, 1 Jul 2016 11:23:20 +0200 Subject: [PATCH] MAN: Config file merging Related to: https://fedorahosted.org/sssd/ticket/2247 Explain configuration merging in sssd.conf man page. --- src/man/sssd.conf.5.xml | 39 +++ 1 file changed, 39 insertions(+) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index d5442d0..924dead 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -55,6 +55,45 @@ + +CONFIGURATION SNIPPETS FROM INCLUDE DIRECTORY + + +The configuration file sssd.conf will +include configuration snippets using the include directory +conf.d. This feature is available if +SSSD was compiled with libini version 1.3.0 or later. + + + +Any file placed in conf.d +that ends in .conf +and does not begin with a dot (.) will +be used together with sssd.conf +to configure SSSD. + + + +The configuration snippets from conf.d +have higher priority than sssd.conf +and will override sssd.conf when +conflicts occur. If several snippets are present in +conf.d, then they are included in +alphabetical order (based on locale). +Files included later have higher priority. Numerical +prefixes (01_snippet.conf, +02_snippet.conf etc.) can help +visualize the priority (higher number means higher +priority). + + + +The snippet files require the same owner and permissions +as sssd.conf. Which are by default +root:root and 0600. + + + GENERAL OPTIONS -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssctl: Add config-check command
On 07/01/2016 05:35 PM, Lukas Slebodnik wrote: On (01/07/16 17:26), Michal Židek wrote: Hello! This patch adds new command config-check for sssctl tool. The output looks like this: Issues identified by validators: 3 [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for typos. [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section 'sssd'. Check for typos. [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in section 'sssd'. Check for typos. Messages generated during configuration merging: 4 File conf.dd did not match provided patterns. Skipping. Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf. Error (5) on line 5: Equal sign is missing. Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. Skipping. Used configuration snippet files: 1 /etc/sssd/conf.d/snip-good.conf Currently it can only be used by root and checks only configuration in default path. Michal From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 15:10:30 +0200 Subject: [PATCH 1/4] sss_ini: Small refacoring of sss_ini_call_validators Separate logic to fill errobj so that the errors can be printed by the caller. --- src/util/sss_ini.c | 49 - src/util/sss_ini.h | 12 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c index b4dbb07..78f5490 100644 --- a/src/util/sss_ini.c +++ b/src/util/sss_ini.c @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, { #ifdef HAVE_LIBINI_CONFIG_V1_3 int ret; -struct ini_cfgobj *rules_cfgobj = NULL; struct ini_errobj *errobj = NULL; ret = ini_errobj_create(); @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, goto done; } -ret = ini_rules_read_from_file(rules_path, _cfgobj); +ret = sss_ini_call_validators_errobj(data, + rules_path, + errobj); if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); -goto done; -} - -ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); -if (ret != EOK) { -DEBUG(SSSDBG_FATAL_FAILURE, - "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to get errors from validators.\n"); goto done; } @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data, ini_errobj_next(errobj); } +ret = EOK; + done: -if (rules_cfgobj) ini_config_destroy(rules_cfgobj); ini_errobj_destroy(); - return ret; #else DEBUG(SSSDBG_TRACE_FUNC, @@ -590,3 +584,32 @@ done: return EOK; #endif /* HAVE_LIBINI_CONFIG_V1_3 */ } + +#ifdef HAVE_LIBINI_CONFIG_V1_3 +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj) +{ +int ret; +struct ini_cfgobj *rules_cfgobj = NULL; + +ret = ini_rules_read_from_file(rules_path, _cfgobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to read sssd.conf schema %d [%s]\n", ret, strerror(ret)); +goto done; +} + +ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj); +if (ret != EOK) { +DEBUG(SSSDBG_FATAL_FAILURE, + "ini_rules_check failed %d [%s]\n", ret, strerror(ret)); +goto done; +} + +done: +if (rules_cfgobj) ini_config_destroy(rules_cfgobj); + +return ret; +} +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h index 7734bab..77943d6 100644 --- a/src/util/sss_ini.h +++ b/src/util/sss_ini.h @@ -27,6 +27,10 @@ #ifndef __SSS_INI_H__ #define __SSS_INI_H__ +#ifdef HAVE_LIBINI_CONFIG_V1_3 +#include +#endif + /* Structure declarations */ /* INI data structure */ @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx, int sss_ini_call_validators(struct sss_ini_initdata *data, const char *rules_path); +#ifdef HAVE_LIBINI_CONFIG_V1_3 +/* Get errors from validators with ini_errobj */ +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data, + const char *rules_path, + struct ini_errobj *errobj); +#endif /* HAVE_LIBINI_CONFIG_V1_3 */ + NACK src/util/sss_ini.h must not contain conditional build. That's the putpose of implementation file. Header file shoudl contain just a prototypes becuase there might be missing prototypes if you forgot include "conf
[SSSD] Re: [PATCH] sssctl: manual page
On 07/04/2016 12:26 PM, Lukas Slebodnik wrote: On (04/07/16 12:21), Pavel Březina wrote: I don't think it is necessary to duplicate information from the tool itself into a manual page so a tried to keep the man page simple. This way we will not have to maintain it with any change in sssctl. +1 I let the review for a native speaker, but having a simple man page and a help for each command instead of maintaining the same info on two places is IMO good idea. If you think it is better to include all commands and options in the man page as well, I can do it... From 5c8005bab6c82d4723ae9ed7583b88b3874a260b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=Date: Mon, 4 Jul 2016 12:19:37 +0200 Subject: [PATCH] sssctl: manual page Resolves: https://fedorahosted.org/sssd/ticket/3055 --- contrib/sssd.spec.in | 1 + src/man/Makefile.am | 2 +- src/man/sssctl.8.xml | 69 3 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 src/man/sssctl.8.xml diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b8a9efc07e2cb9325b5d445d546ec145391b95c2..d5730a474eb554fa81123e03b1815d2a71da7496 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -950,6 +950,7 @@ done %{_mandir}/man8/sss_override.8* %{_mandir}/man8/sss_debuglevel.8* %{_mandir}/man8/sss_seed.8* +%{_mandir}/man8/sssctl.8* %files -n python-sssdconfig -f python2_sssdconfig.lang %defattr(-,root,root,-) diff --git a/src/man/Makefile.am b/src/man/Makefile.am index 433d1cddcca75c35b38846e97e455765fe99f77c..cd23b02f6db05dd5f08b6764df4663eafc6f6e7a 100644 --- a/src/man/Makefile.am +++ b/src/man/Makefile.am @@ -51,7 +51,7 @@ man_MANS = \ sssd-krb5.5 sssd-simple.5 \ sssd_krb5_locator_plugin.8 sss_groupshow.8 \ pam_sss.8 sss_obfuscate.8 sss_cache.8 sss_debuglevel.8 sss_seed.8 \ -sss_override.8 idmap_sss.8 \ +sss_override.8 idmap_sss.8 sssctl.8 \ $(NULL) if BUILD_SAMBA diff --git a/src/man/sssctl.8.xml b/src/man/sssctl.8.xml I think you will also need to add this file to src/man/po/po4a.cfg otherwise it could not be translated. I let review of man page to a native speaker. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] MAN: Update documentation of sss_cache
On 07/04/2016 11:54 AM, Lukas Slebodnik wrote: ehlo, sss_cache -E can invalidate sudo rules since sssd 1.14 alpha. We just forgot to update man page for option --everything LS Ack. I do not think this needs CI run. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] MAN: Config file merging
On 07/01/2016 03:58 PM, Lukas Slebodnik wrote: On (01/07/16 12:14), Michal Židek wrote: On 07/01/2016 11:55 AM, Michal Židek wrote: Hi! This patch adds man page for config file merging. Michal I had extra newline in the previous patch. Fixed in the attached patch. Michal From 98d38c25bfff26330ba1ab01b0199abf2a224a24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 11:23:20 +0200 Subject: [PATCH] MAN: Config file merging Related to: https://fedorahosted.org/sssd/ticket/2247 There is separate tcket for this https://fedorahosted.org/sssd/ticket/3062 Please assing it to yourself and mark that patch hs beedn submitted. Explain config file merging in sssd.conf man page. --- src/man/sssd.conf.5.xml | 38 ++ 1 file changed, 38 insertions(+) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index d5442d0..1c9d966 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -55,6 +55,44 @@ + +MERGING WITH CONFIGURATION FILE SNIPETS + + +Since SSSD version 1.14 it is possible to merge SSSD version is not important But I let review for native speaker. LS I removed the SSSD version note. Other than that, the patch is the same. Michal >From 4a5232fc40d8670dc69cf248be8fb551a77096d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com> Date: Fri, 1 Jul 2016 11:23:20 +0200 Subject: [PATCH] MAN: Config file merging Related to: https://fedorahosted.org/sssd/ticket/2247 Explain config file merging in sssd.conf man page. --- src/man/sssd.conf.5.xml | 38 ++ 1 file changed, 38 insertions(+) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index d5442d0..d556d07 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -55,6 +55,44 @@ + +MERGING WITH CONFIGURATION FILE SNIPETS + + +It is possible to merge +sssd.conf file with configuration +file snippets from include directory +conf.d. This feature is available +if SSSD was compiled with libini version 1.3.0 or later. + + + +Any file placed in the conf.d +that ends with suffix .conf +and does not begin with a dot (.) will +be merged with sssd.conf. + + + +The configuration snippets from conf.d +have higher priority than sssd.conf +and override sssd.conf when +conflicts occur. If several snippet files +are present in conf.d, then they are +merged in alphabetical order (based on locale). +Files merged later have higher priority. Numerical +prefixes (01_snippet.conf, +02_snippet.conf etc.) can help +visualize the priority (higher number means higher +priority). + + + +The snippet files have the same file ownership and file +permissions requirements as sssd.conf. + + + GENERAL OPTIONS -- 2.5.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org