diff --git a/client/ovpncli.cpp b/client/ovpncli.cpp index fe76225f..7b1b28db 100644 --- a/client/ovpncli.cpp +++ b/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) diff --git a/client/ovpncli.hpp b/client/ovpncli.hpp index 74dca0f7..a3014cee 100644 --- a/client/ovpncli.hpp +++ b/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 diff --git a/openvpn/client/cliconnect.hpp b/openvpn/client/cliconnect.hpp index 60241661..1020225c 100644 --- a/openvpn/client/cliconnect.hpp +++ b/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 diff --git a/openvpn/client/clicreds.hpp b/openvpn/client/clicreds.hpp index b00dc4a9..ea6a6012 100644 --- a/openvpn/client/clicreds.hpp +++ b/openvpn/client/clicreds.hpp @@ -42,13 +42,7 @@ class ClientCreds : public RC public: typedef RCPtr 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 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 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 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 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 bool session_id_defined() const { - return did_replace_password_with_session_id; - } - - // 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() - { - 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; + return !session_id.empty(); } void purge_session_id() { - if (!reset_to_cached_password()) + session_id.clear(); + session_id_username.clear(); + } + + void purge_user_pass() + { + username.clear(); + password.clear(); + } + + void save_username_for_session_id() + { + 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 } else if (response.empty()) { - if (!username.empty()) - ret += "Username"; - else - ret += "UsernameEmpty"; - ret += '/'; - if (!password.empty()) + if (!session_id_username.empty() || !username.empty()) { - if (did_replace_password_with_session_id) - ret += "SessionID"; - else - ret += "Password"; + ret += "Username"; } else + { + ret += "UsernameEmpty"; + } + ret += '/'; + if (!session_id.empty()) + { + ret += "SessionID"; + } + else if (!password.empty()) + { + ret += "Password"; + } + else + { ret += "PasswordEmpty"; + } } else { @@ -241,23 +244,20 @@ class ClientCreds : public RC 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; + // Need user interaction to authenticate - such as static/dynamic challenge or SAML + bool need_user_interaction_ = false; + + // Non-empty password provided + bool password_needed_ = false; + // Info describing a dynamic challenge ChallengeResponse::Ptr dynamic_challenge; - - // 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; - - // true if password has been replaced with Session ID - bool did_replace_password_with_session_id; }; } // namespace openvpn diff --git a/openvpn/client/clievent.hpp b/openvpn/client/clievent.hpp index 4ac7b5b9..93b55c64 100644 --- a/openvpn/client/clievent.hpp +++ b/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) diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index fb82fc82..6ee3b650 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -175,7 +175,6 @@ class ClientOptions : public RC bool synchronous_dns_lookup = false; bool disable_client_cert = false; int default_key_direction = -1; - bool autologin_sessions = false; bool allow_local_lan_access = false; PeerInfo::Set::Ptr extra_peer_info; @@ -234,7 +233,7 @@ class ClientOptions : public RC // creds userlocked_username = pcc.userlockedUsername(); autologin = pcc.autologin(); - autologin_sessions = (autologin && config.autologin_sessions); + autologin_sessions = (autologin && clientconf.autologinSessions); // digest factory DigestFactory::Ptr digest_factory(new CryptoDigestFactory()); @@ -540,15 +539,11 @@ class ClientOptions : public RC { cc->set_username(userlocked_username); cc->set_password(pcc.embeddedPassword()); - cc->enable_password_cache(true); - cc->set_replace_password_with_session_id(true); submit_creds(cc); creds_locked = true; } else if (autologin_sessions) { - // autologin sessions require replace_password_with_session_id - cc->set_replace_password_with_session_id(true); submit_creds(cc); creds_locked = true; } @@ -687,7 +682,6 @@ class ClientOptions : public RC std::unordered_set settings_ignoreWithWarning = { "allow-compression", /* TODO: maybe check against our client option compression setting? */ "allow-recursive-routing", - "auth-nocache", "auth-retry", "block-outside-dns", /* Core will decide on its own when to block outside dns, so this is not 100% identical in behaviour, so still warn */ "compat-mode", @@ -1178,6 +1172,13 @@ class ClientOptions : public RC cli_config->echo = clientconf.echo; cli_config->info = clientconf.info; cli_config->autologin_sessions = autologin_sessions; + + // if the previous client instance had session-id, it must be used by the new instance too + if (creds && creds->session_id_defined()) + { + cli_config->proto_context_config->set_xmit_creds(true); + } + return cli_config; } @@ -1202,7 +1203,10 @@ class ClientOptions : public RC // if no username is defined in creds and userlocked_username is defined // in profile, set the creds username to be the userlocked_username if (!creds_arg->username_defined() && !userlocked_username.empty()) + { creds_arg->set_username(userlocked_username); + creds_arg->save_username_for_session_id(); + } creds = creds_arg; } } diff --git a/openvpn/client/cliproto.hpp b/openvpn/client/cliproto.hpp index c2d62636..06c3d4a2 100644 --- a/openvpn/client/cliproto.hpp +++ b/openvpn/client/cliproto.hpp @@ -160,7 +160,6 @@ class Session : ProtoContextCallbackInterface, cli_events(config.cli_events), echo(config.echo), info(config.info), - autologin_sessions(config.autologin_sessions), pushed_options_limit(config.pushed_options_limit), pushed_options_filter(config.pushed_options_filter), inactive_timer(io_context_arg), @@ -580,9 +579,7 @@ class Session : ProtoContextCallbackInterface, #else OPENVPN_LOG("Session token: [redacted]"); #endif - autologin_sessions = true; proto_context.conf().set_xmit_creds(true); - creds->set_replace_password_with_session_id(true); creds->set_session_id(username, sess_id); } } @@ -696,26 +693,61 @@ class Session : ProtoContextCallbackInterface, if (msg.length() >= 13) reason = string::trim_left_copy(std::string(msg, 12)); - // If session token problem (such as expiration), and we have a cached - // password, retry with it. Otherwise, fail without retry. - if (string::starts_with(reason, "SESSION:") - && ((creds && creds->reset_to_cached_password()) - || autologin_sessions)) - { - if (creds && creds->session_id_defined()) - creds->purge_session_id(); - log_reason = "SESSION_AUTH_FAILED"; - } - else if (string::starts_with(reason, "TEMP")) + if (string::starts_with(reason, "TEMP")) { log_reason = "AUTH_FAILED_TEMP:" + parse_auth_failed_temp(std::string(reason, 4)); } else { - fatal_ = Error::AUTH_FAILED; - fatal_reason_ = std::move(reason); - log_reason = "AUTH_FAILED"; + bool password_defined = false; + bool session_id_defined = false; + if (creds) + { + password_defined = creds->password_defined(); + session_id_defined = creds->session_id_defined(); + + // authentication failure, purge auth-token + creds->purge_session_id(); + } + + // do we have a session-id? + if (session_id_defined) + { + bool reconnect = false; + + // if there was an OOB auth (server pushed AUTH_PENDING) throw a fatal error since we need a user input + if (!creds->need_user_interaction()) + { + // reconnect if we have a password OR password is not needed + if (!creds->password_needed() || password_defined) + { + reconnect = true; + } + } + + OPENVPN_LOG("need_user_interaction: " << creds->need_user_interaction() << ", pw_needed: " << creds->password_needed() << ", pw_defined: " << password_defined << ", reconnect: " << reconnect); + + if (reconnect) + { + log_reason = "SESSION_AUTH_FAILED"; + } + else + { + // we don't have a password and we need a user input, throw a fatal error and let the client to re-authenticate + fatal_ = Error::SESSION_EXPIRED; + fatal_reason_ = reason; + log_reason = "SESSION_EXPIRED"; + } + } + else + { + // no session-id, throw fatal error + fatal_ = Error::AUTH_FAILED; + fatal_reason_ = reason; + log_reason = "AUTH_FAILED"; + } } + if (notify_callback) { OPENVPN_LOG(log_reason); @@ -756,8 +788,6 @@ class Session : ProtoContextCallbackInterface, } } - - if (notify_callback && timeout > 0) { notify_callback->client_proto_auth_pending_timeout(timeout); @@ -798,9 +828,17 @@ class Session : ProtoContextCallbackInterface, // requires the VPN tunnel to be ready. ClientEvent::Base::Ptr ev; if (info_pre) + { ev = new ClientEvent::Info(msg.substr(std::strlen("INFO_PRE,"))); + if ((string::starts_with(ev->render(), "WEB_AUTH:") || string::starts_with(ev->render(), "CR_TEXT:")) && creds) + { + creds->set_need_user_interaction(); + } + } else + { ev = new ClientEvent::Info(msg.substr(std::strlen("INFO,"))); + } // INFO_PRE is like INFO but it is never buffered if (info_hold && !info_pre) @@ -913,6 +951,12 @@ class Session : ProtoContextCallbackInterface, // modify proto config (cipher, auth, key-derivation and compression methods) proto_context.process_push(received_options, *proto_context_options); + // process pushed auth-nocache + if (creds && proto_context.conf().auth_nocache) + { + creds->purge_user_pass(); + } + // initialize tun/routing tun = tun_factory->new_tun_client_obj(io_context, *this, transport.get()); tun->tun_start(received_options, *transport, proto_context.dc_settings()); @@ -1059,6 +1103,14 @@ class Session : ProtoContextCallbackInterface, { proto_context.write_auth_string(creds->get_password(), buf); } + + // save username for auth-token, which might be pushed later + creds->save_username_for_session_id(); + + if (proto_context.conf().auth_nocache) + { + creds->purge_user_pass(); + } } else { @@ -1451,7 +1503,6 @@ class Session : ProtoContextCallbackInterface, bool echo; bool info; - bool autologin_sessions; Error::Type fatal_ = Error::UNDEF; std::string fatal_reason_; diff --git a/openvpn/error/error.hpp b/openvpn/error/error.hpp index b5a7df3f..765bcb0a 100644 --- a/openvpn/error/error.hpp +++ b/openvpn/error/error.hpp @@ -94,6 +94,7 @@ enum Type EARLY_NEG_INVALID, // Early protoctol negotiation information invalid/parse error NTLM_MISSING_CRYPTO, // crypto primitives requires for NTLM are unavailable UNUSED_OPTIONS, // unused/unknown options found in configuration + SESSION_EXPIRED, // authentication error when using session-id and password is not cached // key event errors KEV_NEGOTIATE_ERROR, @@ -180,6 +181,7 @@ inline const char *name(const size_t type) "EARLY_NEG_INVALID", "NTLM_MISSING_CRYPTO", "UNUSED_OPTIONS_ERROR", + "SESSION_EXPIRED", "KEV_NEGOTIATE_ERROR", "KEV_PENDING_ERROR", "N_KEV_EXPIRE", diff --git a/openvpn/omi/openvpn.cpp b/openvpn/omi/openvpn.cpp index 218a69eb..ea9af184 100644 --- a/openvpn/omi/openvpn.cpp +++ b/openvpn/omi/openvpn.cpp @@ -483,16 +483,12 @@ class OMI : public OMICore, public ClientAPI::LogReceiver // response contains only challenge text creds->response = auth_password; } - creds->cachePassword = !auth_nocache; - creds->replacePasswordWithSessionID = true; } else if (type == "Auth") { creds.reset(new ClientAPI::ProvideCreds); creds->username = username; creds->password = password; - creds->replacePasswordWithSessionID = true; - creds->cachePassword = !auth_nocache; } else if (type == "HTTP Proxy") { diff --git a/openvpn/ssl/proto.hpp b/openvpn/ssl/proto.hpp index 7d7b1f18..e5815780 100644 --- a/openvpn/ssl/proto.hpp +++ b/openvpn/ssl/proto.hpp @@ -440,6 +440,8 @@ class ProtoContext : public logging::LoggingMixin #include #include +#include namespace openvpn { namespace Win { diff --git a/test/ovpncli/cli.cpp b/test/ovpncli/cli.cpp index 358acf75..98020214 100644 --- a/test/ovpncli/cli.cpp +++ b/test/ovpncli/cli.cpp @@ -1015,7 +1015,6 @@ int openvpn_client(int argc, char *argv[], const std::string *profile_content) std::string appCustomProtocols; bool eval = false; bool self_test = false; - bool cachePassword = false; bool disableClientCert = false; bool proxyAllowCleartextAuth = false; int defaultKeyDirection = -1; @@ -1078,9 +1077,6 @@ int openvpn_client(int argc, char *argv[], const std::string *profile_content) case 'T': self_test = true; break; - case 'C': - cachePassword = true; - break; case 'x': disableClientCert = true; break; @@ -1375,8 +1371,6 @@ int openvpn_client(int argc, char *argv[], const std::string *profile_content) creds.http_proxy_pass = proxyPassword; creds.response = response; creds.dynamicChallengeCookie = dynamicChallengeCookie; - creds.replacePasswordWithSessionID = true; - creds.cachePassword = cachePassword; ClientAPI::Status creds_status = client.provide_creds(creds); if (creds_status.error) OPENVPN_THROW_EXCEPTION("creds error: " << creds_status.message);