| Message ID | 20210520151148.2565578-5-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state | expand |
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 <arne@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,
Hi, On 14/06/2021 03:06, Antonio Quartulli wrote: > 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 <arne@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). self-answer: when ks is initialized it is zero'd. ACF_PENDING is zero so it's implicitly assigned until further change. For this reason the assumption here is correct. @Gert: could you apply the '>' -> '>=' ont he fly? For me the rest looks good. Acked-by: Antonio Quartulli <antonio@openvpn.net>
Stared at the code - it's "just moving code", though the part about
"where does the KS_AUTH_DEFERRED come from?" needs a bit more digging.
The new code is slightly ugly - it's the old if/else deep nesting
style, with early returns intermixed (but keeping the old else branch,
so "yes, same code" is easier to see). A followup patch might want to
refactor this to "consistent early return style".
Tested on the server side test rig (all OK), including deferred auth
by plugin and by script.
I have adjusted the "now >= ks->auth_deferred_expire" clause as
instructed and agreed on IRC, and killed a blank line.
Your patch has been applied to the master branch.
commit e55bedd4ea435f95d288e60691abc6e53b8bf014
Author: Arne Schwabe
Date: Thu May 20 17:11:44 2021 +0200
Extracting key_state deferred auth status update into function
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210520151148.2565578-5-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22420.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
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) + { + /* Window to authenticate the key has expired, mark + * the key as unauthenticated */ + ks->authenticated = KS_AUTH_FALSE; + } + } + 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; } } }
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 <arne@rfc2549.org> --- src/openvpn/ssl_verify.c | 96 ++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 34 deletions(-)