Hi,
On 20/05/2021 17:11, Arne Schwabe wrote:
> This extract the update of a deferred key status into into own
> function.
>
> Patch v2: Do not ignore auth_deferred_expire. Minor format changes.
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
> ---
> src/openvpn/ssl_verify.c | 96 ++++++++++++++++++++++++++--------------
> 1 file changed, 62 insertions(+), 34 deletions(-)
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 7992a6eb9..455a5cd1b 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1073,6 +1073,60 @@ key_state_test_auth_control_file(struct
> auth_deferred_status *ads, bool cached)
> return ACF_DISABLED;
> }
>
> +/**
> + * This method takes a key_state and if updates the state
> + * of the key if it is deferred.
> + * @param cached If auth control files should be tried to be opened or th
> + * cached results should be used
> + * @param ks The key_state to update
> + */
> +static void
> +update_key_auth_status(bool cached, struct key_state *ks)
> +{
> + if (ks->authenticated == KS_AUTH_FALSE)
> + {
> + return;
> + }
> + else
> + {
> + 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
> + ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
> +
> + if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
> + || auth_man == ACF_FAILED)
> + {
> + ks->authenticated = KS_AUTH_FALSE;
> + return;
> + }
> + else if (auth_plugin == ACF_PENDING || auth_script == ACF_PENDING
> + || auth_man == ACF_PENDING)
> + {
> + if (now > ks->auth_deferred_expire)
To be 100% semantically the same of the code we have now this comparison
should be '>=' - just for consistency I'd change it.
> + {
> + /* Window to authenticate the key has expired, mark
> + * the key as unauthenticated */
> + ks->authenticated = KS_AUTH_FALSE;
> + }
If the if-condition above is false, we assume that authenticated is
KS_AUTH_DEFERRED. Can we safely rely on this assumption?
I could not find anywhere in the code an explicit link between
KS_AUTH_DEFERRED and ACF_PENDING (actually I could not find anywhere
setting the value ACF_PENDING).
> + }
> + else
> + {
> + /* all auth states (auth_plugin, auth_script, auth_man)
> + * are either ACF_DISABLED or ACF_SUCCEDED now, which
> + * translates to "not checked" or "auth succeeded"
> + */
> + ks->authenticated = KS_AUTH_TRUE;
> + }
> + }
> +}
> +
> +
> /**
> * The minimum times to have passed to update the cache. Older versions
> * of OpenVPN had code path that did not do any caching, so we start
> @@ -1115,46 +1169,20 @@ tls_authentication_status(struct tls_multi *multi)
> if (TLS_AUTHENTICATED(multi, ks))
> {
> active++;
> + update_key_auth_status(cached, ks);
> +
> if (ks->authenticated == KS_AUTH_FALSE)
> {
> failed_auth = true;
> }
> - else
> + else if (ks->authenticated == KS_AUTH_DEFERRED)
> {
> - 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
> - ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
>
> - if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
> - || auth_man == ACF_FAILED)
> - {
> - ks->authenticated = KS_AUTH_FALSE;
> - failed_auth = true;
> - }
> - else if (auth_plugin == ACF_PENDING
> - || auth_script == ACF_PENDING
> - || auth_man == ACF_PENDING)
> - {
> - if (now < ks->auth_deferred_expire)
> - {
> - deferred = true;
> - }
> - }
> - else
> - {
> - /* all auth states (auth_plugin, auth_script, auth_man)
> - * are either ACF_DISABLED or ACF_SUCCEDED now, which
> - * translates to "not checked" or "auth succeeded"
> - */
> - success = true;
> - ks->authenticated = KS_AUTH_TRUE;
> - }
> + deferred = true;
> + }
> + else if (ks->authenticated == KS_AUTH_TRUE)
> + {
> + success = true;
> }
> }
> }
>
Other that that, the patch looks good and passed all my tests.
So, despite my doubts as reported above, I could not hit any issue after
applying this patch.
Tested with deferred auth, client/server and p2p.
Regards,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
