[Openvpn-devel,v2,02/11] Implement client side handling of AUTH_PENDING message

Message ID 20210125125628.30364-3-arne@rfc2549.org
State Accepted
Headers show
Series Pending authentication improvements | expand

Commit Message

Arne Schwabe Jan. 25, 2021, 1:56 a.m. UTC
This allows a client to extend the timeout of pull-request response
while waiting for the user to complete a pending authentication. A
timeout of 60s for a normal authentication might still works for a
simple 2FA (but still challenging). With a sophisticated (or overly
complicated) web based authentication 60s are quite short.

To avoid not detecting network problem in this phase, we use the
constant sending of PUSH_REQUEST/AUTH_PENDING as keepalive signal
and still timeout the session after the handshake window time.

patch v2: typo fixes, invert if for sscanf

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/server-options.rst |  4 ++
 doc/management-notes.txt            | 39 +++++++++++++----
 src/openvpn/forward.c               | 11 ++++-
 src/openvpn/integer.h               | 25 +++++++++++
 src/openvpn/push.c                  | 68 ++++++++++++++++++++++++++++-
 src/openvpn/push.h                  |  9 ++++
 src/openvpn/ssl.c                   |  3 ++
 src/openvpn/ssl.h                   |  3 ++
 src/openvpn/ssl_common.h            |  1 +
 9 files changed, 152 insertions(+), 11 deletions(-)

Comments

Lev Stipakov Jan. 28, 2021, 9:06 p.m. UTC | #1
Compared with V1 - all concerns are addressed. Compiled with MSVC.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Feb. 14, 2021, 4:19 a.m. UTC | #2
Your patch has been applied to the master branch.

I'm not sure I understand the code, though.  It receives the new timeout
from the server (that is easy), but then caps it by "hand_window", which
is never increased - so the maximum timeout stays at "60", unless increased
manually on the client.  Where am I misreading this?  Or is this a 
prerequisite for this functionality?

    c->c2.push_request_timeout = ks->established + min_uint(max_timeout, server_timeout);

(or am I totally misunderstanding push_request_timeout, and this has
nothing to do with hand-window, except "it's using this as a default
value", but only for triggering re-sends, not for "final give up"?)

However, for my standard test cases, this does not break anything, and
the code looks reasonable.  So in it goes :)

commit 3f8fb2b2c1d664f421d36181846da89c4330c6cc
Author: Arne Schwabe
Date:   Mon Jan 25 13:56:19 2021 +0100

     Implement client side handling of AUTH_PENDING message

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20210125125628.30364-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21491.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Arne Schwabe Feb. 15, 2021, 12:31 a.m. UTC | #3
Am 14.02.21 um 16:19 schrieb Gert Doering:
> Your patch has been applied to the master branch.
> 
> I'm not sure I understand the code, though.  It receives the new timeout
> from the server (that is easy), but then caps it by "hand_window", which
> is never increased - so the maximum timeout stays at "60", unless increased

hm  max_timeout is maximum of handshake_window and
renegotiate_seconds/2, so max(60,1800) in a default configuration.

> manually on the client.  Where am I misreading this?  Or is this a 
> prerequisite for this functionality?
> 
>     c->c2.push_request_timeout = ks->established + min_uint(max_timeout, server_timeout);

And here we normally take a minimum of 1800 and whatever the server pushed.


> (or am I totally misunderstanding push_request_timeout, and this has
> nothing to do with hand-window, except "it's using this as a default
> value", but only for triggering re-sends, not for "final give up"?)

This patch also splits two timeouts that before just happend to be
identical into two distinct timeouts. One time is the one that says how
TLS (re)negotiation is allowed to be (handshake_window) and the other
one is the pull timeout. The if condition in push.c shows their
relationship:

    if (c->c2.push_request_timeout > now
        && (now - ks->peer_last_packet) < c->options.handshake_window)

Basically give the user "max_timeout" time to answer the 2FA challenge
but only if the server has been alive in the last handshake_window seconds.

> However, for my standard test cases, this does not break anything, and
> the code looks reasonable.  So in it goes :)

Patch

diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index 5a689452..271c54d0 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -473,6 +473,10 @@  fast hardware. SSL/TLS authentication must be used in this mode.
     - bit 1: The peer supports peer-id floating mechanism
     - bit 2: The client expects a push-reply and the server may
       send this reply without waiting for a push-request first.
+    - bit 3: The client is capable of doing key derivation using
+      RFC5705 key material exporter.
+    - bit 4: The client is capable of accepting additional arguments
+      to the `AUTH_PENDING` message.
 
   :code:`IV_NCP=2`
         Negotiable ciphers, client supports ``--cipher`` pushed by
diff --git a/doc/management-notes.txt b/doc/management-notes.txt
index 50f0f567..3aff6eb6 100644
--- a/doc/management-notes.txt
+++ b/doc/management-notes.txt
@@ -610,14 +610,30 @@  to signal a pending authenticating to the client. A pending auth means
 that the connecting requires extra authentication like a one time
 password or doing a single sign one via web.
 
-    client-pending-auth {CID} {EXTRA}
-
-The server will send AUTH_PENDING and INFO_PRE,{EXTRA} to the client.
-The client is expected to inform the user that authentication is pending and
-display the extra information. For the format of EXTRA see below
-For the OpenVPN server this is stateless operation and needs to be
-followed by a client-deny/client-auth[-nt] command (that is the result of the
-out of band authentication).
+    client-pending-auth {CID} {EXTRA} {TIMEOUT}
+
+The server will send AUTH_PENDING and INFO_PRE,{EXTRA} to the client. If the
+client supports accepting keywords to AUTH_PENDING (announced via IV_PROTO),
+TIMEOUT parameter will be also be announced to the client to allow it to modify
+its own timeout. The client is expected to inform the user that authentication
+is pending and display the extra information and also show the user the
+remaining time to complete the auth if applicable.
+
+Receiving an AUTH_PENDING message will make the client change its timeout to
+the timeout proposed by the server, even if the timeout is shorter.
+If the client does not receive a packet from the server for hand-window the
+connection times out regardless of the timeout. This ensures that the connection
+still times out relatively quickly in case of network problems. The client will
+continously send PULL_REQUEST messages to the server until the timeout is reached.
+This message also triggers an ACK message from the server that resets the
+hand-window based timeout.
+
+Both client and server limit the maximum timeout to the smaller value of half the
+--tls-reneg minimum time and --hand-window time (defaults to 60s).
+
+For the format of EXTRA see below. For the OpenVPN server this is a stateless
+operation and needs to be followed by a client-deny/client-auth[-nt] command
+(that is the result of the out of band authentication).
 
 Before issuing a client-pending-auth to a client instead of a
 client-auth/client-deny, the server should check the IV_SSO
@@ -630,7 +646,7 @@  set
     setenv IV_SSO openurl,crtext
 
 The variable name IV_SSO is historic as AUTH_PENDING was first used
-to signal single sign on support. To keep compatiblity with existing
+to signal single sign on support. To keep compatibility with existing
 implementations the name IV_SSO is kept in lieu of a better name.
 
 openurl
@@ -646,6 +662,11 @@  The space in a control message is limited, so this url should be kept
 short to avoid issues. If a loger url is required a URL that redirects
 to the longer URL should be sent instead.
 
+A complete documentation how URLs should be handled on the client is available
+in the openvpn3 repository:
+
+https://github.com/OpenVPN/openvpn3/blob/master/doc/webauth.md
+
 url_proxy
 ========
 To avoid issues with OpenVPN connection persist-tun and not able
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 76f5a032..2e1e1490 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -240,6 +240,10 @@  check_incoming_control_channel(struct context *c)
         {
             receive_cr_response(c, &buf);
         }
+        else if (buf_string_match_head_str(&buf, "AUTH_PENDING"))
+        {
+            receive_auth_pending(c, &buf);
+        }
         else
         {
             msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(&buf));
@@ -299,7 +303,12 @@  check_connection_established(struct context *c)
             }
 #endif
             /* fire up push request right away (already 1s delayed) */
-            c->c2.push_request_timeout = now + c->options.handshake_window;
+            /* We might receive a AUTH_PENDING request before we armed this
+             * timer. In that case we don't change the value */
+            if (c->c2.push_request_timeout < now)
+            {
+                c->c2.push_request_timeout = now + c->options.handshake_window;
+            }
             event_timeout_init(&c->c2.push_request_interval, 0, now);
             reset_coarse_timers(c);
         }
diff --git a/src/openvpn/integer.h b/src/openvpn/integer.h
index 3755f43f..0c3511e6 100644
--- a/src/openvpn/integer.h
+++ b/src/openvpn/integer.h
@@ -39,6 +39,31 @@ 
 /*
  * min/max functions
  */
+static inline unsigned int
+max_uint(unsigned int x, unsigned int y)
+{
+    if (x > y)
+    {
+        return x;
+    }
+    else
+    {
+        return y;
+    }
+}
+
+static inline unsigned int
+min_uint(unsigned int x, unsigned int y)
+{
+    if (x < y)
+    {
+        return x;
+    }
+    else
+    {
+        return y;
+    }
+}
 
 static inline int
 max_int(int x, int y)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index fb6b9d17..2ceee2c4 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -231,6 +231,62 @@  receive_cr_response(struct context *c, const struct buffer *buffer)
     msg(D_PUSH, "CR response was sent by client ('%s')", m);
 }
 
+/**
+ * Parse the keyword for the AUTH_PENDING request
+ * @param buffer                buffer containing the keywords, the buffer's
+ *                              content will be modified by this function
+ * @param server_timeout        timeout pushed by the server or unchanged
+ *                              if the server does not push a timeout
+ */
+static void
+parse_auth_pending_keywords(const struct buffer *buffer,
+                            unsigned int *server_timeout)
+{
+    struct buffer buf = *buffer;
+
+    /* does the buffer start with "AUTH_PENDING," ? */
+    if (!buf_advance(&buf, strlen("AUTH_PENDING"))
+        || !(buf_read_u8(&buf) == ',') || !BLEN(&buf))
+    {
+        return;
+    }
+
+    /* parse the keywords in the same way that push options are parsed */
+    char line[OPTION_LINE_SIZE];
+
+    while (buf_parse(&buf, ',', line, sizeof(line)))
+    {
+        if (sscanf(line, "timeout %u", server_timeout) != 1)
+        {
+            msg(D_PUSH, "ignoring AUTH_PENDING parameter: %s", line);
+        }
+    }
+}
+
+void
+receive_auth_pending(struct context *c, const struct buffer *buffer)
+{
+    if (!c->options.pull)
+        return;
+
+    /* Cap the increase at the maximum time we are willing stay in the
+     * pending authentication state */
+    unsigned int max_timeout = max_uint(c->options.renegotiate_seconds/2,
+                               c->options.handshake_window);
+
+    /* try to parse parameter keywords, default to hand-winow timeout if the
+     * server does not supply a timeout */
+    unsigned int server_timeout = c->options.handshake_window;
+    parse_auth_pending_keywords(buffer, &server_timeout);
+
+    msg(D_PUSH, "AUTH_PENDING received, extending handshake timeout from %us "
+                "to %us", c->options.handshake_window,
+                min_uint(max_timeout, server_timeout));
+
+    struct key_state *ks = &c->c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY];
+    c->c2.push_request_timeout = ks->established + min_uint(max_timeout, server_timeout);
+}
+
 /**
  * Add an option to the given push list by providing a format string.
  *
@@ -372,7 +428,17 @@  send_push_request(struct context *c)
     struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
     struct key_state *ks = &session->key[KS_PRIMARY];
 
-    if (c->c2.push_request_timeout > now)
+    /* We timeout here under two conditions:
+     * a) we reached the hard limit of push_request_timeout
+     * b) we have not seen anything from the server in hand_window time
+     *
+     * for non auth-pending scenario, push_request_timeout is the same as
+     * hand_window timeout. For b) every PUSH_REQUEST is a acknowledged by
+     * the server by a P_ACK_V1 packet that reset the keepalive timer
+     */
+
+    if (c->c2.push_request_timeout > now
+        && (now - ks->peer_last_packet) < c->options.handshake_window)
     {
         return send_control_channel_string(c, "PUSH_REQUEST", D_PUSH);
     }
diff --git a/src/openvpn/push.h b/src/openvpn/push.h
index 2faf19a6..01847671 100644
--- a/src/openvpn/push.h
+++ b/src/openvpn/push.h
@@ -89,5 +89,14 @@  void send_restart(struct context *c, const char *kill_msg);
  */
 void send_push_reply_auth_token(struct tls_multi *multi);
 
+/**
+ * Parses an AUTH_PENDING message and if in pull mode extends the timeout
+ *
+ * @param c             The context struct
+ * @param buffer        Buffer containing the control message with AUTH_PENDING
+ */
+void
+receive_auth_pending(struct context *c, const struct buffer *buffer);
+
 #endif /* if P2MP */
 #endif /* ifndef PUSH_H */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index efbf688f..5a231387 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2268,6 +2268,7 @@  push_peer_info(struct buffer *buf, struct tls_session *session)
         if (session->opt->pull)
         {
             iv_proto |= IV_PROTO_REQUEST_PUSH;
+            iv_proto |= IV_PROTO_AUTH_PENDING_KW;
         }
 
         /* support for Negotiable Crypto Parameters */
@@ -3733,6 +3734,8 @@  tls_pre_decrypt(struct tls_multi *multi,
             }
         }
     }
+    /* Remember that we received a valid control channel packet */
+    ks->peer_last_packet = now;
 
 done:
     buf->len = 0;
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 56034540..8c8cbe02 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -119,6 +119,9 @@ 
 /** Supports key derivation via TLS key material exporter [RFC5705] */
 #define IV_PROTO_TLS_KEY_EXPORT (1<<3)
 
+/** Supports signaling keywords with AUTH_PENDING, e.g. timeout=xy */
+#define IV_PROTO_AUTH_PENDING_KW (1<<4)
+
 /* Default field in X509 to be username */
 #define X509_USERNAME_FIELD_DEFAULT "CN"
 
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index a826e37f..bbb8135d 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -178,6 +178,7 @@  struct key_state
     time_t established;         /* when our state went S_ACTIVE */
     time_t must_negotiate;      /* key negotiation times out if not finished before this time */
     time_t must_die;            /* this object is destroyed at this time */
+    time_t peer_last_packet;    /* Last time we received a packet in this control session */
 
     int initial_opcode;         /* our initial P_ opcode */
     struct session_id session_id_remote; /* peer's random session ID */