| Message ID | 20200709101603.11941-2-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/8] Deprecate ncp-disable and add improved ncp to Changes.rst | expand |
Typo On 09/07/2020 11:15, Arne Schwabe wrote: > This order the states from unauthenticated to authenticated and also > changes the comparison for KS_AUTH_FALSE from != to > > > It also add comments and documents part using the state machine > better. > > Remove a now obsolete comment and two obsolete ifdefs. While > keeping the ifdef in ssl_verify would save a few bytes of code, > this is too minor to justify keeping the ifdef > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/multi.c | 12 +++++++++--- > src/openvpn/ssl.c | 7 ++++--- > src/openvpn/ssl_common.h | 22 ++++++++++++++++------ > src/openvpn/ssl_verify.c | 18 +++++++----------- > 4 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f1ced9b7..f1332c8d 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context)))) > { > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - bool was_authenticated = false; > + bool was_unauthenticated = true; > struct key_state *ks = NULL; > if (mi->context.c2.tls_multi) > { > ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY]; > - was_authenticated = ks->authenticated; > + was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE); > } > #endif > > @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > pre_select(&mi->context); > > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated) > + /* > + * if we see the state transition from unauthenticated to deferred > + * and a auth_control_file, we assume it got just added and add > + * inotify watch to that file > + */ > + if (ks && ks->auth_control_file && was_unauthenticated > + && (ks->authenticated == KS_AUTH_DEFERRED)) > { > /* watch acf file */ > long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT); > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 9df7552d..f3fe0ecf 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > if (session->opt->server && !(session->opt->ncp_enabled > && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > { > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > if (!tls_session_generate_data_channel_keys(session)) > { > @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > secure_memzero(up, sizeof(*up)); > > /* Perform final authentication checks */ > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > verify_final_auth_checks(multi, session); > } > @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final > * veto opportunity over authentication decision. > */ > - if ((ks->authenticated != KS_AUTH_FALSE) > + if ((ks->authenticated > KS_AUTH_FALSE) > && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL)) > { > key_state_export_keying_material(&ks->ks_ssl, session); > @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > return true; > > error: > + ks->authenticated = KS_AUTH_FALSE; > secure_memzero(ks->key_src, sizeof(*ks->key_src)); > if (up) > { > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index fdf589b5..dd82b77d 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -127,10 +127,23 @@ struct key_source2 { > struct key_source server; /**< Random provided by server. */ > }; > > + > +/** > + * This refelects the (server side) state auf authentication after the TLS refelects -> reflects
Hi, thanks for bearing with me and for adding the requested comment. I believe the status enum is now easier to grasp and to extend. Code was basically already reviewed ad this revision did not bring any functional change with it. "It still looks good". Tested only on the client side and it did not break anything visible (as expected). Acked-by: Antonio Quartulli <a@unstable.c> On 09/07/2020 12:15, Arne Schwabe wrote: > This order the states from unauthenticated to authenticated and also > changes the comparison for KS_AUTH_FALSE from != to > > > It also add comments and documents part using the state machine > better. > > Remove a now obsolete comment and two obsolete ifdefs. While > keeping the ifdef in ssl_verify would save a few bytes of code, > this is too minor to justify keeping the ifdef > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/multi.c | 12 +++++++++--- > src/openvpn/ssl.c | 7 ++++--- > src/openvpn/ssl_common.h | 22 ++++++++++++++++------ > src/openvpn/ssl_verify.c | 18 +++++++----------- > 4 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f1ced9b7..f1332c8d 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context)))) > { > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - bool was_authenticated = false; > + bool was_unauthenticated = true; > struct key_state *ks = NULL; > if (mi->context.c2.tls_multi) > { > ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY]; > - was_authenticated = ks->authenticated; > + was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE); > } > #endif > > @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > pre_select(&mi->context); > > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated) > + /* > + * if we see the state transition from unauthenticated to deferred > + * and a auth_control_file, we assume it got just added and add > + * inotify watch to that file > + */ > + if (ks && ks->auth_control_file && was_unauthenticated > + && (ks->authenticated == KS_AUTH_DEFERRED)) > { > /* watch acf file */ > long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT); > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 9df7552d..f3fe0ecf 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > if (session->opt->server && !(session->opt->ncp_enabled > && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > { > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > if (!tls_session_generate_data_channel_keys(session)) > { > @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > secure_memzero(up, sizeof(*up)); > > /* Perform final authentication checks */ > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > verify_final_auth_checks(multi, session); > } > @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final > * veto opportunity over authentication decision. > */ > - if ((ks->authenticated != KS_AUTH_FALSE) > + if ((ks->authenticated > KS_AUTH_FALSE) > && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL)) > { > key_state_export_keying_material(&ks->ks_ssl, session); > @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > return true; > > error: > + ks->authenticated = KS_AUTH_FALSE; > secure_memzero(ks->key_src, sizeof(*ks->key_src)); > if (up) > { > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index fdf589b5..dd82b77d 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -127,10 +127,23 @@ struct key_source2 { > struct key_source server; /**< Random provided by server. */ > }; > > + > +/** > + * This refelects the (server side) state auf authentication after the TLS > + * session has been established and key_method_2_read is called. If async auth > + * is enabled the state will first move to KS_AUTH_DEFERRED before eventually > + * being set to KS_AUTH_TRUE or KS_AUTH_FALSE > + * Only KS_AUTH_TRUE is fully authenticated > + */ > enum ks_auth_state { > - KS_AUTH_FALSE, > - KS_AUTH_TRUE, > - KS_AUTH_DEFERRED > + KS_AUTH_FALSE, /**< Key state is not authenticated */ > + KS_AUTH_DEFERRED, /**< Key state authentication is being deferred, > + * by async auth */ > + KS_AUTH_TRUE /**< Key state is authenticated. TLS and user/pass > + * succeeded. This include AUTH_PENDING/OOB > + * authentication as those hold the > + * connection artifically in KS_AUTH_DEFERRED > + */ > }; > > /** > @@ -194,8 +207,6 @@ struct key_state > enum ks_auth_state authenticated; > time_t auth_deferred_expire; > > -#ifdef ENABLE_DEF_AUTH > - /* If auth_deferred is true, authentication is being deferred */ > #ifdef MANAGEMENT_DEF_AUTH > unsigned int mda_key_id; > unsigned int mda_status; > @@ -205,7 +216,6 @@ struct key_state > time_t acf_last_mod; > char *auth_control_file; > #endif > -#endif > }; > > /** Control channel wrapping (--tls-auth/--tls-crypt) context */ > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index e28f1f3a..990fba99 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) > if (DECRYPT_KEY_ENABLED(multi, ks)) > { > active = true; > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > #ifdef ENABLE_DEF_AUTH > unsigned int s1 = ACF_DISABLED; > @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session, > > /* > * Main username/password verification entry point > + * > + * Will set session->ks[KS_PRIMARY].authenticated according to > + * result of the username/password verifcation > */ > void > verify_user_pass(struct user_pass *up, struct tls_multi *multi, > @@ -1414,17 +1417,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > */ > send_push_reply_auth_token(multi); > } > -#ifdef ENABLE_DEF_AUTH > msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", > (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded", > up->username, > (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); > -#else > - msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", > - "succeeded", > - up->username, > - (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); > -#endif > } > else > { > @@ -1445,7 +1441,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* Don't allow the CN to change once it's been locked */ > - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn) > + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn) > { > const char *cn = session->common_name; > if (cn && strcmp(cn, multi->locked_cn)) > @@ -1461,7 +1457,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* Don't allow the cert hashes to change once they have been locked */ > - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set) > + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set) > { > const struct cert_hash_set *chs = session->cert_hash_set; > if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set)) > @@ -1475,7 +1471,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* verify --client-config-dir based authentication */ > - if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) > + if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) > { > struct gc_arena gc = gc_new(); > >
typo x3 On 09/07/2020 11:15, Arne Schwabe wrote: > This order the states from unauthenticated to authenticated and also > changes the comparison for KS_AUTH_FALSE from != to > > > It also add comments and documents part using the state machine > better. > > Remove a now obsolete comment and two obsolete ifdefs. While > keeping the ifdef in ssl_verify would save a few bytes of code, > this is too minor to justify keeping the ifdef > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/multi.c | 12 +++++++++--- > src/openvpn/ssl.c | 7 ++++--- > src/openvpn/ssl_common.h | 22 ++++++++++++++++------ > src/openvpn/ssl_verify.c | 18 +++++++----------- > 4 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f1ced9b7..f1332c8d 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context)))) > { > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - bool was_authenticated = false; > + bool was_unauthenticated = true; > struct key_state *ks = NULL; > if (mi->context.c2.tls_multi) > { > ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY]; > - was_authenticated = ks->authenticated; > + was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE); > } > #endif > > @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > pre_select(&mi->context); > > #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) > - if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated) > + /* > + * if we see the state transition from unauthenticated to deferred > + * and a auth_control_file, we assume it got just added and add and aN auth_control_file > + * inotify watch to that file > + */ > + if (ks && ks->auth_control_file && was_unauthenticated > + && (ks->authenticated == KS_AUTH_DEFERRED)) > { > /* watch acf file */ > long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT); > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 9df7552d..f3fe0ecf 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > if (session->opt->server && !(session->opt->ncp_enabled > && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > { > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > if (!tls_session_generate_data_channel_keys(session)) > { > @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > secure_memzero(up, sizeof(*up)); > > /* Perform final authentication checks */ > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > verify_final_auth_checks(multi, session); > } > @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final > * veto opportunity over authentication decision. > */ > - if ((ks->authenticated != KS_AUTH_FALSE) > + if ((ks->authenticated > KS_AUTH_FALSE) > && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL)) > { > key_state_export_keying_material(&ks->ks_ssl, session); > @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > return true; > > error: > + ks->authenticated = KS_AUTH_FALSE; > secure_memzero(ks->key_src, sizeof(*ks->key_src)); > if (up) > { > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index fdf589b5..dd82b77d 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -127,10 +127,23 @@ struct key_source2 { > struct key_source server; /**< Random provided by server. */ > }; > > + > +/** > + * This refelects the (server side) state auf authentication after the TLS > + * session has been established and key_method_2_read is called. If async auth > + * is enabled the state will first move to KS_AUTH_DEFERRED before eventually > + * being set to KS_AUTH_TRUE or KS_AUTH_FALSE > + * Only KS_AUTH_TRUE is fully authenticated > + */ > enum ks_auth_state { > - KS_AUTH_FALSE, > - KS_AUTH_TRUE, > - KS_AUTH_DEFERRED > + KS_AUTH_FALSE, /**< Key state is not authenticated */ > + KS_AUTH_DEFERRED, /**< Key state authentication is being deferred, > + * by async auth */ > + KS_AUTH_TRUE /**< Key state is authenticated. TLS and user/pass > + * succeeded. This include AUTH_PENDING/OOB > + * authentication as those hold the > + * connection artifically in KS_AUTH_DEFERRED artifically -> artificially > + */ > }; > > /** > @@ -194,8 +207,6 @@ struct key_state > enum ks_auth_state authenticated; > time_t auth_deferred_expire; > > -#ifdef ENABLE_DEF_AUTH > - /* If auth_deferred is true, authentication is being deferred */ > #ifdef MANAGEMENT_DEF_AUTH > unsigned int mda_key_id; > unsigned int mda_status; > @@ -205,7 +216,6 @@ struct key_state > time_t acf_last_mod; > char *auth_control_file; > #endif > -#endif > }; > > /** Control channel wrapping (--tls-auth/--tls-crypt) context */ > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index e28f1f3a..990fba99 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) > if (DECRYPT_KEY_ENABLED(multi, ks)) > { > active = true; > - if (ks->authenticated != KS_AUTH_FALSE) > + if (ks->authenticated > KS_AUTH_FALSE) > { > #ifdef ENABLE_DEF_AUTH > unsigned int s1 = ACF_DISABLED; > @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session, > > /* > * Main username/password verification entry point > + * > + * Will set session->ks[KS_PRIMARY].authenticated according to > + * result of the username/password verifcation verifcation -> verification > */ > void > verify_user_pass(struct user_pass *up, struct tls_multi *multi, > @@ -1414,17 +1417,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > */ > send_push_reply_auth_token(multi); > } > -#ifdef ENABLE_DEF_AUTH > msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", > (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded", > up->username, > (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); > -#else > - msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", > - "succeeded", > - up->username, > - (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); > -#endif > } > else > { > @@ -1445,7 +1441,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* Don't allow the CN to change once it's been locked */ > - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn) > + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn) > { > const char *cn = session->common_name; > if (cn && strcmp(cn, multi->locked_cn)) > @@ -1461,7 +1457,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* Don't allow the cert hashes to change once they have been locked */ > - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set) > + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set) > { > const struct cert_hash_set *chs = session->cert_hash_set; > if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set)) > @@ -1475,7 +1471,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) > } > > /* verify --client-config-dir based authentication */ > - if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) > + if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) > { > struct gc_arena gc = gc_new(); > >
Your patch has been applied to the master branch.
I have fixed the typos tincanteksup noticed plus one of my own, and
tested this on the t_server testbed (tcp, udp, normal auth, async-auth
plugin). Happy server, with 2.3, 2.4 and git master client.
While it already got an ACK, a good and thorough beating is always good
for this stuff :-)
commit 5608041c7b343fbcd2d3317a8a49f43cb168a390
Author: Arne Schwabe
Date: Thu Jul 9 12:15:57 2020 +0200
Make key_state->authenticated more state machine like
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20200709101603.11941-2-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20258.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f1ced9b7..f1332c8d 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context)))) { #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) - bool was_authenticated = false; + bool was_unauthenticated = true; struct key_state *ks = NULL; if (mi->context.c2.tls_multi) { ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY]; - was_authenticated = ks->authenticated; + was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE); } #endif @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns pre_select(&mi->context); #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) - if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated) + /* + * if we see the state transition from unauthenticated to deferred + * and a auth_control_file, we assume it got just added and add + * inotify watch to that file + */ + if (ks && ks->auth_control_file && was_unauthenticated + && (ks->authenticated == KS_AUTH_DEFERRED)) { /* watch acf file */ long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT); diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 9df7552d..f3fe0ecf 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) if (session->opt->server && !(session->opt->ncp_enabled && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) { - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { if (!tls_session_generate_data_channel_keys(session)) { @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio secure_memzero(up, sizeof(*up)); /* Perform final authentication checks */ - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { verify_final_auth_checks(multi, session); } @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final * veto opportunity over authentication decision. */ - if ((ks->authenticated != KS_AUTH_FALSE) + if ((ks->authenticated > KS_AUTH_FALSE) && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL)) { key_state_export_keying_material(&ks->ks_ssl, session); @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio return true; error: + ks->authenticated = KS_AUTH_FALSE; secure_memzero(ks->key_src, sizeof(*ks->key_src)); if (up) { diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index fdf589b5..dd82b77d 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -127,10 +127,23 @@ struct key_source2 { struct key_source server; /**< Random provided by server. */ }; + +/** + * This refelects the (server side) state auf authentication after the TLS + * session has been established and key_method_2_read is called. If async auth + * is enabled the state will first move to KS_AUTH_DEFERRED before eventually + * being set to KS_AUTH_TRUE or KS_AUTH_FALSE + * Only KS_AUTH_TRUE is fully authenticated + */ enum ks_auth_state { - KS_AUTH_FALSE, - KS_AUTH_TRUE, - KS_AUTH_DEFERRED + KS_AUTH_FALSE, /**< Key state is not authenticated */ + KS_AUTH_DEFERRED, /**< Key state authentication is being deferred, + * by async auth */ + KS_AUTH_TRUE /**< Key state is authenticated. TLS and user/pass + * succeeded. This include AUTH_PENDING/OOB + * authentication as those hold the + * connection artifically in KS_AUTH_DEFERRED + */ }; /** @@ -194,8 +207,6 @@ struct key_state enum ks_auth_state authenticated; time_t auth_deferred_expire; -#ifdef ENABLE_DEF_AUTH - /* If auth_deferred is true, authentication is being deferred */ #ifdef MANAGEMENT_DEF_AUTH unsigned int mda_key_id; unsigned int mda_status; @@ -205,7 +216,6 @@ struct key_state time_t acf_last_mod; char *auth_control_file; #endif -#endif }; /** Control channel wrapping (--tls-auth/--tls-crypt) context */ diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index e28f1f3a..990fba99 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) if (DECRYPT_KEY_ENABLED(multi, ks)) { active = true; - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { #ifdef ENABLE_DEF_AUTH unsigned int s1 = ACF_DISABLED; @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session, /* * Main username/password verification entry point + * + * Will set session->ks[KS_PRIMARY].authenticated according to + * result of the username/password verifcation */ void verify_user_pass(struct user_pass *up, struct tls_multi *multi, @@ -1414,17 +1417,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, */ send_push_reply_auth_token(multi); } -#ifdef ENABLE_DEF_AUTH msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded", up->username, (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); -#else - msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", - "succeeded", - up->username, - (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); -#endif } else { @@ -1445,7 +1441,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* Don't allow the CN to change once it's been locked */ - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn) + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn) { const char *cn = session->common_name; if (cn && strcmp(cn, multi->locked_cn)) @@ -1461,7 +1457,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* Don't allow the cert hashes to change once they have been locked */ - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set) + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set) { const struct cert_hash_set *chs = session->cert_hash_set; if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set)) @@ -1475,7 +1471,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* verify --client-config-dir based authentication */ - if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) + if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) { struct gc_arena gc = gc_new();
This order the states from unauthenticated to authenticated and also changes the comparison for KS_AUTH_FALSE from != to > It also add comments and documents part using the state machine better. Remove a now obsolete comment and two obsolete ifdefs. While keeping the ifdef in ssl_verify would save a few bytes of code, this is too minor to justify keeping the ifdef Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/multi.c | 12 +++++++++--- src/openvpn/ssl.c | 7 ++++--- src/openvpn/ssl_common.h | 22 ++++++++++++++++------ src/openvpn/ssl_verify.c | 18 +++++++----------- 4 files changed, 36 insertions(+), 23 deletions(-)