Dne 12.1.2012 12:17, Pavel Březina napsal(a):
Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
#0001:
line 105: talloc_array -> talloc
talloc_array is better choice there...we want to allocate an array of
"struct sysdb_attrs pointers"
I know, I wrote a wrong line number. I meant line 40 (in the DEBUG
message). Sorry for the confusion.
#0003:
I'm not sure about the NULL_CHECK macro. I don't condemn it but if I
choose to accept it, I'll want it to be used everywhere it's
possible.
At least in the scope if this patch.
That was the intent but I forgot.
Nitpick: in the for on line 231 I'd add spaces around the = char
OK
line 359: I don't think it is necessary to end purging the tree. I
think continuing is better action in this case.
I was trying to be defensive and avoid ending up with rules that
are no
longer present on the server which might grant access to users that
are
not supposed to be in sudoers anymore..
Exactly my point. If you get out of purging cycle on the first rule
that
doesn't have a name, you can potentially skip many more rules.
#0004:
Nitpick: line 166: I'd move the condition to the following block
starting with if (!dbus_ret)
Yes, it's supposed to be there.
line 199: really EOK?
Thanks, it's supposed to say 'ret'
Actually there should be EOK. At least the other be handlers returns EOK
in any case.
If it returns value different than EOK it will send some generic message
back to the responder. This way I can specify format of the message.
#0005:
line 33: really SDAP_NETGROUP_SEARCH_BASE?
That should say SDAP_SUDO_SEARCH_BASE. This code is commented out with
#if 0, though, until we provide support for IPA sudo rules.
I know, but for the sake of reducing possibility to overlook it in the
future, I'd prefer to fix it now.
line 109: I find this misleading - the map is called native but
several
lines above, there is a statement that native sudo rules are not
supported
Right, that's a mismatch between "native SUDO rules" and "native IPA
SUDO rules".
line 131: for the comment to be true, you have to add
SDAP_SUDO_SEARCH_BASE to search_base_options
Correct.
The comment is invalid, we load sudo search base in
ldap_get_sudo_options().
#0007:
line 534: is the TODO still valid?
No. I love removing TODOs :)
I used the first patch from "SUDO Integration - periodical update thread".
line 461: in the NSS responder, there is a check for NULL termination
of the string. Can something similar happen here as well?
According to the current sudo sources, it can't, but better safe than
sorry.
If you mean line 461 in #0008 then I believe it is done in
sudosrv_get_sudorules_parse_query().
sudosrv_dp.c: 92 - missing dbus_message_unref() in the error handling
block
Right. We should fix it until we reuse the tevent_req DP requests in
that code.
client patches:
First of all I'd like to have sss_sudo_get_result() renamed. The name
is quite confusing considering that it doesn't only fetch the result,
but it also makes the request. I suggest using something like
sss_sudo_get_rules(), that would make things much more readable.
ok, let's rename it to sss_sudo_send_recv()
in sss_sudo_parse_rule: I don't think it's necessary to start
variables
with underscore if you don't have their local variable counterparts.
But that's just a readability suggestion.
I'm not entirely sure about this newxt issue, but when you send
response to the client, you use SAFEALIGN_SET_UINT32 macro, but when
receiving the variable you don't use anything like that. I'm a bit
concerned that this might cause some issues on exotic architectures.
sorry, but where in the code is the issue? Plain casting pointer
dereference would be bad on architectures that don't align on word
boundary.
Well, it's not quite like that, memcpy is used. See
sss_sudo_parse_uint32(). I think that's the only place that hit me.
We want the client API to be as small as possible because it will be
moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
use memcpy() directly.
Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
macros
won't be subjects to change in the foreseeable future. Please consider at
least a comment in the code that this way is safe because it is the
same what
SAFEALIGN_* does.
Thank you for the review.
Patch addressing these issues is attached.
It also includes:
- sudo configuration options added to SSSDConfig.py
- attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
- in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
strlen() to check validity of the input
Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
I'm sending it one more time with the options included in
config/etc/sssd.api.d/sssd-ldap.conf
From 606768e6faffedab90273ada63a2ebc78b66acd3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Thu, 12 Jan 2012 09:41:52 +0100
Subject: [PATCH] SUDO Integration review issues
---
src/config/SSSDConfig.py | 14 +++++++++++++
src/config/SSSDConfigTest.py | 2 +-
src/config/etc/sssd.api.d/sssd-ldap.conf | 14 +++++++++++++
src/db/sysdb.c | 2 +-
src/db/sysdb_sudo.c | 30 ++++++++++-----------------
src/providers/data_provider_be.c | 8 +++---
src/providers/ipa/ipa_common.c | 4 +-
src/providers/ldap/ldap_common.c | 2 +-
src/providers/ldap/sdap_sudo.c | 3 +-
src/responder/sudo/sudosrv_dp.c | 2 +
src/responder/sudo/sudosrv_get_sudorules.c | 3 +-
src/sss_client/sudo/sss_sudo.c | 6 ++--
src/sss_client/sudo/sss_sudo.h | 6 ++--
src/sss_client/sudo/sss_sudo_response.c | 1 +
src/sss_client/sudo_testcli/sudo_testcli.c | 2 +-
15 files changed, 61 insertions(+), 38 deletions(-)
diff --git a/src/config/SSSDConfig.py b/src/config/SSSDConfig.py
index 44bfb69..f103ed3 100644
--- a/src/config/SSSDConfig.py
+++ b/src/config/SSSDConfig.py
@@ -237,6 +237,20 @@ option_strings = {
# [provider/ldap/chpass]
'ldap_chpass_uri' : _('URI of an LDAP server where password changes are
allowed'),
'ldap_chpass_dns_service_name' : _('DNS service name for LDAP password
change server'),
+
+ # [provider/ldap/sudo]
+ 'ldap_sudo_search_base' : _('Base DN for sudo rules lookups'),
+ 'ldap_sudorule_object_class' : _('Object class for sudo rules'),
+ 'ldap_sudorule_name' : _('Sudo rule name'),
+ 'ldap_sudorule_command' : _('Sudo rule command attribute'),
+ 'ldap_sudorule_host' : _('Sudo rule host attribute'),
+ 'ldap_sudorule_user' : _('Sudo rule user attribute'),
+ 'ldap_sudorule_option' : _('Sudo rule option attribute'),
+ 'ldap_sudorule_runasuser' : _('Sudo rule runasuser attribute'),
+ 'ldap_sudorule_runasgroup' : _('Sudo rule runasgroup attribute'),
+ 'ldap_sudorule_notbefore' : _('Sudo rule notbefore attribute'),
+ 'ldap_sudorule_notafter' : _('Sudo rule notafter attribute'),
+ 'ldap_sudorule_order' : _('Sudo rule order attribute'),
# [provider/simple/access]
'simple_allow_users' : _('Comma separated list of allowed users'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 8421a09..07e772c 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -681,7 +681,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase):
control_provider_dict = {
'ipa': ['id', 'auth', 'access', 'chpass'],
'local': ['id', 'auth', 'chpass'],
- 'ldap': ['id', 'auth', 'access', 'chpass'],
+ 'ldap': ['id', 'auth', 'access', 'chpass', 'sudo'],
'krb5': ['auth', 'access', 'chpass'],
'proxy': ['id', 'auth'],
'simple': ['access'],
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf
b/src/config/etc/sssd.api.d/sssd-ldap.conf
index 9a89bfe..b155c2b 100644
--- a/src/config/etc/sssd.api.d/sssd-ldap.conf
+++ b/src/config/etc/sssd.api.d/sssd-ldap.conf
@@ -106,3 +106,17 @@ ldap_access_order = str, None, false
[provider/ldap/chpass]
ldap_chpass_uri = str, None, false
ldap_chpass_dns_service_name = str, None, false
+
+[provider/ldap/sudo]
+ldap_sudo_search_base = str, None, false
+ldap_sudorule_object_class = str, None, false
+ldap_sudorule_name = str, None, false
+ldap_sudorule_command = str, None, false
+ldap_sudorule_host = str, None, false
+ldap_sudorule_user = str, None, false
+ldap_sudorule_option = str, None, false
+ldap_sudorule_runasuser = str, None, false
+ldap_sudorule_runasgroup = str, None, false
+ldap_sudorule_notbefore = str, None, false
+ldap_sudorule_notafter = str, None, false
+ldap_sudorule_order = str, None, false
diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 8ca4c17..9fcb7ae 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1782,7 +1782,7 @@ errno_t sysdb_msg2attrs(TALLOC_CTX *mem_ctx, size_t count,
for (i = 0; i < count; i++) {
a[i] = talloc(a, struct sysdb_attrs);
if (a[i] == NULL) {
- DEBUG(1, ("talloc_array failed.\n"));
+ DEBUG(1, ("talloc failed.\n"));
talloc_free(a);
return ENOMEM;
}
diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c
index 1703e78..81b330c 100644
--- a/src/db/sysdb_sudo.c
+++ b/src/db/sysdb_sudo.c
@@ -73,7 +73,7 @@ sysdb_get_sudo_filter(TALLOC_CTX *mem_ctx, const char
*username,
int i;
tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) return ENOMEM;
+ NULL_CHECK(tmp_ctx, ret, done);
/* AND with objectclass */
filter = talloc_asprintf(tmp_ctx, "(&(%s=%s)",
@@ -156,7 +156,7 @@ sysdb_get_sudo_user_info(TALLOC_CTX *mem_ctx, const char
*username,
int i;
tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) return ENOMEM;
+ NULL_CHECK(tmp_ctx, ret, done);
attrs[0] = SYSDB_MEMBEROF;
attrs[1] = SYSDB_UIDNUM;
@@ -181,13 +181,10 @@ sysdb_get_sudo_user_info(TALLOC_CTX *mem_ctx, const char
*username,
sysdb_groupnames = NULL;
} else {
sysdb_groupnames = talloc_array(tmp_ctx, char *, groups->num_values+1);
- if (!sysdb_groupnames) {
- ret = ENOMEM;
- goto done;
- }
+ NULL_CHECK(sysdb_groupnames, ret, done);
/* Get a list of the groups by groupname only */
- for (i=0; i < groups->num_values; i++) {
+ for (i = 0; i < groups->num_values; i++) {
ret = sysdb_group_dn_name(sysdb,
sysdb_groupnames,
(const char *)groups->values[i].data,
@@ -218,17 +215,10 @@ sysdb_sudo_purge_subdir(struct sysdb_ctx *sysdb,
errno_t ret;
tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_new() failed\n"));
- ret = ENOMEM;
- goto done;
- }
+ NULL_CHECK(tmp_ctx, ret, done);
base_dn = sysdb_custom_subtree_dn(sysdb, tmp_ctx, domain->name, subdir);
- if (base_dn == NULL) {
- ret = ENOMEM;
- goto done;
- }
+ NULL_CHECK(base_dn, ret, done);
ret = sysdb_delete_recursive(sysdb, base_dn, true);
if (ret != EOK) {
@@ -289,7 +279,8 @@ sysdb_purge_sudorule_subtree(struct sysdb_ctx *sysdb,
errno_t ret;
const char *attrs[] = { SYSDB_OBJECTCLASS
SYSDB_SUDO_CACHE_AT_OC,
- SYSDB_SUDO_CACHE_AT_CN };
+ SYSDB_SUDO_CACHE_AT_CN,
+ NULL };
/* just purge all if there's no filter */
if (!filter) {
@@ -297,7 +288,7 @@ sysdb_purge_sudorule_subtree(struct sysdb_ctx *sysdb,
}
tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) return ENOMEM;
+ NULL_CHECK(tmp_ctx, ret, done);
/* match entries based on the filter and remove them one by one */
ret = sysdb_search_custom(tmp_ctx, sysdb, filter,
@@ -316,7 +307,8 @@ sysdb_purge_sudorule_subtree(struct sysdb_ctx *sysdb,
name = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL);
if (name == NULL) {
DEBUG(SSSDBG_OP_FAILURE, ("A rule without a name?\n"));
- goto done;
+ /* skip this one but still delete other entries */
+ continue;
}
ret = sysdb_delete_custom(sysdb, name, SUDORULE_SUBDIR);
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index bf77c5f..e30395d 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -696,11 +696,11 @@ static int be_sudo_handler(DBusMessage *message, struct
sbus_connection *conn)
DBUS_TYPE_STRING,
&(be_sudo_req->username),
DBUS_TYPE_INVALID);
- if (dbus_error_is_set(&dbus_error)) {
- dbus_error_free(&dbus_error);
- }
-
if (!dbus_ret) {
+ if (dbus_error_is_set(&dbus_error)) {
+ dbus_error_free(&dbus_error);
+ }
+
DEBUG(SSSDBG_CRIT_FAILURE, ("dbus_message_get_args failed.\n"));
ret = EINVAL;
goto fail;
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index 4f90b18..c3ea8c3 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -468,14 +468,14 @@ int ipa_get_id_options(struct ipa_options *ipa_opts,
if (NULL == dp_opt_get_string(ipa_opts->id->basic,
SDAP_SUDO_SEARCH_BASE)) {
#if 0
- ret = dp_opt_set_string(ipa_opts->id->basic, SDAP_NETGROUP_SEARCH_BASE,
+ ret = dp_opt_set_string(ipa_opts->id->basic, SDAP_SUDO_SEARCH_BASE,
dp_opt_get_string(ipa_opts->id->basic,
SDAP_SEARCH_BASE));
if (ret != EOK) {
goto done;
}
#else
- /* We don't yet have support for the native representation
+ /* We don't yet have support for the representation
* of sudo in IPA. For now, we need to point at the
* compat tree
*/
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 7192196..6ca6f34 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -271,7 +271,7 @@ int ldap_get_options(TALLOC_CTX *memctx,
/* Handle search bases */
search_base = dp_opt_get_string(opts->basic, SDAP_SEARCH_BASE);
if (search_base != NULL) {
- /* set user/group/netgroup/sudo search bases if they are not */
+ /* set user/group/netgroup search bases if they are not */
for (o = 0; search_base_options[o] != -1; o++) {
if (NULL == dp_opt_get_string(opts->basic,
search_base_options[o])) {
ret = dp_opt_set_string(opts->basic, search_base_options[o],
diff --git a/src/providers/ldap/sdap_sudo.c b/src/providers/ldap/sdap_sudo.c
index 68cb47c..387cf0c 100644
--- a/src/providers/ldap/sdap_sudo.c
+++ b/src/providers/ldap/sdap_sudo.c
@@ -408,8 +408,6 @@ void sdap_sudo_load_sudoers_done(struct tevent_req *req)
DEBUG(SSSDBG_TRACE_FUNC, ("Received %d rules\n", rules_count));
/* purge cache */
- /* TODO purge with filter */
- DEBUG(SSSDBG_TRACE_FUNC, ("Purging sudo cache with filter %s\n", ""));
ret = sdap_sudo_purge_sudoers(sudo_ctx);
if (ret != EOK) {
goto done;
@@ -448,6 +446,7 @@ int sdap_sudo_purge_sudoers(struct sdap_sudo_ctx *sudo_ctx)
}
/* Purge rules */
+ DEBUG(SSSDBG_TRACE_FUNC, ("Purging sudo cache with filter [%s]\n",
filter));
ret = sysdb_purge_sudorule_subtree(sysdb_ctx, sudo_ctx->be_ctx->domain,
filter);
if (ret != EOK) {
diff --git a/src/responder/sudo/sudosrv_dp.c b/src/responder/sudo/sudosrv_dp.c
index 0c621f5..27f01f9 100644
--- a/src/responder/sudo/sudosrv_dp.c
+++ b/src/responder/sudo/sudosrv_dp.c
@@ -107,6 +107,8 @@ struct tevent_req * sudosrv_dp_refresh_send(struct resp_ctx
*rctx,
error:
tevent_req_error(req, ret);
tevent_req_post(req, rctx->ev);
+ dbus_message_unref(msg);
+
return req;
}
diff --git a/src/responder/sudo/sudosrv_get_sudorules.c
b/src/responder/sudo/sudosrv_get_sudorules.c
index 5d54f95..fca6257 100644
--- a/src/responder/sudo/sudosrv_get_sudorules.c
+++ b/src/responder/sudo/sudosrv_get_sudorules.c
@@ -481,7 +481,8 @@ char * sudosrv_get_sudorules_parse_query(TALLOC_CTX
*mem_ctx,
const char *query_body,
int query_len)
{
- if (query_len < 2 || ((query_len - 1) != strlen(query_body))) {
+ /* empty string or not NULL terminated */
+ if (query_len < 2 || strnlen(query_body, query_len) == query_len) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid query.\n"));
return NULL;
}
diff --git a/src/sss_client/sudo/sss_sudo.c b/src/sss_client/sudo/sss_sudo.c
index 9a74945..01fdee0 100644
--- a/src/sss_client/sudo/sss_sudo.c
+++ b/src/sss_client/sudo/sss_sudo.c
@@ -36,9 +36,9 @@ static void sss_sudo_free_rules(unsigned int num_rules,
static void sss_sudo_free_attrs(unsigned int num_attrs,
struct sss_attr *attrs);
-int sss_sudo_get_result(const char *username,
- uint32_t *_error,
- struct sss_result **_result)
+int sss_sudo_send_recv(const char *username,
+ uint32_t *_error,
+ struct sss_result **_result)
{
struct sss_result *result = NULL;
struct sss_cli_req_data request;
diff --git a/src/sss_client/sudo/sss_sudo.h b/src/sss_client/sudo/sss_sudo.h
index 04e19db..1b55467 100644
--- a/src/sss_client/sudo/sss_sudo.h
+++ b/src/sss_client/sudo/sss_sudo.h
@@ -45,9 +45,9 @@ struct sss_result {
struct sss_rule *rules;
};
-int sss_sudo_get_result(const char *username,
- uint32_t *_error,
- struct sss_result **_result);
+int sss_sudo_send_recv(const char *username,
+ uint32_t *_error,
+ struct sss_result **_result);
void sss_sudo_free_result(struct sss_result *result);
diff --git a/src/sss_client/sudo/sss_sudo_response.c
b/src/sss_client/sudo/sss_sudo_response.c
index d33215a..2b158b7 100644
--- a/src/sss_client/sudo/sss_sudo_response.c
+++ b/src/sss_client/sudo/sss_sudo_response.c
@@ -188,6 +188,7 @@ int sss_sudo_parse_uint32(const char *message,
return EINVAL;
}
+ /* expanded SAFEALIGN_COPY_UINT32 macro from util.h */
memcpy(_number, message + start_pos, sizeof(uint32_t));
*_cursor = start_pos + sizeof(uint32_t);
diff --git a/src/sss_client/sudo_testcli/sudo_testcli.c
b/src/sss_client/sudo_testcli/sudo_testcli.c
index e7da035..be31037 100644
--- a/src/sss_client/sudo_testcli/sudo_testcli.c
+++ b/src/sss_client/sudo_testcli/sudo_testcli.c
@@ -96,7 +96,7 @@ int main(int argc, char **argv)
/* get sss_result - it will send new query to responder */
- ret = sss_sudo_get_result(username, &error, &result);
+ ret = sss_sudo_send_recv(username, &error, &result);
if (ret != EOK) {
fprintf(stderr, "Usss_sudo_get_result() failed: %s\n", strerror(ret));
goto fail;
--
1.7.6.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
