[SCM] Samba Shared Repository - branch master updated
Stefan Metzmacher
metze at samba.org
Tue Oct 30 19:21:02 UTC 2018
The branch, master has been updated
via aeef8b4 dsdb group audit tests: log_membership_changes extra tests
via c952fc1 dsdb group audit tests: check_version improve diagnostics
via e2970887 dsdb group audit tests: check_timestamp improve diagnostics
via 8420a4d dsdb group audit: align dn_compare with memcmp
via eeb4089 dsdb group_audit: Test to replicate BUG 13664
from 852e1db dsdb: Add comments explaining the limitations of our current backlink behaviour
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit aeef8b41fa03a32859f824f4a09560ad83bd2b50
Author: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu Oct 25 10:52:55 2018 +1300
dsdb group audit tests: log_membership_changes extra tests
Add extra tests to ensure better test coverage of log_membership_changes
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
Autobuild-User(master): Stefan Metzmacher <metze at samba.org>
Autobuild-Date(master): Tue Oct 30 20:20:26 CET 2018 on sn-devel-144
commit c952fc1273397c04fddf177bcd809551d6324bdd
Author: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu Oct 25 14:38:31 2018 +1300
dsdb group audit tests: check_version improve diagnostics
Change check_version to display the expected, actual along with the
line and name of the failing test, rather than the line in check_version
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit e2970887140d558c6359fd9b3f8c2a4c26d2cf35
Author: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu Oct 25 13:28:09 2018 +1300
dsdb group audit tests: check_timestamp improve diagnostics
Change check_timestamp to display the expected, actual along with the
line and name of the failing test, rather than the line in
check_timestamp.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 8420a4d0fddd71af608635a707ef20f37fa9b627
Author: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu Oct 25 10:52:27 2018 +1300
dsdb group audit: align dn_compare with memcmp
Rename the parameter names and adjust the return codes from dn_compare
so that:
dn_compare(a, b) =>
LESS_THAN means a is less than b.
GREATER_THAN means a is greater than b.
Thanks to metze for suggesting the correct semantics for dn_compare
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13664
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit eeb4089dafc45277d8af19073ef9348451c1836a
Author: Gary Lockyer <gary at catalyst.net.nz>
Date: Tue Oct 23 17:14:34 2018 +1300
dsdb group_audit: Test to replicate BUG 13664
The group audit code incorrectly logs member additions and deletions.
Thanks to metze for the debugging that isolated the issue, and for
suggesting the fix to dn_compare.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13664
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
-----------------------------------------------------------------------
Summary of changes:
source4/dsdb/samdb/ldb_modules/group_audit.c | 31 +-
.../samdb/ldb_modules/tests/test_group_audit.c | 716 ++++++++++++++++++++-
2 files changed, 718 insertions(+), 29 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source4/dsdb/samdb/ldb_modules/group_audit.c b/source4/dsdb/samdb/ldb_modules/group_audit.c
index 1c74805..47b6943 100644
--- a/source4/dsdb/samdb/ldb_modules/group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/group_audit.c
@@ -311,35 +311,36 @@ enum dn_compare_result {
GREATER_THAN
};
/*
- * @brief compare parsed_dns
+ * @brief compare parsed_dn, using GUID ordering
*
- * Compare two parsed_dn structures, parsing the entries if necessary.
+ * Compare two parsed_dn structures, using GUID ordering.
* To avoid the overhead of parsing the DN's this function does a binary
- * compare first. Only parsing the DN's they are not equal at a binary level.
+ * compare first. The DN's tre only parsed if they are not equal at a binary
+ * level.
*
* @param ctx talloc context that will own the parsed dsdb_dn
* @param ldb ldb_context
- * @param old_val The old value
- * @param new_val The old value
+ * @param dn1 The first dn
+ * @param dn2 The second dn
*
* @return BINARY_EQUAL values are equal at a binary level
* EQUAL DN's are equal but the meta data is different
- * LESS_THAN old value < new value
- * GREATER_THAN old value > new value
+ * LESS_THAN dn1's GUID is less than dn2's GUID
+ * GREATER_THAN dn1's GUID is greater than dn2's GUID
*
*/
static enum dn_compare_result dn_compare(
TALLOC_CTX *mem_ctx,
struct ldb_context *ldb,
- struct parsed_dn *old_val,
- struct parsed_dn *new_val) {
+ struct parsed_dn *dn1,
+ struct parsed_dn *dn2) {
int res = 0;
/*
* Do a binary compare first to avoid unnecessary parsing
*/
- if (data_blob_cmp(new_val->v, old_val->v) == 0) {
+ if (data_blob_cmp(dn1->v, dn2->v) == 0) {
/*
* Values are equal at a binary level so no need
* for further processing
@@ -351,22 +352,22 @@ static enum dn_compare_result dn_compare(
* do a GUID ordering compare. To do this we will need to ensure
* that the dn's have been parsed.
*/
- if (old_val->dsdb_dn == NULL) {
+ if (dn1->dsdb_dn == NULL) {
really_parse_trusted_dn(
mem_ctx,
ldb,
- old_val,
+ dn1,
LDB_SYNTAX_DN);
}
- if (new_val->dsdb_dn == NULL) {
+ if (dn2->dsdb_dn == NULL) {
really_parse_trusted_dn(
mem_ctx,
ldb,
- new_val,
+ dn2,
LDB_SYNTAX_DN);
}
- res = ndr_guid_compare(&new_val->guid, &old_val->guid);
+ res = ndr_guid_compare(&dn1->guid, &dn2->guid);
if (res < 0) {
return LESS_THAN;
} else if (res == 0) {
diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index 8551dcb..6ec218b 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -64,9 +64,151 @@ int dsdb_search_one(struct ldb_context *ldb,
}
/*
+ * Mock version of audit_log_json
+ */
+
+#define MAX_EXPECTED_MESSAGES 16
+static struct json_object messages[MAX_EXPECTED_MESSAGES];
+static size_t messages_sent = 0;
+
+void audit_message_send(
+ struct imessaging_context *msg_ctx,
+ const char *server_name,
+ uint32_t message_type,
+ struct json_object *message)
+{
+ messages[messages_sent].root = json_deep_copy(message->root);
+ messages[messages_sent].valid = message->valid;
+ messages_sent++;
+}
+
+#define check_group_change_message(m, u, a)\
+ _check_group_change_message(m, u, a, __FILE__, __LINE__);
+/*
+ * declare the internal cmocka cm_print_error so that we can output messages
+ * in sub unit format
+ */
+void cm_print_error(const char * const format, ...);
+
+/*
+ * Validate a group change JSON audit message
+ *
+ * It should contain 3 elements.
+ * Have a type of "groupChange"
+ * Have a groupChange element
+ *
+ * The group change element should have 10 elements.
+ *
+ * There should be a user element matching the expected value
+ * There should be an action matching the expected value
+ */
+static void _check_group_change_message(
+ const int message,
+ const char *user,
+ const char *action,
+ const char *file,
+ const int line)
+{
+ struct json_object json;
+ json_t *audit = NULL;
+ json_t *v = NULL;
+ const char* value;
+ json = messages[message];
+
+ /*
+ * Validate the root JSON element
+ * check the number of elements
+ */
+ if (json_object_size(json.root) != 3) {
+ cm_print_error(
+ "Unexpected number of elements in root %zu != %d\n",
+ json_object_size(json.root),
+ 3);
+ _fail(file, line);
+ }
+
+ /*
+ * Check the type element
+ */
+ v = json_object_get(json.root, "type");
+ if (v == NULL) {
+ cm_print_error( "No \"type\" element\n");
+ _fail(file, line);
+ }
+
+ value = json_string_value(v);
+ if (strncmp("groupChange", value, strlen("groupChange") != 0)) {
+ cm_print_error(
+ "Unexpected type \"%s\" != \"groupChange\"\n",
+ value);
+ _fail(file, line);
+ }
+
+
+ audit = json_object_get(json.root, "groupChange");
+ if (audit == NULL) {
+ cm_print_error("No groupChange element\n");
+ _fail(file, line);
+ }
+
+ /*
+ * Validate the groupChange element
+ */
+ if (json_object_size(audit) != 10) {
+ cm_print_error(
+ "Unexpected number of elements in groupChange "
+ "%zu != %d\n",
+ json_object_size(audit),
+ 10);
+ _fail(file, line);
+ }
+ /*
+ * Validate the user element
+ */
+ v = json_object_get(audit, "user");
+ if (v == NULL) {
+ cm_print_error( "No user element\n");
+ _fail(file, line);
+ }
+
+ value = json_string_value(v);
+ if (strncmp(user, value, strlen(user) != 0)) {
+ cm_print_error(
+ "Unexpected user name \"%s\" != \"%s\"\n",
+ value,
+ user);
+ _fail(file, line);
+ }
+
+ /*
+ * Validate the action element
+ */
+ v = json_object_get(audit, "action");
+ if (v == NULL) {
+ cm_print_error( "No action element\n");
+ _fail(file, line);
+ }
+
+ value = json_string_value(v);
+ if (strncmp(action, value, strlen(action) != 0)) {
+ print_error(
+ "Unexpected action \"%s\" != \"%s\"\n",
+ value,
+ action);
+ _fail(file, line);
+ }
+}
+
+#define check_timestamp(b, t)\
+ _check_timestamp(b, t, __FILE__, __LINE__);
+/*
* Test helper to check ISO 8601 timestamps for validity
*/
-static void check_timestamp(time_t before, const char *timestamp)
+static void _check_timestamp(
+ time_t before,
+ const char *timestamp,
+ const char *file,
+ const int line)
{
int rc;
int usec, tz;
@@ -102,29 +244,97 @@ static void check_timestamp(time_t before, const char *timestamp)
actual = mktime(&tm);
/*
- * The timestamp should be before <= actual <= after
+ * The time stamp should be before <= actual <= after
*/
- assert_true(difftime(actual, before) >= 0);
- assert_true(difftime(after, actual) >= 0);
+ if (difftime(actual, before) < 0) {
+ char buffer[40];
+ strftime(buffer,
+ sizeof(buffer)-1,
+ "%Y-%m-%dT%T",
+ localtime(&before));
+ cm_print_error(
+ "time stamp \"%s\" is before start time \"%s\"\n",
+ timestamp,
+ buffer);
+ _fail(file, line);
+ }
+ if (difftime(after, actual) < 0) {
+ char buffer[40];
+ strftime(buffer,
+ sizeof(buffer)-1,
+ "%Y-%m-%dT%T",
+ localtime(&after));
+ cm_print_error(
+ "time stamp \"%s\" is after finish time \"%s\"\n",
+ timestamp,
+ buffer);
+ _fail(file, line);
+ }
}
+#define check_version(v, m, n)\
+ _check_version(v, m, n, __FILE__, __LINE__);
/*
* Test helper to validate a version object.
*/
-static void check_version(struct json_t *version, int major, int minor)
+static void _check_version(
+ struct json_t *version,
+ int major,
+ int minor,
+ const char* file,
+ const int line)
{
struct json_t *v = NULL;
+ int value;
+
+ if (!json_is_object(version)) {
+ cm_print_error("version is not a JSON object\n");
+ _fail(file, line);
+ }
- assert_true(json_is_object(version));
- assert_int_equal(2, json_object_size(version));
+ if (json_object_size(version) != 2) {
+ cm_print_error(
+ "Unexpected number of elements in version %zu != %d\n",
+ json_object_size(version),
+ 2);
+ _fail(file, line);
+ }
+ /*
+ * Validate the major version number element
+ */
v = json_object_get(version, "major");
- assert_non_null(v);
- assert_int_equal(major, json_integer_value(v));
+ if (v == NULL) {
+ cm_print_error( "No major element\n");
+ _fail(file, line);
+ }
+
+ value = json_integer_value(v);
+ if (value != major) {
+ print_error(
+ "Unexpected major version number \"%d\" != \"%d\"\n",
+ value,
+ major);
+ _fail(file, line);
+ }
+ /*
+ * Validate the minor version number element
+ */
v = json_object_get(version, "minor");
- assert_non_null(v);
- assert_int_equal(minor, json_integer_value(v));
+ if (v == NULL) {
+ cm_print_error( "No minor element\n");
+ _fail(file, line);
+ }
+
+ value = json_integer_value(v);
+ if (value != minor) {
+ print_error(
+ "Unexpected minor version number \"%d\" != \"%d\"\n",
+ value,
+ minor);
+ _fail(file, line);
+ }
}
/*
@@ -426,7 +636,7 @@ static void test_dn_compare(void **state)
b->v = &bb;
res = dn_compare(ctx, ldb, a, b);
- assert_int_equal(GREATER_THAN, res);
+ assert_int_equal(LESS_THAN, res);
/*
* DN's should have been parsed
*/
@@ -454,7 +664,7 @@ static void test_dn_compare(void **state)
b->v = &bb;
res = dn_compare(ctx, ldb, a, b);
- assert_int_equal(LESS_THAN, res);
+ assert_int_equal(GREATER_THAN, res);
/*
* DN's should have been parsed
*/
@@ -618,7 +828,481 @@ static void test_audit_group_json(void **state)
json_free(&json);
TALLOC_FREE(ctx);
+}
+
+static void setup_ldb(
+ TALLOC_CTX *ctx,
+ struct ldb_context **ldb,
+ struct ldb_module **module,
+ const char *ip,
+ const char *session,
+ const char *sid)
+{
+ struct tsocket_address *ts = NULL;
+ struct audit_context *context = NULL;
+
+ *ldb = ldb_init(ctx, NULL);
+ ldb_register_samba_handlers(*ldb);
+
+ *module = talloc_zero(ctx, struct ldb_module);
+ (*module)->ldb = *ldb;
+
+ context = talloc_zero(*module, struct audit_context);
+ context->send_events = true;
+ context->msg_ctx = (struct imessaging_context *) 0x01;
+
+ ldb_module_set_private(*module, context);
+
+ tsocket_address_inet_from_strings(ctx, "ip", "127.0.0.1", 0, &ts);
+ ldb_set_opaque(*ldb, "remoteAddress", ts);
+
+ add_session_data(ctx, *ldb, session, sid);
+}
+
+/*
+ * Test the removal of a user from a group.
+ *
+ * The new element contains one group member
+ * The old element contains two group member
+ *
+ * Expect to see the removed entry logged.
+ *
+ * This test confirms bug 13664
+ * https://bugzilla.samba.org/show_bug.cgi?id=13664
+ */
+static void test_log_membership_changes_removed(void **state)
+{
+ struct ldb_context *ldb = NULL;
+ struct ldb_module *module = NULL;
+ const char * const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+ const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+ const char * const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+ const char * const IP = "127.0.0.1";
+ struct ldb_request *req = NULL;
+ struct ldb_message_element *new_el = NULL;
+ struct ldb_message_element *old_el = NULL;
+ int status = 0;
+ TALLOC_CTX *ctx = talloc_new(NULL);
+
+ setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+ /*
+ * Build the ldb_request
+ */
+ req = talloc_zero(ctx, struct ldb_request);
+ req->operation = LDB_ADD;
+ add_transaction_id(req, TRANSACTION);
+
+ /*
+ * Populate the new elements, containing one entry.
+ * Indicating that one element has been removed
+ */
+ new_el = talloc_zero(ctx, struct ldb_message_element);
+ new_el->num_values = 1;
+ new_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+ new_el->values[0] = data_blob_string_const(
+ "<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+ "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+ "DC=example,DC=com");
+
+ /*
+ * Populate the old elements, with two elements
+ * The first is the same as the one in new elements.
+ */
+ old_el = talloc_zero(ctx, struct ldb_message_element);
+ old_el->num_values = 2;
+ old_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+ old_el->values[0] = data_blob_string_const(
+ "<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+ "cn=grpadttstuser01,cn=users,DC=addom,"
+ "DC=samba,DC=example,DC=com");
+ old_el->values[1] = data_blob_string_const(
+ "<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+ "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+ "DC=example,DC=com");
+
+ /*
+ * call log_membership_changes
+ */
+ messages_sent = 0;
+ log_membership_changes(module, req, new_el, old_el, status);
+
+ /*
+ * Check the results
+ */
+ assert_int_equal(1, messages_sent);
+
+ check_group_change_message(
+ 0,
+ "cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+ "Removed");
+
+ /*
+ * Clean up
+ */
+ json_free(&messages[0]);
+ TALLOC_FREE(ctx);
+}
+
+/* test log_membership_changes
+ *
+ * old contains 2 user dn's
+ * new contains 0 user dn's
+ *
+ * Expect to see both dn's logged as deleted.
+ */
+static void test_log_membership_changes_remove_all(void **state)
+{
+ struct ldb_context *ldb = NULL;
+ struct ldb_module *module = NULL;
+ const char * const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+ const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+ const char * const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+ const char * const IP = "127.0.0.1";
+ struct ldb_request *req = NULL;
+ struct ldb_message_element *new_el = NULL;
--
Samba Shared Repository
More information about the samba-cvs
mailing list