[PATCH] Simplify auth_check_ntlm_password
Volker Lendecke
vl at samba.org
Tue Mar 7 13:28:47 UTC 2017
Hi!
While working on the auth code I had a hard time following the logic
in auth_check_ntlm_password. The attached patchset is supposed to make
that more traceable. It does not change behaviour, those patches will
come at a later step and will make the routine even simpler. The
behaviour change will be to always break out of the loop if
!NT_STATUS_EQUAL(nt_status,NT_STATUS_NOT_IMPLEMENTED);
Review appreciated!
Thanks, Volker
-------------- next part --------------
>From 7929e637e9997a8d1f0e1ffd18f8b5cb426ba4da Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:24:22 +0100
Subject: [PATCH 1/6] auth3: Centralize auth_check_ntlm_password failure
handling
Preparation for simplified talloc handling. Slight behaviour change:
We now ZERO_STRUCTP(pserver_info) in all failure cases.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 50d0188..7d0d4c0 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -182,7 +182,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
if (auth_context->challenge.length != 8) {
DEBUG(0, ("check_ntlm_password: Invalid challenge stored for this auth context - cannot continue\n"));
- return NT_STATUS_LOGON_FAILURE;
+ nt_status = NT_STATUS_LOGON_FAILURE;
+ goto fail;
}
if (auth_context->challenge_set_by)
@@ -202,8 +203,11 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
#endif
/* This needs to be sorted: If it doesn't match, what should we do? */
- if (!check_domain_match(user_info->client.account_name, user_info->mapped.domain_name))
- return NT_STATUS_LOGON_FAILURE;
+ if (!check_domain_match(user_info->client.account_name,
+ user_info->mapped.domain_name)) {
+ nt_status = NT_STATUS_LOGON_FAILURE;
+ goto fail;
+ }
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
struct auth_serversupplied_info *server_info;
@@ -275,7 +279,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
rhost = tsocket_address_inet_addr_string(user_info->remote_host,
talloc_tos());
if (rhost == NULL) {
- return NT_STATUS_NO_MEMORY;
+ nt_status = NT_STATUS_NO_MEMORY;
+ goto fail;
}
} else {
rhost = "127.0.0.1";
@@ -308,6 +313,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
return nt_status;
}
+fail:
+
/* failed authentication; check for guest lapping */
DEBUG(2, ("check_ntlm_password: Authentication for user [%s] -> [%s] FAILED with error %s\n",
--
1.9.1
>From bcad36e4ffe182f8b0c9a9fd47c4d246b7f38ea4 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 19 Feb 2017 14:23:58 +0100
Subject: [PATCH 2/6] auth3: Use talloc_move instead of _steal
That's the more "modern" way to steal
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 7d0d4c0..c5392a5 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -257,7 +257,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- *pserver_info = talloc_steal(mem_ctx, server_info);
+ *pserver_info = talloc_move(mem_ctx, &server_info);
TALLOC_FREE(tmp_ctx);
break;
}
--
1.9.1
>From c555f3f1ceb0a40eb73e081b1868b139519d6c80 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:26:09 +0100
Subject: [PATCH 3/6] auth3: Simplify auth_check_ntlm_password talloc handling
Use talloc_stackframe and talloc_tos. Don't bother to talloc_free
within the loop, we don't have many iterations.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index c5392a5..2f84c70 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -165,6 +165,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
const struct auth_usersupplied_info *user_info,
struct auth_serversupplied_info **pserver_info)
{
+ TALLOC_CTX *frame;
/* if all the modules say 'not for me' this is reasonable */
NTSTATUS nt_status = NT_STATUS_NO_SUCH_USER;
const char *unix_username;
@@ -174,6 +175,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
return NT_STATUS_LOGON_FAILURE;
}
+ frame = talloc_stackframe();
+
DEBUG(3, ("check_ntlm_password: Checking password for unmapped user [%s]\\[%s]@[%s] with the new password interface\n",
user_info->client.domain_name, user_info->client.account_name, user_info->workstation_name));
@@ -211,7 +214,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
struct auth_serversupplied_info *server_info;
- TALLOC_CTX *tmp_ctx;
NTSTATUS result;
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY
@@ -219,23 +221,15 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
continue;
}
- tmp_ctx = talloc_named(mem_ctx,
- 0,
- "%s authentication for user %s\\%s",
- auth_method->name,
- user_info->mapped.domain_name,
- user_info->client.account_name);
-
result = auth_method->auth(auth_context,
auth_method->private_data,
- tmp_ctx,
+ talloc_tos(),
user_info,
&server_info);
/* check if the module did anything */
if (NT_STATUS_EQUAL(result, NT_STATUS_NOT_IMPLEMENTED)) {
DEBUG(10,("check_ntlm_password: %s had nothing to say\n", auth_method->name));
- TALLOC_FREE(tmp_ctx);
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY) {
/* we don't expose the NT_STATUS_NOT_IMPLEMENTED
* internals, except when the caller is only probing
@@ -258,11 +252,8 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
if (NT_STATUS_IS_OK(nt_status)) {
*pserver_info = talloc_move(mem_ctx, &server_info);
- TALLOC_FREE(tmp_ctx);
break;
}
-
- TALLOC_FREE(tmp_ctx);
}
/* successful authentication */
@@ -310,6 +301,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
unix_username));
}
+ TALLOC_FREE(frame);
return nt_status;
}
@@ -322,6 +314,8 @@ fail:
nt_errstr(nt_status)));
ZERO_STRUCTP(pserver_info);
+ TALLOC_FREE(frame);
+
return nt_status;
}
--
1.9.1
>From 51eec7c11e5199029aa647e6a65b06539b21f917 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:34:58 +0100
Subject: [PATCH 4/6] auth3: Simplify auth_check_ntlm_password server_info
handling
Instead of directly assigning (*pserver_info), work on a local copy
first and assign it once when successful
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 2f84c70..437cca7 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -170,6 +170,7 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
NTSTATUS nt_status = NT_STATUS_NO_SUCH_USER;
const char *unix_username;
auth_methods *auth_method;
+ struct auth_serversupplied_info *server_info;
if (user_info == NULL || auth_context == NULL || pserver_info == NULL) {
return NT_STATUS_LOGON_FAILURE;
@@ -213,7 +214,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
for (auth_method = auth_context->auth_method_list;auth_method; auth_method = auth_method->next) {
- struct auth_serversupplied_info *server_info;
NTSTATUS result;
if (user_info->flags & USER_INFO_LOCAL_SAM_ONLY
@@ -251,7 +251,6 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- *pserver_info = talloc_move(mem_ctx, &server_info);
break;
}
}
@@ -259,11 +258,11 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
/* successful authentication */
if (NT_STATUS_IS_OK(nt_status)) {
- unix_username = (*pserver_info)->unix_name;
+ unix_username = server_info->unix_name;
/* We skip doing this step if the caller asked us not to */
if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
- && !(*pserver_info)->guest) {
+ && !(server_info->guest)) {
const char *rhost;
if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
@@ -293,12 +292,14 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG((*pserver_info)->guest ? 5 : 2,
+ DEBUG(server_info->guest ? 5 : 2,
("check_ntlm_password: %sauthentication for user [%s] -> [%s] -> [%s] succeeded\n",
- (*pserver_info)->guest ? "guest " : "",
+ server_info->guest ? "guest " : "",
user_info->client.account_name,
user_info->mapped.account_name,
unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
}
TALLOC_FREE(frame);
--
1.9.1
>From 1999a13abf552dfb7f8d0849896c20b970d52cca Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:38:56 +0100
Subject: [PATCH 5/6] auth3: Simplify auth_check_ntlm_password logic with a
"goto fail"
No intended code change, just reformatting and a goto fail with
inverted logic
Best viewed with "git show -b" :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 83 +++++++++++++++++++++++++++--------------------------
1 file changed, 43 insertions(+), 40 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 437cca7..249d0cb 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -257,55 +257,58 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
/* successful authentication */
- if (NT_STATUS_IS_OK(nt_status)) {
- unix_username = server_info->unix_name;
-
- /* We skip doing this step if the caller asked us not to */
- if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
- && !(server_info->guest)) {
- const char *rhost;
-
- if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
- rhost = tsocket_address_inet_addr_string(user_info->remote_host,
- talloc_tos());
- if (rhost == NULL) {
- nt_status = NT_STATUS_NO_MEMORY;
- goto fail;
- }
- } else {
- rhost = "127.0.0.1";
- }
+ if (!NT_STATUS_IS_OK(nt_status)) {
+ goto fail;
+ }
- /* We might not be root if we are an RPC call */
- become_root();
- nt_status = smb_pam_accountcheck(unix_username,
- rhost);
- unbecome_root();
+ unix_username = server_info->unix_name;
- if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(5, ("check_ntlm_password: PAM Account for user [%s] succeeded\n",
- unix_username));
- } else {
- DEBUG(3, ("check_ntlm_password: PAM Account for user [%s] FAILED with error %s\n",
- unix_username, nt_errstr(nt_status)));
- }
+ /* We skip doing this step if the caller asked us not to */
+ if (!(user_info->flags & USER_INFO_INFO3_AND_NO_AUTHZ)
+ && !(server_info->guest)) {
+ const char *rhost;
+
+ if (tsocket_address_is_inet(user_info->remote_host, "ip")) {
+ rhost = tsocket_address_inet_addr_string(
+ user_info->remote_host, talloc_tos());
+ if (rhost == NULL) {
+ nt_status = NT_STATUS_NO_MEMORY;
+ goto fail;
+ }
+ } else {
+ rhost = "127.0.0.1";
}
+ /* We might not be root if we are an RPC call */
+ become_root();
+ nt_status = smb_pam_accountcheck(unix_username, rhost);
+ unbecome_root();
+
if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(server_info->guest ? 5 : 2,
- ("check_ntlm_password: %sauthentication for user [%s] -> [%s] -> [%s] succeeded\n",
- server_info->guest ? "guest " : "",
- user_info->client.account_name,
- user_info->mapped.account_name,
- unix_username));
-
- *pserver_info = talloc_move(mem_ctx, &server_info);
+ DEBUG(5, ("check_ntlm_password: PAM Account for user [%s] "
+ "succeeded\n", unix_username));
+ } else {
+ DEBUG(3, ("check_ntlm_password: PAM Account for user [%s] "
+ "FAILED with error %s\n",
+ unix_username, nt_errstr(nt_status)));
}
+ }
- TALLOC_FREE(frame);
- return nt_status;
+ if (NT_STATUS_IS_OK(nt_status)) {
+ DEBUG(server_info->guest ? 5 : 2,
+ ("check_ntlm_password: %sauthentication for user "
+ "[%s] -> [%s] -> [%s] succeeded\n",
+ server_info->guest ? "guest " : "",
+ user_info->client.account_name,
+ user_info->mapped.account_name,
+ unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
}
+ TALLOC_FREE(frame);
+ return nt_status;
+
fail:
/* failed authentication; check for guest lapping */
--
1.9.1
>From 5bf07fd93a85adb8711a6c4e9f6f604921acb9b2 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 11 Feb 2017 11:38:56 +0100
Subject: [PATCH 6/6] auth3: Simplify auth_check_ntlm_password logic with a
"goto fail"
No intended code change, just reformatting and a goto fail with
inverted logic
Best viewed with "git show -b"
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/auth/auth.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/source3/auth/auth.c b/source3/auth/auth.c
index 249d0cb..1cbe46e 100644
--- a/source3/auth/auth.c
+++ b/source3/auth/auth.c
@@ -294,20 +294,22 @@ NTSTATUS auth_check_ntlm_password(TALLOC_CTX *mem_ctx,
}
}
- if (NT_STATUS_IS_OK(nt_status)) {
- DEBUG(server_info->guest ? 5 : 2,
- ("check_ntlm_password: %sauthentication for user "
- "[%s] -> [%s] -> [%s] succeeded\n",
- server_info->guest ? "guest " : "",
- user_info->client.account_name,
- user_info->mapped.account_name,
- unix_username));
-
- *pserver_info = talloc_move(mem_ctx, &server_info);
+ if (!NT_STATUS_IS_OK(nt_status)) {
+ goto fail;
}
+ DEBUG(server_info->guest ? 5 : 2,
+ ("check_ntlm_password: %sauthentication for user "
+ "[%s] -> [%s] -> [%s] succeeded\n",
+ server_info->guest ? "guest " : "",
+ user_info->client.account_name,
+ user_info->mapped.account_name,
+ unix_username));
+
+ *pserver_info = talloc_move(mem_ctx, &server_info);
+
TALLOC_FREE(frame);
- return nt_status;
+ return NT_STATUS_OK;
fail:
--
1.9.1
More information about the samba-technical
mailing list