On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:
On 12/18/2015 02:36 PM, Pavel Březina wrote:
>Hello list,
>the support of IPA sudo schema is almost complete. The only thing that
>remains is to finish smart refresh implementation and one patch to
>reduce code duplication between LDAP and IPA implementation. Then I need
>to run some tests but I don't expect much troubles here since I tested
>it a lot during development. I'll finish all of this after my Christmas
>vacation.
>
>The patches are probably too big to be sent as an attachment, so until I
>complete the last two patches, you can check it on my git repo, branch
>sudo [1]. I don't really expect anyone to review them during Christmas
>break, but I thought it's a good thing to present if in case someone
>will get really bored from all the candies and family visits :-)
>
>Happy reviewing.
>
>[1]
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo
Final patches are attached. Happy reviewing.
Here are my first comments:
0001:
sdap_search_bases_send():
+ if (timeout == 0) {
+ timeout = dp_opt_get_int(opts->basic, SDAP_SEARCH_TIMEOUT);
+ }
see below
+
+ if (bases == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "No search base specified!\n");
+ ret = ERR_INTERNAL;
+ goto immediately;
+ }
+
+ state->ev = ev;
+ state->opts = opts;
+ state->sh = sh;
+ state->bases = bases;
+ state->map = map;
+ state->filter = filter;
+ state->attrs = attrs;
+ state->allow_paging = allow_paging;
+
+ state->timeout = timeout == 0
+ ? dp_opt_get_int(opts->basic, SDAP_SEARCH_TIMEOUT)
+ : timeout;
timeout == 0 is checked twice
+
+ for (state->map_num_attrs = 0;
+ state->map[state->map_num_attrs].opt_name != NULL;
wouldn't is be safer to check if map is not NULL before deferencinf it?
+ state->map_num_attrs++) {
+ /* no op */;
+ }
+
+ if (state->attrs == NULL) {
+ ret = build_attrs_from_map(state, state->map, state->map_num_attrs,
+ NULL, &state->attrs, NULL);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Unable to build attrs from map "
+ "[%d]: %s\n", ret, sss_strerror(ret));
+ goto immediately;
+ }
+ }
Is there a reason for incrementing base_iter at the end of
sdap_search_bases_next_base(), I think it would be more clear to increment it
in sdap_search_bases_done() immediately before calling
sdap_search_bases_next_base() again?
0005:
+int sysdb_compare_usn(const char *a, const char *b)
+{
+ if (a == NULL) {
+ return -1;
+ }
+
+ if (b == NULL) {
+ return 1;
+ }
+
+ /* less digits means lower number */
this is only true as long as a and b do not start with 0. I would recommend to
add a
if (*a != '0' && *b != '0') {
+ if (strlen(a) < strlen(b)) {
+ return -1;
+ }
+
+ /* more digits means bigger number */
+ if (strlen(a) > strlen(b)) {
+ return 1;
+ }
+
+ /* now we can compare digits since alphabetical order is the same
+ * as numeric order */
+ return strcmp(a, b);
+}
+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
+ struct sysdb_attrs **attrs,
+ size_t num_attrs,
+ char **_usn)
+{
+ const char *highest = NULL;
+ const char *current = NULL;
+ char *usn;
+ errno_t ret;
+ size_t i;
+
+ if (num_attrs == 0 || attrs == NULL) {
you can save a few lines since highest == NULL with:
goto done;
+ usn = talloc_strdup(mem_ctx, "0");
+ if (usn == NULL) {
+ return ENOMEM;
+ }
+
+ *_usn = usn;
+ return EOK;
+ }
+
+ for (i = 0; i < num_attrs; i++) {
+ ret = sysdb_attrs_get_string(attrs[i], SYSDB_USN, ¤t);
+ if (ret == ENOENT) {
+ /* USN value is not present, assuming zero. */
+ current = "0";
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "Failed to retrieve USN value "
+ "[%d]: %s\n", ret, sss_strerror(ret));
+
+ return ret;
+ }
+
+ if (current == NULL) {
+ continue;
+ }
+
+ if (highest == NULL) {
+ highest = current;
+ continue;
+ }
+
+ if (sysdb_compare_usn(current, highest) > 0 ) {
+ highest = current;
+ }
+ }
+
done:
+ if (highest == NULL) {
+ usn = talloc_strdup(mem_ctx, "0");
+ } else {
+ usn = talloc_strdup(mem_ctx, highest);
+ }
+
+ if (usn == NULL) {
+ return ENOMEM;
+ }
+
+ *_usn = usn;
+ return EOK;
...
+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC
")"
+
I would prefer a more specific name like e.g. SUDO_ALL_FILTER.
0008:
diff --git a/src/providers/ldap/sdap_async_sudo.c
b/src/providers/ldap/sdap_async_sudo.c
index a44edd72b43da47903c56f34434e810d6c407d94..f24e1d7ac7c0fc07dcde5c635408d3746f8b6c6a
100644
--- a/src/providers/ldap/sdap_async_sudo.c
+++ b/src/providers/ldap/sdap_async_sudo.c
@@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq)
tevent_req_error(req, ret);
}
return;
+ } else if (ret != EOK) {
+ tevent_req_error(req, ret);
Isn't a return missing here?
}
DEBUG(SSSDBG_TRACE_FUNC, "Received %zu rules\n", rules_count);
0009:
+static bool check_dn(struct ldb_dn *dn,
+ const char *rdn_attr,
+ va_list in_ap)
+{
+ const struct ldb_val *ldbval;
+ const char *strval;
+ const char *ldbattr;
+ const char *attr;
+ const char *val;
+ va_list ap;
+ int num_comp;
+ int comp;
+
+ /* check RDN attribute */
+ ldbattr = ldb_dn_get_rdn_name(dn);
+ if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) {
+ return false;
+ }
+
+ /* Check DN components. First we check if all attr=value pairs match input.
+ * Then we check that the next attribute is a domain component.
+ */
+
+ comp = 1;
+ num_comp = ldb_dn_get_comp_num(dn);
+
+ va_copy(ap, in_ap);
+ while ((attr = va_arg(ap, const char *)) != NULL) {
+ val = va_arg(ap, const char *);
+ if (val == NULL) {
+ return false;
+ }
+
+ if (comp > num_comp) {
+ return false;
+ }
+
+ ldbattr = ldb_dn_get_component_name(dn, comp);
+ if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
+ return false;
+ }
+
+ ldbval = ldb_dn_get_component_val(dn, comp);
+ if (ldbval == NULL) {
+ return false;
+ }
+
+ strval = (const char *)ldbval->data;
+ if (strval == NULL || strncasecmp(strval, val, ldbval->length) != 0) {
+ return false;
+ }
+
+ comp++;
+ }
+ va_end(ap);
+
+ ldbattr = ldb_dn_get_component_name(dn, comp);
+ if (ldbattr == NULL || strcmp(ldbattr, "dc") != 0) {
If I see it correctly the "dc" is the only thing specific to IPA (and it would
work for AD as well). I would recommend to make the name of the domain
component a parameter of check_dn() and not put it in ipa/ipa_dn.c but
ldap/sdap_dn.c. This would avoid in the next patch to include a file from the
IPA provider in libsss_ldap_common_la_SOURCES.
+ return false;
+ }
+
+ return true;
+}
That's all for now, more will come later.
bye,
Sumit