Refactor reauthentication logic

The reauthentication logic differs from openvpn2
and the code is a bit hard to follow. Simplify
the code and make it behave like in openvpn2.

 - password is cached by default

 - password is purged when auth-nocache is presented in a local config or pushed

 - when AUTH_FAILED is received and we have no session-id, throw a fatal error

 - when AUTH_FAILED is received and user interaction is required for
   authentication (MFA), throw a fatal error

 - when AUTH_FAILED is received, user interaction is not required
   for authentication and either we have a cached password OR password is not
   needed, we reconnect.

Password is "needed" when non-empty password is provided.

User interaction is required for static/dynamic challenge and SAML.

Signed-off-by: Lev Stipakov <[email protected]>

Author: lstipakov

client/ovpncli.cpp

@@ -795,13 +795,12 @@ OPENVPN_CLIENT_EXPORT Status OpenVPNClient::provide_creds(const ProvideCreds &cr
     {
         ClientCreds::Ptr cc = new ClientCreds();
         cc->set_username(creds.username);
+        cc->save_username_for_session_id();
         cc->set_password(creds.password);
         cc->set_http_proxy_username(creds.http_proxy_user);
         cc->set_http_proxy_password(creds.http_proxy_pass);
         cc->set_response(creds.response);
         cc->set_dynamic_challenge_cookie(creds.dynamicChallengeCookie, creds.username);
-        cc->set_replace_password_with_session_id(creds.replacePasswordWithSessionID);
-        cc->enable_password_cache(creds.cachePassword);
         state->creds = cc;
     }
     catch (const std::exception &e)
@@ -972,15 +971,6 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::connect_setup(Status &status, bool &se
 #if defined(OPENVPN_EXTERNAL_TRANSPORT_FACTORY)
     cc.extern_transport_factory = this;
 #endif
-    // force Session ID use and disable password cache if static challenge is enabled
-    if (state->creds
-        && !state->creds->get_replace_password_with_session_id()
-        && !state->eval.autologin
-        && !state->eval.staticChallenge.empty())
-    {
-        state->creds->set_replace_password_with_session_id(true);
-        state->creds->enable_password_cache(false);
-    }
 
     // external PKI
 #if !defined(USE_APPLE_SSL)

client/ovpncli.hpp

@@ -119,19 +119,6 @@ struct ProvideCreds
 
     // Dynamic challenge/response cookie
     std::string dynamicChallengeCookie;
-
-    // If true, on successful connect, we will replace the password
-    // with the session ID we receive from the server (if provided).
-    // If false, the password will be cached for future reconnects
-    // and will not be replaced with a session ID, even if the
-    // server provides one.
-    bool replacePasswordWithSessionID = false;
-
-    // If true, and if replacePasswordWithSessionID is true, and if
-    // we actually receive a session ID from the server, cache
-    // the user-provided password for future use before replacing
-    // the active password with the session ID.
-    bool cachePassword = false;
 };
 
 // used to get session token from VPN core

openvpn/client/cliconnect.hpp

@@ -463,6 +463,7 @@ class ClientConnect : ClientProto::NotifyCallback,
                     // Errors below will cause the client to NOT retry the connection,
                     // or otherwise give the error special handling.
 
+                case Error::SESSION_EXPIRED:
                 case Error::AUTH_FAILED:
                     {
                         const std::string &reason = client->fatal_reason();
@@ -474,9 +475,13 @@ class ClientConnect : ClientProto::NotifyCallback,
                         }
                         else
                         {
-                            ClientEvent::Base::Ptr ev = new ClientEvent::AuthFailed(reason);
+                            ClientEvent::Base::Ptr ev;
+                            if (client->fatal() == Error::SESSION_EXPIRED)
+                                ev = new ClientEvent::SessionExpired(reason);
+                            else
+                                ev = new ClientEvent::AuthFailed(reason);
                             client_options->events().add_event(std::move(ev));
-                            client_options->stats().error(Error::AUTH_FAILED);
+                            client_options->stats().error(client->fatal());
                             if (client_options->retry_on_auth_failed())
                                 queue_restart(5000);
                             else

openvpn/client/clicreds.hpp

@@ -42,13 +42,7 @@ class ClientCreds : public RC<thread_unsafe_refcount>
   public:
     typedef RCPtr<ClientCreds> Ptr;
 
-    ClientCreds()
-        : allow_cache_password(false),
-          password_save_defined(false),
-          replace_password_with_session_id(false),
-          did_replace_password_with_session_id(false)
-    {
-    }
+    ClientCreds() = default;
 
     void set_username(const std::string &username_arg)
     {
@@ -58,7 +52,10 @@ class ClientCreds : public RC<thread_unsafe_refcount>
     void set_password(const std::string &password_arg)
     {
         password = password_arg;
-        did_replace_password_with_session_id = false;
+        if (!password.empty())
+        {
+            password_needed_ = true;
+        }
     }
 
     void set_http_proxy_username(const std::string &username)
@@ -74,6 +71,10 @@ class ClientCreds : public RC<thread_unsafe_refcount>
     void set_response(const std::string &response_arg)
     {
         response = response_arg;
+        if (!response.empty())
+        {
+            need_user_interaction_ = true;
+        }
     }
 
     void set_dynamic_challenge_cookie(const std::string &cookie, const std::string &username)
@@ -82,51 +83,31 @@ class ClientCreds : public RC<thread_unsafe_refcount>
             dynamic_challenge.reset(new ChallengeResponse(cookie, username));
     }
 
-    void set_replace_password_with_session_id(const bool value)
-    {
-        replace_password_with_session_id = value;
-    }
-
-    void enable_password_cache(const bool value)
-    {
-        allow_cache_password = value;
-    }
-
-    bool get_replace_password_with_session_id() const
-    {
-        return replace_password_with_session_id;
-    }
-
     void set_session_id(const std::string &user, const std::string &sess_id)
     {
-        // force Session ID use if dynamic challenge is enabled
-        if (dynamic_challenge && !replace_password_with_session_id)
-            replace_password_with_session_id = true;
-
-        if (replace_password_with_session_id)
+        if (dynamic_challenge)
         {
-            if (allow_cache_password && !password_save_defined)
-            {
-                password_save = password;
-                password_save_defined = true;
-            }
-            password = sess_id;
-            response = "";
-            if (dynamic_challenge)
-            {
-                username = dynamic_challenge->get_username();
-                dynamic_challenge.reset();
-            }
-            else if (!user.empty())
-                username = user;
-            did_replace_password_with_session_id = true;
+            session_id_username = dynamic_challenge->get_username();
+            // for dynamic challenge we use dynamic password only once
+            dynamic_challenge.reset();
+        }
+        else if (!user.empty())
+        {
+            session_id_username = user;
         }
+
+        // response is used only once
+        response.clear();
+
+        session_id = sess_id;
     }
 
     std::string get_username() const
     {
         if (dynamic_challenge)
             return dynamic_challenge->get_username();
+        else if (!session_id_username.empty())
+            return session_id_username;
         else
             return username;
     }
@@ -136,7 +117,12 @@ class ClientCreds : public RC<thread_unsafe_refcount>
         if (dynamic_challenge)
             return dynamic_challenge->construct_dynamic_password(response);
         else if (response.empty())
-            return password;
+        {
+            if (!session_id.empty())
+                return session_id;
+            else
+                return password;
+        }
         else
             return ChallengeResponse::construct_static_password(password, response);
     }
@@ -173,34 +159,44 @@ class ClientCreds : public RC<thread_unsafe_refcount>
 
     bool session_id_defined() const
     {
-        return did_replace_password_with_session_id;
+        return !session_id.empty();
     }
 
-    // If we have a saved password that is not a session ID,
-    // restore it and wipe any existing session ID.
-    bool reset_to_cached_password()
+    void purge_session_id()
     {
-        if (password_save_defined)
-        {
-            password = password_save;
-            password_save.clear();
-            password_save_defined = false;
-            did_replace_password_with_session_id = false;
-            return true;
-        }
-        else
-            return false;
+        session_id.clear();
+        session_id_username.clear();
     }
 
-    void purge_session_id()
+    void purge_user_pass()
+    {
+        username.clear();
+        password.clear();
+    }
+
+    void save_username_for_session_id()
     {
-        if (!reset_to_cached_password())
+        if (session_id_username.empty())
         {
-            password.clear();
-            did_replace_password_with_session_id = false;
+            session_id_username = username;
         }
     }
 
+    void set_need_user_interaction()
+    {
+        need_user_interaction_ = true;
+    }
+
+    bool need_user_interaction() const
+    {
+        return need_user_interaction_;
+    }
+
+    bool password_needed() const
+    {
+        return password_needed_;
+    }
+
     std::string auth_info() const
     {
         std::string ret;
@@ -210,20 +206,27 @@ class ClientCreds : public RC<thread_unsafe_refcount>
         }
         else if (response.empty())
         {
-            if (!username.empty())
+            if (!session_id_username.empty() || !username.empty())
+            {
                 ret += "Username";
+            }
             else
+            {
                 ret += "UsernameEmpty";
+            }
             ret += '/';
-            if (!password.empty())
+            if (!session_id.empty())
+            {
+                ret += "SessionID";
+            }
+            else if (!password.empty())
             {
-                if (did_replace_password_with_session_id)
-                    ret += "SessionID";
-                else
-                    ret += "Password";
+                ret += "Password";
             }
             else
+            {
                 ret += "PasswordEmpty";
+            }
         }
         else
         {
@@ -241,23 +244,20 @@ class ClientCreds : public RC<thread_unsafe_refcount>
     std::string http_proxy_user;
     std::string http_proxy_pass;
 
-    // Password caching
-    bool allow_cache_password;
-    bool password_save_defined;
-    std::string password_save;
+    std::string session_id;
+    std::string session_id_username;
 
-    // Response to challenge
+    // Response to a challenge
     std::string response;
 
-    // Info describing a dynamic challenge
-    ChallengeResponse::Ptr dynamic_challenge;
+    // Need user interaction to authenticate - such as static/dynamic challenge or SAML
+    bool need_user_interaction_ = false;
 
-    // If true, on successful connect, we will replace the password
-    // with the session ID we receive from the server.
-    bool replace_password_with_session_id;
+    // Non-empty password provided
+    bool password_needed_ = false;
 
-    // true if password has been replaced with Session ID
-    bool did_replace_password_with_session_id;
+    // Info describing a dynamic challenge
+    ChallengeResponse::Ptr dynamic_challenge;
 };
 
 } // namespace openvpn

openvpn/client/clievent.hpp

@@ -92,6 +92,7 @@ enum Type
     RELAY_ERROR,
     COMPRESS_ERROR,
     NTLM_MISSING_CRYPTO,
+    SESSION_EXPIRED,
 
     N_TYPES
 };
@@ -153,7 +154,8 @@ inline const char *event_name(const Type type)
         "EPKI_INVALID_ALIAS",
         "RELAY_ERROR",
         "COMPRESS_ERROR",
-        "NTLM_MISSING_CRYPTO"};
+        "NTLM_MISSING_CRYPTO",
+        "SESSION_EXPIRED"};
 
     static_assert(N_TYPES == array_size(names), "event names array inconsistency");
     if (type < N_TYPES)
@@ -451,6 +453,14 @@ struct AuthFailed : public ReasonBase
     }
 };
 
+struct SessionExpired : public ReasonBase
+{
+    SessionExpired(std::string reason)
+        : ReasonBase(SESSION_EXPIRED, std::move(reason))
+    {
+    }
+};
+
 struct CertVerifyFail : public ReasonBase
 {
     CertVerifyFail(std::string reason)