Cleanup handling of initial auth token

This changes that auth_token_initial is set when the token is
initially generated instead when pushing the token. Even I do not
know anymore why I did it in this way in the first place. Also use
multi->auth_token_initial as source for the sesssion ID since it should
now always be available. Also set auth_token_initial directly to
up->password once we verified that we have gotten a valid token from
a client. This cleans ups the logic in generating the environment and
makes the code flow clearer.

Since the change makes auth_token_initial always available we need to add
a check to only send a PUSH reply to update the token on renegotiations.
The old code relied on multi->auth_token not being set in this case.

This commit also removes the workaround for old OpenVPN clients. These
were only available as commercial OpenVPN Connect client and not in use
anymore.

Furthermore, introduce a check if the session ID has changed during a
session.  Even though this is still a valid authentication changing to
a different auth token mid session is highly irregular and should never
occur naturally.

Patch V2: rebase.
Patch V3: fix formatting, clarifying commit message, remove initial
          token workaround for old v3.
Patch v4: move sending the auth-token for renegotiations to a sane place
          and trigger it when the TLS session reaches its fully authenticated
          state.
Patch v5: Move also setting auth_token_inital from up->password to a more
          logical place, general cleanups, add session id mismatch check
Patch v6: Rework some comments and general cleanup of small things

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210719133132.128783-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22645.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

1 files changed, 40 insertions, 9 deletions

diff --git a/tests/unit_tests/openvpn/test_auth_token.c b/tests/unit_tests/openvpn/test_auth_token.c
index 4030052..6bfad6e 100644
--- a/tests/unit_tests/openvpn/test_auth_token.c
+++ b/tests/unit_tests/openvpn/test_auth_token.c
@@ -174,7 +174,10 @@ auth_token_test_timeout(void **state)
 
     now = 100000;
     generate_auth_token(&ctx->up, &ctx->multi);
+
     strcpy(ctx->up.password, ctx->multi.auth_token);
+    free(ctx->multi.auth_token_initial);
+    ctx->multi.auth_token_initial = NULL;
 
     /* No time has passed */
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
@@ -195,11 +198,6 @@ auth_token_test_timeout(void **state)
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
 
-    /* Check if the mode for a client that never updates its token works */
-    ctx->multi.auth_token_initial = strdup(ctx->up.password);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
-                     AUTH_TOKEN_HMAC_OK);
-
     /* But not when we reached our timeout */
     now = 100000 + ctx->session->opt->auth_token_lifetime + 1;
     assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
@@ -244,10 +242,10 @@ auth_token_test_known_keys(void **state)
 
     now = 0;
     /* Preload the session id so the same session id is used here */
-    ctx->multi.auth_token = strdup(now0key0);
+    ctx->multi.auth_token_initial = strdup(now0key0);
 
     /* Zero the hmac part to ensure we have a newly generated token */
-    zerohmac(ctx->multi.auth_token);
+    zerohmac(ctx->multi.auth_token_initial);
 
     generate_auth_token(&ctx->up, &ctx->multi);
 
@@ -268,6 +266,38 @@ setenv_str(struct env_set *es, const char *name, const char *value)
     }
 }
 
+void
+auth_token_test_session_mismatch(void **state)
+{
+    struct test_context *ctx = (struct test_context *) *state;
+
+    /* Generate first auth token and check it is correct */
+    generate_auth_token(&ctx->up, &ctx->multi);
+    strcpy(ctx->up.password, ctx->multi.auth_token);
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
+                     AUTH_TOKEN_HMAC_OK);
+
+    char *token_sessiona = strdup(ctx->multi.auth_token);
+
+    /* Generate second token */
+    wipe_auth_token(&ctx->multi);
+
+    generate_auth_token(&ctx->up, &ctx->multi);
+    strcpy(ctx->up.password, ctx->multi.auth_token);
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
+                     AUTH_TOKEN_HMAC_OK);
+
+    assert_int_not_equal(0, memcmp(ctx->multi.auth_token_initial + strlen(SESSION_ID_PREFIX),
+                                   token_sessiona + strlen(SESSION_ID_PREFIX),
+                                   AUTH_TOKEN_SESSION_ID_BASE64_LEN));
+
+    /* The first token is valid but should trigger the invalid response since
+     * the session id is not the same */
+    strcpy(ctx->up.password, token_sessiona);
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), 0);
+    free(token_sessiona);
+}
+
 static void
 auth_token_test_empty_user(void **state)
 {
@@ -341,13 +371,13 @@ auth_token_test_random_keys(void **state)
 
     now = 0x5c331e9c;
     /* Preload the session id so the same session id is used here */
-    ctx->multi.auth_token = strdup(random_token);
+    ctx->multi.auth_token_initial = strdup(random_token);
 
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     auth_token_init_secret(&ctx->multi.opt.auth_token_key, random_key, true);
 
     /* Zero the hmac part to ensure we have a newly generated token */
-    zerohmac(ctx->multi.auth_token);
+    zerohmac(ctx->multi.auth_token_initial);
 
     generate_auth_token(&ctx->up, &ctx->multi);
 
@@ -385,6 +415,7 @@ main(void)
         cmocka_unit_test_setup_teardown(auth_token_test_random_keys, setup, teardown),
         cmocka_unit_test_setup_teardown(auth_token_test_key_load, setup, teardown),
         cmocka_unit_test_setup_teardown(auth_token_test_timeout, setup, teardown),
+        cmocka_unit_test_setup_teardown(auth_token_test_session_mismatch, setup, teardown)
     };
 
 #if defined(ENABLE_CRYPTO_OPENSSL)