On 09/22/2015 03:03 PM, Pavel Reichl wrote:
Hello,
please see attached patch.
Thanks!
Hi,
- state->filter = talloc_asprintf(state, "(|(%s=*)(%s=*))",
- state->filter = talloc_asprintf(state,
"(|(&(%s=*)(objectclass=%s))(&(%s=*)(objectclass=%s)))", opts->user_map[SDAP_AT_USER_UID].name,
opts->group_map[SDAP_AT_GROUP_GID].name);
opts->user_map[SDAP_OC_USER].name,
opts->group_map[SDAP_AT_GROUP_GID].name,
opts->group_map[SDAP_OC_GROUP].name);
I don't understand, how it got relaxed? The original filter already tests only for presence of uid or gid. Or am I missing something?
Yes, you are missing the rest of the patch :-)
errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) { - DEBUG(SSSDBG_OP_FAILURE, + DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n", vals[0]->bv_val); - goto done; }
We do not require attribute to be a positive number, we just warn about that. But not to make it too relaxed we added check on objectclass.
----- Original Message ----- From: "Pavel Březina" pbrezina@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, September 23, 2015 9:26:53 PM Subject: Re: [SSSD] [PATCH] SDAP: Relax POSIX check
On 09/22/2015 03:03 PM, Pavel Reichl wrote:
Hello,
please see attached patch.
Thanks!
Hi,
- state->filter = talloc_asprintf(state, "(|(%s=*)(%s=*))",
- state->filter = talloc_asprintf(state,
"(|(&(%s=*)(objectclass=%s))(&(%s=*)(objectclass=%s)))", opts->user_map[SDAP_AT_USER_UID].name,
opts->group_map[SDAP_AT_GROUP_GID].name);
opts->user_map[SDAP_OC_USER].name,
opts->group_map[SDAP_AT_GROUP_GID].name,
opts->group_map[SDAP_OC_GROUP].name);
I don't understand, how it got relaxed? The original filter already tests only for presence of uid or gid. Or am I missing something?
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 09/23/2015 10:55 PM, Pavel Reichl wrote:
Yes, you are missing the rest of the patch :-)
errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) {
DEBUG(SSSDBG_OP_FAILURE,
DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n", vals[0]->bv_val);
goto done; }
We do not require attribute to be a positive number, we just warn about that. But not to make it too relaxed we added check on objectclass.
Thanks. Code wise ack, but could you rephrase the commit message?
----- Original Message ----- From: "Pavel Březina" pbrezina@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, September 23, 2015 9:26:53 PM Subject: Re: [SSSD] [PATCH] SDAP: Relax POSIX check
On 09/22/2015 03:03 PM, Pavel Reichl wrote:
Hello,
please see attached patch.
Thanks!
Hi,
- state->filter = talloc_asprintf(state, "(|(%s=*)(%s=*))",
- state->filter = talloc_asprintf(state,
"(|(&(%s=*)(objectclass=%s))(&(%s=*)(objectclass=%s)))", opts->user_map[SDAP_AT_USER_UID].name,
opts->group_map[SDAP_AT_GROUP_GID].name);
opts->user_map[SDAP_OC_USER].name,
opts->group_map[SDAP_AT_GROUP_GID].name,
opts->group_map[SDAP_OC_GROUP].name);
I don't understand, how it got relaxed? The original filter already tests only for presence of uid or gid. Or am I missing something?
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
On 09/24/2015 02:12 PM, Pavel Březina wrote:
On 09/23/2015 10:55 PM, Pavel Reichl wrote:
Yes, you are missing the rest of the patch :-)
errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) {
DEBUG(SSSDBG_OP_FAILURE,
DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n", vals[0]->bv_val);
goto done; }
We do not require attribute to be a positive number, we just warn about that. But not to make it too relaxed we added check on objectclass.
Thanks. Code wise ack, but could you rephrase the commit message?
OK, I hope it's better now.
On (24/09/15 17:11), Pavel Reichl wrote:
On 09/24/2015 02:12 PM, Pavel Březina wrote:
On 09/23/2015 10:55 PM, Pavel Reichl wrote:
Yes, you are missing the rest of the patch :-)
errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) {
DEBUG(SSSDBG_OP_FAILURE,
DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n", vals[0]->bv_val);
}goto done;
We do not require attribute to be a positive number, we just warn about that. But not to make it too relaxed we added check on objectclass.
Thanks. Code wise ack, but could you rephrase the commit message?
OK, I hope it's better now.
From 4ff85ab612da94fab6591ac7a178d42c314dc63e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 22 Sep 2015 04:41:18 -0400 Subject: [PATCH] SDAP: Relax POSIX check
Relax the check on UID or GID just to check if at least one of them is present but do not require them to be positive numbers.
Add requirement on objectclass attributes to be user or group to make check more reliable.
Resolves: https://fedorahosted.org/sssd/ticket/2800
src/providers/ldap/sdap_async.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index 97c9ea5df61a6516ca74bb73edc9a116b1266c71..b81431f79f21755469bb9ff123d695a2a166e353 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -2586,9 +2586,12 @@ sdap_posix_check_send(TALLOC_CTX *memctx, struct tevent_context *ev, state->attrs[2] = opts->group_map[SDAP_AT_GROUP_GID].name; state->attrs[3] = NULL;
- state->filter = talloc_asprintf(state, "(|(%s=*)(%s=*))",
- state->filter = talloc_asprintf(state,
"(|(&(%s=*)(objectclass=%s))(&(%s=*)(objectclass=%s)))", opts->user_map[SDAP_AT_USER_UID].name,
opts->group_map[SDAP_AT_GROUP_GID].name);
opts->user_map[SDAP_OC_USER].name,
opts->group_map[SDAP_AT_GROUP_GID].name,
if (state->filter == NULL) { ret = ENOMEM; goto fail;opts->group_map[SDAP_OC_GROUP].name);
@@ -2671,9 +2674,8 @@ static errno_t sdap_posix_check_parse(struct sdap_handle *sh, errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) {
DEBUG(SSSDBG_OP_FAILURE,
DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n", vals[0]->bv_val);
goto done;
}
state->has_posix = true;
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
LS
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
LS
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
On 10/01/2015 08:44 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Are we going to to prepare such test now? If so, are you going to work on it or should I?
On Fri, Oct 02, 2015 at 09:45:32AM +0200, Pavel Reichl wrote:
On 10/01/2015 08:44 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Are we going to to prepare such test now? If so, are you going to work on it or should I?
I'm trying to add it now; it's time I learned how to add tests anyway. I wonder if we won't trip over adding the negative UID...
On Thu, Oct 01, 2015 at 08:44:11PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Turns out that the test hits the POSIX check, but doesn't error out with: id_provider = ldap ldap_schema = ad
Because the code that triggers the next connection (and eventually marks sssd as offline) is only in the AD provider.
The WIP test can be found here: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_tes...
So we really need Samba4 AD to test this..sorry, I'll push the patch w/o tests.
On Fri, Oct 02, 2015 at 12:29:01PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:44:11PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote:
There is a common agreement (at least from management side) that we want to improve code coverage This patch is great opportunity to add regression test. You can choose either integration test or unit test; you should know what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Turns out that the test hits the POSIX check, but doesn't error out with: id_provider = ldap ldap_schema = ad
Because the code that triggers the next connection (and eventually marks sssd as offline) is only in the AD provider.
The WIP test can be found here: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_tes...
So we really need Samba4 AD to test this..sorry, I'll push the patch w/o tests.
* master: 6735c0451d4e80d7cd4b480a8c1f7dafb2b536ea
On 10/02/2015 12:43 PM, Jakub Hrozek wrote:
On Fri, Oct 02, 2015 at 12:29:01PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:44:11PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote:
On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote: > There is a common agreement (at least from management side) that we want > to improve code coverage This patch is great opportunity to add regression > test. You can choose either integration test or unit test; you should know > what suits better to this case.
I tried to create a test (see my tests branch at fp.o) but it's really not easy, the sdap_async.c module has quite some dependencies and the generic search request is real pita to mock. And we need to apply this patch to downstream.
So while I agree with you we need tests, I think this one needs to be done together with the LDAP refactoring, sorry.
What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Turns out that the test hits the POSIX check, but doesn't error out with: id_provider = ldap ldap_schema = ad
Because the code that triggers the next connection (and eventually marks sssd as offline) is only in the AD provider.
The WIP test can be found here: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_tes...
So we really need Samba4 AD to test this..sorry, I'll push the patch w/o tests.
- master: 6735c0451d4e80d7cd4b480a8c1f7dafb2b536ea
Reviewed-by tag is missing.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Oct 02, 2015 at 12:44:31PM +0200, Pavel Reichl wrote:
On 10/02/2015 12:43 PM, Jakub Hrozek wrote:
On Fri, Oct 02, 2015 at 12:29:01PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:44:11PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:39:15PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 08:31:50PM +0200, Lukas Slebodnik wrote:
On (01/10/15 20:13), Jakub Hrozek wrote: >On Fri, Sep 25, 2015 at 07:49:23AM +0200, Lukas Slebodnik wrote: >>There is a common agreement (at least from management side) that we want >>to improve code coverage This patch is great opportunity to add regression >>test. You can choose either integration test or unit test; you should know >>what suits better to this case. > >I tried to create a test (see my tests branch at fp.o) but it's really >not easy, the sdap_async.c module has quite some dependencies and the >generic search request is real pita to mock. And we need to apply this >patch to downstream. > >So while I agree with you we need tests, I think this one needs to be >done together with the LDAP refactoring, sorry. What about integration test?
Calling sdap_posix_check_send requires LDAP backend with ID mapping and AD schema..
Actually ID mapping should be disabled of course...
So what we could do is start a test while the server only contains entry with a wrong POSIX data to make sure the request hits it and then add a correct entry and request it..
Turns out that the test hits the POSIX check, but doesn't error out with: id_provider = ldap ldap_schema = ad
Because the code that triggers the next connection (and eventually marks sssd as offline) is only in the AD provider.
The WIP test can be found here: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_tes...
So we really need Samba4 AD to test this..sorry, I'll push the patch w/o tests.
- master: 6735c0451d4e80d7cd4b480a8c1f7dafb2b536ea
Reviewed-by tag is missing.
Yes, sorry about that. Luckily it was Pavel Brezina's review and IIRC I previously gave him one :-)
On Fri, Oct 02, 2015 at 12:46:02PM +0200, Jakub Hrozek wrote:
On Fri, Oct 02, 2015 at 12:43:01PM +0200, Jakub Hrozek wrote:
- master: 6735c0451d4e80d7cd4b480a8c1f7dafb2b536ea
- sssd-1-13: cc04876ec64b338f61ca275386f70baf91ce700f
* sssd-1-12: a73c89f8672a12878a8668bc321e6742bc45b924
On 09/24/2015 05:11 PM, Pavel Reichl wrote:
On 09/24/2015 02:12 PM, Pavel Březina wrote:
On 09/23/2015 10:55 PM, Pavel Reichl wrote:
Yes, you are missing the rest of the patch :-)
errno = 0; strtouint32(vals[0]->bv_val, &endptr, 10); if (errno || *endptr || (vals[0]->bv_val == endptr)) {
DEBUG(SSSDBG_OP_FAILURE,
DEBUG(SSSDBG_MINOR_FAILURE, "POSIX attribute is not a number: %s\n",
vals[0]->bv_val);
goto done; }
We do not require attribute to be a positive number, we just warn about that. But not to make it too relaxed we added check on objectclass.
Thanks. Code wise ack, but could you rephrase the commit message?
OK, I hope it's better now.
Yes, it is. Thank you!
Ack.
On Wed, Sep 23, 2015 at 09:26:53PM +0200, Pavel Březina wrote:
On 09/22/2015 03:03 PM, Pavel Reichl wrote:
Hello,
please see attached patch.
Thanks!
Hi,
- state->filter = talloc_asprintf(state, "(|(%s=*)(%s=*))",
- state->filter = talloc_asprintf(state,
"(|(&(%s=*)(objectclass=%s))(&(%s=*)(objectclass=%s)))", opts->user_map[SDAP_AT_USER_UID].name,
opts->group_map[SDAP_AT_GROUP_GID].name);
opts->user_map[SDAP_OC_USER].name,
opts->group_map[SDAP_AT_GROUP_GID].name,
opts->group_map[SDAP_OC_GROUP].name);
I don't understand, how it got relaxed? The original filter already tests only for presence of uid or gid. Or am I missing something?
I think the main point is the removal of "goto done" if str-to-num failed.
That should have been spelled out in the commit message :-)
sssd-devel@lists.fedorahosted.org