Re: [SSSD] [PATCH] confdb: Remove unused function confdb_get_long
On 09/30/2015 07:42 AM, Lukas Slebodnik wrote: On (29/09/15 17:45), Jakub Hrozek wrote: On Tue, Sep 29, 2015 at 11:38:58AM +0200, Sumit Bose wrote: On Tue, Sep 29, 2015 at 10:50:06AM +0200, Pavel Reichl wrote: On 09/29/2015 10:44 AM, Lukas Slebodnik wrote: On (29/09/15 10:42), Pavel Reichl wrote: On 09/29/2015 10:35 AM, Lukas Slebodnik wrote: On (29/09/15 10:16), Pavel Reichl wrote: On 09/29/2015 10:08 AM, Lukas Slebodnik wrote: On (29/09/15 08:45), Pavel Reichl wrote: On 09/29/2015 08:31 AM, Lukas Slebodnik wrote: On (27/09/15 12:49), Pavel Reichl wrote: Hello, please see trivial patch attached. >From b9f938087973444f0ec26fc24ad68dca7ac63034 Mon Sep 17 00:00:00 2001 From: Pavel ReichlDate: Sun, 27 Sep 2015 12:34:20 +0200 Subject: [PATCH] confdb: Remove unused function confdb_get_long --- src/confdb/confdb.c | 51 --- 1 file changed, 51 deletions(-) diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index c097aad7745eda4fff051c7da027776f95db0f03..eebd478f74041d2050df9edc283df43a65462340 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -475,57 +475,6 @@ failed: return ret; } -long confdb_get_long(struct confdb_ctx *cdb, - const char *section, const char *attribute, - long defval, long *result) -{ Would it be better to consider this function as confdb API and add to src/confdb/confdb.h? It could be. It might be useful in the future. I thought that out policy towards unused functions were to remove them. Could you point me to the description of such policy? I'm not aware of it. I don't think it's written anywhere Good to know. I thought I missed something I just saw we did it repeatedly before and thought it's our general practice. One more time: If we consider confdb as library than we should never remove functions. Removing functions from other parts of code is something else. We did so in commit 2b94ab415b30861f42b68725d9231905baf8c3bd I didn't notice that patch. I'm sorry I do not have a time to follow each patchset. We can always resurrect removed code if needed. If we consider confdb as library than we should never remove functions. Then we should decide which parts of SSSD are public libraries and which are not, then we could avoid this kind of discussions. I wrote "consider confdb as library" and not as a public library with stable API. And libraries many times contains function which are not used. So we should not remove them. Even though it would be still in git history. Why? If we don't use them and nobody else does why do we have to keep them? libraries many times contains function which are not used Well, but if they are not public we can change that, right? We can change them as we need and removing unused code is one of those things. I don't see the difference between private library and 'other parts of code'. I think we should be explicit here. If we want to consider confdb as a library we should make it a public library with a stable API with all the consequences. In general I think this is not a bad idea e.g. with respect to all the config related tickets we have planned for 1.14. But we should define the initial version of a stable API, based on the result and requirements from the 1.14 tickets. I'm more with Simo here that we shouldn't consider confdb and sysdb as stable ABIs, at least not yet. I think we need the flexibility of a private, unstable library. At the same time, I think we should try harder to avoid polluting the sysdb/confdb APIs with new calls and in general strive to make even the internal APIs easily usable. Here I agree with Lukas that the current interface is not very nice -- and the sysdb interface would be something that any new external contributor would be confronted with. Maybe when we review (hint, nudge, nudge) Michal's sysdb patches, we can take a look at the API and massage it a bit, move related functions together in the header files and merge or remove extra functions? (Yes, we can also file a ticket, but I don't think it would land in a milestone where we would touch it soon...) Having private/internal libraries with a more relaxed policy might turn out to be problematic as e.g. can be seen by some internal samba libraries which are used by us, OpenChange and maybe other projects where upstream changes might force changes in the other projects as well. Ok, the argument here is that they shouldn't have been used in the first place, but what's the point in re-implementing existing functionality. Currently I wouldn't see libsss_util where confdb.c is included as a library at all, but just as a mean to make the build process easier and faster. Coming to the original topic of the discussion, I would agree to remove confdb_get_long() because it is nowhere used (and it looks like it never was) and it does not even have a test. Is that an ack? :-) No,
Re: [SSSD] [PATCHES] fix minor memory leaks
On 09/29/2015 05:49 PM, Jakub Hrozek wrote: On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote: In my opinion we can drop the change. This is not an imminent bug it's rather code style dispute and I don't think that we have to bother Sumit for that. Unless of course pbrezina feels different. I don't mind too much, it just doesn't seem to be helpful change, that's all, so I'll be a bit happier if we drop it. Updated patch set attached. I removed the discussed 'talloc_new' line from 1st patch. Bye. >From 9cae5048afcfe16b053aa8322a275a9682d8bcdc Mon Sep 17 00:00:00 2001 From: Pavel ReichlDate: Fri, 4 Sep 2015 07:02:42 -0400 Subject: [PATCH 1/4] AD: fix minor memory leak --- src/providers/ad/ad_common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c index 130cdeb613aae3843f7453a478815daaae6aab77..88f36f8ead64443e07b2393aa22ad4d9708d0a0e 100644 --- a/src/providers/ad/ad_common.c +++ b/src/providers/ad/ad_common.c @@ -745,7 +745,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx *bectx, ret = be_add_online_cb(bectx, bectx, ad_online_cb, service, NULL); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Could not set up AD online callback\n"); -return ret; +goto done; } ret = be_fo_service_add_callback(mem_ctx, bectx, ad_service, @@ -797,7 +797,8 @@ ad_resolve_callback(void *private_data, struct fo_server *server) sdata = fo_get_server_user_data(server); if (fo_is_srv_lookup(server) == false && sdata == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "No user data?\n"); -return; +ret = EINVAL; +goto done; } service = talloc_get_type(private_data, struct ad_service); -- 2.4.3 >From 5a20e0120278dba83381cc41f95024f2850ff745 Mon Sep 17 00:00:00 2001 From: Pavel Reichl Date: Fri, 4 Sep 2015 07:03:45 -0400 Subject: [PATCH 2/4] IPA: fix minor memory leak --- src/providers/ipa/ipa_hbac_rules.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_hbac_rules.c b/src/providers/ipa/ipa_hbac_rules.c index ffef6dc4ce4229f2063d1b00308892bd3765f398..6ce1f76bb656352ec61332007a6fb62568374203 100644 --- a/src/providers/ipa/ipa_hbac_rules.c +++ b/src/providers/ipa/ipa_hbac_rules.c @@ -86,7 +86,7 @@ ipa_hbac_rule_info_send(TALLOC_CTX *mem_ctx, req = tevent_req_create(mem_ctx, , struct ipa_hbac_rule_state); if (req == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n"); -return NULL; +goto error; } state->ev = ev; -- 2.4.3 >From d33c71c28261f357965344e6c48a989cf168e060 Mon Sep 17 00:00:00 2001 From: Pavel Reichl Date: Fri, 4 Sep 2015 07:04:10 -0400 Subject: [PATCH 3/4] SDAP: fix minor memory leak --- src/providers/ldap/sdap_async_groups.c | 2 +- src/providers/ldap/sdap_idmap.c| 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 653187b3add172d37f234850ccab8ddf109e229c..609668339b7d7d37e578bd3cca8f8c7cac1e6671 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -592,7 +592,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Error: Failed to mark group as non-posix!\n"); -return ret; +goto done; } } diff --git a/src/providers/ldap/sdap_idmap.c b/src/providers/ldap/sdap_idmap.c index dd959b2c133b342f105f76c26c889d678ce40391..36d529836eb7e4becd27721df15cfbccf035ae3b 100644 --- a/src/providers/ldap/sdap_idmap.c +++ b/src/providers/ldap/sdap_idmap.c @@ -206,7 +206,8 @@ sdap_idmap_init(TALLOC_CTX *mem_ctx, if (err != IDMAP_SUCCESS) { /* This should never happen */ DEBUG(SSSDBG_CRIT_FAILURE, "sss_idmap_ctx corrupted\n"); -return EIO; +ret = EIO; +goto done; } -- 2.4.3 >From e63211aaa15887c78deee5a76ed60684f9e6aaf7 Mon Sep 17 00:00:00 2001 From: Pavel Reichl Date: Fri, 4 Sep 2015 07:04:40 -0400 Subject: [PATCH 4/4] PROXY: fix minor memory leak --- src/providers/proxy/proxy_services.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/providers/proxy/proxy_services.c b/src/providers/proxy/proxy_services.c index 2aa44dbf7f061b99a551b199d0265393e5399a06..ee545cedc9f8dc36981eb3190ff16763e1da390f 100644 --- a/src/providers/proxy/proxy_services.c +++ b/src/providers/proxy/proxy_services.c @@ -130,7 +130,7 @@ get_serv_byname(struct proxy_id_ctx *ctx, if (status != NSS_STATUS_SUCCESS && status != NSS_STATUS_NOTFOUND) { DEBUG(SSSDBG_MINOR_FAILURE, "getservbyname_r failed for service [%s].\n", name); -return ret; +goto done; } if (status ==
Re: [SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
On 09/30/2015 09:38 AM, Jakub Hrozek wrote: Hi, to help the OpenSCAP integration, I prepared a wiki page that contains options which have a security impact -- either positive (drop root) or negative (ignore certificate validation issues). I also tried to explain the effect of the options along with the description. There are some more items that can be included, but I wasn't sure about them myself, like: * should obfuscated passwords be mentioned? I wasn't sure because on one hand it really doesn't provide any benefit, on the other hand, the option can be used to check a compliance box that requires no passwords be stored in files.. * should the page warn against the auth-option-that-shall-not-be-mentioned or politely deny its existence? :-) * What about fd_limit ? Should resource consumption be considered a security property, especially if we already honor system default? I think here the default is enough, so I didn't document that option. Please provide your comments or edit the wiki directly. Thanks! ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel I was thinking about 'cached_auth_timeout' timeout. If misconfigured and set for really long time changes of passwords on server would be ignored. I know that same effect could be achieved by maintaining SSSD in offline mode... What do you think? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] confdb: Remove unused function confdb_get_long
On Wed, Sep 30, 2015 at 08:31:57AM +0200, Pavel Reichl wrote: > > > On 09/30/2015 07:42 AM, Lukas Slebodnik wrote: > >On (29/09/15 17:45), Jakub Hrozek wrote: > >>On Tue, Sep 29, 2015 at 11:38:58AM +0200, Sumit Bose wrote: > >>>On Tue, Sep 29, 2015 at 10:50:06AM +0200, Pavel Reichl wrote: > > > On 09/29/2015 10:44 AM, Lukas Slebodnik wrote: > >On (29/09/15 10:42), Pavel Reichl wrote: > >> > >> > >>On 09/29/2015 10:35 AM, Lukas Slebodnik wrote: > >>>On (29/09/15 10:16), Pavel Reichl wrote: > On 09/29/2015 10:08 AM, Lukas Slebodnik wrote: > >On (29/09/15 08:45), Pavel Reichl wrote: > >> > >> > >>On 09/29/2015 08:31 AM, Lukas Slebodnik wrote: > >>>On (27/09/15 12:49), Pavel Reichl wrote: > Hello, please see trivial patch attached. > >>> > From b9f938087973444f0ec26fc24ad68dca7ac63034 Mon Sep 17 00:00:00 > 2001 > From: Pavel Reichl> Date: Sun, 27 Sep 2015 12:34:20 +0200 > Subject: [PATCH] confdb: Remove unused function confdb_get_long > > --- > src/confdb/confdb.c | 51 > --- > 1 file changed, 51 deletions(-) > > diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c > index > c097aad7745eda4fff051c7da027776f95db0f03..eebd478f74041d2050df9edc283df43a65462340 > 100644 > --- a/src/confdb/confdb.c > +++ b/src/confdb/confdb.c > @@ -475,57 +475,6 @@ failed: > return ret; > } > > -long confdb_get_long(struct confdb_ctx *cdb, > - const char *section, const char *attribute, > - long defval, long *result) > -{ > >>>Would it be better to consider this function as confdb API > >>>and add to src/confdb/confdb.h? > >> > >>It could be. > >> > >>>It might be useful in the future. > >> > >>I thought that out policy towards unused functions were to remove > >>them. > >Could you point me to the description of such policy? > >I'm not aware of it. > > I don't think it's written anywhere > >>>Good to know. I thought I missed something > >>> > I just saw we did it repeatedly before and thought it's our general > practice. > > >>>One more time: > >>>If we consider confdb as library than we should never remove functions. > >>> > >>>Removing functions from other parts of code is something > >>>else. > >>> > > > >>We did so in commit 2b94ab415b30861f42b68725d9231905baf8c3bd > >I didn't notice that patch. I'm sorry I do not have a time > >to follow each patchset. > > > >>We can always resurrect removed code if needed. > >> > >If we consider confdb as library than > >we should never remove functions. > > Then we should decide which parts of SSSD are public libraries and > which are not, then we could avoid this kind of discussions. > > >>>I wrote "consider confdb as library" and not as a public library with > >>>stable API. And libraries many times contains function which are not > >>>used. > >>>So we should not remove them. Even though it would be still in git > >>>history. > >> > >>Why? If we don't use them and nobody else does why do we have to keep > >>them? > >> > >libraries many times contains function which are not used > > Well, but if they are not public we can change that, right? We can change > them as we need and removing unused code is one of those things. I don't > see the difference between private library and 'other parts of code'. > >>> > >>>I think we should be explicit here. If we want to consider confdb as a > >>>library we should make it a public library with a stable API with all > >>>the consequences. In general I think this is not a bad idea e.g. with > >>>respect to all the config related tickets we have planned for 1.14. But > >>>we should define the initial version of a stable API, based on the > >>>result and requirements from the 1.14 tickets. > >> > >>I'm more with Simo here that we shouldn't consider confdb and sysdb as > >>stable ABIs, at least not yet. I think we need the flexibility of a private, > >>unstable library. > >> > >>At the same time, I think we should try harder to avoid polluting the > >>sysdb/confdb APIs with new calls and in general strive to make even the > >>internal APIs easily usable. Here I agree with Lukas that the > >>current interface is not very nice -- and the sysdb interface would be > >>something that any new external contributor
Re: [SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
On (30/09/15 09:38), Jakub Hrozek wrote: >Hi, > >to help the OpenSCAP integration, I prepared a wiki page that contains >options which have a security impact -- either positive (drop root) or >negative (ignore certificate validation issues). > >I also tried to explain the effect of the options along with the >description. There are some more items that can be included, but I >wasn't sure about them myself, like: >* should obfuscated passwords be mentioned? I wasn't sure because on > one hand it really doesn't provide any benefit, on the other hand, > the option can be used to check a compliance box that requires no > passwords be stored in files.. >* should the page warn against the > auth-option-that-shall-not-be-mentioned or politely deny its > existence? :-) >* What about fd_limit ? Should resource consumption be considered > a security property, especially if we already honor system default? I > think here the default is enough, so I didn't document that option. > >Please provide your comments or edit the wiki directly. Thanks! What about: simple_deny_users (string) Comma separated list of users who are explicitly denied access. simple_deny_groups (string) Comma separated list of groups that are explicitly denied access. This applies only to groups within this SSSD domain. Local groups are not evaluated. Whitelisting is much secure than blacklisting. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Run integration tests on debian testing
On (29/09/15 16:47), Nikolai Kondrashov wrote: >On 09/29/2015 03:41 PM, Lukas Slebodnik wrote: >>ehlo, >> >>Integration tests are enabled on debian with the last patch. > >So we've got more cwrap packages into Debian? Awesome :)! Or were they there >all the time and I simply failed to notice them? > uid-wrapper 1.0.2-1 was in testing since 2014-08-14 nss-wrapper 1.0.3-2 was in testing since 2015-06-05 https://packages.qa.debian.org/u/uid-wrapper.html https://packages.qa.debian.org/n/nss-wrapper.html >>I just changed DEPS_INTGCHECK_SATISFIED to true for debian >>because in future we might introduce new dependencies >>which will not be in debian (su_wrapper). > >Ah, alright. > >>The 1st patch is prequisity for the last patch because >>installation of slapd requires user interaction. >>The ticket #2433 is finally fixed after 13 months. > >Thank you, Lukas. > You are welcome >>If we do not want to introduce new dependency /usr/bin/libtool >>for debian then there is alternative solution of bug fixed >>in the 2nd patch. We can run libtool from CWD generated by autotools. >>In both cases it's a oneliner :-) >> >>Here is an alternative version: >> >>diff --git a/contrib/ci/run b/contrib/ci/run >>index 5f668ff..1f64e67 100755 >>--- a/contrib/ci/run >>+++ b/contrib/ci/run >>@@ -204,7 +204,7 @@ function build_debug() >> CK_FORK=no \ >> stage make-check-valgrind \ >> make-check-wrap -j $CPU_NUM check -- \ >>-libtool --mode=execute \ >>+./libtool --mode=execute \ >> valgrind-condense 99 \ >> '!(*.py|*dlopen-tests)' -- \ >> --trace-children=yes \ > >I like this way a little better, but not enough to argue :) > hmm, I tried it but it failed in cwrap test due to different directory. I could use full path but nicer solution is to install missing dependency = sssd 1.13.1: src/tests/cwrap/test-suite.log = # TOTAL: 4 # PASS: 0 # SKIP: 0 # XFAIL: 0 # FAIL: 4 # XPASS: 0 # ERROR: 0 .. contents:: :depth: 2 FAIL: become_user-tests === /home/build/sssd/build/test-driver: line 107: ./libtool: No such file or directory FAIL become_user-tests (exit status: 127) FAIL: server-tests == /home/build/sssd/build/test-driver: line 107: ./libtool: No such file or directory FAIL server-tests (exit status: 127) FAIL: usertools-tests = /home/build/sssd/build/test-driver: line 107: ./libtool: No such file or directory FAIL usertools-tests (exit status: 127) FAIL: responder_common-tests /home/build/sssd/build/test-driver: line 107: ./libtool: No such file or directory FAIL responder_common-tests (exit status: 127) >> From 6175bd3f05cbfd5f7d9b26770d9a98012803745c Mon Sep 17 00:00:00 2001 >>From: Lukas Slebodnik>>Date: Tue, 29 Sep 2015 11:52:30 +0200 >>Subject: [PATCH 1/3] CI: Don't depend on user input with apt-get >> >>Resolves: >>https://fedorahosted.org/sssd/ticket/2433 >>--- >> contrib/ci/README.md | 6 ++ >> contrib/ci/distro.sh | 2 ++ >> 2 files changed, 8 insertions(+) >> >>diff --git a/contrib/ci/README.md b/contrib/ci/README.md >>index >>6b5f7f30eac8327d5aa45c3bfefd57e8d3109fe0..075bc3e074cb13916619f46c12c6d1a4de0158a2 >> 100644 >>--- a/contrib/ci/README.md >>+++ b/contrib/ci/README.md >>@@ -47,6 +47,12 @@ and Debian-based distros: >> >> Where `` is the user invoking CI. >> >>+You might also want to allow to keep environment variable DEBIAN_FRONTEND >>+on debian. So script can perform an unattended installation of a Debian >>package >>+with apt-get. >>+Defaults!/usr/bin/apt-get env_keep += "DEBIAN_FRONTEND >>+ >>+ > >The terminating double quote is missing here. cop problem >Also, may I suggest simplifying >the text a little? Like this: > >You may also want to allow passing DEBIAN_FRONTEND environment variable to >apt-get on Debian, so CI can request non-interactive package installation: > >Defaults!/usr/bin/apt-get env_keep += "DEBIAN_FRONTEND" > Done >> On Red Hat distros a repository carrying dependencies missing from some >> distros needs to be added to yum configuration. See instructions on the >> [Copr project >> page](http://copr-fe.cloud.fedoraproject.org/coprs/lslebodn/sssd-deps/). >>diff --git a/contrib/ci/distro.sh b/contrib/ci/distro.sh >>index >>5416bfff325c4e5d0a10ebea67cba26e20e03fd5..095985ccae81e54bcd79607e455a1c9295aad867 >> 100644 >>--- a/contrib/ci/distro.sh >>+++ b/contrib/ci/distro.sh >>@@ -30,6 +30,8 @@ if [ -e /etc/redhat-release ]; then >> DISTRO_FAMILY=redhat >> elif [ -e /etc/debian_version ]; then >> DISTRO_FAMILY=debian >>+# Perform an unattended installation of a Debian package with apt-get >>+export DEBIAN_FRONTEND=noninteractive
Re: [SSSD] [PATCH] Add a client-side hook to prevent pushes without Reviewed-By
On Tue, Sep 29, 2015 at 08:28:30AM +0200, Lukas Slebodnik wrote: > On (28/09/15 14:19), Jakub Hrozek wrote: > >Hi, > > > >to activate this hook, copy it from contrib to .git/hooks and make sure > >the executable flag is on. Attempting to push a commit without > >Reviewed-By will then trigger an error. > > > Good idea. > > >If we want to be truly strict about not pushing commits without a RB > >tag, then we need a server-side hook. > As you wish. > > > >From 391666fda49fef9ac003d192e7ae8a3c2b00e113 Mon Sep 17 00:00:00 2001 > >From: Jakub Hrozek> >Date: Mon, 28 Sep 2015 13:46:39 +0200 > >Subject: [PATCH] contrib: Add a pre-push hook to warn about commits without > > Reviewed-By > > > >--- > > contrib/git/pre-push | 62 > > > > 1 file changed, 62 insertions(+) > > create mode 100755 contrib/git/pre-push > > > >diff --git a/contrib/git/pre-push b/contrib/git/pre-push > Initially, I thought it does not work because I tested with > creating a new branch > git push origin master:new_branch > However it works after trying to push another patch to this branch. > > There are few pep8 warning > sh$ pep8 contrib/git/pre-push > contrib/git/pre-push:10:1: E302 expected 2 blank lines, found 1 > contrib/git/pre-push:11:13: E201 whitespace after '[' > contrib/git/pre-push:11:70: E202 whitespace before ']' > contrib/git/pre-push:14:13: E201 whitespace after '[' > contrib/git/pre-push:14:74: E202 whitespace before ']' > contrib/git/pre-push:16:1: E302 expected 2 blank lines, found 1 > contrib/git/pre-push:17:13: E201 whitespace after '[' > contrib/git/pre-push:17:54: E202 whitespace before ']' > contrib/git/pre-push:22:1: E302 expected 2 blank lines, found 1 > contrib/git/pre-push:31:1: E302 expected 2 blank lines, found 1 > contrib/git/pre-push:39:1: E302 expected 2 blank lines, found 1 pep8 should be happy about the attached patch. > > It would be also good to add small howto from mail. > "copy it from contrib to .git/hooks and make sure > the executable flag is on" to the commit message or better to > README into directory contrib/git/ OK, Added. > > and you can also mention hint about .git-commit-template I tried to add something, hope it makes sense. >From 66df6b096bcb3044431b1d65cb426a1f1fb1f37c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 28 Sep 2015 13:46:39 +0200 Subject: [PATCH] contrib: Add a pre-push hook to warn about commits without Reviewed-By --- contrib/git/pre-push | 73 1 file changed, 73 insertions(+) create mode 100755 contrib/git/pre-push diff --git a/contrib/git/pre-push b/contrib/git/pre-push new file mode 100755 index ..d05202dfdd2f59a885b843e25e89878d985909e4 --- /dev/null +++ b/contrib/git/pre-push @@ -0,0 +1,73 @@ +#!/usr/bin/env python + +# A git pre-push hook that declines commits that don't contain a Reviewed-By: +# tag. The tag must be present on the beginning of the line. To activate, copy +# to $GIT_DIR/hooks/pre-push and make sure the executable flag is on. + +# The commit message should also be based on .git-commit-template, although +# that is just best practice and not enforced + +import sys +import re +import subprocess + + +def get_all_commits(ref_from, ref_to): +args = ['git', 'rev-list', '{:s}..{:s}'.format(ref_from, ref_to)] +p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +out, err = p.communicate() +return [commit.strip() for commit in out.split('\n') if commit != ''] + + +def commit_message(commit_hash): +args = ['git', 'cat-file', 'commit', commit_hash] +p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) +out, err = p.communicate() +return out + + +def commit_has_rb(commit): +msg = commit_message(commit) +for l in msg.split('\n'): +has_rb = re.search('^Reviewed-by:', l) +if has_rb: +return True + +return False + + +def report_commit(commit_hash): +print("Commit {:s} does not have Reviewed-By!".format(commit_hash)) +print("Full message:\n==") +print("{:s}".format(commit_message(commit_hash))) +print("==") + + +# man 5 githooks says: +# Information about what is to be pushed is provided on the hook's +# standard input with lines of the form: +#SP SP SP LF +def check_push(hook_input): +ref_to = hook_input.split()[1][:6] +ref_from = hook_input.split()[3][:6] +commit_list = get_all_commits(ref_from, ref_to) + +no_rb_list = [] +for commit in commit_list: +if not commit_has_rb(commit): +no_rb_list.append(commit) + +return no_rb_list + +# Don't warn when pushing to personal repositories, only origin +remote = sys.argv[1] +if remote != 'origin': +sys.exit(0) + +for hook_input in sys.stdin.readlines(): +no_rb_list = check_push(hook_input) + +if len(no_rb_list)
[SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
Hi, to help the OpenSCAP integration, I prepared a wiki page that contains options which have a security impact -- either positive (drop root) or negative (ignore certificate validation issues). I also tried to explain the effect of the options along with the description. There are some more items that can be included, but I wasn't sure about them myself, like: * should obfuscated passwords be mentioned? I wasn't sure because on one hand it really doesn't provide any benefit, on the other hand, the option can be used to check a compliance box that requires no passwords be stored in files.. * should the page warn against the auth-option-that-shall-not-be-mentioned or politely deny its existence? :-) * What about fd_limit ? Should resource consumption be considered a security property, especially if we already honor system default? I think here the default is enough, so I didn't document that option. Please provide your comments or edit the wiki directly. Thanks! ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
On 09/30/2015 09:38 AM, Jakub Hrozek wrote: Hi, to help the OpenSCAP integration, I prepared a wiki page that contains options which have a security impact -- either positive (drop root) or negative (ignore certificate validation issues). I also tried to explain the effect of the options along with the description. There are some more items that can be included, but I wasn't sure about them myself, like: * should obfuscated passwords be mentioned? I wasn't sure because on one hand it really doesn't provide any benefit, on the other hand, the option can be used to check a compliance box that requires no passwords be stored in files.. * should the page warn against the auth-option-that-shall-not-be-mentioned or politely deny its existence? :-) * What about fd_limit ? Should resource consumption be considered a security property, especially if we already honor system default? I think here the default is enough, so I didn't document that option. Please provide your comments or edit the wiki directly. Thanks! ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Should not we also mention dreadful option ldap_auth_disable_tls_never_use_in_production ? I know it's undocumented, but I suppose OpenSCAP should report its presence anyway. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] confdb: Remove unused function confdb_get_long
On (30/09/15 10:06), Sumit Bose wrote: >On Wed, Sep 30, 2015 at 08:31:57AM +0200, Pavel Reichl wrote: >> >> >> On 09/30/2015 07:42 AM, Lukas Slebodnik wrote: >> >On (29/09/15 17:45), Jakub Hrozek wrote: >> >>On Tue, Sep 29, 2015 at 11:38:58AM +0200, Sumit Bose wrote: >> >>>On Tue, Sep 29, 2015 at 10:50:06AM +0200, Pavel Reichl wrote: >> >> >> On 09/29/2015 10:44 AM, Lukas Slebodnik wrote: >> >On (29/09/15 10:42), Pavel Reichl wrote: >> >> >> >> >> >>On 09/29/2015 10:35 AM, Lukas Slebodnik wrote: >> >>>On (29/09/15 10:16), Pavel Reichl wrote: >> On 09/29/2015 10:08 AM, Lukas Slebodnik wrote: >> >On (29/09/15 08:45), Pavel Reichl wrote: >> >> >> >> >> >>On 09/29/2015 08:31 AM, Lukas Slebodnik wrote: >> >>>On (27/09/15 12:49), Pavel Reichl wrote: >> Hello, please see trivial patch attached. >> >>> >> From b9f938087973444f0ec26fc24ad68dca7ac63034 Mon Sep 17 >> 00:00:00 2001 >> From: Pavel Reichl>> Date: Sun, 27 Sep 2015 12:34:20 +0200 >> Subject: [PATCH] confdb: Remove unused function confdb_get_long >> >> --- >> src/confdb/confdb.c | 51 >> --- >> 1 file changed, 51 deletions(-) >> >> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c >> index >> c097aad7745eda4fff051c7da027776f95db0f03..eebd478f74041d2050df9edc283df43a65462340 >> 100644 >> --- a/src/confdb/confdb.c >> +++ b/src/confdb/confdb.c >> @@ -475,57 +475,6 @@ failed: >> return ret; >> } >> >> -long confdb_get_long(struct confdb_ctx *cdb, >> - const char *section, const char *attribute, >> - long defval, long *result) >> -{ >> >>>Would it be better to consider this function as confdb API >> >>>and add to src/confdb/confdb.h? >> >> >> >>It could be. >> >> >> >>>It might be useful in the future. >> >> >> >>I thought that out policy towards unused functions were to remove >> >>them. >> >Could you point me to the description of such policy? >> >I'm not aware of it. >> >> I don't think it's written anywhere >> >>>Good to know. I thought I missed something >> >>> >> I just saw we did it repeatedly before and thought it's our general >> practice. >> >> >>>One more time: >> >>>If we consider confdb as library than we should never remove >> >>>functions. >> >>> >> >>>Removing functions from other parts of code is something >> >>>else. >> >>> >> > >> >>We did so in commit 2b94ab415b30861f42b68725d9231905baf8c3bd >> >I didn't notice that patch. I'm sorry I do not have a time >> >to follow each patchset. >> > >> >>We can always resurrect removed code if needed. >> >> >> >If we consider confdb as library than >> >we should never remove functions. >> >> Then we should decide which parts of SSSD are public libraries and >> which are not, then we could avoid this kind of discussions. >> >> >>>I wrote "consider confdb as library" and not as a public library with >> >>>stable API. And libraries many times contains function which are not >> >>>used. >> >>>So we should not remove them. Even though it would be still in git >> >>>history. >> >> >> >>Why? If we don't use them and nobody else does why do we have to keep >> >>them? >> >> >> >libraries many times contains function which are not used >> >> Well, but if they are not public we can change that, right? We can >> change them as we need and removing unused code is one of those things. >> I don't see the difference between private library and 'other parts of >> code'. >> >>> >> >>>I think we should be explicit here. If we want to consider confdb as a >> >>>library we should make it a public library with a stable API with all >> >>>the consequences. In general I think this is not a bad idea e.g. with >> >>>respect to all the config related tickets we have planned for 1.14. But >> >>>we should define the initial version of a stable API, based on the >> >>>result and requirements from the 1.14 tickets. >> >> >> >>I'm more with Simo here that we shouldn't consider confdb and sysdb as >> >>stable ABIs, at least not yet. I think we need the flexibility of a >> >>private, >> >>unstable library. >> >> >> >>At the same time, I think we should try harder to avoid polluting the >> >>sysdb/confdb APIs with new calls and in general strive to make even the >> >>internal
Re: [SSSD] RFC: Improving the debug messages
On Tue, Sep 29, 2015 at 07:53:41PM +0200, Jakub Hrozek wrote: > On Tue, Sep 29, 2015 at 06:15:35PM +0200, Sumit Bose wrote: > > On Tue, Sep 29, 2015 at 05:57:32PM +0200, Jakub Hrozek wrote: > > > On Mon, Sep 28, 2015 at 10:18:07AM +0200, Sumit Bose wrote: > > > > On Mon, Jun 29, 2015 at 11:07:30PM -0400, Dan Lavu wrote: > > > > > I've been watching various logs for the past few minutes, FWIW, I > > > > > think a more casual message will help people better understand what > > > > > SSSD is doing. Look at debug level 4, the first instance of a user > > > > > name look up (getent passwd dlavu) a common command we tell folks to > > > > > test to see if SSSD is working > > > > > > > > > > (Mon Jun 29 22:10:59 2015) [sssd[be[lab.runlevelone.lan]]] > > > > > [be_get_account_info] (0x0200): Got request for > > > > > [0x1001][1][name=dlavu] > > > > > > > > > > Just imagine if you knew nothing about SSSD and how it worked, what > > > > > can you gather from this line of text? Timestamp, process, domain, > > > > > get_account_info, got_request and userid and the rest is sort of > > > > > gibberish and what is the difference between be_get_account_info and > > > > > got_request_for? I think it's confusing, now looking at some other > > > > > logs messages. > > > > > > > > What is missing in the list of components above is '(0x0200)' which is > > > > the debug_level. We recently had a number of questions about messages > > > > from a high debug level which in general have an information character, > > > > but since it not easy to identify the level of the message it was > > > > considered as an error message. Maybe it would be good to have a some > > > > translations here as well to make it easier to separate errors from > > > > infos. > > > > > > Do you mean printing the level as string, so that instead of: > > > [sssd[be[lab.runlevelone.lan]]] [be_get_account_info] (0x0200): Got > > > request for [0x1001][1][name=dlavu] > > > we would have: > > > [sssd[be[lab.runlevelone.lan]]] [be_get_account_info] > > > (SSSDBG_FUNC_DATA): Got request for [0x1001][1][name=dlavu] > > > > > > ? > > > > > > If yes, then I agree. But at the same time, I think it's more irritating > > > > yes, that's what I meant, but maybe we should not use the internal > > macros here, but something more explicit like ERR_CRIT, ERR_FUNC, > > INF_CONFIG, INF_DATA, TRACE_... to make it more obvious what the message > > might be about. > > Yes, sure. Care to file a ticket? :-) It's https://fedorahosted.org/sssd/ticket/2808 . Please add ideas and suggestions how those tags shall look like. bye, Sumit > > > > > > that so many debug levels (mainly those that were mass-converted) are > > > misplaced. > > > > of course, but maybe having the level spelled out will help to identify > > the ones that are misplaced more easy? > > Yes, it would be easier to read a word than a hexa number. We could > even colorize the output when running interactively :-) > > What we can also do easily and with relatively low effort is to > select a couple of the most common usage patterns like: > * login with the correct password, online and offline > * login with wrong password > * run sudo > * change password > * > > And make sure they don't print any messages with debug_level <= 3. > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] AD: inicialize root_domain_attrs field
On Tue, Sep 29, 2015 at 09:25:13AM +0200, Jakub Hrozek wrote: > On Fri, Sep 25, 2015 at 11:00:07AM +0200, Jakub Hrozek wrote: > > On Fri, Sep 25, 2015 at 10:31:02AM +0200, Lukas Slebodnik wrote: > > > >The test_ipa_subdom_server test took me some time, that's why I don't > > > >think we need to block the release over a test. > > > We are not in hurry. > > > > Define "We". The Prominent North American Enterprise Linux Vendor > > downstream kinda is in hurry. > > Since I broke the code in ece345a74cec793e6d970a4955beb3d4a05935b3 which > had no tests for the modified source, I should write the tests now: > https://fedorahosted.org/sssd/ticket/2806 ACK and I pushed the patch to master: master: 101628a48d25ffae3b13c75d0b0b01577188c803 I just confused this patch and another one and set a wrong R-B :-/ sorry about that. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Fix configure script arguments for CentOS
On 09/30/2015 08:31 AM, Lukas Slebodnik wrote: On (29/09/15 17:02), Nikolai Kondrashov wrote: Er, no, I don't mind which way we go, but we'd better be consistent, so please either change Lukas's patch, or all the other places :) Or, Lukas, did you mean that "||" outside []/[[]] tests is different and separate and we should have these changed only? yes, it's something different. It's hard to notice || outside of tests []/[[]]. But it's better to be consistent and || before next command outsde tests is more awkward and more confusing. Updated patch is attached. Thank you, Lukas! I don't have CentOS handy, so I haven't tested it, but I trust you did. ACK. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] MAN: Clarify pam_trusted_users option description
Hi, while working on the hardening wiki page, I realized the pam_trusted_users option can be improved. Please see the attached patch. >From ac09c8dabb706ad1a870354a2879eb899d17c5fc Mon Sep 17 00:00:00 2001 From: Jakub HrozekDate: Wed, 30 Sep 2015 09:33:17 +0200 Subject: [PATCH] MAN: Clarify pam_trusted_users option description --- src/man/sssd.conf.5.xml | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 0ab4381819fa2f47e9a4bfb4897cb94144b96ff8..21acb63805c3906859130222cf335ad53be5894a 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -927,14 +927,18 @@ fallback_homedir = /home/%u pam_trusted_users (string) -Specifies the comma-separated list of UID values or -user names that are allowed to access the PAM -responder. User names are resolved to UIDs at +Specifies the comma-separated list of UID +values or user names that are allowed to run +PAM conversations against trusted domains. +Users not included in this list can only access +domains marked as public with +pam_public_domains. +User names are resolved to UIDs at startup. -Default: all (All users are allowed to access -the PAM responder) +Default: All users are considered trusted +by default Please note that UID 0 is always allowed to access -- 2.4.3 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] MAN: proxy and krb5 are valid access control modules
Hi, while documenting the security options I realized man sssd.conf doesn't include the krb5 and proxy access control modules. I hope I worded the sentence about krb5 correctly. >From 9c8ce997e738c537061d53c5499ad0d0417c012e Mon Sep 17 00:00:00 2001 From: Jakub HrozekDate: Wed, 30 Sep 2015 04:25:54 +0200 Subject: [PATCH] MAN: proxy and krb5 are valid access control modules --- src/man/sssd.conf.5.xml | 10 ++ 1 file changed, 10 insertions(+) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 9701f2a15fd67c799e199a99049bdcf259fdc254..0ab4381819fa2f47e9a4bfb4897cb94144b96ff8 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1716,6 +1716,16 @@ pam_account_expired_message = Account expired, please call help desk. information on configuring the simple access module. +krb5: .k5login based access control. +See +sssd-krb5 +5 for more +information on configuring Kerberos. + + +proxy for relaying access control to another PAM module. + + Default: permit -- 2.4.3 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
On Wed, Sep 30, 2015 at 10:37:30AM +0200, Lukas Slebodnik wrote: > On (30/09/15 09:38), Jakub Hrozek wrote: > >Hi, > > > >to help the OpenSCAP integration, I prepared a wiki page that contains > >options which have a security impact -- either positive (drop root) or > >negative (ignore certificate validation issues). > > > >I also tried to explain the effect of the options along with the > >description. There are some more items that can be included, but I > >wasn't sure about them myself, like: > >* should obfuscated passwords be mentioned? I wasn't sure because on > > one hand it really doesn't provide any benefit, on the other hand, > > the option can be used to check a compliance box that requires no > > passwords be stored in files.. > >* should the page warn against the > > auth-option-that-shall-not-be-mentioned or politely deny its > > existence? :-) > >* What about fd_limit ? Should resource consumption be considered > > a security property, especially if we already honor system default? I > > think here the default is enough, so I didn't document that option. > > > >Please provide your comments or edit the wiki directly. Thanks! > > What about: > >simple_deny_users (string) >Comma separated list of users who are explicitly denied access. > >simple_deny_groups (string) >Comma separated list of groups that are explicitly denied access. > This applies only to >groups within this SSSD domain. Local groups are not evaluated. > > Whitelisting is much secure than blacklisting. Good idea, added: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions?action=diff=6_version=5 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] RFC: Improving the debug messages
On Wed, Sep 30, 2015 at 01:10:20PM +0200, Petr Cech wrote: > On 09/30/2015 11:15 AM, Jakub Hrozek wrote: > >On Wed, Sep 30, 2015 at 09:53:24AM +0200, Sumit Bose wrote: > >>It's https://fedorahosted.org/sssd/ticket/2808 . Please add ideas and > >>suggestions how those tags shall look like. > > > >Thanks, I ressurected > >https://fedorahosted.org/sssd/ticket/1372 from Deferred as well. > >___ > > This topic resonates with me. Text instead of hexadecimal numbers is better > and it could make our logs more understandable. > And usage patterns are very nice guides for orientation in logs. > > I would like to work on this ticket. Sure, pick it up. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] Announcing SSSD 1.13.1
== SSSD 1.13.1 === The SSSD team is proud to announce the release of version 1.13.1 of the System Security Services Daemon. As always, the source is available from https://fedorahosted.org/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 == * Initial support for Smart Card authentication was added. The feature can be activated with the new pam_cert_auth option * The PAM prompting was enhanced so that when Two-Factor Authentication is used, both factors (password and token) can be entered separately on separate prompts. At the same time, only the long-term password is cached, so offline access would still work using the long term password * A new command line tool sss_override is present in this release. The tools allows to override attributes on the SSSD side. It's helpful in environment where e.g. some hosts need to have a different view of POSIX attributes than others. Please note that the overrides are stored in the cache as well, so removing the cache will also remove the overrides * New methods were added to the SSSD D-Bus interface. Notably support for looking up a user by certificate and looking up multiple users using a wildcard was added. Please see the interface introspection or the design pages for full details * Several enhancements to the dynamic DNS update code. Notably, clients that update multiple interfaces work better with this release * This release supports authenticating againt a KDC proxy * The fail over code was enhanced so that if a trusted domain is not reachable, only that domain will be marked as inactive but the backed would stay in online mode * Several fixes to the GPO access control code are present == Packaging Changes == * The Smart Card authentication feature requires a helper process p11_child that needs to be marked as setgid if SSSD needs to be able to. Please note the p11_child requires the NSS crypto library at the moment * The sss_override tool was added along with its own manpage * The upstream RPM can now build on RHEL/CentOS 6.7 == Documentation Changes == * The config_file_version configuration option now defaults to 2. As an effect, this option doesn't have to be set anymore unless the config file format is changed again by SSSD upstream * It is now possible to specify a comma-separated list of interfaces in the dyndns_iface option * The InfoPipe responder and the LDAP provider gained a new option wildcard_lookup that specifies an upper limit on the number of entries that can be returned with a wildcard lookup * A new option dyndns_server was added. This option allows to attempt a fallback DNS update against a specific DNS server. Please note this option only works as a fallback, the first attempt will always be performed against autodiscovered servers. * The PAM responder gained a new option ca_db that allows the storage of trusted CA certificates to be specified * The time the p11_child is allowed to operate can be specified using a new option p11_child_timeout == Tickets Fixed == https://fedorahosted.org/sssd/ticket/546 [RFE] Support for smart cards https://fedorahosted.org/sssd/ticket/1697 sssd: incorrect checks on length values during packet decoding https://fedorahosted.org/sssd/ticket/1926 [RFE] Start the dynamic DNS update after the SSSD has been setup for the first time https://fedorahosted.org/sssd/ticket/1994 Complain loudly if backend doesn't start due to missing or invalid keytab https://fedorahosted.org/sssd/ticket/2275 nested netgroups do not work in IPA provider https://fedorahosted.org/sssd/ticket/2283 test dyndns failed. https://fedorahosted.org/sssd/ticket/2335 Investigate using the krb5 responder for driving the PAM conversation with OTPs https://fedorahosted.org/sssd/ticket/2463 Pass error messages via the extdom plugin https://fedorahosted.org/sssd/ticket/2495 [RFE]Allow sssd to add a new option that would specify which server to update DNS with https://fedorahosted.org/sssd/ticket/2549 RFE: Support multiple interfaces with the dyndns_iface option https://fedorahosted.org/sssd/ticket/2553 RFE: Add support for wildcard-based cache updates https://fedorahosted.org/sssd/ticket/2558 Add dualstack and multihomed support https://fedorahosted.org/sssd/ticket/2561 Too much logging https://fedorahosted.org/sssd/ticket/2579 TRACKER: Support one-way trusts for IPA https://fedorahosted.org/sssd/ticket/2581 Re-check memcache after acquiring the lock in the client code https://fedorahosted.org/sssd/ticket/2584 RFE: Support client-side overrides
Re: [SSSD] RFC: Improving the debug messages
On Wed, Sep 30, 2015 at 09:53:24AM +0200, Sumit Bose wrote: > It's https://fedorahosted.org/sssd/ticket/2808 . Please add ideas and > suggestions how those tags shall look like. Thanks, I ressurected https://fedorahosted.org/sssd/ticket/1372 from Deferred as well. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Please review: https://fedorahosted.org/sssd/wiki/SecuritySensitiveOptions
On Wed, Sep 30, 2015 at 10:07:48AM +0200, Pavel Reichl wrote: > Should not we also mention dreadful option > ldap_auth_disable_tls_never_use_in_production ? That's what I was trying to ask :) > > * should the page warn against the > > auth-option-that-shall-not-be-mentioned or politely deny its > > existence? :-) I think it depends on the presentation a bit, if the tool just tells user 'remove this option' than it would be fine to include it. If the tool would present a list of options, like the wiki page does, then no. I guess we can sort this out with the SCAP developers.. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] RFC: Improving the debug messages
On 09/30/2015 11:15 AM, Jakub Hrozek wrote: On Wed, Sep 30, 2015 at 09:53:24AM +0200, Sumit Bose wrote: It's https://fedorahosted.org/sssd/ticket/2808 . Please add ideas and suggestions how those tags shall look like. Thanks, I ressurected https://fedorahosted.org/sssd/ticket/1372 from Deferred as well. ___ This topic resonates with me. Text instead of hexadecimal numbers is better and it could make our logs more understandable. And usage patterns are very nice guides for orientation in logs. I would like to work on this ticket. Petr ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] DYDNDS: update quality of input for nsupdate
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote: > > > On 09/21/2015 05:47 PM, Jakub Hrozek wrote: > >On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: > >> > >> > >>On 09/21/2015 10:52 AM, Pavel Březina wrote: > >>>On 08/28/2015 11:05 AM, Pavel Reichl wrote: > On 08/14/2015 04:12 PM, Jakub Hrozek wrote: > >> > Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and > server commands only as fallback > > I must have skipped it while respinning the patch set, sorry. > > I attached this missing patch and test that was failing because of it. > >>> > >>>Hi, > >>> > DYNDNS: use realm and server commands only as fallback > > Resolves: > https://fedorahosted.org/sssd/ticket/2195 > >>> > >>>You have a wrong ticket number here, I think. The patches look good > >>>otherwise. > >> > >>Yes, thanks for noticing! Updated patchset attached. > > > >https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is > >closed, did you mean another one? > > We cleared that off-list - patch was accidentally omitted from the original > patch set. Can you rebase these patches, please? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Run integration tests on debian testing
On 09/30/2015 09:59 AM, Lukas Slebodnik wrote: On (29/09/15 16:47), Nikolai Kondrashov wrote: >On 09/29/2015 03:41 PM, Lukas Slebodnik wrote: >>ehlo, >> >>Integration tests are enabled on debian with the last patch. > >So we've got more cwrap packages into Debian? Awesome:)! Or were they there >all the time and I simply failed to notice them? > uid-wrapper 1.0.2-1 was in testing since 2014-08-14 nss-wrapper 1.0.3-2 was in testing since 2015-06-05 https://packages.qa.debian.org/u/uid-wrapper.html https://packages.qa.debian.org/n/nss-wrapper.html Ah, so it was added while I was working on it and I didn't notice that. Ouch. >>Here is an alternative version: >> >>diff --git a/contrib/ci/run b/contrib/ci/run >>index 5f668ff..1f64e67 100755 >>--- a/contrib/ci/run >>+++ b/contrib/ci/run >>@@ -204,7 +204,7 @@ function build_debug() >> CK_FORK=no \ >> stage make-check-valgrind \ >> make-check-wrap -j $CPU_NUM check -- \ >>-libtool --mode=execute \ >>+./libtool --mode=execute \ >> valgrind-condense 99 \ >> '!(*.py|*dlopen-tests)' -- \ >> --trace-children=yes \ > >I like this way a little better, but not enough to argue:) > hmm, I tried it but it failed in cwrap test due to different directory. I could use full path but nicer solution is to install missing dependency Right, dependency it is then. >>All test failed due to missing /usr/bin/libtool >> >>e.g. >>/home/build/sssd/build/test-driver: line 107: libtool: command not found >>FAIL test-io (exit status: 127) > >Whoa! When did that start happening? > It happens if you run script on machine with minimal dependencies. libtool-bin could be a dependency of another package in past. D'oh, right. Looks fine now, tested on my Debian laptop - worked fine. Thank you, Lukas! ACK. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] [sssd-1.11] pysss_nss_idmap: Use wrapper for older python
On (19/06/15 14:25), Jakub Hrozek wrote: >On Tue, Jun 16, 2015 at 11:40:21PM +0200, Lukas Slebodnik wrote: >> ehlo, >> >> we dropped support for old version of python (<2.6) >> in recent version of sssd. It should work in 1.11 branch >> but there was a small issue in pysss_nss_idmap bindings. >> >> Attached is a simple patch wich reuse our internal python wrappers. >> >> LS > >ACK, builds fine: >http://koji.fedoraproject.org/koji/taskinfo?taskID=10164975 sssd-1-11: * 0ffc58e2407f1756b0c09fe31ecd21a75e88e833 LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Fix configure script arguments for CentOS
On (30/09/15 11:02), Nikolai Kondrashov wrote: >On 09/30/2015 08:31 AM, Lukas Slebodnik wrote: >>On (29/09/15 17:02), Nikolai Kondrashov wrote: >>>Er, no, I don't mind which way we go, but we'd better be consistent, so >>>please >>>either change Lukas's patch, or all the other places :) >>> >>>Or, Lukas, did you mean that "||" outside []/[[]] tests is different and >>>separate and we should have these changed only? >>yes, it's something different. >>It's hard to notice || outside of tests []/[[]]. >> >>But it's better to be consistent and || before next command >>outsde tests is more awkward and more confusing. >> >>Updated patch is attached. > >Thank you, Lukas! > >I don't have CentOS handy, so I haven't tested it, but I trust you did. > >ACK. > master: * 09365a02c9ff68f16227b69a348511bb584060bc LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] MAN: proxy and krb5 are valid access control modules
On Wed, 30 Sep 2015, Jakub Hrozek wrote: Hi, while documenting the security options I realized man sssd.conf doesn't include the krb5 and proxy access control modules. I hope I worded the sentence about krb5 correctly. ACK -- / Alexander Bokovoy ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] MAN: Clarify pam_trusted_users option description
On Wed, 30 Sep 2015, Jakub Hrozek wrote: Hi, while working on the hardening wiki page, I realized the pam_trusted_users option can be improved. Please see the attached patch. ACK, this is much better explanation than it was before. -- / Alexander Bokovoy ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Run integration tests on debian testing
On 09/30/2015 04:08 PM, Nikolai Kondrashov wrote: Looks fine now, tested on my Debian laptop - worked fine. Additionally, it worked on a fresh Debian Testing Docker image - niiice :) Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] confdb: warn if memcache_timeout > than entry_cache
On Wed, Sep 23, 2015 at 08:55:11PM +0200, Pavel Březina wrote: > Ack. Thank you! CI: http://sssd-ci.duckdns.org/logs/job/28/14/summary.html * master: 3fb1ee96f508784d7e06f079111d4d32d401a99b btw the ticket was in Backlog, not in the currently-in-progress 1.13.2, so I just moved it. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] CI: Already fixed bug for TODO
On 09/30/2015 08:20 AM, Lukas Slebodnik wrote: On (29/09/15 17:35), Nikolai Kondrashov wrote: On 09/29/2015 05:13 PM, Nikolai Kondrashov wrote: On 09/29/2015 03:54 PM, Lukas Slebodnik wrote: ehlo, I touched the CI script and I found an interesting todo distro.sh-52-{ distro.sh-53-declare prompt=$'Need root permissions to install packages.\n' distro.sh-54-prompt+="Enter sudo password for $USER: " distro.sh-55-if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then distro.sh-56-[ $# != 0 ] && sudo -p "$prompt" yum --assumeyes install -- "$@" |& distro.sh-57-# Pass input to output, fail if a missing package is reported distro.sh-58-# TODO Remove and switch to DNF once distro.sh:59:# https://bugzilla.redhat.com/show_bug.cgi?id=1128139 is fixed distro.sh-60-awk 'BEGIN {s=0} distro.sh-61- /^No package .* available.$/ {s=1} distro.sh-62- {print} distro.sh-63- END {exit s}' distro.sh-64-elif [[ "$DISTRO_BRANCH" == -debian-* ]]; then distro.sh-65-[ $# != 0 ] && sudo -p "$prompt" apt-get --yes install -- "$@" distro.sh-66-else The BZ1128139 was closed as duplicate of BZ1107737. The BZ1107737 was fixed 7 months ago and is available in dnf >= 0.6.4-2. The Fedora 21 currently have dnf-0.6.4-7. I think it's the best time for removing this todo. I would very much like to get rid of it, but unfortunately we're blocked by this DNF bug here: https://bugzilla.redhat.com/show_bug.cgi?id=1215208 Due to this bug we still have to use yum-deprecated on Fedora Rawhide (by modifying /usr/bin/yum to invoke yum-deprecated instead of dnf) and the bug mentioned in the TODO wasn't and likely won't be fixed in yum. So, I think we can only update the bug number for now. The patch is attached. Nick Would you mind open upstream tracking ticket rather then changing number of BZ? We have Continuous integration bucket. I opened the ticket: https://fedorahosted.org/sssd/ticket/2809 However, I think it's better to also update the source, so the next person reading it doesn't have the same question and/or tries to switch to DNF only to find another problem, the one described in the bug I mention. It's fine there is a plan how to fix BZ1215208 but it would be also good to know what is an ETA (at least approximately :-) I asked one of the DNF developers in a comment to the DNF bug. Let's see if we get any answer. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] AD: add debug messages for netlogon get info
On Wed, Sep 23, 2015 at 07:11:32AM +0200, Petr Cech wrote: > On 09/22/2015 05:25 PM, Pavel Reichl wrote: > >Hello, please see trivial patch attached. > Hello, > it works. > CI tests: http://sssd-ci.duckdns.org/logs/job/27/54/summary.html > > ACK > > Petr * master: 1e87219471c1220c773ea75b211ad0a4d087d869 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] intg: Add more LDAP tests
On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote: NOTE: These still break test_memory_cache.py as seen in the attached log file. Here's a fresher log, with the correct line numbers: http://sssd-ci.duckdns.org/logs/job/28/19/rhel7/ci-build-debug/ci-make-intgcheck.log Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] CI: Run integration tests on debian testing
On Wed, Sep 30, 2015 at 04:08:09PM +0300, Nikolai Kondrashov wrote: > Thank you, Lukas! > > ACK. CI: http://sssd-ci.duckdns.org/logs/job/28/16/summary.html * master: * cf37196dca93a8785c5a4e0af6e9a5053bff4e3a * 90063840941efb2015d4375333677e3c26b1f4e6 * cccbf6f5b2b80cd3f6311ccad96780faab93e1f7 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] intg: Add more LDAP tests
On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote: -create_conf_fixture(request, conf) +create_conf_fixture(request, format_basic_conf(ldap_conn, False, True)) Actually, I wanted to do one more thing with this and forgot: name the "bis" and "enum" parameters explicitly on each invocation to make it clear what's happening. E.g.: create_conf_fixture(request, format_basic_conf(ldap_conn, bis=False, enum=True)) Anyone in favor of/against this? Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] man: Minor fixes to filter_groups description
Hi everyone, I noticed one little thing was wrong with the combined filter_users/filter_groups description on the sssd.conf(5) manpage and also wanted to add a note WRT nested groups behavior with filter_groups which was a bit surprising to me. The trivial patches are attached. Nick >From 6cfaeaaf82f9cd8e1400ba4db6875646749c0375 Mon Sep 17 00:00:00 2001 From: Nikolai KondrashovDate: Wed, 30 Sep 2015 18:33:04 +0300 Subject: [PATCH 1/2] man: Mention groups in filter_groups description Mention groups (not only users) in the combined "filter_users"/"filter_groups" option description on the sssd.conf(5) manpage. --- src/man/sssd.conf.5.xml | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 9701f2a..a49938b 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -583,11 +583,11 @@ subdomain_inherit = ldap_purge_cache_timeout filter_users, filter_groups (string) -Exclude certain users from being fetched from the sss -NSS database. This is particularly useful for system -accounts. This option can also be set per-domain or -include fully-qualified names to filter only users from -the particular domain. +Exclude certain users or groups from being fetched +from the sss NSS database. This is particularly +useful for system accounts. This option can also +be set per-domain or include fully-qualified names +to filter only users from the particular domain. Default: root -- 2.5.1 >From 761e845dc0fac806e6c1587239a54d393452970c Mon Sep 17 00:00:00 2001 From: Nikolai Kondrashov Date: Wed, 30 Sep 2015 18:34:44 +0300 Subject: [PATCH 2/2] man: Note filter_groups are not affecting nesting Note that the "filter_groups" option doesn't affect nested member inheritance, on the sssd.conf(5) manpage. --- src/man/sssd.conf.5.xml | 8 1 file changed, 8 insertions(+) diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index a49938b..fa1ea31 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -590,6 +590,14 @@ subdomain_inherit = ldap_purge_cache_timeout to filter only users from the particular domain. +NOTE: The filter_groups option doesn't affect +inheritance of nested group members, since +filtering happens after they are propagated for +returning via NSS. E.g. a group having a member +group filtered out will still have the member +users of the latter listed. + + Default: root -- 2.5.1 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] intg: Add more LDAP tests
Hi everyone, Here is a patch set fixing some things in integration tests and adding more LDAP tests: * Adding/removing a user/group/membership with rfc2307(bis) schema. * Filtering users/groups with rfc2307(bis) schema. * The effect of override_homedir option. * The effect of fallback_homedir option. * The effect of override_shell option. * The effect of shell_fallback option. * The effect of default_shell option. * The effect of vetoed_shells option. These are pretty basic, but I think they're good for the start. Suggestions for more tests are welcome :) NOTE: These still break test_memory_cache.py as seen in the attached log file. We need to figure out why and do something with it. Otherwise, the tests work fine. Nick >From d09a103901a6f86873f9510152c600a062e255ed Mon Sep 17 00:00:00 2001 From: Nikolai KondrashovDate: Tue, 29 Sep 2015 20:00:14 +0300 Subject: [PATCH 1/7] intg: Get base DN from LDAP connection object Don't use the global LDAP_BASE_DN in integration tests and fixtures, but instead take it from the LDAP connection object (ldap_conn) passed to them explicitly. This makes the tests and fixtures a bit more modular. --- src/tests/intg/ldap_test.py | 8 src/tests/intg/test_memory_cache.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index ea114dc..0287a28 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -105,7 +105,7 @@ def create_sssd_fixture(request): @pytest.fixture def sanity_rfc2307(request, ldap_conn): -ent_list = ldap_ent.List(LDAP_BASE_DN) +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) ent_list.add_user("user2", 1002, 2002) ent_list.add_user("user3", 1003, 2003) @@ -150,7 +150,7 @@ def sanity_rfc2307(request, ldap_conn): @pytest.fixture def simple_rfc2307(request, ldap_conn): -ent_list = ldap_ent.List(LDAP_BASE_DN) +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user('usr001', 181818, 181818) ent_list.add_group("group1", 181818) @@ -181,7 +181,7 @@ def simple_rfc2307(request, ldap_conn): @pytest.fixture def sanity_rfc2307_bis(request, ldap_conn): -ent_list = ldap_ent.List(LDAP_BASE_DN) +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) ent_list.add_user("user2", 1002, 2002) ent_list.add_user("user3", 1003, 2003) @@ -309,7 +309,7 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis): @pytest.fixture def refresh_after_cleanup_task(request, ldap_conn): -ent_list = ldap_ent.List(LDAP_BASE_DN) +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) ent_list.add_group_bis("group1", 2001, ["user1"]) diff --git a/src/tests/intg/test_memory_cache.py b/src/tests/intg/test_memory_cache.py index 1f4ccb0..76d85fd 100644 --- a/src/tests/intg/test_memory_cache.py +++ b/src/tests/intg/test_memory_cache.py @@ -109,7 +109,7 @@ def create_sssd_fixture(request): def load_data_to_ldap(request, ldap_conn): -ent_list = ldap_ent.List(LDAP_BASE_DN) +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) ent_list.add_user("user2", 1002, 2002) ent_list.add_user("user3", 1003, 2003) -- 2.5.1 >From 8eb90565998f43fdc6b9ea3564527620c862f7fb Mon Sep 17 00:00:00 2001 From: Nikolai Kondrashov Date: Tue, 29 Sep 2015 20:13:04 +0300 Subject: [PATCH 2/7] intg: Remove _rfc2307 from function names Remove "_rfc2307" from integration test function names for brevity. --- src/tests/intg/ldap_test.py | 12 +++ src/tests/intg/test_memory_cache.py | 70 ++--- 2 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index 0287a28..e359ab4 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -104,7 +104,7 @@ def create_sssd_fixture(request): @pytest.fixture -def sanity_rfc2307(request, ldap_conn): +def sanity(request, ldap_conn): ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001) ent_list.add_user("user2", 1002, 2002) @@ -149,7 +149,7 @@ def sanity_rfc2307(request, ldap_conn): @pytest.fixture -def simple_rfc2307(request, ldap_conn): +def simple(request, ldap_conn): ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user('usr001', 181818, 181818) ent_list.add_group("group1", 181818) @@ -180,7 +180,7 @@ def simple_rfc2307(request, ldap_conn): @pytest.fixture -def sanity_rfc2307_bis(request, ldap_conn): +def sanity_bis(request, ldap_conn): ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) ent_list.add_user("user1", 1001, 2001)