Re: [SSSD] [WIKI] Contribute and DevelTips are duplicate
On 07/17/2015 01:26 PM, Petr Cech wrote: Hi, I have read the wiki pages. And I have the edited version. It would be difficult to send the diff, so I started a new pages where you can view the result. Original pages: [ 1] https://fedorahosted.org/sssd/wiki/Contribute [ 2] https://fedorahosted.org/sssd/wiki/DevelTips [ 3] https://fedorahosted.org/sssd/wiki/DevelTutorials [ 4] https://fedorahosted.org/sssd/wiki/Reporting_sssd_bugs [ 5] https://fedorahosted.org/sssd/wiki/BugLifecycle [ 6] https://fedorahosted.org/sssd/wiki/Repositories Content of [3] has been divided between [1] and [3], content of [5] has been divided between [1] and [4]. Then [3,5,6] will be deleted. Test of new pages: [ 7] https://fedorahosted.org/sssd/wiki/pcech_test_contribute [ 8] https://fedorahosted.org/sssd/wiki/pcech_test_devel_tips [ 9] https://fedorahosted.org/sssd/wiki/pcech_test_reporting_sssd_bugs Note that the links lead to the original pages. At [7] you can find COPR Repository section, but I am not sure with text here. Please look at it. I did not pass the whole wiki. I think there might be a link from [8] (perhaps [9]) on Troubleshooting. I look forward to your comments, I need the opinions of another persons. Petr Hi, a did some little edits according to talk with Jakub: * deleting Code Submission Process in Contribute * simplifying the structure of the headings in Contribute * adding link to tevent documentation in Devel tips * merging SSSD bug report and we would like to move link to COPR repo to the homepage (and add note about Ubuntu package, is it right?) So new version (without homepage and link to Ubuntu repo) is on the same place: [ 7] https://fedorahosted.org/sssd/wiki/pcech_test_contribute [ 8] https://fedorahosted.org/sssd/wiki/pcech_test_devel_tips [ 9] https://fedorahosted.org/sssd/wiki/pcech_test_reporting_sssd_bugs Petr ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] p11child: set restrictive umask and clear environment
On Mon, Aug 17, 2015 at 12:56:58PM +0200, Lukas Slebodnik wrote:
On (17/08/15 10:35), Jakub Hrozek wrote:
On Mon, Aug 17, 2015 at 09:01:24AM +0200, Lukas Slebodnik wrote:
On (13/08/15 17:46), Jakub Hrozek wrote:
Hi,
the attached patch hardens the p11_child process.
From 4023fb832cc5c5122c235b713c0ef401c5d21dd0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Wed, 5 Aug 2015 17:25:20 +0200
Subject: [PATCH] p11child: set restrictive umask and clear environment
https://fedorahosted.org/sssd/ticket/2754
Before doing any calls, set a very restrictive umask and clear
environment variables to harden p11child execution.
---
src/p11_child/p11_child_nss.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index
6948c142aa7843cda5ff6d18f5853b10c387c224..44ba6678893408dbfc0c6c7cfd5edcdaa789f518
100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -481,6 +481,9 @@ int main(int argc, const char *argv[])
/* Set debug level to invalid value so we can decide if -d 0 was
used. */
debug_level = SSSDBG_INVALID;
+clearenv();
+umask(077);
+
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
--
2.4.3
LGTM
Thanks, btw this code is unit-tested.
Do you think it would be good idea to the same for proxy_child.
Yes, good idea.
But I would propose to do two patches, because downstream only cares
about p11_child hardening.
In this case,
ACK to this patch.
Thank you for the review.
* master: 13f30f69eec02d0c0aaccc7b544dee1326a5e9d4
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On (16/08/15 17:59), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as transaction so
no changes are made if some error occurs, but I had some troubles with it so
I gave up eventually.
Of course, it would be quite difficult to make it safe across domains thus my
intention was only to ensure that either all changes are done in a domain or
none. But still leaving the possibility that changes are commited in one
domain but cancelled in another.
I tried to start sysdb transaction on all used domains and then commit/cancel
it, writing some neat mechanism for it. However, I occasionally run into a
problem when data provider hangs when trying to safe a user. It looked like
some sort of race condition.
Unfortunately I managed to delete the code so I can't show it to you, I think
it would be a nice feature so if anyone familiar with ldb want to step in,
he's welcome.
I briefly look at the patches and here are few comments
From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrez...@redhat.com
Date: Sat, 15 Aug 2015 16:42:27 +0200
Subject: [PATCH 2/2] sss_override: add import and export
Resolves:
https://fedorahosted.org/sssd/ticket/2737
---
Makefile.am| 2 +
src/tools/common/sss_colondb.c | 298 ++
src/tools/common/sss_colondb.h | 71
src/tools/sss_override.c | 866 +++--
4 files changed, 1128 insertions(+), 109 deletions(-)
create mode 100644 src/tools/common/sss_colondb.c
create mode 100644 src/tools/common/sss_colondb.h
diff --git a/Makefile.am b/Makefile.am
index
ed107fd5dc76b768176a3d7236b0bf1c75f212bf..f8c7c29df08c2ba9e74508c0f84ac57f6e6bac26
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -651,6 +651,7 @@ dist_noinst_HEADERS = \
src/lib/sifp/sss_sifp_private.h \
src/tests/cmocka/test_utils.h \
src/tools/common/sss_tools.h \
+src/tools/common/sss_colondb.h \
$(NULL)
@@ -1331,6 +1332,7 @@ sss_signal_LDADD = \
sss_override_SOURCES = \
src/tools/sss_override.c \
+src/tools/common/sss_colondb.c \
$(SSSD_TOOLS_OBJ) \
$(NULL)
sss_override_LDADD = \
diff --git a/src/tools/common/sss_colondb.c b/src/tools/common/sss_colondb.c
new file mode 100644
index
..be9396832e93fa3b646255dae214a34fe4d6c29c
--- /dev/null
+++ b/src/tools/common/sss_colondb.c
@@ -0,0 +1,298 @@
+/*
+Authors:
+Pavel Březina pbrez...@redhat.com
+
+Copyright (C) 2015 Red Hat
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU 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 General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+
+#include stdlib.h
+
+#include util/util.h
+#include util/strtonum.h
+#include tools/common/sss_colondb.h
+
+#define IS_STD_FILE(db) ((db)-file == stdin || (db)-file == stdout)
+#define IS_LINE_END(str) ((str) == NULL || *(str) == '\0' || *(str) == '\n')
^^^
This macro is unused.
Side note: if there is expectation occurrence '\n' should be higher
that occurrence of '\0' then you should change order in condition.
//snip
+errno_t sss_colondb_writeline(struct sss_colondb *db,
+ struct sss_colondb_write_field *table)
+{
+TALLOC_CTX *tmp_ctx;
+char *line = NULL;
+errno_t ret;
+int i;
+
+if (db-mode != SSS_COLONDB_WRITE) {
+return ERR_INTERNAL;
+}
+
+tmp_ctx = talloc_new(NULL);
+if (tmp_ctx == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE, talloc_new() failed.\n);
+return ENOMEM;
+}
+
+for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) {
+switch (table[i].type) {
+case SSS_COLONDB_UINT32:
+if (table[i].data.uint32 == 0) {
+line = talloc_asprintf_append(line, :);
+} else {
+line = talloc_asprintf_append(line, :%u,
table[i].data.uint32);
uint32_t has different fotmat specifier
@see man inttypes.h
+break;
+case SSS_COLONDB_STRING:
+if (table[i].data.str == NULL) {
+line = talloc_asprintf_append(line, :);
+} else {
+line = talloc_asprintf_append(line, :%s, table[i].data.str);
Maybe I'm wrong but the first entry in /etc/passwd is
Re: [SSSD] [PATCH] p11child: set restrictive umask and clear environment
On (17/08/15 10:35), Jakub Hrozek wrote:
On Mon, Aug 17, 2015 at 09:01:24AM +0200, Lukas Slebodnik wrote:
On (13/08/15 17:46), Jakub Hrozek wrote:
Hi,
the attached patch hardens the p11_child process.
From 4023fb832cc5c5122c235b713c0ef401c5d21dd0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Wed, 5 Aug 2015 17:25:20 +0200
Subject: [PATCH] p11child: set restrictive umask and clear environment
https://fedorahosted.org/sssd/ticket/2754
Before doing any calls, set a very restrictive umask and clear
environment variables to harden p11child execution.
---
src/p11_child/p11_child_nss.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index
6948c142aa7843cda5ff6d18f5853b10c387c224..44ba6678893408dbfc0c6c7cfd5edcdaa789f518
100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -481,6 +481,9 @@ int main(int argc, const char *argv[])
/* Set debug level to invalid value so we can decide if -d 0 was used.
*/
debug_level = SSSDBG_INVALID;
+clearenv();
+umask(077);
+
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
--
2.4.3
LGTM
Thanks, btw this code is unit-tested.
Do you think it would be good idea to the same for proxy_child.
Yes, good idea.
But I would propose to do two patches, because downstream only cares
about p11_child hardening.
In this case,
ACK to this patch.
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On (17/08/15 10:32), Jakub Hrozek wrote: On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2737 Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually. Thanks a lot for the patches. I didn't do any review yet, just scrolled through them. My first reaction was that the second patch should be split up and at least the colondb should be unit tested. btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb? Is there a design page for this? Maybe I missed something but I haven't noticed any discussion about export format for local views. I'm not very happy with custom format. Yes, it's already late because patches are ready and deadline is comming. Did you consider a ldif format? libldb already provide such functions. [alcik@unused-4-233][~/projects/sssd]$objdump -T /usr/lib64/libldb.so | grep ldb_ldif afe0 gDF .text 0641 LDB_1.1.0 ldb_ldif_parse_modrdn afc0 gDF .text 0008 LDB_0.9.10 ldb_ldif_write bcc0 gDF .text 000f LDB_1.1.5 ldb_ldif_read_file_state be00 gDF .text 0019 LDB_0.9.10 ldb_ldif_message_string bd40 gDF .text 0054 LDB_1.1.12 ldb_ldif_write_redacted_trace_string bcd0 gDF .text 0015 LDB_0.9.10 ldb_ldif_read_file bd20 gDF .text 001f LDB_0.9.10 ldb_ldif_write_file b630 gDF .text 0683 LDB_0.9.10 ldb_ldif_read afd0 gDF .text 000f LDB_0.9.10 ldb_ldif_read_free bda0 gDF .text 0054 LDB_0.9.10 ldb_ldif_write_string bcf0 gDF .text 002b LDB_0.9.10 ldb_ldif_read_string e.g. ldb_ldif_message_string can be used to format struct ldb_message as string /* Produce a string form of an ldb message convenient function to turn a ldb_message into a string. Useful for debugging */ char *ldb_ldif_message_string(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, enum ldb_changetype changetype, const struct ldb_message *msg); LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On 08/17/2015 10:52 AM, Lukas Slebodnik wrote: On (17/08/15 10:32), Jakub Hrozek wrote: On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2737 Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually. Thanks a lot for the patches. I didn't do any review yet, just scrolled through them. My first reaction was that the second patch should be split up and at least the colondb should be unit tested. btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb? Is there a design page for this? Maybe I missed something but I haven't noticed any discussion about export format for local views. I'm not very happy with custom format. Yes, it's already late because patches are ready and deadline is comming. The format is compatible with Dell VAS which is used by customer who requested local overrides. Did you consider a ldif format? libldb already provide such functions. [alcik@unused-4-233][~/projects/sssd]$objdump -T /usr/lib64/libldb.so | grep ldb_ldif afe0 gDF .text 0641 LDB_1.1.0 ldb_ldif_parse_modrdn afc0 gDF .text 0008 LDB_0.9.10 ldb_ldif_write bcc0 gDF .text 000f LDB_1.1.5 ldb_ldif_read_file_state be00 gDF .text 0019 LDB_0.9.10 ldb_ldif_message_string bd40 gDF .text 0054 LDB_1.1.12 ldb_ldif_write_redacted_trace_string bcd0 gDF .text 0015 LDB_0.9.10 ldb_ldif_read_file bd20 gDF .text 001f LDB_0.9.10 ldb_ldif_write_file b630 gDF .text 0683 LDB_0.9.10 ldb_ldif_read afd0 gDF .text 000f LDB_0.9.10 ldb_ldif_read_free bda0 gDF .text 0054 LDB_0.9.10 ldb_ldif_write_string bcf0 gDF .text 002b LDB_0.9.10 ldb_ldif_read_string e.g. ldb_ldif_message_string can be used to format struct ldb_message as string /* Produce a string form of an ldb message convenient function to turn a ldb_message into a string. Useful for debugging */ char *ldb_ldif_message_string(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, enum ldb_changetype changetype, const struct ldb_message *msg); LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On Mon, Aug 17, 2015 at 10:52:41AM +0200, Lukas Slebodnik wrote: On (17/08/15 10:32), Jakub Hrozek wrote: On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2737 Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually. Thanks a lot for the patches. I didn't do any review yet, just scrolled through them. My first reaction was that the second patch should be split up and at least the colondb should be unit tested. btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb? Is there a design page for this? Maybe I missed something but I haven't noticed any discussion about export format for local views. Maybe in future we could add support for extra formats, but the first iterations should use the format as described in man 5 passwd (except we don't override passwords) and man 5 groups (except we don't override group members). ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] pam: Incerease p11 child timeout
On (14/08/15 14:40), Michal Židek wrote: Hi, this patch is a hotfix for pam-srv-failing tests. Increasing the timeout to 30 seconds seems to be enough. I do not want to make it too big because the timeout is currently not configurable. The observation seems to be correct; at least according to log file http://sssd-ci.duckdns.org/logs/job/23/21/rhel7/ci-build-debug/pam-srv-tests.log There is a 3.5 seconds delays between creating child handler and 1st message in child. (Mon Aug 17 10:50:35:304490 2015) [sssd] [child_handler_setup] (0x2000): Setting up signal handler up for pid [30764] (Mon Aug 17 10:50:35:305471 2015) [sssd] [child_handler_setup] (0x2000): Signal handler set up for pid [30764] (Mon Aug 17 10:50:35:306615 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [pamuser] removed from PAM initgroup cache (Mon Aug 17 10:50:35:307109 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [wronguser] removed from PAM initgroup cache (Mon Aug 17 10:50:38:972629 2015) [[sssd[p11_child[30764 [main] (0x0400): p11_child started. And another 5 seconds between starting real work in the child (Mon Aug 17 10:50:39:091652 2015) [[sssd[p11_child[30764 [main] (0x0020): setuid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:093917 2015) [[sssd[p11_child[30764 [main] (0x0020): setgid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:094746 2015) [[sssd[p11_child[30764 [main] (0x2000): Running with real IDs [499][499]. (Mon Aug 17 10:50:44:375594 2015) [[sssd[p11_child[30764 [do_work] (0x4000): Default Module List: It might be caused either by valgrind or by high load on machine. IMHO changing default from 10 seconds to 30 is too much. We can use the same trick in dyndns_test dyndns_tests_CFLAGS = \ $(AM_CFLAGS) \ -DDYNDNS_TIMEOUT=2 and in pamsrv_cmd.c use conditional default value. #ifndef SSS_P11_CHILD_TIMEOUT # define SSS_P11_CHILD_TIMEOUT 30 #endif I'd like to talk to Sumit about what he thinks the proper solution should be. I am not sure if it times out because we do something unnecessary in p11 child. See the simple patch. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] test_memory_cache: Wait short time after cache invalidation
On 08/17/2015 02:42 PM, Lukas Slebodnik wrote: On (13/08/15 13:31), Michal Židek wrote: On 08/13/2015 07:05 AM, Lukas Slebodnik wrote: On (12/08/15 14:35), Michal Židek wrote: Seriously? Is that your proposed solution? Yes, it's better to remove test rather then have intermittent failures. You are twisting the plot here. Removing the test would only be an option if we had no idea how to fix them. Which was definitely not the case. the other alternative is we cannot agree on solution. BTW I agree with Nikolai, On Thu, Aug 13, 2015 at 01:40:04PM +0300, Nikolai Kondrashov wrote: I suggest we disable the tests that fail, even if intermittently, and file corresponding bugs (if not filed yet). Otherwise CI will soon become useless. So it's better to immediately disable problematic tests. It is better to go with the patches you sent before (not the one that removes the tests, that would be very wrong) rather than trying to continue this discussion. There's nothing to continue with discussion, Q:Why? You proposed to add unnecessary code complication to workaround race condition. It will not fix the race condition. Because the same issue can happen next time. It was not unnecessary and it can barely be called a complication. As you said it is very unlikely to happen but it definitely can happen. It can happen in very x 107 rare cases. But it did not solve it completly. So it is just unnecessary complication. Q:How is it possible? Becausue veryx107 unlikely race condition is typical TOCTOU[1]. In teory, you can hit this same situation when sombody restarted sssd. My paches added 1 second delay and you proposed try one more time. It is even less likely that it will happen but sssd can be restarted after 1 second as well and you will hit the same race condition next time. So we would add unnecessary complicated code which does not solve anything. The most important reason to not use your proposal is that patches for ticket #2748 did not introduce the race condition. Q:So does it mean that race condition is already in master? yes, I do know it is in master. What is the point if this? Yes, it is in master and I also provided a hash :-) So the race condition is caused by two main factors. a) detections of running sssd_nss is not reliable. It's just sufficient enough. If there is a process which owns file locks it needn;t be a sssd_nss. In teory, it can be another sss_cache utility. b) sending message to sssd_nss is not releable either. It's just sufficient enough. Nobody guarantee that if you create the file clear_mc_flag. It does not guarantee that sssd_nss will recieve the message. So here is how can the race condition happen 1. sss_cache will cannot obtain file locks to memory caches (there is an asumption that sssd_nss is running) 2. sssd is stopped 3. sss_cache will send the message to sssd_nss (create file clear_mc_flag) However sssd_nss cannot process such message; because it is not running. BTW it's the same situation as with TCP and UDP on local network. UDP on local network is sufficient enough for sending simple messages but nobody guarantee receiving of that message. If you want to use something reliable you need to use TCP. My patch didn't touch anything about file locks and did't touch code with sending message to sssd_nss. So it could not create this race condition. Moreover, such race condition cannot happen in integration tests; stopping sssd and calling sss_cache will never happen in parallel. It was introduced by commit 33cbb789ff71be5dccbb4a0acd68814b0d53da34 and will not solve the ticket #2748 commit 33cbb789ff71be5dccbb4a0acd68814b0d53da34 Author: Michal Zidek mzi...@redhat.com AuthorDate: Fri Oct 26 17:36:51 2012 +0200 Commit: Jakub Hrozek jhro...@redhat.com CommitDate: Tue Nov 6 12:29:28 2012 +0100 sss_cache: Remove fastcache even if sssd is not running. https://fedorahosted.org/sssd/ticket/1584 Q: But it's a race conditions and we should fix it; or no? As I already wrote it's a is typical TOCTOU. The problematic code is neither in library nor daemon part of sssd. The proper solution without atomic function would be very complicated. We do not have atomic function obtain locks or create file. The full solution would require complicated analysis of all corner case. The ideal would be to use final state automata(FSA) or another formal method to prove there race condition is fixed. It's not a solution to retry one more time after a second. It will not fix the race condition and code will become complicated. The proper solution would be even more complicated. FSA converted to programming language are not very readable. It really does not worth such solution in a command line utility The change I proposed is simple. It basically goes like this: 1. You hit the race condition - try again few times 2. If you try at least one more time and you are not constantly restarting sssd, the problem is solved and the caches
Re: [SSSD] [PATCH] test_memory_cache: Wait short time after cache invalidation
On (13/08/15 13:31), Michal Židek wrote: On 08/13/2015 07:05 AM, Lukas Slebodnik wrote: On (12/08/15 14:35), Michal Židek wrote: Seriously? Is that your proposed solution? Yes, it's better to remove test rather then have intermittent failures. You are twisting the plot here. Removing the test would only be an option if we had no idea how to fix them. Which was definitely not the case. the other alternative is we cannot agree on solution. BTW I agree with Nikolai, On Thu, Aug 13, 2015 at 01:40:04PM +0300, Nikolai Kondrashov wrote: I suggest we disable the tests that fail, even if intermittently, and file corresponding bugs (if not filed yet). Otherwise CI will soon become useless. So it's better to immediately disable problematic tests. It is better to go with the patches you sent before (not the one that removes the tests, that would be very wrong) rather than trying to continue this discussion. There's nothing to continue with discussion, Q:Why? You proposed to add unnecessary code complication to workaround race condition. It will not fix the race condition. Because the same issue can happen next time. It was not unnecessary and it can barely be called a complication. As you said it is very unlikely to happen but it definitely can happen. It can happen in very x 107 rare cases. But it did not solve it completly. So it is just unnecessary complication. Q:How is it possible? Becausue veryx107 unlikely race condition is typical TOCTOU[1]. In teory, you can hit this same situation when sombody restarted sssd. My paches added 1 second delay and you proposed try one more time. It is even less likely that it will happen but sssd can be restarted after 1 second as well and you will hit the same race condition next time. So we would add unnecessary complicated code which does not solve anything. The most important reason to not use your proposal is that patches for ticket #2748 did not introduce the race condition. Q:So does it mean that race condition is already in master? yes, I do know it is in master. What is the point if this? Yes, it is in master and I also provided a hash :-) So the race condition is caused by two main factors. a) detections of running sssd_nss is not reliable. It's just sufficient enough. If there is a process which owns file locks it needn;t be a sssd_nss. In teory, it can be another sss_cache utility. b) sending message to sssd_nss is not releable either. It's just sufficient enough. Nobody guarantee that if you create the file clear_mc_flag. It does not guarantee that sssd_nss will recieve the message. So here is how can the race condition happen 1. sss_cache will cannot obtain file locks to memory caches (there is an asumption that sssd_nss is running) 2. sssd is stopped 3. sss_cache will send the message to sssd_nss (create file clear_mc_flag) However sssd_nss cannot process such message; because it is not running. BTW it's the same situation as with TCP and UDP on local network. UDP on local network is sufficient enough for sending simple messages but nobody guarantee receiving of that message. If you want to use something reliable you need to use TCP. My patch didn't touch anything about file locks and did't touch code with sending message to sssd_nss. So it could not create this race condition. Moreover, such race condition cannot happen in integration tests; stopping sssd and calling sss_cache will never happen in parallel. It was introduced by commit 33cbb789ff71be5dccbb4a0acd68814b0d53da34 and will not solve the ticket #2748 commit 33cbb789ff71be5dccbb4a0acd68814b0d53da34 Author: Michal Zidek mzi...@redhat.com AuthorDate: Fri Oct 26 17:36:51 2012 +0200 Commit: Jakub Hrozek jhro...@redhat.com CommitDate: Tue Nov 6 12:29:28 2012 +0100 sss_cache: Remove fastcache even if sssd is not running. https://fedorahosted.org/sssd/ticket/1584 Q: But it's a race conditions and we should fix it; or no? As I already wrote it's a is typical TOCTOU. The problematic code is neither in library nor daemon part of sssd. The proper solution without atomic function would be very complicated. We do not have atomic function obtain locks or create file. The full solution would require complicated analysis of all corner case. The ideal would be to use final state automata(FSA) or another formal method to prove there race condition is fixed. It's not a solution to retry one more time after a second. It will not fix the race condition and code will become complicated. The proper solution would be even more complicated. FSA converted to programming language are not very readable. It really does not worth such solution in a command line utility The change I proposed is simple. It basically goes like this: 1. You hit the race condition - try again few times 2. If you try at least one more time and you are not constantly restarting sssd, the problem is solved and the caches will be removed with the second attempt. 3. Even if you are constantly
Re: [SSSD] [PATCH] pam: Incerease p11 child timeout
On Mon, Aug 17, 2015 at 03:03:35PM +0200, Lukas Slebodnik wrote: On (17/08/15 14:45), Michal Židek wrote: On 08/17/2015 01:22 PM, Lukas Slebodnik wrote: On (14/08/15 14:40), Michal Židek wrote: Hi, this patch is a hotfix for pam-srv-failing tests. Increasing the timeout to 30 seconds seems to be enough. I do not want to make it too big because the timeout is currently not configurable. The observation seems to be correct; at least according to log file http://sssd-ci.duckdns.org/logs/job/23/21/rhel7/ci-build-debug/pam-srv-tests.log There is a 3.5 seconds delays between creating child handler and 1st message in child. (Mon Aug 17 10:50:35:304490 2015) [sssd] [child_handler_setup] (0x2000): Setting up signal handler up for pid [30764] (Mon Aug 17 10:50:35:305471 2015) [sssd] [child_handler_setup] (0x2000): Signal handler set up for pid [30764] (Mon Aug 17 10:50:35:306615 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [pamuser] removed from PAM initgroup cache (Mon Aug 17 10:50:35:307109 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [wronguser] removed from PAM initgroup cache (Mon Aug 17 10:50:38:972629 2015) [[sssd[p11_child[30764 [main] (0x0400): p11_child started. And another 5 seconds between starting real work in the child (Mon Aug 17 10:50:39:091652 2015) [[sssd[p11_child[30764 [main] (0x0020): setuid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:093917 2015) [[sssd[p11_child[30764 [main] (0x0020): setgid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:094746 2015) [[sssd[p11_child[30764 [main] (0x2000): Running with real IDs [499][499]. (Mon Aug 17 10:50:44:375594 2015) [[sssd[p11_child[30764 [do_work] (0x4000): Default Module List: It might be caused either by valgrind or by high load on machine. IMHO changing default from 10 seconds to 30 is too much. We can use the same trick in dyndns_test dyndns_tests_CFLAGS = \ $(AM_CFLAGS) \ -DDYNDNS_TIMEOUT=2 and in pamsrv_cmd.c use conditional default value. #ifndef SSS_P11_CHILD_TIMEOUT # define SSS_P11_CHILD_TIMEOUT 30 #endif I'd like to talk to Sumit about what he thinks the proper solution should be. I am not sure if it times out because we do something unnecessary in p11 child. See the simple patch. LS Thank you for comments. I pushed the newly attached patch to CI (5 times, essential tests only, with disabled be_req tests). To see if it fails. I am waiting for the results now. I'm definitelly sure it will work. I tested first version with hardcoded 30 seconds and it passed more then 25 runs :-) BTW it would be good to find out what is a reason so big (3-5 seconds) timeouts. From 9b218d50bcfb31bbb8167ea9b02711de9a6de14b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com Date: Thu, 13 Aug 2015 14:03:24 +0200 Subject: [PATCH] pam: Incerease p11 child timeout Ticket: https://fedorahosted.org/sssd/ticket/2746 It was timeouting often in CI machines. --- ACK * master: 9da121c08b785b56733a11fa46e14c708dda62e9 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] pam: Incerease p11 child timeout
On (17/08/15 14:45), Michal Židek wrote: On 08/17/2015 01:22 PM, Lukas Slebodnik wrote: On (14/08/15 14:40), Michal Židek wrote: Hi, this patch is a hotfix for pam-srv-failing tests. Increasing the timeout to 30 seconds seems to be enough. I do not want to make it too big because the timeout is currently not configurable. The observation seems to be correct; at least according to log file http://sssd-ci.duckdns.org/logs/job/23/21/rhel7/ci-build-debug/pam-srv-tests.log There is a 3.5 seconds delays between creating child handler and 1st message in child. (Mon Aug 17 10:50:35:304490 2015) [sssd] [child_handler_setup] (0x2000): Setting up signal handler up for pid [30764] (Mon Aug 17 10:50:35:305471 2015) [sssd] [child_handler_setup] (0x2000): Signal handler set up for pid [30764] (Mon Aug 17 10:50:35:306615 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [pamuser] removed from PAM initgroup cache (Mon Aug 17 10:50:35:307109 2015) [sssd] [pam_initgr_cache_remove] (0x2000): [wronguser] removed from PAM initgroup cache (Mon Aug 17 10:50:38:972629 2015) [[sssd[p11_child[30764 [main] (0x0400): p11_child started. And another 5 seconds between starting real work in the child (Mon Aug 17 10:50:39:091652 2015) [[sssd[p11_child[30764 [main] (0x0020): setuid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:093917 2015) [[sssd[p11_child[30764 [main] (0x0020): setgid failed: 1, p11_child might not work! (Mon Aug 17 10:50:39:094746 2015) [[sssd[p11_child[30764 [main] (0x2000): Running with real IDs [499][499]. (Mon Aug 17 10:50:44:375594 2015) [[sssd[p11_child[30764 [do_work] (0x4000): Default Module List: It might be caused either by valgrind or by high load on machine. IMHO changing default from 10 seconds to 30 is too much. We can use the same trick in dyndns_test dyndns_tests_CFLAGS = \ $(AM_CFLAGS) \ -DDYNDNS_TIMEOUT=2 and in pamsrv_cmd.c use conditional default value. #ifndef SSS_P11_CHILD_TIMEOUT # define SSS_P11_CHILD_TIMEOUT 30 #endif I'd like to talk to Sumit about what he thinks the proper solution should be. I am not sure if it times out because we do something unnecessary in p11 child. See the simple patch. LS Thank you for comments. I pushed the newly attached patch to CI (5 times, essential tests only, with disabled be_req tests). To see if it fails. I am waiting for the results now. I'm definitelly sure it will work. I tested first version with hardcoded 30 seconds and it passed more then 25 runs :-) BTW it would be good to find out what is a reason so big (3-5 seconds) timeouts. From 9b218d50bcfb31bbb8167ea9b02711de9a6de14b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com Date: Thu, 13 Aug 2015 14:03:24 +0200 Subject: [PATCH] pam: Incerease p11 child timeout Ticket: https://fedorahosted.org/sssd/ticket/2746 It was timeouting often in CI machines. --- ACK LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] pam: Incerease p11 child timeout
On 08/17/2015 01:22 PM, Lukas Slebodnik wrote:
On (14/08/15 14:40), Michal Židek wrote:
Hi,
this patch is a hotfix for pam-srv-failing
tests.
Increasing the timeout to 30 seconds seems
to be enough. I do not want to make it too
big because the timeout is currently not
configurable.
The observation seems to be correct; at least according to log file
http://sssd-ci.duckdns.org/logs/job/23/21/rhel7/ci-build-debug/pam-srv-tests.log
There is a 3.5 seconds delays between creating child handler and 1st message in
child.
(Mon Aug 17 10:50:35:304490 2015) [sssd] [child_handler_setup] (0x2000):
Setting up signal handler up for pid [30764]
(Mon Aug 17 10:50:35:305471 2015) [sssd] [child_handler_setup] (0x2000): Signal
handler set up for pid [30764]
(Mon Aug 17 10:50:35:306615 2015) [sssd] [pam_initgr_cache_remove] (0x2000):
[pamuser] removed from PAM initgroup cache
(Mon Aug 17 10:50:35:307109 2015) [sssd] [pam_initgr_cache_remove] (0x2000):
[wronguser] removed from PAM initgroup cache
(Mon Aug 17 10:50:38:972629 2015) [[sssd[p11_child[30764 [main] (0x0400):
p11_child started.
And another 5 seconds between starting real work in the child
(Mon Aug 17 10:50:39:091652 2015) [[sssd[p11_child[30764 [main] (0x0020):
setuid failed: 1, p11_child might not work!
(Mon Aug 17 10:50:39:093917 2015) [[sssd[p11_child[30764 [main] (0x0020):
setgid failed: 1, p11_child might not work!
(Mon Aug 17 10:50:39:094746 2015) [[sssd[p11_child[30764 [main] (0x2000):
Running with real IDs [499][499].
(Mon Aug 17 10:50:44:375594 2015) [[sssd[p11_child[30764 [do_work]
(0x4000): Default Module List:
It might be caused either by valgrind or by high load on machine.
IMHO changing default from 10 seconds to 30 is too much.
We can use the same trick in dyndns_test
dyndns_tests_CFLAGS = \
$(AM_CFLAGS) \
-DDYNDNS_TIMEOUT=2
and in pamsrv_cmd.c use conditional default value.
#ifndef SSS_P11_CHILD_TIMEOUT
# define SSS_P11_CHILD_TIMEOUT 30
#endif
I'd like to talk to Sumit about what he thinks
the proper solution should be. I am not sure
if it times out because we do something
unnecessary in p11 child.
See the simple patch.
LS
Thank you for comments. I pushed the newly attached
patch to CI (5 times, essential tests only, with
disabled be_req tests). To see if it fails. I
am waiting for the results now.
Michal
From 9b218d50bcfb31bbb8167ea9b02711de9a6de14b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com
Date: Thu, 13 Aug 2015 14:03:24 +0200
Subject: [PATCH] pam: Incerease p11 child timeout
Ticket:
https://fedorahosted.org/sssd/ticket/2746
It was timeouting often in CI machines.
---
Makefile.am| 1 +
src/responder/pam/pamsrv_cmd.c | 9 +++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 8b64317..5e5c559 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1892,6 +1892,7 @@ pam_srv_tests_SOURCES = \
pam_srv_tests_CFLAGS = \
-U SSSD_LIBEXEC_PATH -DSSSD_LIBEXEC_PATH=\$(abs_builddir)\ \
$(AM_CFLAGS) \
+-DSSS_P11_CHILD_TIMEOUT=30 \
$(NULL)
pam_srv_tests_LDFLAGS = \
-Wl,-wrap,sss_packet_get_body \
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 3b84fb8..aa5c209 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -43,6 +43,11 @@ enum pam_verbosity {
#define DEFAULT_PAM_VERBOSITY PAM_VERBOSITY_IMPORTANT
+/* TODO: Should we make this configurable? */
+#ifndef SSS_P11_CHILD_TIMEOUT
+#define SSS_P11_CHILD_TIMEOUT 10
+#endif
+
static errno_t
pam_null_last_online_auth_with_curr_token(struct sss_domain_info *domain,
const char *username);
@@ -1122,7 +1127,7 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
if (may_do_cert_auth(pctx, pd)) {
req = pam_check_cert_send(cctx, cctx-ev, pctx-p11_child_debug_fd,
- pctx-nss_db, 10, pd);
+ pctx-nss_db, SSS_P11_CHILD_TIMEOUT, pd);
if (req == NULL) {
DEBUG(SSSDBG_OP_FAILURE, pam_check_cert_send failed.\n);
ret = ENOMEM;
@@ -1338,7 +1343,7 @@ static void pam_forwarder_cb(struct tevent_req *req)
if (may_do_cert_auth(pctx, pd)) {
req = pam_check_cert_send(cctx, cctx-ev, pctx-p11_child_debug_fd,
- pctx-nss_db, 10, pd);
+ pctx-nss_db, SSS_P11_CHILD_TIMEOUT, pd);
if (req == NULL) {
DEBUG(SSSDBG_OP_FAILURE, pam_check_cert_send failed.\n);
ret = ENOMEM;
--
2.1.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHSET] LDAP: sanitize group name when used in filter
On Thu, Aug 13, 2015 at 12:23:02PM +0200, Pavel Březina wrote:
On 08/06/2015 02:31 PM, Pavel Reichl wrote:
On 08/05/2015 02:44 PM, Pavel Březina wrote:
On 08/05/2015 12:11 PM, Pavel Reichl wrote:
On 08/05/2015 11:34 AM, Pavel Březina wrote:
On 08/04/2015 03:52 PM, Pavel Reichl wrote:
Hello,
please see 2 simple patches attached.
I could not find function to sanitize DN so it could be used as
part of
filter (sanitize ()*/\...) so I had to write one.
sysdb_dn_sanitize is not the right choice,
sysdb_dn_sanitize(name=expired-group(2016),cn=groups,cn=LOCAL,cn=sysdb)
-
name\\3Dexpired-group(2016)\\,cn\\3Dgroups\\,cn\\3DLOCAL\\,cn\\3Dsysdb
Thanks!
Hi, I did just a quick read of your patches... can you take one more
step with creating a sanitized dn and create a more generic function
for that?
Have you considered to modify sysdb_dn_sanitize to also escape
parentheses (that's what is misssing, isn't it)?
no because sysdb_dn_sanitize escapes also ',' and '=' and I need them to
stat as they are
This is what I have:
name=expired-group(2016),cn=groups,cn=LOCAL,cn=sysdb
This is what I need:
name=expired-group\282016\29,cn=groups,cn=LOCAL,cn=sysdb // just
escape '(' and ')'
This is what sysdb_dn_sanitize returns:
name\\3Dexpired-group(2016)\\,cn\\3Dgroups\\,cn\\3DLOCAL\\,cn\\3Dsysdb
Failing filter:
((objectClass=user)(|(memberOf=name=VDI-US02_Corporate-Environment(2013),cn=groups,cn=qut.edu.au,cn=sysdb)
Corrent filter
((objectClass=user)(|(memberOf=name=VDI-US02_Corporate-Environment\282013\29,cn=groups,cn=qut.edu.au,cn=sysdb)
I hope it's clearer now.
Of course... sysdb_dn_sanitize is not supposed to be called on the
whole dn. Just on the name part. It mean sanitize value so it can be
used in dn. But changing it to also escape parentheses would require
sysdb and code update, so it is not worth it.
+static errno_t
+get_group_dn_with_filter_sanitized_name(TALLOC_CTX *mem_ctx,
+struct sss_domain_info *domain,
+const char *grp_name,
+const char **_grp_dn);
Can you use group_name and _group_dn? Two characters won't kill
anybody :-) Otherwise we can keep the code as is. I have just one
recommendation for tests:
Sure, done.
+/* let records to expire */
+usleep(110);
It will be better to expire the records manually by setting expiration
time to zero. I'm not sure if we have already a function for that, if
not, please write one. It may be quite useful for tests.
I agree with you and I know that you would prefer the function to be
generic and part of sysdb. But I am afraid that It would take too much
time to do it properly and we should also handle code duplication that
would be introduced to sss_cache.c. Would static function in this test
be sufficient temporal solution for now? I would also file a ticket for
proper solution. Is this OK with you?
Thanks!
Ack from me.
* master:
* e2e334b2f51118cb14c7391c4e4e44ff247ef638
* 4772d3f1fe5015a25ba1fb4c3779ee3117ec6fcb
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Use sss_unique_file instead of calling mkstemp directly
On Thu, Aug 13, 2015 at 03:43:43PM +0200, Pavel Březina wrote: On 08/12/2015 02:23 PM, Jakub Hrozek wrote: Hi, there is a patchset that adds sss_unique_file with an optional destructor: https://patchwork.acksyn.org/patch/11351/ Attached are patches that use the function instead of mkstemp(). As you can see in the patches, we already fix some bugs this way, especially removing the tmpfiles left behind on error. In some cases, I also added closing the fd on error. Ack. * master: * 84493af37d4b57294e94b7bb0596dec51e06b7b0 * 51ae9cb4ed85b60cfe00eaf6d3a4af39ed409ddc * df07d54f881e6210c9cb6650de5617e6a99602b9 * f5db13d4462faa531c9924181f0fd51364647e2d ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH v3] Remove trailing whitespace
- Original Message - From: Nikolai Kondrashov nikolai.kondras...@redhat.com To: Development of the System Security Services Daemon sssd-devel@lists.fedorahosted.org Sent: Monday, August 17, 2015 6:40:36 PM Subject: [SSSD] [PATCH v3] Remove trailing whitespace On 08/17/2015 09:37 AM, Lukas Slebodnik wrote: On (13/08/15 15:48), Nikolai Kondrashov wrote: On 08/13/2015 02:57 PM, Lukas Slebodnik wrote: On (13/08/15 14:47), Nikolai Kondrashov wrote: +if GIT_CHECKOUT +export GIT_DIR=$(abs_top_srcdir)/.git +export GIT_WORK_TREE=$(abs_top_srcdir) +endif It's not good idea to export such in general code. you might greak something. I didn't try but you might break something in fedpkg + local build. Better solution would to to export make variable abs_top_srcdir as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT and print warnig/error from test if this variable is not defined. Yeah, it's not nice. I doubt that it will break anything as it is very specific as to where the git repo is, but I wouldn't vouch for it. But on the other hand ABS_TOP_SRCDIR is more abstract and there is higher probability it will be used in other tests. We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets as well, which will make it uglier, but I'll do it. You can still export GIT_DIR and GIT_WORK_TREE inside script if ABS_TOP_SRCDIR is defined. Otherwise test can be skipped https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html Alright here's another version. I didn't change it much. Simply moved export of GIT_DIR and GIT_WORK_TREE lower, to TESTS_ENVIRONMENT and *rpm* targets. I didn't add exporting of ABS_TOP_SRCDIR, because that way the whitespace test would always require it, making manual execution inconvenient, or would need additional logic with assumptions there. This way we avoid having another variable and a way to pass build parameters around. BTW, Lukas, could you please describe what kind of problems you expect in fedpkg + local build? Perhaps it's not a problem after all? fedpkg compile usage: fedpkg compile [-h] [--arch ARCH] [--short-circuit] This command calls rpmbuild to compile the source. By default the prep and configure stages will be done as well, unless the short-circuit option is used And where I can see a problem. fedpkg is a git wrapper. The git contains patches + spec file. If you call fedpkg compile then rpmbuild will extract tarball and apply patches. configure will detect there is a git and the test will fail because patches contains trailing whitespaces. So GIT_WORK_TREE is not an equivalent to ABS_TOP_SRCDIR. This is an example of for fedora. There might be other distribution where it can cause troubles as well. So it's better to be defensive. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH v3] Remove trailing whitespace
On 08/17/2015 09:37 AM, Lukas Slebodnik wrote: On (13/08/15 15:48), Nikolai Kondrashov wrote: On 08/13/2015 02:57 PM, Lukas Slebodnik wrote: On (13/08/15 14:47), Nikolai Kondrashov wrote: +if GIT_CHECKOUT +export GIT_DIR=$(abs_top_srcdir)/.git +export GIT_WORK_TREE=$(abs_top_srcdir) +endif It's not good idea to export such in general code. you might greak something. I didn't try but you might break something in fedpkg + local build. Better solution would to to export make variable abs_top_srcdir as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT and print warnig/error from test if this variable is not defined. Yeah, it's not nice. I doubt that it will break anything as it is very specific as to where the git repo is, but I wouldn't vouch for it. But on the other hand ABS_TOP_SRCDIR is more abstract and there is higher probability it will be used in other tests. We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets as well, which will make it uglier, but I'll do it. You can still export GIT_DIR and GIT_WORK_TREE inside script if ABS_TOP_SRCDIR is defined. Otherwise test can be skipped https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html Alright here's another version. I didn't change it much. Simply moved export of GIT_DIR and GIT_WORK_TREE lower, to TESTS_ENVIRONMENT and *rpm* targets. I didn't add exporting of ABS_TOP_SRCDIR, because that way the whitespace test would always require it, making manual execution inconvenient, or would need additional logic with assumptions there. This way we avoid having another variable and a way to pass build parameters around. BTW, Lukas, could you please describe what kind of problems you expect in fedpkg + local build? Perhaps it's not a problem after all? Nick From cd0dfbe1706c6fffa9c1a08b9877c9f4409c4a52 Mon Sep 17 00:00:00 2001 From: Pavel Reichl prei...@redhat.com Date: Tue, 11 Aug 2015 04:54:50 -0400 Subject: [PATCH 1/3] Remove trailing whitespace --- README | 2 +- src/conf_macros.m4 | 4 ++-- src/external/glib.m4| 4 ++-- src/external/pkg.m4 | 6 +++--- src/man/sssd.conf.5.xml | 8 src/sss_client/ssh/sss_ssh_client.c | 6 +++--- src/tests/pyhbac-test.py| 1 - src/util/dlinklist.h| 8 src/util/signal.c | 8 9 files changed, 23 insertions(+), 24 deletions(-) diff --git a/README b/README index ed08970..189f66f 100644 --- a/README +++ b/README @@ -9,7 +9,7 @@ an NSS and PAM interface toward the system and a pluggable backend system to connect to multiple different account sources. - More information about SSSD can be found on its project page - + More information about SSSD can be found on its project page - http://fedorahosted.org/sssd/ Building and installation diff --git a/src/conf_macros.m4 b/src/conf_macros.m4 index c6ce000..cd731e6 100644 --- a/src/conf_macros.m4 +++ b/src/conf_macros.m4 @@ -596,11 +596,11 @@ AC_DEFUN([WITH_UNICODE_LIB], if test x$with_unicode_lib != x; then unicode_lib=$with_unicode_lib fi - + if test x$unicode_lib != xlibunistring -a x$unicode_lib != xglib2; then AC_MSG_ERROR([Unsupported unicode library]) fi - + AM_CONDITIONAL([WITH_LIBUNISTRING], test x$unicode_lib = xlibunistring) AM_CONDITIONAL([WITH_GLIB], test x$unicode_lib = xglib2) ]) diff --git a/src/external/glib.m4 b/src/external/glib.m4 index 8cec763..3db2513 100644 --- a/src/external/glib.m4 +++ b/src/external/glib.m4 @@ -3,9 +3,9 @@ PKG_CHECK_MODULES([GLIB2],[glib-2.0]) if test x$has_glib2 != xno; then SAFE_LIBS=$LIBS LIBS=$GLIB2_LIBS - + AC_CHECK_FUNC([g_utf8_validate], AC_DEFINE([HAVE_G_UTF8_VALIDATE], [1], [Define if g_utf8_validate exists])) LIBS=$SAFE_LIBS -fi \ No newline at end of file +fi diff --git a/src/external/pkg.m4 b/src/external/pkg.m4 index a8b3d06..568127f 100644 --- a/src/external/pkg.m4 +++ b/src/external/pkg.m4 @@ -1,5 +1,5 @@ # pkg.m4 - Macros to locate and utilise pkg-config.-*- Autoconf -*- -# +# # Copyright © 2004 Scott James Remnant sc...@netsplit.com. # # This program is free software; you can redistribute it and/or modify @@ -38,7 +38,7 @@ if test -n $PKG_CONFIG; then AC_MSG_RESULT([no]) PKG_CONFIG= fi - + fi[]dnl ])# PKG_PROG_PKG_CONFIG @@ -119,7 +119,7 @@ if test $pkg_failed = yes; then _PKG_SHORT_ERRORS_SUPPORTED if test $_pkg_short_errors_supported = yes; then $1[]_PKG_ERRORS=`$PKG_CONFIG --short-errors --errors-to-stdout --print-errors $2` -else +else $1[]_PKG_ERRORS=`$PKG_CONFIG --errors-to-stdout --print-errors $2` fi # Put the nasty error message in config.log where it belongs diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 3f57862..b858347 100644 ---
Re: [SSSD] [PATCH] sss_override: support import and export
On Mon, Aug 17, 2015 at 10:32:00AM +0200, Jakub Hrozek wrote:
On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as transaction so
no changes are made if some error occurs, but I had some troubles with it so
I gave up eventually.
Thanks a lot for the patches. I didn't do any review yet, just scrolled
through them.
My first reaction was that the second patch should be split up and at
least the colondb should be unit tested.
btw did you get inspired by some existing code, like libuser or
shadow-utils with the colondb?
The patches are quite big, so maybe the review will be delivered in
chunks.
1) There are some Coverity errors:
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c:279:12: warning: 'fmode' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# fp = fopen(filename, fmode);
#^
# 277| if (filename != NULL) {
# 278| errno = 0;
# 279|- fp = fopen(filename, fmode);
# 280| if (fp == NULL) {
# 281| DEBUG(SSSDBG_CRIT_FAILURE, Unable to open file %s\n,
filename);
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.1/src/tools/common/sss_colondb.c:279: alloc_fn: Storage is returned
from allocation function fopen.
sssd-1.13.1/src/tools/common/sss_colondb.c:279: var_assign: Assigning: fp =
storage returned from fopen(filename, fmode).
sssd-1.13.1/src/tools/common/sss_colondb.c:289: leaked_storage: Variable fp
going out of scope leaks the storage it points to.
# 287| if (db == NULL) {
# 288| DEBUG(SSSDBG_CRIT_FAILURE, talloc_zero() failed\n);
# 289|- return NULL;
# 290| }
# 291|
Error: COMPILER_WARNING:
sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function
'sss_colondb_open'
sssd-1.13.1/src/tools/common/sss_colondb.c:292:14: warning: 'fp' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# db-file = fp;
# ^
# 290| }
# 291|
# 292|- db-file = fp;
# 293| db-mode = mode;
# 294|
Error: UNINIT (CWE-457):
sssd-1.13.1/src/tools/sss_override.c:997: var_decl: Declaring variable
tmp_ctx without initializer.
sssd-1.13.1/src/tools/sss_override.c:1076: uninit_use_in_call: Using
uninitialized value tmp_ctx when calling _talloc_free.
# 1074|
# 1075| done:
# 1076|- talloc_free(tmp_ctx);
# 1077| return exit;
# 1078| }
Error: UNINIT (CWE-457):
sssd-1.13.1/src/tools/sss_override.c:1207: var_decl: Declaring variable
tmp_ctx without initializer.
sssd-1.13.1/src/tools/sss_override.c:1282: uninit_use_in_call: Using
uninitialized value tmp_ctx when calling _talloc_free.
# 1280|
# 1281| done:
# 1282|- talloc_free(tmp_ctx);
# 1283| return exit;
# 1284| }
2) There is no documentation. It would be nice to have some examples in
the man page also.
3) I should probably test this before asking, but do the import and
export commands support blank (skipped) fields?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On Mon, Aug 17, 2015 at 10:52:41AM +0200, Lukas Slebodnik wrote: On (17/08/15 10:32), Jakub Hrozek wrote: On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2737 Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually. Thanks a lot for the patches. I didn't do any review yet, just scrolled through them. My first reaction was that the second patch should be split up and at least the colondb should be unit tested. btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb? Is there a design page for this? Maybe I missed something but I haven't noticed any discussion about export format for local views. I'm not very happy with custom format. Yes, it's already late because patches are ready and deadline is comming. btw you're right there was no design page. But basically, we should support this: https://support.software.dell.com/authentication-services/kb/91145 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [WIP] [TEST]: Observation patch
On (14/08/15 15:17), Petr Cech wrote:
Hi,
I wrote patch.
Regards
Petr
From c871c97862997df4e724647f1a0ce7297f2f059b Mon Sep 17 00:00:00 2001
From: Petr Cech pc...@redhat.com
Date: Fri, 14 Aug 2015 13:17:22 +0200
Subject: [PATCH] TEST: Fix for responder_cache_req-tests
Tests, that do not pass, have a problem with time. Time for writing
records into database varied from time of creating a request, that is
used for filtering records internally.
The patch modifies the time of creation record (adds one second to
now()), so it should not be different times there.
Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
src/tests/cmocka/test_responder_cache_req.c | 18 --
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
index
032fe429ac88b8cc9113976329ea04837f287276..4f77fe767e016496652a97c7a73fc9e29ba7faf0
100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -1721,9 +1721,10 @@ void test_users_by_filter_valid(void **state)
test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
test_ctx-create_user = true;
+/* set (time+1) to avoid failure request time filter */
ret = sysdb_store_user(test_ctx-tctx-dom, TEST_USER_NAME2, pwd, 1001,
1001,
NULL, NULL, NULL, cn=TEST_USER_NAME2,dc=test,
NULL,
- NULL, 1000, time(NULL));
+ NULL, 1000, time(NULL)+1);
assert_int_equal(ret, EOK);
Although, this patch fix intermitent failures
there are few problems.
The protopype of function sysdb_store_user is:
/* this function does not check that all user members are actually present */
/* if one of the basic attributes is empty () as opposed to NULL,
* this will just remove it */
int sysdb_store_user(struct sss_domain_info *domain,
const char *name,
const char *pwd,
uid_t uid, gid_t gid,
const char *gecos,
const char *homedir,
const char *shell,
const char *orig_dn,
struct sysdb_attrs *attrs,
char **remove_attrs,
uint64_t cache_timeout,
time_t now);
and if now is 0 then we will get the current time.
1912 /* get transaction timestamp */
1913 if (!now) {
1914 now = time(NULL);
1915 }
I do not understand why we shoudl set current time (now)
to future time(NULL)+1. I didn't check it properly, but
if now is used as transaction timestamp (according to comment)
it should not be from futire.
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] p11child: set restrictive umask and clear environment
On (13/08/15 17:46), Jakub Hrozek wrote:
Hi,
the attached patch hardens the p11_child process.
From 4023fb832cc5c5122c235b713c0ef401c5d21dd0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Wed, 5 Aug 2015 17:25:20 +0200
Subject: [PATCH] p11child: set restrictive umask and clear environment
https://fedorahosted.org/sssd/ticket/2754
Before doing any calls, set a very restrictive umask and clear
environment variables to harden p11child execution.
---
src/p11_child/p11_child_nss.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index
6948c142aa7843cda5ff6d18f5853b10c387c224..44ba6678893408dbfc0c6c7cfd5edcdaa789f518
100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -481,6 +481,9 @@ int main(int argc, const char *argv[])
/* Set debug level to invalid value so we can decide if -d 0 was used. */
debug_level = SSSDBG_INVALID;
+clearenv();
+umask(077);
+
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
--
2.4.3
LGTM
Do you think it would be good idea to the same for proxy_child.
So ticket #2689 would be just partialy fixed but we could
backport it in a single patch to downstream.
Should we also add 'chdir(/)' to p11child?
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH v2] Remove trailing whitespace
On (13/08/15 15:48), Nikolai Kondrashov wrote: On 08/13/2015 02:57 PM, Lukas Slebodnik wrote: On (13/08/15 14:47), Nikolai Kondrashov wrote: On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote: Hi Pavel, On 08/11/2015 03:44 PM, Pavel Reichl wrote: On 08/11/2015 02:11 PM, Lukas Slebodnik wrote: On (11/08/15 15:04), Nikolai Kondrashov wrote: Hi Pavel, It's a good idea to have such a test, thank you! Please see my comments regarding the script (as you requested) below. BTW, shouldn't we take care to not use git, or to not run this test in case we're running outside git tree, e.g. from unpacked tarball? +1 LS Nick, thank you for the comments. Would you take over this patch? I understand that you already have a better solution and I think there's no need to update mine in that case. Would you send a patch with proposed solution? There's no time pressure on this effort so you would not have to hurry. Sure, no problem. I'll send an update this week. Alright, here are the updated patches. Changes from the last version by Pavel include: * Reworked and renamed the whitespace test, fixing a number of issues. * Made the whitespace test run only if the source is in a Git worktree (checkout). * Added another patch making Git worktree (checkout) detection work in a complete out-of-tree dir and at the same time don't trigger erroneously from a subdir of (possibly unrelated) worktree. This is also needed to have the whitespace test not run during make distcheck, but run in CI. CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html Nick From c22d9d9103f443c8fd134cd57f0eb7b6d65fd7b9 Mon Sep 17 00:00:00 2001 From: Nikolai Kondrashov nikolai.kondras...@redhat.com Date: Wed, 12 Aug 2015 13:27:07 +0300 Subject: [PATCH 2/3] BUILD: Check for Git worktree at srcdir exactly Check for Git worktree (checkout) presence at $srcdir exactly, instead of any directory at or above $builddir. Use that directory and the associated repo for all Git invocations during build and tests. This allows Git invocations to work completely out-of-tree (not just in a sub-directory), when configured from a Git worktree. This also prevents erroneous detection of a Git worktree with an $srcdir located deeper, such as a distcheck build, or a tarball build under an unrelated worktree. --- Makefile.am | 5 + configure.ac | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index 8b64317..8016591 100644 --- a/Makefile.am +++ b/Makefile.am @@ -84,6 +84,11 @@ INSTALL = @INSTALL@ SSSD_USER = @SSSD_USER@ +if GIT_CHECKOUT +export GIT_DIR=$(abs_top_srcdir)/.git +export GIT_WORK_TREE=$(abs_top_srcdir) +endif It's not good idea to export such in general code. you might greak something. I didn't try but you might break something in fedpkg + local build. Better solution would to to export make variable abs_top_srcdir as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT and print warnig/error from test if this variable is not defined. Yeah, it's not nice. I doubt that it will break anything as it is very specific as to where the git repo is, but I wouldn't vouch for it. But on the other hand ABS_TOP_SRCDIR is more abstract and there is higher probability it will be used in other tests. We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets as well, which will make it uglier, but I'll do it. You can still export GIT_DIR and GIT_WORK_TREE inside script if ABS_TOP_SRCDIR is defined. Otherwise test can be skipped https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] SYSDB: Index the objectSIDString attribute
Hi,
the attached patch was confirmed to work, so the code review should be
easy. But because it adds an index to objectSID, which all AD objects
have, there are two catches:
1) How log the upgrade takes
2) How much the database grows
To test, I created an AD instance with 10.000 users and 10.000 groups
(adcli is great for this type of testing btw). Fetched all groups and
users with the old db, then ran the upgrade.
On a VM running on my local laptop (granted, it has an SSD drive), the
upgrade takes 10 seconds. The default systemd startup timeout
(DefaultTimeoutStartSec) seems to be 90 seconds, which sounds OK to me.
The size, though, has nearly doubled. This is backup before upgrade:
[root@adclient ~]# du -sh /root/cache_win.trust.test.ldb
47M /root/cache_win.trust.test.ldb
And this is the cache after I upgraded:
[root@adclient ~]# du -sh /var/lib/sss/db/cache_win.trust.test.ldb
97M /var/lib/sss/db/cache_win.trust.test.ldb
Unfortunately, I don't see us having another option than doing the
upgrade. For attributes that are indexed, an index miss means that ldb
is not going to search sequentially, but just return not found -- so
adding indexes for newly added entries is not possible.
I would like to document that for huge databases (tens of thousands of
cached entries), the timeout might need to be raised during the upgrade
and than for deployments whose cache resides in tmpfs, the cache size
might grow.
Is everyone OK with this?
From c50681fc7a1296c278466e834eaddfbf2bc54afb Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Thu, 25 Jun 2015 17:33:47 +0200
Subject: [PATCH] SYSDB: Index the objectSIDString attribute
---
src/db/sysdb.c | 7 +++
src/db/sysdb_private.h | 5 -
src/db/sysdb_upgrade.c | 50 ++
3 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index
9da655759c0c35d52854b668693195b3360c5f8b..07a83a8a8e30df1b8e461a8d04866f2dbc53baf8
100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1265,6 +1265,13 @@ int sysdb_domain_init_internal(TALLOC_CTX *mem_ctx,
}
}
+if (strcmp(version, SYSDB_VERSION_0_16) == 0) {
+ret = sysdb_upgrade_16(sysdb, version);
+if (ret != EOK) {
+goto done;
+}
+}
+
/* The version should now match SYSDB_VERSION.
* If not, it means we didn't match any of the
* known older versions. The DB might be
diff --git a/src/db/sysdb_private.h b/src/db/sysdb_private.h
index
2adb9ff9166441014a8b446ffc170225f2a9629d..c2c8d7a0585463806997c43bd020436e8517
100644
--- a/src/db/sysdb_private.h
+++ b/src/db/sysdb_private.h
@@ -23,6 +23,7 @@
#ifndef __INT_SYS_DB_H__
#define __INT_SYS_DB_H__
+#define SYSDB_VERSION_0_17 0.17
#define SYSDB_VERSION_0_16 0.16
#define SYSDB_VERSION_0_15 0.15
#define SYSDB_VERSION_0_14 0.14
@@ -40,7 +41,7 @@
#define SYSDB_VERSION_0_2 0.2
#define SYSDB_VERSION_0_1 0.1
-#define SYSDB_VERSION SYSDB_VERSION_0_16
+#define SYSDB_VERSION SYSDB_VERSION_0_17
#define SYSDB_BASE_LDIF \
dn: @ATTRIBUTES\n \
@@ -68,6 +69,7 @@
@IDXATTR: serviceProtocol\n \
@IDXATTR: sudoUser\n \
@IDXATTR: sshKnownHostsExpire\n \
+ @IDXATTR: objectSIDString\n \
@IDXONE: 1\n \
\n \
dn: @MODULES\n \
@@ -120,6 +122,7 @@ int sysdb_upgrade_12(struct sysdb_ctx *sysdb, const char
**ver);
int sysdb_upgrade_13(struct sysdb_ctx *sysdb, const char **ver);
int sysdb_upgrade_14(struct sysdb_ctx *sysdb, const char **ver);
int sysdb_upgrade_15(struct sysdb_ctx *sysdb, const char **ver);
+int sysdb_upgrade_16(struct sysdb_ctx *sysdb, const char **ver);
int add_string(struct ldb_message *msg, int flags,
const char *attr, const char *value);
diff --git a/src/db/sysdb_upgrade.c b/src/db/sysdb_upgrade.c
index
6cebc877b86df7a8035bf95cfa51aa4dae464372..113f24644e3e3de1d4c46d375492c2fe1e6b2f83
100644
--- a/src/db/sysdb_upgrade.c
+++ b/src/db/sysdb_upgrade.c
@@ -1584,6 +1584,56 @@ done:
return ret;
}
+int sysdb_upgrade_16(struct sysdb_ctx *sysdb, const char **ver)
+{
+struct ldb_message *msg;
+struct upgrade_ctx *ctx;
+errno_t ret;
+
+ret = commence_upgrade(sysdb, sysdb-ldb, SYSDB_VERSION_0_17, ctx);
+if (ret) {
+return ret;
+}
+
+msg = ldb_msg_new(ctx);
+if (msg == NULL) {
+ret = ENOMEM;
+goto done;
+}
+
+msg-dn = ldb_dn_new(msg, sysdb-ldb, @INDEXLIST);
+if (msg-dn == NULL) {
+ret = ENOMEM;
+goto done;
+}
+
+/* add index for objectSIDString */
+ret = ldb_msg_add_empty(msg, @IDXATTR, LDB_FLAG_MOD_ADD, NULL);
+if (ret != LDB_SUCCESS) {
+ret = ENOMEM;
+goto done;
+}
+
+ret = ldb_msg_add_string(msg, @IDXATTR, objectSIDString);
+if (ret != LDB_SUCCESS) {
+ret = ENOMEM;
+goto done;
+}
+
+ret =
Re: [SSSD] [WIP] [TEST]: Observation patch
On 08/17/2015 08:52 AM, Lukas Slebodnik wrote:
From c871c97862997df4e724647f1a0ce7297f2f059b Mon Sep 17 00:00:00 2001
From: Petr Cechpc...@redhat.com
Date: Fri, 14 Aug 2015 13:17:22 +0200
Subject: [PATCH] TEST: Fix for responder_cache_req-tests
Tests, that do not pass, have a problem with time. Time for writing
records into database varied from time of creating a request, that is
used for filtering records internally.
The patch modifies the time of creation record (adds one second to
now()), so it should not be different times there.
Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
src/tests/cmocka/test_responder_cache_req.c | 18 --
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
index
032fe429ac88b8cc9113976329ea04837f287276..4f77fe767e016496652a97c7a73fc9e29ba7faf0
100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -1721,9 +1721,10 @@ void test_users_by_filter_valid(void **state)
test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
test_ctx-create_user = true;
+/* set (time+1) to avoid failure request time filter */
ret = sysdb_store_user(test_ctx-tctx-dom, TEST_USER_NAME2, pwd, 1001,
1001,
NULL, NULL, NULL, cn=TEST_USER_NAME2,dc=test,
NULL,
- NULL, 1000, time(NULL));
+ NULL, 1000, time(NULL)+1);
assert_int_equal(ret, EOK);
Although, this patch fix intermitent failures
there are few problems.
The protopype of function sysdb_store_user is:
/* this function does not check that all user members are actually present */
/* if one of the basic attributes is empty () as opposed to NULL,
* this will just remove it */
int sysdb_store_user(struct sss_domain_info *domain,
const char *name,
const char *pwd,
uid_t uid, gid_t gid,
const char *gecos,
const char *homedir,
const char *shell,
const char *orig_dn,
struct sysdb_attrs *attrs,
char **remove_attrs,
uint64_t cache_timeout,
time_t now);
and if now is 0 then we will get the current time.
1912 /* get transaction timestamp */
1913 if (!now) {
1914 now = time(NULL);
1915 }
I do not understand why we shoudl set current time (now)
to future time(NULL)+1. I didn't check it properly, but
if now is used as transaction timestamp (according to comment)
it should not be from futire.
LS
The initial value was time(now) and it could be simply 0, I agree with that.
(I've tried time(now) - 0, but unfortunately it was not enough. The
problem is elsewhere.)
The problem is reading the data. There is a filter from a certain time,
internally used time is set to time of creating request for reading
data. But this request is creating after inserting data. Therefore, you
can insert a timestamp data and timestamp of request creation vary,
especially if the machine is busy.
Completely correct solution (meaning clear) would be create a request to
read data in the beginning of the test, then insert data and then try to
read it. I tried this, I had complication with mock then.
Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as transaction so
no changes are made if some error occurs, but I had some troubles with it so
I gave up eventually.
Thanks a lot for the patches. I didn't do any review yet, just scrolled
through them.
My first reaction was that the second patch should be split up and at
least the colondb should be unit tested.
btw did you get inspired by some existing code, like libuser or
shadow-utils with the colondb?
Of course, it would be quite difficult to make it safe across domains thus
my intention was only to ensure that either all changes are done in a domain
or none. But still leaving the possibility that changes are commited in one
domain but cancelled in another.
I tried to start sysdb transaction on all used domains and then
commit/cancel it, writing some neat mechanism for it. However, I
occasionally run into a problem when data provider hangs when trying to safe
a user. It looked like some sort of race condition.
Unfortunately I managed to delete the code so I can't show it to you, I
think it would be a nice feature so if anyone familiar with ldb want to step
in, he's welcome.
From a8063d92d84c13b55f880f09993b5b9769dec8a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrez...@redhat.com
Date: Sat, 15 Aug 2015 13:53:25 +0200
Subject: [PATCH 1/2] sss_override: print input name if unable to parse it
---
src/tools/sss_override.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
index
5e901e2e31de64dacb171337defc03d428f8ed57..e84a7b922dfcf179f8010dc4cced0eafd89a2c76
100644
--- a/src/tools/sss_override.c
+++ b/src/tools/sss_override.c
@@ -74,7 +74,7 @@ static int parse_cmdline(struct sss_cmdline *cmdline,
ret = sss_tool_parse_name(tool_ctx, tool_ctx, input_name,
orig_name, domain);
if (ret != EOK) {
-fprintf(stderr, _(Unable to parse name.\n));
+fprintf(stderr, _(Unable to parse name %s.\n), input_name);
return ret;
}
--
1.9.3
From 24655f677acf9f64201a611ababdcfeff4317037 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrez...@redhat.com
Date: Sat, 15 Aug 2015 16:42:27 +0200
Subject: [PATCH 2/2] sss_override: add import and export
Resolves:
https://fedorahosted.org/sssd/ticket/2737
---
Makefile.am| 2 +
src/tools/common/sss_colondb.c | 298 ++
src/tools/common/sss_colondb.h | 71
src/tools/sss_override.c | 866
+++--
4 files changed, 1128 insertions(+), 109 deletions(-)
create mode 100644 src/tools/common/sss_colondb.c
create mode 100644 src/tools/common/sss_colondb.h
diff --git a/Makefile.am b/Makefile.am
index
ed107fd5dc76b768176a3d7236b0bf1c75f212bf..f8c7c29df08c2ba9e74508c0f84ac57f6e6bac26
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -651,6 +651,7 @@ dist_noinst_HEADERS = \
src/lib/sifp/sss_sifp_private.h \
src/tests/cmocka/test_utils.h \
src/tools/common/sss_tools.h \
+src/tools/common/sss_colondb.h \
$(NULL)
@@ -1331,6 +1332,7 @@ sss_signal_LDADD = \
sss_override_SOURCES = \
src/tools/sss_override.c \
+src/tools/common/sss_colondb.c \
$(SSSD_TOOLS_OBJ) \
$(NULL)
sss_override_LDADD = \
diff --git a/src/tools/common/sss_colondb.c b/src/tools/common/sss_colondb.c
new file mode 100644
index
..be9396832e93fa3b646255dae214a34fe4d6c29c
--- /dev/null
+++ b/src/tools/common/sss_colondb.c
@@ -0,0 +1,298 @@
+/*
+Authors:
+Pavel Březina pbrez...@redhat.com
+
+Copyright (C) 2015 Red Hat
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU 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 General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+
+#include stdlib.h
+
+#include util/util.h
+#include util/strtonum.h
+#include tools/common/sss_colondb.h
+
+#define IS_STD_FILE(db) ((db)-file == stdin || (db)-file == stdout)
+#define IS_LINE_END(str) ((str) == NULL || *(str) == '\0' || *(str) == '\n')
+
+static char *read_field_as_string(char *line,
+ const char **_value)
+{
+char *rest;
+char *value;
+
+if
Re: [SSSD] [PATCH] p11child: set restrictive umask and clear environment
On Mon, Aug 17, 2015 at 09:01:24AM +0200, Lukas Slebodnik wrote:
On (13/08/15 17:46), Jakub Hrozek wrote:
Hi,
the attached patch hardens the p11_child process.
From 4023fb832cc5c5122c235b713c0ef401c5d21dd0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek jhro...@redhat.com
Date: Wed, 5 Aug 2015 17:25:20 +0200
Subject: [PATCH] p11child: set restrictive umask and clear environment
https://fedorahosted.org/sssd/ticket/2754
Before doing any calls, set a very restrictive umask and clear
environment variables to harden p11child execution.
---
src/p11_child/p11_child_nss.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index
6948c142aa7843cda5ff6d18f5853b10c387c224..44ba6678893408dbfc0c6c7cfd5edcdaa789f518
100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -481,6 +481,9 @@ int main(int argc, const char *argv[])
/* Set debug level to invalid value so we can decide if -d 0 was used.
*/
debug_level = SSSDBG_INVALID;
+clearenv();
+umask(077);
+
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
--
2.4.3
LGTM
Thanks, btw this code is unit-tested.
Do you think it would be good idea to the same for proxy_child.
Yes, good idea.
But I would propose to do two patches, because downstream only cares
about p11_child hardening.
So ticket #2689 would be just partialy fixed but we could
backport it in a single patch to downstream.
Should we also add 'chdir(/)' to p11child?
We can, it wouldn't hurt, but I'm pretty sure all sssd processes call
chdir to root already (I remember this because it was one of my first
patches to sssd :-))
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: support import and export
On 08/17/2015 10:32 AM, Jakub Hrozek wrote: On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote: https://fedorahosted.org/sssd/ticket/2737 Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually. Thanks a lot for the patches. I didn't do any review yet, just scrolled through them. My first reaction was that the second patch should be split up and at least the colondb should be unit tested. Ok. Do you have any specific split on mind? btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb? Nope. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [WIP] [TEST]: Observation patch
On (17/08/15 10:17), Petr Cech wrote:
On 08/17/2015 08:52 AM, Lukas Slebodnik wrote:
From c871c97862997df4e724647f1a0ce7297f2f059b Mon Sep 17 00:00:00 2001
From: Petr Cechpc...@redhat.com
Date: Fri, 14 Aug 2015 13:17:22 +0200
Subject: [PATCH] TEST: Fix for responder_cache_req-tests
Tests, that do not pass, have a problem with time. Time for writing
records into database varied from time of creating a request, that is
used for filtering records internally.
The patch modifies the time of creation record (adds one second to
now()), so it should not be different times there.
Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
src/tests/cmocka/test_responder_cache_req.c | 18 --
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
index
032fe429ac88b8cc9113976329ea04837f287276..4f77fe767e016496652a97c7a73fc9e29ba7faf0
100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -1721,9 +1721,10 @@ void test_users_by_filter_valid(void **state)
test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
test_ctx-create_user = true;
+/* set (time+1) to avoid failure request time filter */
ret = sysdb_store_user(test_ctx-tctx-dom, TEST_USER_NAME2, pwd,
1001, 1001,
NULL, NULL, NULL,
cn=TEST_USER_NAME2,dc=test, NULL,
- NULL, 1000, time(NULL));
+ NULL, 1000, time(NULL)+1);
assert_int_equal(ret, EOK);
Although, this patch fix intermitent failures
there are few problems.
The protopype of function sysdb_store_user is:
/* this function does not check that all user members are actually present */
/* if one of the basic attributes is empty () as opposed to NULL,
* this will just remove it */
int sysdb_store_user(struct sss_domain_info *domain,
const char *name,
const char *pwd,
uid_t uid, gid_t gid,
const char *gecos,
const char *homedir,
const char *shell,
const char *orig_dn,
struct sysdb_attrs *attrs,
char **remove_attrs,
uint64_t cache_timeout,
time_t now);
and if now is 0 then we will get the current time.
1912 /* get transaction timestamp */
1913 if (!now) {
1914 now = time(NULL);
1915 }
I do not understand why we shoudl set current time (now)
to future time(NULL)+1. I didn't check it properly, but
if now is used as transaction timestamp (according to comment)
it should not be from futire.
LS
The initial value was time(now) and it could be simply 0, I agree with that.
(I've tried time(now) - 0, but unfortunately it was not enough. The problem
is elsewhere.)
The problem is reading the data. There is a filter from a certain time,
internally used time is set to time of creating request for reading data. But
this request is creating after inserting data. Therefore, you can insert a
timestamp data and timestamp of request creation vary, especially if the
machine is busy.
But busy machine can be also in production.
Can this situation occur in real deployment?
The unit test should be considered as additional documentation.
Your solution would mean we can/should use time(NULL)+1 even in production.
Which does not seems to be a correct solution.
Completely correct solution (meaning clear) would be create a request to read
data in the beginning of the test, then insert data and then try to read it.
I tried this, I had complication with mock then.
If you have troubles with mock/cmocka
then please at least try to bisect which patch broke it
and ask an author for help.
BTW I cannot see it in coding guidelines but it's better to add spaces
around operators. time(NULL)+1 - time(NULL) + 1
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
