[Patches] The way to remove gensec_update_ev()
Stefan Metzmacher
metze at samba.org
Tue Jul 4 08:41:30 UTC 2017
Hi,
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-gensec-tmp
> (it makes spnego.c fully async) passed private autobuilds a few times now.
>
> I'll try to it up into useful commits tomorrow.
>
> It would be good if you could start reviewing the final spnego.c
> (attached)
>
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-gensec
> (implements the async NTLMSSP server with basic support for trusts) also
> seems to pass, but I need to fix up some things there.
I deferred these, the changes are just too large to rush into 4.7.
Here's a next small chunk ready for master.
Please review and push:-)
Thanks!
metze
-------------- next part --------------
From 9df6a0169693629a87e7f0254e006e083b7997b0 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 30 Jun 2017 11:00:12 +0200
Subject: [PATCH 1/8] auth/spnego: rename gensec_spnego_server_negTokenTarg()
into gensec_spnego_server_response()
gensec_spnego_server_negTokenTarg() will reappear as function that
handles the whole negTokenTarg processing.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 964f44f..3256f76 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -569,12 +569,12 @@ static NTSTATUS gensec_spnego_create_negTokenInit(struct gensec_security *gensec
* This is the case, where the client is the first one who sends data
*/
-static NTSTATUS gensec_spnego_server_negTokenTarg(struct spnego_state *spnego_state,
- TALLOC_CTX *out_mem_ctx,
- NTSTATUS nt_status,
- const DATA_BLOB unwrapped_out,
- DATA_BLOB mech_list_mic,
- DATA_BLOB *out)
+static NTSTATUS gensec_spnego_server_response(struct spnego_state *spnego_state,
+ TALLOC_CTX *out_mem_ctx,
+ NTSTATUS nt_status,
+ const DATA_BLOB unwrapped_out,
+ DATA_BLOB mech_list_mic,
+ DATA_BLOB *out)
{
struct spnego_data spnego_out;
@@ -1106,12 +1106,12 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
mech_list_mic = unwrapped_out;
}
- nt_status = gensec_spnego_server_negTokenTarg(spnego_state,
- out_mem_ctx,
- nt_status,
- unwrapped_out,
- mech_list_mic,
- out);
+ nt_status = gensec_spnego_server_response(spnego_state,
+ out_mem_ctx,
+ nt_status,
+ unwrapped_out,
+ mech_list_mic,
+ out);
spnego_free_data(&spnego);
@@ -1248,12 +1248,12 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
}
server_response:
- nt_status = gensec_spnego_server_negTokenTarg(spnego_state,
- out_mem_ctx,
- nt_status,
- unwrapped_out,
- mech_list_mic,
- out);
+ nt_status = gensec_spnego_server_response(spnego_state,
+ out_mem_ctx,
+ nt_status,
+ unwrapped_out,
+ mech_list_mic,
+ out);
spnego_free_data(&spnego);
--
1.9.1
From 48f7d117edb8c41b979d2099311078c706ef41ff Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 14 Jun 2017 03:33:21 +0200
Subject: [PATCH 2/8] auth/spnego: use a helper variable for
spnego.negTokenInit.targetPrincipal
This makes the lines a bit shorter and the future diff easier to review.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 3256f76..831dafd 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -641,6 +641,7 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
const char *my_mechs[] = {NULL, NULL};
NTSTATUS nt_status = NT_STATUS_INVALID_PARAMETER;
bool ok;
+ const char *tp = NULL;
if (!in.length) {
/* client to produce negTokenInit */
@@ -668,11 +669,11 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
return NT_STATUS_INVALID_PARAMETER;
}
- if (spnego.negTokenInit.targetPrincipal
- && strcmp(spnego.negTokenInit.targetPrincipal, ADS_IGNORE_PRINCIPAL) != 0) {
- DEBUG(5, ("Server claims it's principal name is %s\n", spnego.negTokenInit.targetPrincipal));
+ tp = spnego.negTokenInit.targetPrincipal;
+ if (tp != NULL && strcmp(tp, ADS_IGNORE_PRINCIPAL) != 0) {
+ DEBUG(5, ("Server claims it's principal name is %s\n", tp));
if (lpcfg_client_use_spnego_principal(gensec_security->settings->lp_ctx)) {
- gensec_set_target_principal(gensec_security, spnego.negTokenInit.targetPrincipal);
+ gensec_set_target_principal(gensec_security, tp);
}
}
--
1.9.1
From 0915ca1479cd2c75d5c56413eb7d860836520275 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 14 Jun 2017 03:36:22 +0200
Subject: [PATCH 3/8] auth/spnego: add a struct spnego_negTokenTarg *ta
variable to make some lines shorter
This makes future modifications easier to review.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 831dafd..4dcb927 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -723,6 +723,7 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
case SPNEGO_CLIENT_TARG:
{
NTSTATUS nt_status = NT_STATUS_INTERNAL_ERROR;
+ const struct spnego_negTokenTarg *ta = NULL;
if (!in.length) {
return NT_STATUS_INVALID_PARAMETER;
@@ -744,11 +745,11 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
spnego_free_data(&spnego);
return NT_STATUS_INVALID_PARAMETER;
}
+ ta = &spnego.negTokenTarg;
spnego_state->num_targs++;
- if (spnego.negTokenTarg.negResult == SPNEGO_REJECT) {
- spnego_free_data(&spnego);
+ if (ta->negResult == SPNEGO_REJECT) {
return NT_STATUS_LOGON_FAILURE;
}
@@ -757,13 +758,13 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
}
/* Server didn't like our choice of mech, and chose something else */
- if (((spnego.negTokenTarg.negResult == SPNEGO_ACCEPT_INCOMPLETE) ||
- (spnego.negTokenTarg.negResult == SPNEGO_REQUEST_MIC)) &&
- spnego.negTokenTarg.supportedMech &&
- strcmp(spnego.negTokenTarg.supportedMech, spnego_state->neg_oid) != 0) {
+ if (((ta->negResult == SPNEGO_ACCEPT_INCOMPLETE) ||
+ (ta->negResult == SPNEGO_REQUEST_MIC)) &&
+ ta->supportedMech != NULL&&
+ strcmp(ta->supportedMech, spnego_state->neg_oid) != 0) {
DEBUG(3,("GENSEC SPNEGO: client preferred mech (%s) not accepted, server wants: %s\n",
gensec_get_name_by_oid(gensec_security, spnego_state->neg_oid),
- gensec_get_name_by_oid(gensec_security, spnego.negTokenTarg.supportedMech)));
+ gensec_get_name_by_oid(gensec_security, ta->supportedMech)));
spnego_state->downgraded = true;
gensec_spnego_update_sub_abort(spnego_state);
nt_status = gensec_subcontext_start(spnego_state,
@@ -775,14 +776,14 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
}
/* select the sub context */
nt_status = gensec_start_mech_by_oid(spnego_state->sub_sec_security,
- spnego.negTokenTarg.supportedMech);
+ ta->supportedMech);
if (!NT_STATUS_IS_OK(nt_status)) {
spnego_free_data(&spnego);
return nt_status;
}
spnego_state->neg_oid = talloc_strdup(spnego_state,
- spnego.negTokenTarg.supportedMech);
+ ta->supportedMech);
if (spnego_state->neg_oid == NULL) {
spnego_free_data(&spnego);
return NT_STATUS_NO_MEMORY;
@@ -1032,7 +1033,7 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
/* all done - server has accepted, and we agree */
*out = data_blob_null;
- if (spnego.negTokenTarg.negResult != SPNEGO_ACCEPT_COMPLETED) {
+ if (ta->negResult != SPNEGO_ACCEPT_COMPLETED) {
/* unless of course it did not accept */
DEBUG(1,("gensec_update ok but not accepted\n"));
nt_status = NT_STATUS_INVALID_PARAMETER;
--
1.9.1
From 8857fcd067637de214c3a61c8dee071e61d8acb8 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 14 Jun 2017 02:46:29 +0200
Subject: [PATCH 4/8] auth/spnego: don't pass 'in' to
gensec_spnego_create_negTokenInit()
It's always en empty blob.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 4dcb927..65aa569 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -438,7 +438,7 @@ static NTSTATUS gensec_spnego_create_negTokenInit(struct gensec_security *gensec
struct spnego_state *spnego_state,
TALLOC_CTX *out_mem_ctx,
struct tevent_context *ev,
- const DATA_BLOB in, DATA_BLOB *out)
+ DATA_BLOB *out)
{
int i;
NTSTATUS nt_status = NT_STATUS_INVALID_PARAMETER;
@@ -646,7 +646,7 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
if (!in.length) {
/* client to produce negTokenInit */
nt_status = gensec_spnego_create_negTokenInit(gensec_security, spnego_state,
- out_mem_ctx, ev, in, out);
+ out_mem_ctx, ev, out);
spnego_state->state_position = SPNEGO_CLIENT_TARG;
spnego_state->expected_packet = SPNEGO_NEG_TOKEN_TARG;
return nt_status;
@@ -1120,7 +1120,7 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
return nt_status;
} else {
nt_status = gensec_spnego_create_negTokenInit(gensec_security, spnego_state,
- out_mem_ctx, ev, in, out);
+ out_mem_ctx, ev, out);
spnego_state->state_position = SPNEGO_SERVER_START;
spnego_state->expected_packet = SPNEGO_NEG_TOKEN_INIT;
return nt_status;
--
1.9.1
From edebd64d47037c4b998e4f14059a5bf4df22910d Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 29 Jun 2017 16:55:09 +0200
Subject: [PATCH 5/8] auth/spnego: set
spnego_state->{state_position,expected_packet}
gensec_spnego_create_negTokenInit()
We should only do the state change in a defined place
and not with any error gensec_spnego_create_negTokenInit() might return.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 65aa569..594de18 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -555,6 +555,14 @@ static NTSTATUS gensec_spnego_create_negTokenInit(struct gensec_security *gensec
/* set next state */
spnego_state->neg_oid = all_sec[i].oid;
+ if (spnego_state->state_position == SPNEGO_SERVER_START) {
+ spnego_state->state_position = SPNEGO_SERVER_START;
+ spnego_state->expected_packet = SPNEGO_NEG_TOKEN_INIT;
+ } else {
+ spnego_state->state_position = SPNEGO_CLIENT_TARG;
+ spnego_state->expected_packet = SPNEGO_NEG_TOKEN_TARG;
+ }
+
return NT_STATUS_MORE_PROCESSING_REQUIRED;
}
gensec_spnego_update_sub_abort(spnego_state);
@@ -645,11 +653,10 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
if (!in.length) {
/* client to produce negTokenInit */
- nt_status = gensec_spnego_create_negTokenInit(gensec_security, spnego_state,
- out_mem_ctx, ev, out);
- spnego_state->state_position = SPNEGO_CLIENT_TARG;
- spnego_state->expected_packet = SPNEGO_NEG_TOKEN_TARG;
- return nt_status;
+ return gensec_spnego_create_negTokenInit(gensec_security,
+ spnego_state,
+ out_mem_ctx,
+ ev, out);
}
len = spnego_read_data(gensec_security, in, &spnego);
@@ -1119,11 +1126,10 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
return nt_status;
} else {
- nt_status = gensec_spnego_create_negTokenInit(gensec_security, spnego_state,
- out_mem_ctx, ev, out);
- spnego_state->state_position = SPNEGO_SERVER_START;
- spnego_state->expected_packet = SPNEGO_NEG_TOKEN_INIT;
- return nt_status;
+ return gensec_spnego_create_negTokenInit(gensec_security,
+ spnego_state,
+ out_mem_ctx,
+ ev, out);
}
}
--
1.9.1
From 8eb8062f8b4ddce607d3f25975e3a510599be664 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 13 Jun 2017 23:55:00 +0200
Subject: [PATCH 6/8] auth/spnego: move SERVER
gensec_spnego_create_negTokenInit() handling to the top
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 594de18..8e25328 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -1077,8 +1077,15 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
case SPNEGO_SERVER_START:
{
NTSTATUS nt_status;
- if (in.length) {
+ if (in.length == 0) {
+ return gensec_spnego_create_negTokenInit(gensec_security,
+ spnego_state,
+ out_mem_ctx,
+ ev, out);
+ }
+
+ {
len = spnego_read_data(gensec_security, in, &spnego);
if (len == -1) {
return gensec_spnego_server_try_fallback(gensec_security, spnego_state,
@@ -1125,11 +1132,6 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
spnego_free_data(&spnego);
return nt_status;
- } else {
- return gensec_spnego_create_negTokenInit(gensec_security,
- spnego_state,
- out_mem_ctx,
- ev, out);
}
}
--
1.9.1
From d2bd345a84404ed18371e36ff71f84b7dc0151f3 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 13 Jun 2017 23:56:47 +0200
Subject: [PATCH 7/8] auth/spnego: remove useless indentation level for
SPNEGO_SERVER_START
Check with git show -w
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 82 +++++++++++++++++++++++++---------------------------
1 file changed, 40 insertions(+), 42 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index 8e25328..c66cb40 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -1085,54 +1085,52 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
ev, out);
}
- {
- len = spnego_read_data(gensec_security, in, &spnego);
- if (len == -1) {
- return gensec_spnego_server_try_fallback(gensec_security, spnego_state,
- ev, out_mem_ctx, in, out);
- }
- /* client sent NegTargetInit, we send NegTokenTarg */
+ len = spnego_read_data(gensec_security, in, &spnego);
+ if (len == -1) {
+ return gensec_spnego_server_try_fallback(gensec_security, spnego_state,
+ ev, out_mem_ctx, in, out);
+ }
+ /* client sent NegTargetInit, we send NegTokenTarg */
- /* OK, so it's real SPNEGO, check the packet's the one we expect */
- if (spnego.type != spnego_state->expected_packet) {
- DEBUG(1, ("Invalid SPNEGO request: %d, expected %d\n", spnego.type,
- spnego_state->expected_packet));
- dump_data(1, in.data, in.length);
- spnego_free_data(&spnego);
- return NT_STATUS_INVALID_PARAMETER;
- }
+ /* OK, so it's real SPNEGO, check the packet's the one we expect */
+ if (spnego.type != spnego_state->expected_packet) {
+ DEBUG(1, ("Invalid SPNEGO request: %d, expected %d\n", spnego.type,
+ spnego_state->expected_packet));
+ dump_data(1, in.data, in.length);
+ spnego_free_data(&spnego);
+ return NT_STATUS_INVALID_PARAMETER;
+ }
- nt_status = gensec_spnego_parse_negTokenInit(gensec_security,
- spnego_state,
- out_mem_ctx,
- ev,
- spnego.negTokenInit.mechTypes,
- spnego.negTokenInit.mechToken,
- &unwrapped_out);
+ nt_status = gensec_spnego_parse_negTokenInit(gensec_security,
+ spnego_state,
+ out_mem_ctx,
+ ev,
+ spnego.negTokenInit.mechTypes,
+ spnego.negTokenInit.mechToken,
+ &unwrapped_out);
- if (spnego_state->simulate_w2k) {
- /*
- * Windows 2000 returns the unwrapped token
- * also in the mech_list_mic field.
- *
- * In order to verify our client code,
- * we need a way to have a server with this
- * broken behaviour
- */
- mech_list_mic = unwrapped_out;
- }
+ if (spnego_state->simulate_w2k) {
+ /*
+ * Windows 2000 returns the unwrapped token
+ * also in the mech_list_mic field.
+ *
+ * In order to verify our client code,
+ * we need a way to have a server with this
+ * broken behaviour
+ */
+ mech_list_mic = unwrapped_out;
+ }
- nt_status = gensec_spnego_server_response(spnego_state,
- out_mem_ctx,
- nt_status,
- unwrapped_out,
- mech_list_mic,
- out);
+ nt_status = gensec_spnego_server_response(spnego_state,
+ out_mem_ctx,
+ nt_status,
+ unwrapped_out,
+ mech_list_mic,
+ out);
- spnego_free_data(&spnego);
+ spnego_free_data(&spnego);
- return nt_status;
- }
+ return nt_status;
}
case SPNEGO_SERVER_TARG:
--
1.9.1
From e3038b8bd9f0076522cb644b19173be5278df471 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 30 Dec 2016 16:06:49 +0100
Subject: [PATCH 8/8] auth/spnego: pass spnego_in to
gensec_spnego_parse_negTokenInit()
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
auth/gensec/spnego.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c
index c66cb40..6168c93 100644
--- a/auth/gensec/spnego.c
+++ b/auth/gensec/spnego.c
@@ -212,15 +212,24 @@ static NTSTATUS gensec_spnego_parse_negTokenInit(struct gensec_security *gensec_
struct spnego_state *spnego_state,
TALLOC_CTX *out_mem_ctx,
struct tevent_context *ev,
- const char * const *mechType,
- const DATA_BLOB unwrapped_in, DATA_BLOB *unwrapped_out)
+ struct spnego_data *spnego_in,
+ DATA_BLOB *unwrapped_out)
{
int i;
NTSTATUS nt_status = NT_STATUS_INVALID_PARAMETER;
+ const char * const *mechType = NULL;
+ DATA_BLOB unwrapped_in = data_blob_null;
bool ok;
+ const struct gensec_security_ops_wrapper *all_sec = NULL;
- const struct gensec_security_ops_wrapper *all_sec
- = gensec_security_by_oid_list(gensec_security,
+ if (spnego_in->type != SPNEGO_NEG_TOKEN_INIT) {
+ return NT_STATUS_INTERNAL_ERROR;
+ }
+
+ mechType = spnego_in->negTokenInit.mechTypes;
+ unwrapped_in = spnego_in->negTokenInit.mechToken;
+
+ all_sec = gensec_security_by_oid_list(gensec_security,
out_mem_ctx,
mechType,
GENSEC_OID_SPNEGO);
@@ -310,6 +319,7 @@ static NTSTATUS gensec_spnego_parse_negTokenInit(struct gensec_security *gensec_
/* Having tried any optimistic token from the client (if we
* were the server), if we didn't get anywhere, walk our list
* in our preference order */
+ unwrapped_in = data_blob_null;
if (!spnego_state->sub_sec_security) {
for (i=0; all_sec && all_sec[i].op; i++) {
@@ -336,7 +346,7 @@ static NTSTATUS gensec_spnego_parse_negTokenInit(struct gensec_security *gensec_
nt_status = gensec_update_ev(spnego_state->sub_sec_security,
out_mem_ctx,
ev,
- data_blob_null,
+ unwrapped_in,
unwrapped_out);
if (NT_STATUS_IS_OK(nt_status)) {
spnego_state->sub_sec_ready = true;
@@ -688,8 +698,7 @@ static NTSTATUS gensec_spnego_update_client(struct gensec_security *gensec_secur
spnego_state,
out_mem_ctx,
ev,
- spnego.negTokenInit.mechTypes,
- spnego.negTokenInit.mechToken,
+ &spnego,
&unwrapped_out);
if (!NT_STATUS_EQUAL(nt_status, NT_STATUS_MORE_PROCESSING_REQUIRED) && !NT_STATUS_IS_OK(nt_status)) {
@@ -1105,8 +1114,7 @@ static NTSTATUS gensec_spnego_update_server(struct gensec_security *gensec_secur
spnego_state,
out_mem_ctx,
ev,
- spnego.negTokenInit.mechTypes,
- spnego.negTokenInit.mechToken,
+ &spnego,
&unwrapped_out);
if (spnego_state->simulate_w2k) {
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170704/30ad935d/signature.sig>
More information about the samba-technical
mailing list