Re: [Openvpn-devel] [PATCH v2] Make key_state->authenticated more state machine like
Hi,
On 08/07/2020 09:17, 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
> ---
> src/openvpn/multi.c | 12 +---
> src/openvpn/ssl.c| 7 ---
> src/openvpn/ssl_common.h | 14 --
> src/openvpn/ssl_verify.c | 18 +++---
> 4 files changed, 28 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 71565dd3..94fb0c5b 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2465,7 +2465,7 @@ key_method_2_write(struct buffer *buf, struct
> tls_session *session)
> */
> if (session->opt->server && !(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))
> {
> @@ -2646,7 +2646,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);
> }
> @@ -2671,7 +2671,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);
> @@ -2702,6 +2702,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..91e55c7a 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -128,9 +128,14 @@ struct key_source2 {
> };
>
I suggest adding a comment on top of this enum declaration. I'd suggest
something like this:
/*
* The following enum represents the status of the key-state object
* during its life-cycle. Based on the step taken by the server code,
* the status will move forward by one (or more) values. This allows
* us to make more flexible status checks by using the ">" or "<"
* operator
*/
> 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,
> +
[Openvpn-devel] [PATCH v2] Make key_state->authenticated more state machine like
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
---
src/openvpn/multi.c | 12 +---
src/openvpn/ssl.c| 7 ---
src/openvpn/ssl_common.h | 14 --
src/openvpn/ssl_verify.c | 18 +++---
4 files changed, 28 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 71565dd3..94fb0c5b 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2465,7 +2465,7 @@ key_method_2_write(struct buffer *buf, struct tls_session
*session)
*/
if (session->opt->server && !(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))
{
@@ -2646,7 +2646,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);
}
@@ -2671,7 +2671,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);
@@ -2702,6 +2702,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..91e55c7a 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -128,9 +128,14 @@ struct key_source2 {
};
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 +199,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;
@@ -2
