Hi Arne,
after our discussion on IRC I understood you expected this patch to not
change the server behaviour.
If something is suboptimal, it means it was suboptimal also before this
patch.
However, with your patch I can clearly see a longer delay in returning
the AUTH result to the client, in case of deferred auth.
Without your patch (initial lines are the output of my auth-verify script):
PASS=testno
FILE=/tmp/openvpn_acf_662a1165ab3293aa3fb053050e1c2463.tmp
FAIL
2021-05-06 23:40:28 10.10.10.2:1194 TLS: Username/Password
authentication deferred for username 'test'
2021-05-06 23:40:28 10.10.10.2:1194 Control Channel: TLSv1.3, cipher
TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384 bit EC, curve
secp384r1, signature: ecdsa-with-SHA256
2021-05-06 23:40:28 10.10.10.2:1194 [client] Peer Connection Initiated
with [AF_INET6]::ffff:10.10.10.2:1194
2021-05-06 23:40:29 10.10.10.2:1194 PUSH: Received control message:
'PUSH_REQUEST'
2021-05-06 23:40:29 10.10.10.2:1194 Delayed exit in 5 seconds
2021-05-06 23:40:29 10.10.10.2:1194 SENT CONTROL [client]: 'AUTH_FAILED'
(status=1)
My script uses the deferred logic, but it is super fast (as you can see
it fails before the server can even do anything else).
Later, as soon as the first PUSH_REQUEST is received, the server replies
immediately with an AUTH_FAILED.
With your patch (script and everything else stays the same):
PASS=testno
FILE=/tmp/openvpn_acf_4cdab09a9640e8eb4cacadfc1734bb2c.tmp
FAIL
2021-05-06 23:45:22 10.10.10.2:1194 TLS: Username/Password
authentication deferred for username 'test'
2021-05-06 23:45:22 10.10.10.2:1194 Control Channel: TLSv1.3, cipher
TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384 bit EC, curve
secp384r1, signature: ecdsa-with-SHA256
2021-05-06 23:45:22 10.10.10.2:1194 [client] Peer Connection Initiated
with [AF_INET6]::ffff:10.10.10.2:1194
2021-05-06 23:45:23 10.10.10.2:1194 PUSH: Received control message:
'PUSH_REQUEST'
2021-05-06 23:45:28 10.10.10.2:1194 PUSH: Received control message:
'PUSH_REQUEST'
2021-05-06 23:45:33 10.10.10.2:1194 Delayed exit in 5 seconds
2021-05-06 23:45:33 10.10.10.2:1194 SENT CONTROL [client]: 'AUTH_FAILED'
(status=1)
As you can see the script does the deferred auth immediately, but the
server send the AUTH_FAILED only after the PUSH_REQUEST being received
*after some (10?) seconds since the failure of the script*.
To me this sounds like the server is caching the current status for some
(10?) seconds and it is not updating it with the actual result.
However, this does not seem to happen without your patch as the server
is probably forcing a reload of the result from file everytime a
PUSH_REQUEST is received.
I believe this behaviour should stay the same.
Regards,
On 06/05/2021 16:12, Arne Schwabe wrote:
> tls_authentication_status does caching to avoid file I/O more than
> every TLS_MULTI_AUTH_STATUS_INTERVAL (10s) per connection. But
> counter-intuitively it does not return the cached result but rather
> TLS_AUTHENTICATION_UNDEFINED if the cache is not refreshed by the call.
>
> This is workarounded by forcing a refresh in some areas of the code
> (latency = 0).
>
> This patch changes the behaviour by always returning the last known
> status and only updating the file status when the i/o timeout for the
> caches is reached.
>
> The old logic in send_auth_failed is fragile in the sense that if
> it is called again while an exit is scheduled it will reset the timer
> to 5s again. Since we now always report the status from
> tls_authentication_status() instead only every 10s, this caused OpenVPN
> to infinitively reset the timer. Fix this by only setting the status
> if no exit is scheduled. The function is still called multiple times but
> since it is with coarse timer frequency, the 4 extra calls (1 per second)
> are better than to add more extra code to avoid these calls.
>
> The patch also changes the DEFINE enum into a real enum.
>
> Patch v2: only update tas_cache_last_udpate when actually updating the cache.
> Patch v3: avoid rearming timer
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
> ---
> src/openvpn/multi.c | 2 +-
> src/openvpn/push.c | 11 ++++++++-
> src/openvpn/ssl_common.h | 16 +++++++++---
> src/openvpn/ssl_verify.c | 53 ++++++++++++++++++++++------------------
> src/openvpn/ssl_verify.h | 3 +--
> 5 files changed, 54 insertions(+), 31 deletions(-)
>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 666456da9..ab2270a58 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2596,7 +2596,7 @@ static const multi_client_connect_handler
> client_connect_handlers[] = {
> static void
> multi_connection_established(struct multi_context *m, struct multi_instance
> *mi)
> {
> - if (tls_authentication_status(mi->context.c2.tls_multi, 0)
> + if (tls_authentication_status(mi->context.c2.tls_multi,
> TLS_MULTI_AUTH_STATUS_INTERVAL)
> != TLS_AUTHENTICATION_SUCCEEDED)
> {
> return;
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index fcafc5003..671220c3f 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -335,10 +335,18 @@ __attribute__ ((format(__printf__, 4, 5)))
>
> /*
> * Send auth failed message from server to client.
> + *
> + * Does nothing if an exit is already scheduled
> */
> void
> send_auth_failed(struct context *c, const char *client_reason)
> {
> + if (event_timeout_defined(&c->c2.scheduled_exit))
> + {
> + msg(D_TLS_DEBUG, "exit already scheduled for context");
> + return;
> + }
> +
> struct gc_arena gc = gc_new();
> static const char auth_failed[] = "AUTH_FAILED";
> size_t len;
> @@ -855,7 +863,8 @@ process_incoming_push_request(struct context *c)
> {
> int ret = PUSH_MSG_ERROR;
>
> - if (tls_authentication_status(c->c2.tls_multi, 0) ==
> TLS_AUTHENTICATION_FAILED
> +
> + if (tls_authentication_status(c->c2.tls_multi,
> TLS_MULTI_AUTH_STATUS_INTERVAL) == TLS_AUTHENTICATION_FAILED
> || c->c2.tls_multi->multi_state == CAS_FAILED)
> {
> const char *client_reason = tls_client_reason(c->c2.tls_multi);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 9c923f2a6..52488a163 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -152,6 +152,15 @@ struct auth_deferred_status
> unsigned int auth_control_status;
> };
>
> +/* key_state_test_auth_control_file return values, these specify the
> + * current status of a deferred authentication */
> +enum auth_deferred_result {
> + ACF_PENDING, /**< deferred auth still pending */
> + ACF_SUCCEEDED, /**< deferred auth has suceeded */
> + ACF_DISABLED, /**< deferred auth is not used */
> + ACF_FAILED /**< deferred auth has failed */
> +};
> +
> /**
> * Security parameter state of one TLS and data channel %key session.
> * @ingroup control_processor
> @@ -219,7 +228,7 @@ struct key_state
>
> #ifdef ENABLE_MANAGEMENT
> unsigned int mda_key_id;
> - unsigned int mda_status;
> + enum auth_deferred_result mda_status;
> #endif
> time_t acf_last_mod;
>
> @@ -561,8 +570,9 @@ struct tls_multi
> char *locked_username;
> struct cert_hash_set *locked_cert_hash_set;
>
> - /* Time of last call to tls_authentication_status */
> - time_t tas_last;
> + /* Time of last when we updated the cached state of
> + * tls_authentication_status deferred files */
> + time_t tas_cache_last_update;
>
> /*
> * An error message to send to client on AUTH_FAILED
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 34ee19caf..b1b01f777 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -845,13 +845,6 @@ cleanup:
> * user/password authentication.
> ***************************************************************************
> */
>
> -/* key_state_test_auth_control_file return values,
> - * NOTE: acf_merge indexing depends on these values */
> -#define ACF_UNDEFINED 0
> -#define ACF_SUCCEEDED 1
> -#define ACF_DISABLED 2
> -#define ACF_FAILED 3
> -
> void
> auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
> {
> @@ -866,7 +859,7 @@ auth_set_client_reason(struct tls_multi *multi, const
> char *client_reason)
>
> #ifdef ENABLE_MANAGEMENT
>
> -static inline unsigned int
> +static inline enum auth_deferred_result
> man_def_auth_test(const struct key_state *ks)
> {
> if (management_enable_def_auth(management))
> @@ -1041,13 +1034,23 @@ key_state_gen_auth_control_files(struct
> auth_deferred_status *ads,
> return (acf && apf);
> }
>
> -static unsigned int
> -key_state_test_auth_control_file(struct auth_deferred_status *ads)
> +/**
> + * Checks the control status from a file. The function will try to read
> + * and update the cached status if the status is still pending and the
> paramter
> + * cached is false. The function returns the
> + *
> + *
> + * @param ads deferred status control structure
> + * @param cached Return only cached status
> + * @return
> + */
> +static enum auth_deferred_result
> +key_state_test_auth_control_file(struct auth_deferred_status *ads, bool
> cached)
> {
> if (ads->auth_control_file)
> {
> unsigned int ret = ads->auth_control_status;
> - if (ret == ACF_UNDEFINED)
> + if (ret == ACF_PENDING && !cached)
> {
> FILE *fp = fopen(ads->auth_control_file, "r");
> if (fp)
> @@ -1084,11 +1087,7 @@ tls_authentication_status(struct tls_multi *multi,
> const int latency)
> /* at least one key already failed authentication */
> bool failed_auth = false;
>
> - if (latency && multi->tas_last + latency >= now)
> - {
> - return TLS_AUTHENTICATION_UNDEFINED;
> - }
> - multi->tas_last = now;
> + bool cached = multi->tas_cache_last_update + latency >= now;
>
> for (int i = 0; i < KEY_SCAN_SIZE; ++i)
> {
> @@ -1102,11 +1101,11 @@ tls_authentication_status(struct tls_multi *multi,
> const int latency)
> }
> else
> {
> - unsigned int auth_plugin = ACF_DISABLED;
> - unsigned int auth_script = ACF_DISABLED;
> - unsigned int auth_man = ACF_DISABLED;
> - auth_plugin =
> key_state_test_auth_control_file(&ks->plugin_auth);
> - auth_script =
> key_state_test_auth_control_file(&ks->script_auth);
> + enum auth_deferred_result auth_plugin = ACF_DISABLED;
> + enum auth_deferred_result auth_script = ACF_DISABLED;
> + enum auth_deferred_result auth_man = ACF_DISABLED;
> + auth_plugin =
> key_state_test_auth_control_file(&ks->plugin_auth, cached);
> + auth_script =
> key_state_test_auth_control_file(&ks->script_auth, cached);
> #ifdef ENABLE_MANAGEMENT
> auth_man = man_def_auth_test(ks);
> #endif
> @@ -1118,9 +1117,9 @@ tls_authentication_status(struct tls_multi *multi,
> const int latency)
> ks->authenticated = KS_AUTH_FALSE;
> failed_auth = true;
> }
> - else if (auth_plugin == ACF_UNDEFINED
> - || auth_script == ACF_UNDEFINED
> - || auth_man == ACF_UNDEFINED)
> + else if (auth_plugin == ACF_PENDING
> + || auth_script == ACF_PENDING
> + || auth_man == ACF_PENDING)
> {
> if (now < ks->auth_deferred_expire)
> {
> @@ -1140,6 +1139,12 @@ tls_authentication_status(struct tls_multi *multi,
> const int latency)
> }
> }
>
> + /* we did not rely on a cached result, remember the cache update time */
> + if (!cached)
> + {
> + multi->tas_cache_last_update = now;
> + }
> +
> #if 0
> dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d f=%d", active, success,
> deferred, failed_auth);
> #endif
> diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
> index 8358fb986..06b88b568 100644
> --- a/src/openvpn/ssl_verify.h
> +++ b/src/openvpn/ssl_verify.h
> @@ -69,8 +69,7 @@ enum tls_auth_status
> {
> TLS_AUTHENTICATION_SUCCEEDED=0,
> TLS_AUTHENTICATION_FAILED=1,
> - TLS_AUTHENTICATION_DEFERRED=2,
> - TLS_AUTHENTICATION_UNDEFINED=3
> + TLS_AUTHENTICATION_DEFERRED=2
> };
>
> /**
>
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
