[PATCH] s4 dsdb group_audit: Update error handling in logging group changes.

Gary Lockyer gary at catalyst.net.nz
Wed Jan 9 22:10:35 UTC 2019


Generate an appropriate log message in the event of an error in
log_group_membership_changes.  As the changes have not been applied to
the database, there is no easy way to determine the intended changes.
This information is available in the "dsdbChange" audit messages, to
avoid replicating this logic for what should be a very rare occurrence
we simply log it as a "Failure"

Merge request: https://gitlab.com/samba-team/samba/merge_requests/189
CI results: https://gitlab.com/samba-team/devel/samba/pipelines/42630251

Review and comments appreciated.

Ngā mihi
Gary.
-------------- next part --------------
From c369504a1fb673ece82fa18cef111c1256656227 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Tue, 8 Jan 2019 14:23:38 +1300
Subject: [PATCH 1/2] group_audit: Tests for error handling in group change

Add tests to exercise the error handling in
log_group_membership_changes.

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/group_audit              |   2 +
 .../ldb_modules/tests/test_group_audit.c      | 591 +++++++++++++++++-
 2 files changed, 581 insertions(+), 12 deletions(-)
 create mode 100644 selftest/knownfail.d/group_audit

diff --git a/selftest/knownfail.d/group_audit b/selftest/knownfail.d/group_audit
new file mode 100644
index 00000000000..4f5855fea01
--- /dev/null
+++ b/selftest/knownfail.d/group_audit
@@ -0,0 +1,2 @@
+^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_group_membership_changes_read_new_failure\(none\)
+^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_group_membership_changes_error\(none\)
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 a6b9d603977..0bbde9f3e3b 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -39,6 +39,7 @@ uint32_t g_dsdb_flags;
 const char *g_exp_fmt;
 const char *g_dn = NULL;
 int g_status = LDB_SUCCESS;
+struct ldb_result *g_result = NULL;
 
 int dsdb_search_one(struct ldb_context *ldb,
 		    TALLOC_CTX *mem_ctx,
@@ -63,6 +64,24 @@ int dsdb_search_one(struct ldb_context *ldb,
 	return g_status;
 }
 
+int dsdb_module_search_dn(
+	struct ldb_module *module,
+	TALLOC_CTX *mem_ctx,
+	struct ldb_result **res,
+	struct ldb_dn *basedn,
+	const char * const *attrs,
+	uint32_t dsdb_flags,
+	struct ldb_request *parent)
+{
+
+	g_basedn = basedn;
+	g_attrs = attrs;
+	g_dsdb_flags = dsdb_flags;
+
+	*res = g_result;
+
+	return g_status;
+}
 /*
  * Mock version of audit_log_json
  */
@@ -158,7 +177,8 @@ static void _check_group_change_message(const int message,
 	/*
 	 * Validate the groupChange element
 	 */
-	if (json_object_size(audit) != 11) {
+	if ((event_id == EVT_ID_NONE && json_object_size(audit) != 10) ||
+	    (event_id != EVT_ID_NONE && json_object_size(audit) != 11)) {
 		cm_print_error("Unexpected number of elements in groupChange "
 			       "%zu != %d\n",
 			       json_object_size(audit),
@@ -206,17 +226,28 @@ static void _check_group_change_message(const int message,
 	 * Validate the eventId element
 	 */
 	v = json_object_get(audit, "eventId");
-	if (v == NULL) {
-		cm_print_error("No eventId element\n");
-		_fail(file, line);
+	if (event_id == EVT_ID_NONE) {
+		if (v != NULL) {
+			int_value = json_integer_value(v);
+			cm_print_error("Unexpected eventId \"%d\", it should "
+				       "NOT be present",
+				       int_value);
+			_fail(file, line);
+		}
 	}
-
-	int_value = json_integer_value(v);
-	if (int_value != event_id) {
-		cm_print_error("Unexpected eventId \"%d\" != \"%d\"\n",
-			       int_value,
-			       event_id);
-		_fail(file, line);
+	else {
+		if (v == NULL) {
+			cm_print_error("No eventId element\n");
+			_fail(file, line);
+		}
+
+		int_value = json_integer_value(v);
+		if (int_value != event_id) {
+			cm_print_error("Unexpected eventId \"%d\" != \"%d\"\n",
+				       int_value,
+				       event_id);
+			_fail(file, line);
+		}
 	}
 }
 
@@ -802,7 +833,7 @@ static void test_audit_group_json(void **state)
 				"the-user-name",
 				"the-group-name",
 				event_id,
-				LDB_ERR_OPERATIONS_ERROR);
+				LDB_SUCCESS);
 	assert_int_equal(3, json_object_size(json.root));
 
 	v = json_object_get(json.root, "type");
@@ -829,6 +860,111 @@ static void test_audit_group_json(void **state)
 	assert_int_equal(EVT_ID_USER_ADDED_TO_GLOBAL_SEC_GROUP,
 			 json_integer_value(v));
 
+	v = json_object_get(audit, "statusCode");
+	assert_non_null(v);
+	assert_true(json_is_integer(v));
+	assert_int_equal(LDB_SUCCESS, json_integer_value(v));
+
+	v = json_object_get(audit, "status");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("Success", json_string_value(v));
+
+	v = json_object_get(audit, "user");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-user-name", json_string_value(v));
+
+	v = json_object_get(audit, "group");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-group-name", json_string_value(v));
+
+	v = json_object_get(audit, "action");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-action", json_string_value(v));
+
+	json_free(&json);
+	TALLOC_FREE(ctx);
+}
+
+static void test_audit_group_json_error(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+	struct ldb_request *req = NULL;
+
+	struct tsocket_address *ts = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+
+	struct GUID transaction_id;
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+
+	enum event_id_type event_id = EVT_ID_USER_ADDED_TO_GLOBAL_SEC_GROUP;
+
+	struct json_object json;
+	json_t *audit = NULL;
+	json_t *v = NULL;
+	json_t *o = NULL;
+	time_t before;
+
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	ldb = ldb_init(ctx, NULL);
+
+	GUID_from_string(TRANSACTION, &transaction_id);
+
+	module = talloc_zero(ctx, struct ldb_module);
+	module->ldb = ldb;
+
+	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);
+
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	before = time(NULL);
+	json = audit_group_json(module,
+				req,
+				"the-action",
+				"the-user-name",
+				"the-group-name",
+				event_id,
+				LDB_ERR_OPERATIONS_ERROR);
+	assert_int_equal(3, json_object_size(json.root));
+
+	v = json_object_get(json.root, "type");
+	assert_non_null(v);
+	assert_string_equal("groupChange", json_string_value(v));
+
+	v = json_object_get(json.root, "timestamp");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	check_timestamp(before, json_string_value(v));
+
+	audit = json_object_get(json.root, "groupChange");
+	assert_non_null(audit);
+	assert_true(json_is_object(audit));
+	assert_int_equal(11, json_object_size(audit));
+
+	o = json_object_get(audit, "version");
+	assert_non_null(o);
+	check_version(o, AUDIT_MAJOR, AUDIT_MINOR);
+
+	v = json_object_get(audit, "eventId");
+	assert_non_null(v);
+	assert_true(json_is_integer(v));
+	assert_int_equal(
+		EVT_ID_USER_ADDED_TO_GLOBAL_SEC_GROUP,
+		json_integer_value(v));
+
 	v = json_object_get(audit, "statusCode");
 	assert_non_null(v);
 	assert_true(json_is_integer(v));
@@ -858,6 +994,106 @@ static void test_audit_group_json(void **state)
 	TALLOC_FREE(ctx);
 }
 
+static void test_audit_group_json_no_event(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+	struct ldb_request *req = NULL;
+
+	struct tsocket_address *ts = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+
+	struct GUID transaction_id;
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+
+	enum event_id_type event_id = EVT_ID_NONE;
+
+	struct json_object json;
+	json_t *audit = NULL;
+	json_t *v = NULL;
+	json_t *o = NULL;
+	time_t before;
+
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	ldb = ldb_init(ctx, NULL);
+
+	GUID_from_string(TRANSACTION, &transaction_id);
+
+	module = talloc_zero(ctx, struct ldb_module);
+	module->ldb = ldb;
+
+	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);
+
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	before = time(NULL);
+	json = audit_group_json(module,
+				req,
+				"the-action",
+				"the-user-name",
+				"the-group-name",
+				event_id,
+				LDB_SUCCESS);
+	assert_int_equal(3, json_object_size(json.root));
+
+	v = json_object_get(json.root, "type");
+	assert_non_null(v);
+	assert_string_equal("groupChange", json_string_value(v));
+
+	v = json_object_get(json.root, "timestamp");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	check_timestamp(before, json_string_value(v));
+
+	audit = json_object_get(json.root, "groupChange");
+	assert_non_null(audit);
+	assert_true(json_is_object(audit));
+	assert_int_equal(10, json_object_size(audit));
+
+	o = json_object_get(audit, "version");
+	assert_non_null(o);
+	check_version(o, AUDIT_MAJOR, AUDIT_MINOR);
+
+	v = json_object_get(audit, "eventId");
+	assert_null(v);
+
+	v = json_object_get(audit, "statusCode");
+	assert_non_null(v);
+	assert_true(json_is_integer(v));
+	assert_int_equal(LDB_SUCCESS, json_integer_value(v));
+
+	v = json_object_get(audit, "status");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("Success", json_string_value(v));
+
+	v = json_object_get(audit, "user");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-user-name", json_string_value(v));
+
+	v = json_object_get(audit, "group");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-group-name", json_string_value(v));
+
+	v = json_object_get(audit, "action");
+	assert_non_null(v);
+	assert_true(json_is_string(v));
+	assert_string_equal("the-action", json_string_value(v));
+
+	json_free(&json);
+	TALLOC_FREE(ctx);
+}
 static void setup_ldb(
 	TALLOC_CTX *ctx,
 	struct ldb_context **ldb,
@@ -1411,6 +1647,332 @@ static void test_get_remove_member_event(void **state)
 
 	assert_int_equal(EVT_ID_NONE, get_remove_member_event(UINT32_MAX));
 }
+
+/* test log_group_membership_changes
+ *
+ * Happy path test case
+ *
+ */
+static void test_log_group_membership_changes(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 *msg = NULL;
+	struct ldb_message_element *el = NULL;
+	struct audit_callback_context *acc = NULL;
+	struct ldb_result *res = NULL;
+	struct ldb_message *new_msg = NULL;
+	struct ldb_message_element *group_type = NULL;
+	const char *group_type_str = 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 message
+	 */
+	msg = talloc_zero(ctx, struct ldb_message);
+
+	/*
+	 * Populate message elements, adding a new entry to the membership list
+	 *
+	 */
+
+	el = talloc_zero(ctx, struct ldb_message_element);
+	el->name = "member";
+	el->num_values = 1;
+	el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+	msg->elements = el;
+	msg->num_elements = 1;
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation = LDB_ADD;
+	req->op.add.message = msg;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Build the initial state of the database
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->name = "member";
+	old_el->num_values = 1;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+
+	/*
+	 * Build the updated state of the database
+	 */
+	res = talloc_zero(ctx, struct ldb_result);
+	new_msg = talloc_zero(ctx, struct ldb_message);
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->name = "member";
+	new_el->num_values = 2;
+	new_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+	new_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");
+	new_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");
+
+	group_type = talloc_zero(ctx, struct ldb_message_element);
+	group_type->name = "groupType";
+	group_type->num_values = 1;
+	group_type->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	group_type_str = talloc_asprintf(ctx, "%u", GTYPE_SECURITY_GLOBAL_GROUP);
+	group_type->values[0] = data_blob_string_const(group_type_str);
+
+
+	new_msg->elements = talloc_zero_array(ctx, struct ldb_message_element, 2);
+	new_msg->num_elements = 2;
+	new_msg->elements[0] = *new_el;
+	new_msg->elements[1] = *group_type;
+
+	res->count = 1;
+	res->msgs = &new_msg;
+
+	acc = talloc_zero(ctx, struct audit_callback_context);
+	acc->request = req;
+	acc->module = module;
+	acc->members = old_el;
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	g_result = res;
+	g_status = LDB_SUCCESS;
+	log_group_membership_changes(acc, status);
+	g_result = NULL;
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(1, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "CN=testuser131953,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Added",
+	    EVT_ID_USER_ADDED_TO_GLOBAL_SEC_GROUP);
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	TALLOC_FREE(ctx);
+}
+
+/* test log_group_membership_changes
+ *
+ * The ldb query to retrieve the new values failed.
+ *
+ * Should generate group membership change Failure message.
+ *
+ */
+static void test_log_group_membership_changes_read_new_failure(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 *msg = NULL;
+	struct ldb_message_element *el = NULL;
+	struct audit_callback_context *acc = 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 message
+	 */
+	msg = talloc_zero(ctx, struct ldb_message);
+
+	/*
+	 * Populate message elements, adding a new entry to the membership list
+	 *
+	 */
+
+	el = talloc_zero(ctx, struct ldb_message_element);
+	el->name = "member";
+	el->num_values = 1;
+	el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+	msg->elements = el;
+	msg->num_elements = 1;
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation = LDB_ADD;
+	req->op.add.message = msg;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Build the initial state of the database
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->name = "member";
+	old_el->num_values = 1;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+
+	acc = talloc_zero(ctx, struct audit_callback_context);
+	acc->request = req;
+	acc->module = module;
+	acc->members = old_el;
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	g_result = NULL;
+	g_status = LDB_ERR_NO_SUCH_OBJECT;
+	log_group_membership_changes(acc, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(1, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "",
+	    "Failure",
+	    EVT_ID_NONE);
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	TALLOC_FREE(ctx);
+}
+
+/* test log_group_membership_changes
+ *
+ * The operation failed.
+ *
+ * Should generate group membership change Failure message.
+ *
+ */
+static void test_log_group_membership_changes_error(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 *msg = NULL;
+	struct ldb_message_element *el = NULL;
+	struct ldb_message_element *old_el = NULL;
+	struct audit_callback_context *acc = NULL;
+	int status = LDB_ERR_OPERATIONS_ERROR;
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the ldb message
+	 */
+	msg = talloc_zero(ctx, struct ldb_message);
+
+	/*
+	 * Populate message elements, adding a new entry to the membership list
+	 *
+	 */
+
+	el = talloc_zero(ctx, struct ldb_message_element);
+	el->name = "member";
+	el->num_values = 1;
+	el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+	msg->elements = el;
+	msg->num_elements = 1;
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation = LDB_ADD;
+	req->op.add.message = msg;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Build the initial state of the database
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->name = "member";
+	old_el->num_values = 1;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	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");
+
+
+	acc = talloc_zero(ctx, struct audit_callback_context);
+	acc->request = req;
+	acc->module = module;
+	acc->members = old_el;
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	log_group_membership_changes(acc, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(1, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "",
+	    "Failure",
+	    EVT_ID_NONE);
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	TALLOC_FREE(ctx);
+}
+
 /*
  * Note: to run under valgrind us:
  *       valgrind --suppressions=test_group_audit.valgrind bin/test_group_audit
@@ -1421,6 +1983,8 @@ static void test_get_remove_member_event(void **state)
 int main(void) {
 	const struct CMUnitTest tests[] = {
 	    cmocka_unit_test(test_audit_group_json),
+	    cmocka_unit_test(test_audit_group_json_error),
+	    cmocka_unit_test(test_audit_group_json_no_event),
 	    cmocka_unit_test(test_get_transaction_id),
 	    cmocka_unit_test(test_audit_group_hr),
 	    cmocka_unit_test(test_get_parsed_dns),
@@ -1433,6 +1997,9 @@ int main(void) {
 	    cmocka_unit_test(test_log_membership_changes_rmd_flags),
 	    cmocka_unit_test(test_get_add_member_event),
 	    cmocka_unit_test(test_get_remove_member_event),
+	    cmocka_unit_test(test_log_group_membership_changes),
+	    cmocka_unit_test(test_log_group_membership_changes_read_new_failure),
+	    cmocka_unit_test(test_log_group_membership_changes_error),
 	};
 
 	cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
-- 
2.18.1


From f8b0be03017e07394052e2826f5712dd237f79ff Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Tue, 8 Jan 2019 14:24:06 +1300
Subject: [PATCH 2/2] group_audit: error handling in group change

Generate an appropriate log message in the event of an error
log_group_membership_changes.  As the changes have not been applied to
the database, there is no easy way to determine the intended changes.
This information is available in the "dsdbChange" audit messages, to
avoid replicating this logic for what should be a very rare occurrence
we simply log it as a "Failure"

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/group_audit             |  2 --
 source4/dsdb/samdb/ldb_modules/group_audit.c | 31 ++++++++++++++++----
 2 files changed, 25 insertions(+), 8 deletions(-)
 delete mode 100644 selftest/knownfail.d/group_audit

diff --git a/selftest/knownfail.d/group_audit b/selftest/knownfail.d/group_audit
deleted file mode 100644
index 4f5855fea01..00000000000
--- a/selftest/knownfail.d/group_audit
+++ /dev/null
@@ -1,2 +0,0 @@
-^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_group_membership_changes_read_new_failure\(none\)
-^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_group_membership_changes_error\(none\)
diff --git a/source4/dsdb/samdb/ldb_modules/group_audit.c b/source4/dsdb/samdb/ldb_modules/group_audit.c
index 4356046f675..dd991bfbb07 100644
--- a/source4/dsdb/samdb/ldb_modules/group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/group_audit.c
@@ -1012,14 +1012,33 @@ static void log_group_membership_changes(
 			new_val = ldb_msg_find_element(res->msgs[0], "member");
 			group_type = ldb_msg_find_attr_as_uint(
 			    res->msgs[0], "groupType", 0);
+			log_membership_changes(acc->module,
+					       acc->request,
+					       new_val,
+					       acc->members,
+					       group_type,
+					       status);
+			TALLOC_FREE(ctx);
+			return;
 		}
 	}
-	log_membership_changes(acc->module,
-			       acc->request,
-			       new_val,
-			       acc->members,
-			       group_type,
-			       status);
+	/*
+	 * If we get here either
+	 *   one of the lower level modules failed and the group record did
+	 *   not get updated
+	 * or
+	 *   the updated group record could not be read.
+	 *
+	 * In both cases it does not make sense to log individual membership
+	 * changes so we log a group membership change "Failure" message.
+	 *
+	 */
+	log_membership_change(acc->module,
+	                      acc->request,
+			      "Failure",
+			      "",
+			      EVT_ID_NONE,
+			      status);
 	TALLOC_FREE(ctx);
 }
 
-- 
2.18.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190110/c689535c/signature.sig>


More information about the samba-technical mailing list