On Mon, Jul 27, 2015 at 05:27:29PM +0200, Pavel Březina wrote:
On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
>On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
>>On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
>>>On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
>>>>https://fedorahosted.org/sssd/ticket/2584
>>>btw (unrelated to these patches) it seems strange to require the user of
>>>the sysdb database to connect to the db and then separately initialize
>>>subdomains...I would vote for merging that code..
>>
>>+1
>
>I'll file a ticket for michal.
>
>>
>>>>+
>>>>+#include <talloc.h>
>>>>+#include <popt.h>
>>>>+
>>>>+#include "confdb/confdb.h"
>>>>+
>>>>+struct sss_tool_ctx {
>>>>+ struct confdb_ctx *confdb;
>>>>+ char *confdb_path;
>>>>+
>>>>+ char *default_domain;
>>>>+ struct sss_domain_info *domains;
>>>
>>>I wonder if the tool_ctx must be open for consumers? Currently only
>>>domains is used, maybe we could get away with a getter and an opaque
>>>structure? We can always make the structure public in future, but if
>>>it's public from the start, then it's easier to add cruft..
>>
>>I think it is not worth the effort with such simple structure. I am
>>all for opaque structures but here is just no gain in code safety. It would
>>only make code lines bigger.
>
>Fine.
>
>>
>>>>+enum sss_tool_domain {
>>>>+ SSS_TOOL_DOMAIN_REQUIRED,
>>>>+ SSS_TOOL_DOMAIN_OPTIONAL
>>>>+};
>>>>+
>>>>+int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
>>>>+ struct sss_tool_ctx *tool_ctx,
>>>>+ const char *input,
>>>>+ enum sss_tool_domain require_domain,
>>>
>>>Do we need require_domain now that you added the getpwnam?
>>
>>No, removed.
>
>Thanks
>
>[...]
>
>>>
>>>Hmm looks like you should have a check for res validity here, for cases
>>>no domain match..
>>
>>Added a dom validity.
>
>Thanks.
>
>>
>>>btw it might be nice to split this large function into smaller ones.
>>>Maybe the do-while check could be a separate function returning res.
>>
>>There is no way to split it that would be actually beneficial. There are
>>three blocks:
>>a) beginning - getpwnam
>>b) middle - iteration
>>c) end - linearized dn
>>
>>If you take away the iteration you can directly return dn from the new
>>function since you don't need res at all so that would also took away the
>>end. And the beginning belongs to the function were iteration is IMHO.
>
>OK
>
>
>> From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Fri, 24 Jul 2015 09:55:28 +0200
>>Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
>>
>>Objects doesn't have to have overrideDN specified when using LOCAL view.
>>Since the view is not stored on the server we do not want to contact
>>LDAP therefore we special case LOCAL view saying that it is OK that
>>this attribute is missing.
>>
>>Preparation for:
>>https://fedorahosted.org/sssd/ticket/2584
>>---
>> src/db/sysdb.h | 3 +-
>> src/db/sysdb_views.c | 7 +++++
>> src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 71 insertions(+), 1 deletion(-)
>>
>>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>>index
0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644
>>--- a/src/db/sysdb.h
>>+++ b/src/db/sysdb.h
>>@@ -157,9 +157,10 @@
>> #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
>> #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
>>
>>+#define SYSDB_DEFAULT_VIEW_NAME "default"
>>+#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
overrides */
>> #define SYSDB_VIEW_CLASS "view"
>> #define SYSDB_VIEW_NAME "viewName"
>>-#define SYSDB_DEFAULT_VIEW_NAME "default"
>> #define SYSDB_OVERRIDE_CLASS "overrride"
>> #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
>> #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
>>diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
>>index
aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644
>>--- a/src/db/sysdb_views.c
>>+++ b/src/db/sysdb_views.c
>>@@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
sss_domain_info *domain,
>> override_dn_str = ldb_msg_find_attr_as_string(obj,
>> SYSDB_OVERRIDE_DN,
NULL);
>> if (override_dn_str == NULL) {
>>+ if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {
>>+ /* LOCAL view doesn't have to have overrideDN specified. */
>>+ ret = EOK;
>>+ goto done;
>>+ }
>>+
>> DEBUG(SSSDBG_CRIT_FAILURE,
>> "Missing override DN for objext [%s].\n",
>> ldb_dn_get_linearized(obj->dn));
>>+
>> ret = ENOENT;
>> goto done;
>> }
>>diff --git a/src/tests/cmocka/test_sysdb_views.c
b/src/tests/cmocka/test_sysdb_views.c
>>index
1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644
>>--- a/src/tests/cmocka/test_sysdb_views.c
>>+++ b/src/tests/cmocka/test_sysdb_views.c
>>@@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state)
>> assert_int_equal(ldb_val_string_cmp(&el->values[1],
"OVERRIDEKEY2"), 0);
>> }
>>
>>+void test_sysdb_add_overrides_to_object_local(void **state)
>>+{
>>+ int ret;
>>+ struct ldb_message *orig;
>>+ struct ldb_message_element *el;
>>+ char *tmp_str;
>>+ struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
>>+ struct
sysdb_test_ctx);
>>+
>>+ orig = ldb_msg_new(test_ctx);
>>+ assert_non_null(orig);
>>+
>>+ tmp_str = talloc_strdup(orig, "ORIGNAME");
>
>Please put assert_non_null here and to other assignments to tmp_str in
>these two tests, otherwise ACK
>
>>+ ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
>>+ assert_int_equal(ret, EOK);
>>+
>>+ tmp_str = talloc_strdup(orig, "ORIGGECOS");
>>+ ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
>>+ assert_int_equal(ret, EOK);
>>+
>>+ test_ctx->domain->has_views = true;
>>+ test_ctx->domain->view_name = "LOCAL";
>>+
>>+ ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
>>+ assert_int_equal(ret, EOK);
>>+}
>
>> From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Wed, 22 Jul 2015 10:02:02 +0200
>>Subject: [PATCH 2/3] TOOLS: add common command framework
>
>ACK
>
>
>> From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Fri, 24 Jul 2015 09:58:11 +0200
>>Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2584
>
>There was one more Coverity warning:
>Error: UNINIT (CWE-457): [#def1]
>sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
"ret" without initializer.
>sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value
"ret".
># 250|
># 251| done:
># 252|-> if (ret != EOK) {
># 253| talloc_free(attrs);
># 254| return NULL;
>
>Otherwise looks good.
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Thanks, new patches are attached. In addition to your comments:
- one new patch that adds null check in original unit test
- man page and tool change that says that zero ids are not supported
I am still waiting for Coverity and CI results.
From db9ba08b8c359eec318fa3dfc28e038618ea35f7 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 27 Jul 2015 17:24:45 +0200
Subject: [PATCH 1/4] VIEWS TEST: add null-check
ACK
From 9b67c5777f51cedd1b6669d0f103c232e80e9ad6 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 24 Jul 2015 09:55:28 +0200
Subject: [PATCH 2/4] SYSDB: prepare for LOCAL view
ACK
From 39b1bf17cb52e000c8ef74862fb88a4762176730 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 22 Jul 2015 10:02:02 +0200
Subject: [PATCH 3/4] TOOLS: add common command framework
ACK
From 99c97b41a6feeb0edd5be11d27b8e832d4694843 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 24 Jul 2015 09:58:11 +0200
Subject: [PATCH 4/4] TOOLS: add sss_override for local overrides
there is still an issue with user getpwnam finds but sysdb_getpwnam not:
(gdb) set args user-add root -n toor
(gdb) r
Starting program: /usr/sbin/sss_override user-add root -n toor
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.21-5.fc22.x86_64
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
/usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
Program received signal SIGSEGV, Segmentation fault.
override_user_add (cmdline=0x7fffffffe390, tool_ctx=0x6110c0, pvt=0x0)
at /sssd/src/tools/sss_override.c:567
567 fprintf(stderr, _("Unable to find user %s(a)%s.\n"),
Missing separate debuginfos, use: dnf debuginfo-install bzip2-libs-1.0.6-14.fc22.x86_64
cyrus-sasl-lib-2.1.26-22.fc22.x86_64 dbus-libs-1.8.18-1.fc22.x86_64
elfutils-libelf-0.161-6.fc22.x86_64 elfutils-libs-0.161-6.fc22.x86_64
glib2-2.44.1-1.fc22.x86_64 libattr-2.4.47-9.fc22.x86_64
libbasicobjects-0.1.1-24.fc22.x86_64 libcap-2.24-7.fc22.x86_64
libcollection-0.6.2-24.fc22.x86_64 libdhash-0.4.3-24.fc22.x86_64
libgcc-5.1.1-1.fc22.x86_64 libgcrypt-1.6.3-4.fc22.x86_64 libgpg-error-1.17-2.fc22.x86_64
libini_config-1.1.0-24.fc22.x86_64 libldb-1.1.20-1.fc22.x86_64
libpath_utils-0.2.1-24.fc22.x86_64 libref_array-0.1.4-24.fc22.x86_64
libselinux-2.3-10.fc22.x86_64 libtalloc-2.1.2-1.fc22.x86_64 libtdb-1.3.4-1.fc22.x86_64
libtevent-0.9.24-1.fc22.x86_64 nspr-4.10.8-1.fc22.x86_64 nss-3.19.2-1.0.fc22.x86_64
nss-softokn-freebl-3.19.2-1.0.fc22.x86_64 nss-util-3.19.2-1.0.fc22.x86_64
openldap-2.4.40-12.fc22.x86_64 openssl-libs-1.0.1k-11.fc22.x86_64 pcre-8.37-2.fc22.x86_64
popt-1.16-5.fc22.x86_64 systemd-libs-219-19.fc22.x86_64 xz-libs-5.2.0-2.fc22.x86_64
zlib-1.2.8-7.fc22.x86_64
(gdb) list 567
562 return EXIT_FAILURE;
563 }
564
565 dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
566 if (dn == NULL) {
567 fprintf(stderr, _("Unable to find user %s(a)%s.\n"),
568 user.orig_name, user.domain->name);
569 return EXIT_FAILURE;
570 }
571
(gdb) p user
$1 = {input_name = 0x7fffffffe7b5 "root", orig_name = 0x611a30 "root",
domain = 0x0,
name = 0x619c00 "toor", uid = 0, gid = 0, home = 0x0, shell = 0x0, gecos =
0x0}
Same issue is there with groups btw.