On Tue, Jan 20, 2015 at 01:59:48PM +0100, Sumit Bose wrote:
Hi,
The original values of memberOf and the original DN are used in the HBAC
evaluation and should be send to the IPA client for AD users via the
extdom plugin. The attached patches adds those attribute to the response
of of the getorig request. No changes in the extdom plugin are needed.
Since originalMemberOf is a multi-value attribute fill_orig() must be
able to handle multi-value attributes. The first patch adds the needed
changes while the second refactors fill_orig() a bit. To make reviewing
easier I didn't squash them together. The last patch adds the new
attributes to the getorig request.
bye,
Sumit
The patches work well in my testing. I've submitted them for automated
tests. See two questions inline:
From 8ea260876027dbf312c447b615e740986a990f03 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 20 Jan 2015 12:48:19 +0100
Subject: [PATCH 1/3] nss: make fill_orig() multi-value aware
---
src/responder/nss/nsssrv_cmd.c | 86 ++++++++++++++++++++------
src/tests/cmocka/test_nss_srv.c | 131 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 197 insertions(+), 20 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 3c5d450714fb3f7655cd32aeef900b4f5e9782c7..6bb03d81b355fb2aa06508b4d9b756dd6e2391e6
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4612,13 +4612,14 @@ static errno_t fill_orig(struct sss_packet *packet,
{
int ret;
TALLOC_CTX *tmp_ctx;
- const char *tmp_str;
uint8_t *body;
size_t blen;
size_t pctr = 0;
size_t c;
+ size_t d;
size_t sum;
size_t found;
+ size_t array_size;
size_t extra_attrs_count = 0;
const char **extra_attrs_list = NULL;
const char *orig_attr_list[] = {SYSDB_SID_STR,
@@ -4637,6 +4638,8 @@ static errno_t fill_orig(struct sss_packet *packet,
struct sized_string *keys;
struct sized_string *vals;
struct nss_ctx *nctx;
+ struct ldb_message_element *el;
+ struct ldb_val *val;
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {
@@ -4652,10 +4655,9 @@ static errno_t fill_orig(struct sss_packet *packet,
extra_attrs_count++);
}
- keys = talloc_array(tmp_ctx, struct sized_string,
- sizeof(orig_attr_list) + extra_attrs_count);
- vals = talloc_array(tmp_ctx, struct sized_string,
- sizeof(orig_attr_list) + extra_attrs_count);
+ array_size = sizeof(orig_attr_list) + extra_attrs_count;
+ keys = talloc_array(tmp_ctx, struct sized_string, array_size);
+ vals = talloc_array(tmp_ctx, struct sized_string, array_size);
if (keys == NULL || vals == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
ret = ENOMEM;
@@ -4665,26 +4667,72 @@ static errno_t fill_orig(struct sss_packet *packet,
sum = 0;
found = 0;
for (c = 0; orig_attr_list[c] != NULL; c++) {
- tmp_str = ldb_msg_find_attr_as_string(msg, orig_attr_list[c], NULL);
- if (tmp_str != NULL) {
- to_sized_string(&keys[found], orig_attr_list[c]);
- sum += keys[found].len;
- to_sized_string(&vals[found], tmp_str);
- sum += vals[found].len;
+ el = ldb_msg_find_element(msg, orig_attr_list[c]);
+ if (el != NULL && el->num_values > 0) {
+ if (el->num_values > 1) {
+ array_size += el->num_values;
Aren't we allocating one more sized_string than required here? We
already allocated one item previously..
Not a big deal though, it's just tmp_ctx memory..
+ keys = talloc_realloc(tmp_ctx, keys, struct
sized_string,
+ array_size);
+ vals = talloc_realloc(tmp_ctx, vals, struct sized_string,
+ array_size);
+ if (keys == NULL || vals == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+ }
+ for (d = 0; d < el->num_values; d++) {
+ to_sized_string(&keys[found], orig_attr_list[c]);
+ sum += keys[found].len;
+ val = &(el->values[d]);
+ if (val == NULL || val->data == NULL
+ || val->data[val->length] != '\0') {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Unexpected attribute value found for [%s].\n",
+ orig_attr_list[c]);
+ ret = EINVAL;
+ goto done;
+ }
+ to_sized_string(&vals[found], (const char *)val->data);
+ sum += vals[found].len;
- found++;
+ found++;
+ }
}
}
for (c = 0; c < extra_attrs_count; c++) {
- tmp_str = ldb_msg_find_attr_as_string(msg, extra_attrs_list[c], NULL);
- if (tmp_str != NULL) {
- to_sized_string(&keys[found], extra_attrs_list[c]);
- sum += keys[found].len;
- to_sized_string(&vals[found], tmp_str);
- sum += vals[found].len;
+ el = ldb_msg_find_element(msg, extra_attrs_list[c]);
+ if (el != NULL && el->num_values > 0) {
+ if (el->num_values > 1) {
+ array_size += el->num_values;
+ keys = talloc_realloc(tmp_ctx, keys, struct sized_string,
+ array_size);
+ vals = talloc_realloc(tmp_ctx, vals, struct sized_string,
+ array_size);
+ if (keys == NULL || vals == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+ }
+ for (d = 0; d < el->num_values; d++) {
+ to_sized_string(&keys[found], extra_attrs_list[c]);
+ sum += keys[found].len;
+ val = &(el->values[d]);
+ if (val == NULL || val->data == NULL
+ || val->data[val->length] != '\0') {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Unexpected attribute value found for [%s].\n",
+ orig_attr_list[c]);
+ ret = EINVAL;
+ goto done;
I was wondering for a bit if it makes any sense to skip the malformed
items. But IIRC ldb guarantees the data to be NULL-terminated, so I
guess it makes little sense to over-engineer for a condition that won't
happen.
+ }
+ to_sized_string(&vals[found], (const char *)val->data);
+ sum += vals[found].len;
- found++;
+ found++;
+ }
}
}
From e44218e547c894da90e02bebf38aedd27d2712c7 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 20 Jan 2015 13:50:16 +0100
Subject: [PATCH 2/3] nss: refactor fill_orig()
ACK
From 97e50fcfaf11056935a40c3df1b2b9b7b4b1aec7 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 20 Jan 2015 12:51:57 +0100
Subject: [PATCH 3/3] nss: Add original DN and memberOf to origbyname request
ACK