[PATCH] Prepare for samba-tool domain backup (including fixing chgtdcpass)
Andrew Bartlett
abartlet at samba.org
Fri May 11 03:43:36 UTC 2018
Attached please find some patches developed by Aaron Haslett earlier in
aid of a new samba-tool domain backup command.
The actual command isn't done yet (xattrs need to be sorted out I'm
told) but these preparation patches can go in.
https://gitlab.com/catalyst-samba/samba/commits/abartlet-backuptool-for-master
CI results are here:
https://gitlab.com/catalyst-samba/samba/pipelines/21847265
An important fix here is for this bug
https://bugzilla.samba.org/show_bug.cgi?id=13415
The chgtdcpass script used by some administrators to wipe old keys from
the keytab did not actually work.
Aaron found this out when using this command to test the backup.
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Can I get a second team reviewer please?
Thanks,
Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
-------------- next part --------------
From afe0f693426749413ad4e0c57105f3601b295836 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 11:10:24 +1200
Subject: [PATCH 1/6] auth: keytab invalidation test
chgtdcpass should add a new DC password and delete the old ones but the bug
exposed by this test causes the tool to remove only a single record from
the old entries, leaving the old passwords functional. Since the tool is
used by administrators who may have disclosed their domain join password and
want to invalidate it, this is a security concern.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13415
Signed-off-by: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
selftest/knownfail.d/keytab | 1 +
selftest/tests.py | 2 +
source4/auth/tests/kerberos.c | 103 ++++++++++++++++++++++++++++++++++++++++++
source4/auth/wscript_build | 6 +++
4 files changed, 112 insertions(+)
create mode 100644 selftest/knownfail.d/keytab
create mode 100644 source4/auth/tests/kerberos.c
diff --git a/selftest/knownfail.d/keytab b/selftest/knownfail.d/keytab
new file mode 100644
index 00000000000..6777d98ff28
--- /dev/null
+++ b/selftest/knownfail.d/keytab
@@ -0,0 +1 @@
+^samba.unittests.kerberos.test_krb5_remove_obsolete_keytab_entries_many
diff --git a/selftest/tests.py b/selftest/tests.py
index 185ad37fd4a..f354bb57ef5 100644
--- a/selftest/tests.py
+++ b/selftest/tests.py
@@ -187,5 +187,7 @@ plantestsuite("samba.unittests.tldap", "none",
[os.path.join(bindir(), "default/source3/test_tldap")])
plantestsuite("samba.unittests.rfc1738", "none",
[os.path.join(bindir(), "default/lib/util/test_rfc1738")])
+plantestsuite("samba.unittests.kerberos", "none",
+ [os.path.join(bindir(), "test_kerberos")])
plantestsuite("samba.unittests.ms_fnmatch", "none",
[os.path.join(bindir(), "default/lib/util/test_ms_fnmatch")])
diff --git a/source4/auth/tests/kerberos.c b/source4/auth/tests/kerberos.c
new file mode 100644
index 00000000000..179f8eeefa1
--- /dev/null
+++ b/source4/auth/tests/kerberos.c
@@ -0,0 +1,103 @@
+#include <time.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdint.h>
+#include <cmocka.h>
+
+#include "includes.h"
+#include "system/kerberos.h"
+#include "auth/kerberos/kerberos.h"
+#include "auth/credentials/credentials.h"
+#include "auth/credentials/credentials_proto.h"
+#include "auth/credentials/credentials_krb5.h"
+#include "auth/kerberos/kerberos_credentials.h"
+#include "auth/kerberos/kerberos_util.h"
+
+static void internal_obsolete_keytab_test(int num_principals, int num_kvnos,
+ krb5_kvno kvno, const char *kt_name)
+{
+ krb5_context krb5_ctx;
+ krb5_keytab keytab;
+ krb5_keytab_entry kt_entry;
+ krb5_kt_cursor cursor;
+ krb5_error_code code;
+
+ int i,j;
+ char princ_name[6] = "user0";
+ bool found_previous;
+ const char *error_str;
+
+ TALLOC_CTX *tmp_ctx = talloc_new(NULL);
+ krb5_principal *principals = talloc_zero_array(tmp_ctx,
+ krb5_principal,
+ num_principals);
+ krb5_init_context(&krb5_ctx);
+ krb5_kt_resolve(krb5_ctx, kt_name, &keytab);
+ ZERO_STRUCT(kt_entry);
+
+ for(i=0; i<num_principals; i++){
+ princ_name[4] = (char)i+48;
+ krb5_make_principal(krb5_ctx, &(principals[i]),
+ "samba.example.com", princ_name, NULL);
+ kt_entry.principal = principals[i];
+ for(j=0; j<num_kvnos; j++){
+ kt_entry.vno = j+1;
+ krb5_kt_add_entry(krb5_ctx, keytab, &kt_entry);
+ }
+ }
+
+ code = krb5_kt_start_seq_get(krb5_ctx, keytab, &cursor);
+ assert_int_equal(code, 0);
+ for(i=0; i<num_principals; i++){
+ princ_name[4] = (char)i+48;
+ for(j=0; j<num_kvnos; j++){
+ code = krb5_kt_next_entry(krb5_ctx, keytab,
+ &kt_entry, &cursor);
+ assert_int_equal(code, 0);
+ assert_string_equal(princ_name,
+ *(kt_entry.principal->name.name_string.val));
+ assert_int_equal(kt_entry.vno, j+1);
+ }
+ }
+
+ smb_krb5_remove_obsolete_keytab_entries(tmp_ctx, krb5_ctx, keytab,
+ num_principals, principals,
+ kvno, &found_previous,
+ &error_str);
+
+ code = krb5_kt_start_seq_get(krb5_ctx, keytab, &cursor);
+ assert_int_equal(code, 0);
+ for(i=0; i<num_principals; i++){
+ princ_name[4] = (char)i+48;
+ code = krb5_kt_next_entry(krb5_ctx, keytab, &kt_entry, &cursor);
+ assert_int_equal(code, 0);
+ assert_string_equal(princ_name,
+ *(kt_entry.principal->name.name_string.val));
+ assert_int_equal(kt_entry.vno, kvno-1);
+ }
+ code = krb5_kt_next_entry(krb5_ctx, keytab, &kt_entry, &cursor);
+ assert_int_not_equal(code, 0);
+}
+
+static void test_krb5_remove_obsolete_keytab_entries_many(void **state)
+{
+ internal_obsolete_keytab_test(5, 4, (krb5_kvno)5, "MEMORY:LOL2");
+}
+
+static void test_krb5_remove_obsolete_keytab_entries_one(void **state)
+{
+ internal_obsolete_keytab_test(1, 2, (krb5_kvno)3, "MEMORY:LOL");
+}
+
+int main(int argc, const char **argv)
+{
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test(test_krb5_remove_obsolete_keytab_entries_one),
+ cmocka_unit_test(test_krb5_remove_obsolete_keytab_entries_many),
+ };
+
+ cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
+ return cmocka_run_group_tests(tests, NULL, NULL);
+}
diff --git a/source4/auth/wscript_build b/source4/auth/wscript_build
index f7508619cd7..d3452d2ca92 100644
--- a/source4/auth/wscript_build
+++ b/source4/auth/wscript_build
@@ -42,6 +42,12 @@ bld.SAMBA_SUBSYSTEM('auth4_sam',
deps=''
)
+bld.SAMBA_BINARY('test_kerberos',
+ source='tests/kerberos.c',
+ deps='cmocka authkrb5 krb5samba com_err CREDENTIALS_KRB5',
+ local_include=False,
+ install=False
+ )
for env in bld.gen_python_environments():
pytalloc_util = bld.pyembed_libname('pytalloc-util')
--
2.11.0
From 65c9867f3ea8bbccd1d458a6e5a1a7f1d3134603 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 11:10:50 +1200
Subject: [PATCH 2/6] auth: keytab invalidation fix
chgtdcpass should add a new DC password and delete the old ones but the bug
exposed by this test causes the tool to remove only a single record from
the old entries, leaving the old passwords functional. Since the tool is
used by administrators who may have disclosed their domain join password and
want to invalidate it, this is a security concern.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13415
Signed-off-by: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
selftest/knownfail.d/keytab | 1 -
source4/auth/kerberos/kerberos_util.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
delete mode 100644 selftest/knownfail.d/keytab
diff --git a/selftest/knownfail.d/keytab b/selftest/knownfail.d/keytab
deleted file mode 100644
index 6777d98ff28..00000000000
--- a/selftest/knownfail.d/keytab
+++ /dev/null
@@ -1 +0,0 @@
-^samba.unittests.kerberos.test_krb5_remove_obsolete_keytab_entries_many
diff --git a/source4/auth/kerberos/kerberos_util.c b/source4/auth/kerberos/kerberos_util.c
index 618da626652..50bf8feec96 100644
--- a/source4/auth/kerberos/kerberos_util.c
+++ b/source4/auth/kerberos/kerberos_util.c
@@ -633,7 +633,7 @@ krb5_error_code smb_krb5_remove_obsolete_keytab_entries(TALLOC_CTX *mem_ctx,
krb5_kt_free_entry(context, &entry);
/* Make sure we do not double free */
ZERO_STRUCT(entry);
- } while (code != 0);
+ } while (code == 0);
krb5_kt_end_seq_get(context, keytab, &cursor);
--
2.11.0
From 5329c57fc3096771b0ab51e1332a9453c4d1d6d6 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 11:10:40 +1200
Subject: [PATCH 3/6] ldb: removing prior secret from logs
priorSecret, like secret, can contain a machine account password
(for secrets.ldb) and so should not be printed in a debug
trace.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13353
Signed-off-by: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
lib/ldb-samba/ldif_handlers.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c
index 591bd1ee217..ecc02e51c1d 100644
--- a/lib/ldb-samba/ldif_handlers.c
+++ b/lib/ldb-samba/ldif_handlers.c
@@ -1706,7 +1706,8 @@ const struct ldb_schema_syntax *ldb_samba_syntax_by_lDAPDisplayName(struct ldb_c
return s;
}
-static const char *secret_attributes[] = {DSDB_SECRET_ATTRIBUTES, "secret", NULL};
+static const char *secret_attributes[] = {DSDB_SECRET_ATTRIBUTES, "secret",
+ "priorSecret", NULL};
/*
register the samba ldif handlers
--
2.11.0
From af8ea1c97eba9776c530075973aa1efa79abf9aa Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 15:48:38 +1200
Subject: [PATCH 4/6] samba: read backup date field on init and fail if present
This prevents a backup tar file, created with the new official
backup tools, from being extracted and replicated.
This is done here to ensure that samba-tool and ldbsearch can
still operate on the backup (eg for forensics) but starting
Samba as an AD DC will fail.
Signed-off-by: Aaron Haslett<aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
source4/smbd/server.c | 61 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 49 insertions(+), 12 deletions(-)
diff --git a/source4/smbd/server.c b/source4/smbd/server.c
index ed81c01afc1..78475294796 100644
--- a/source4/smbd/server.c
+++ b/source4/smbd/server.c
@@ -44,6 +44,7 @@
#include "nsswitch/winbind_client.h"
#include "libds/common/roles.h"
#include "lib/util/tfork.h"
+#include "dsdb/samdb/ldb_modules/util.h"
#ifdef HAVE_PTHREAD
#include <pthread.h>
@@ -232,24 +233,48 @@ _NORETURN_ static void max_runtime_handler(struct tevent_context *ev,
pre-open the key databases. This saves a lot of time in child
processes
*/
-static void prime_ldb_databases(struct tevent_context *event_ctx)
+static int prime_ldb_databases(struct tevent_context *event_ctx, bool *am_backup)
{
- TALLOC_CTX *db_context;
- db_context = talloc_new(event_ctx);
-
- samdb_connect(db_context,
- event_ctx,
- cmdline_lp_ctx,
- system_session(cmdline_lp_ctx),
- NULL,
- 0);
+ struct ldb_result *res;
+ struct ldb_dn *samba_dsdb_dn;
+ struct ldb_context *ldb_ctx;
+ static const char *attrs[] = { "backupDate", NULL };
+ const char *msg;
+ int ret;
+ TALLOC_CTX *db_context = talloc_new(event_ctx);;
+
+ ldb_ctx = samdb_connect(db_context,
+ event_ctx,
+ cmdline_lp_ctx,
+ system_session(cmdline_lp_ctx),
+ NULL,
+ 0);
privilege_connect(db_context, cmdline_lp_ctx);
+ samba_dsdb_dn = ldb_dn_new(db_context, ldb_ctx, "@SAMBA_DSDB");
+ if (!samba_dsdb_dn) {
+ talloc_free(db_context);
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+
+ ret = dsdb_search_dn(ldb_ctx, db_context, &res, samba_dsdb_dn, attrs,
+ DSDB_FLAG_AS_SYSTEM);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(db_context);
+ return ret;
+ }
+
+ msg = ldb_msg_find_attr_as_string(res->msgs[0], "backupDate", NULL);
+ if (msg != NULL) {
+ *am_backup = true;
+ }
+ *am_backup = false;
+
+ return LDB_SUCCESS;
/* we deliberately leave these open, which allows them to be
* re-used in ldb_wrap_connect() */
}
-
/*
called when a fatal condition occurs in a child task
*/
@@ -366,7 +391,9 @@ static int binary_smbd_main(const char *binary_name,
bool opt_fork = true;
bool opt_interactive = false;
bool opt_no_process_group = false;
+ bool db_is_backup = false;
int opt;
+ int ret;
poptContext pc;
#define _MODULE_PROTO(init) extern NTSTATUS init(TALLOC_CTX *);
STATIC_service_MODULES_PROTO;
@@ -631,7 +658,17 @@ static int binary_smbd_main(const char *binary_name,
"and exited. Check logs for details", EINVAL);
};
- prime_ldb_databases(state->event_ctx);
+ ret = prime_ldb_databases(state->event_ctx, &db_is_backup);
+ if (ret != LDB_SUCCESS) {
+ TALLOC_FREE(state);
+ exit_daemon("Samba failed to prime database", EINVAL);
+ }
+
+ if (db_is_backup) {
+ TALLOC_FREE(state);
+ exit_daemon("Database is a backup. Please run samba-tool domain"
+ " backup restore", EINVAL);
+ }
status = setup_parent_messaging(state, cmdline_lp_ctx);
if (!NT_STATUS_IS_OK(status)) {
--
2.11.0
From 8bd3b42fbb02cf88bd79b60aaad3115e397a5a0f Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 15:51:10 +1200
Subject: [PATCH 5/6] samdb rid: clear cache to prevent old ntds_guid
During the new samba-tool domain backup restore the NTDS GUID changes
as the server is taken over by the new DC record.
Signed-off-by: Aaron Haslett<aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/ridalloc.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/source4/dsdb/samdb/ldb_modules/ridalloc.c b/source4/dsdb/samdb/ldb_modules/ridalloc.c
index abfe14a5a80..b436b9b5435 100644
--- a/source4/dsdb/samdb/ldb_modules/ridalloc.c
+++ b/source4/dsdb/samdb/ldb_modules/ridalloc.c
@@ -443,6 +443,12 @@ int ridalloc_create_own_rid_set(struct ldb_module *module, TALLOC_CTX *mem_ctx,
return ldb_operr(ldb_module_get_ctx(module));
}
+ /* clear the cache so we don't get an old ntds_guid */
+ if (ldb_set_opaque(ldb, "cache.ntds_guid", NULL) != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ldb_operr(ldb_module_get_ctx(module));
+ }
+
our_ntds_guid = samdb_ntds_objectGUID(ldb_module_get_ctx(module));
if (!our_ntds_guid) {
talloc_free(tmp_ctx);
--
2.11.0
From 5add7dd9d5cb3cfcb9dc38fcae70214c106b6e88 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
Date: Tue, 1 May 2018 15:54:07 +1200
Subject: [PATCH 6/6] devel: removing unused code from chgkrbtgtpass
Signed-off-by: Aaron Haslett<aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
python/samba/upgradehelpers.py | 5 ++---
source4/scripting/devel/chgkrbtgtpass | 5 +----
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/python/samba/upgradehelpers.py b/python/samba/upgradehelpers.py
index d4f69445234..14fe3e024c3 100644
--- a/python/samba/upgradehelpers.py
+++ b/python/samba/upgradehelpers.py
@@ -645,11 +645,10 @@ def update_dns_account_password(samdb, secrets_ldb, names):
secrets_ldb.modify(msg)
-def update_krbtgt_account_password(samdb, names):
+def update_krbtgt_account_password(samdb):
"""Update (change) the password of the krbtgt account
- :param samdb: An LDB object related to the sam.ldb file of a given provision
- :param names: List of key provision parameters"""
+ :param samdb: An LDB object related to the sam.ldb file of a given provision"""
expression = "samAccountName=krbtgt"
res = samdb.search(expression=expression, attrs=[])
diff --git a/source4/scripting/devel/chgkrbtgtpass b/source4/scripting/devel/chgkrbtgtpass
index 7e4f9fb791c..12be1bc3bb1 100644
--- a/source4/scripting/devel/chgkrbtgtpass
+++ b/source4/scripting/devel/chgkrbtgtpass
@@ -56,8 +56,5 @@ session = system_session()
ldbs = get_ldbs(paths, creds, session, lp)
ldbs.startTransactions()
-names = find_provision_key_parameters(ldbs.sam, ldbs.secrets, ldbs.idmap,
- paths, smbconf, lp)
-
-update_krbtgt_account_password(ldbs.sam, names)
+update_krbtgt_account_password(ldbs.sam)
ldbs.groupedCommit()
--
2.11.0
More information about the samba-technical
mailing list