From faef663c5b2420c8eb0f5f9e64ec5cf41cef8004 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 25 Feb 2020 14:53:00 -0800 Subject: [PATCH 1/2] Move the state and nonce from an encrypted cookie into the session store - Removes two configuration options that are no longer needed: `cryptor_secret` and `timeout` - Removes the cookie encryption code and related helpers - Does not set or delete the state cookie anymore - Adds the state and nonce values to the session store - Renamed the SessionIdGenerator to SessionStringGenerator because it now also generates state and nonce values [Issue #69] Signed-off-by: Andrew Chang --- ...onfigmap-template-for-authn-and-authz.yaml | 2 - ...hservice-configmap-template-for-authn.yaml | 2 - config/oidc/config.proto | 33 +- docs/README.md | 6 +- src/common/session/BUILD | 50 +- src/common/session/gcm_encryptor.cc | 126 ---- src/common/session/gcm_encryptor.h | 58 -- src/common/session/hkdf_deriver.cc | 61 -- src/common/session/hkdf_deriver.h | 38 -- src/common/session/session_id_generator.cc | 15 - src/common/session/session_id_generator.h | 28 - .../session/session_string_generator.cc | 27 + src/common/session/session_string_generator.h | 31 + src/common/session/token_encryptor.cc | 122 ---- src/common/session/token_encryptor.h | 61 -- src/common/utilities/random.cc | 8 - src/common/utilities/random.h | 10 - src/filters/filter_chain.cc | 9 +- src/filters/oidc/BUILD | 14 +- src/filters/oidc/in_memory_session_store.cc | 146 ++--- src/filters/oidc/in_memory_session_store.h | 11 +- src/filters/oidc/oidc_filter.cc | 148 ++--- src/filters/oidc/oidc_filter.h | 30 +- src/filters/oidc/session_store.h | 32 +- src/filters/oidc/state_cookie_codec.cc | 27 - src/filters/oidc/state_cookie_codec.h | 34 -- test/common/session/BUILD | 40 +- test/common/session/gcm_encryptor_test.cc | 52 -- test/common/session/hkdf_deriver_test.cc | 41 -- test/common/session/mocks.h | 15 +- .../session/session_id_generator_test.cc | 24 - .../session/session_string_generator_test.cc | 52 ++ test/common/session/token_encryptor_test.cc | 28 - test/common/utilities/random_test.cc | 3 - test/config/getconfig_test.cc | 2 - test/filters/filter_chain_test.cc | 1 - test/filters/oidc/BUILD | 11 - .../oidc/in_memory_session_store_test.cc | 187 +++--- test/filters/oidc/oidc_filter_test.cc | 573 ++++++------------ test/filters/oidc/state_cookie_codec_test.cc | 30 - test/fixtures/valid-config.json | 2 - 41 files changed, 605 insertions(+), 1585 deletions(-) delete mode 100644 src/common/session/gcm_encryptor.cc delete mode 100644 src/common/session/gcm_encryptor.h delete mode 100644 src/common/session/hkdf_deriver.cc delete mode 100644 src/common/session/hkdf_deriver.h delete mode 100644 src/common/session/session_id_generator.cc delete mode 100644 src/common/session/session_id_generator.h create mode 100644 src/common/session/session_string_generator.cc create mode 100644 src/common/session/session_string_generator.h delete mode 100644 src/common/session/token_encryptor.cc delete mode 100644 src/common/session/token_encryptor.h delete mode 100644 src/filters/oidc/state_cookie_codec.cc delete mode 100644 src/filters/oidc/state_cookie_codec.h delete mode 100644 test/common/session/gcm_encryptor_test.cc delete mode 100644 test/common/session/hkdf_deriver_test.cc delete mode 100644 test/common/session/session_id_generator_test.cc create mode 100644 test/common/session/session_string_generator_test.cc delete mode 100644 test/common/session/token_encryptor_test.cc delete mode 100644 test/filters/oidc/state_cookie_codec_test.cc diff --git a/bookinfo-example/config/authservice-configmap-template-for-authn-and-authz.yaml b/bookinfo-example/config/authservice-configmap-template-for-authn-and-authz.yaml index 56da4e0a..dd5d7a55 100644 --- a/bookinfo-example/config/authservice-configmap-template-for-authn-and-authz.yaml +++ b/bookinfo-example/config/authservice-configmap-template-for-authn-and-authz.yaml @@ -46,7 +46,6 @@ data: "client_id": "xxxxxx-xxxx-xxxx-xxxx-xxxxxxxxx_CHANGE_ME", "client_secret": "xxxxx-xxxx-xxx-xxx-xxx_CHANGE_ME", "scopes": ["productpage.read", "reviews.read"], - "cryptor_secret": "xxx_CHANGE_ME", "cookie_name_prefix": "productpage", "id_token": { "preamble": "Bearer", @@ -56,7 +55,6 @@ data: "preamble": "Bearer", "header": "Authorization" }, - "timeout": 300, "logout": { "path": "/authservice_logout", "redirect_to_uri": "https:///some/logout/path" diff --git a/bookinfo-example/config/authservice-configmap-template-for-authn.yaml b/bookinfo-example/config/authservice-configmap-template-for-authn.yaml index 59a558a9..1bffd1b5 100644 --- a/bookinfo-example/config/authservice-configmap-template-for-authn.yaml +++ b/bookinfo-example/config/authservice-configmap-template-for-authn.yaml @@ -46,13 +46,11 @@ data: "client_id": "xxxxxx-xxxx-xxxx-xxxx-xxxxxxxxx_CHANGE_ME", "client_secret": "xxxxx-xxxx-xxx-xxx-xxx_CHANGE_ME", "scopes": [], - "cryptor_secret": "xxx_CHANGE_ME", "cookie_name_prefix": "productpage", "id_token": { "preamble": "Bearer", "header": "Authorization" }, - "timeout": 300, "logout": { "path": "/authservice_logout", "redirect_to_uri": "https:///some/logout/path" diff --git a/config/oidc/config.proto b/config/oidc/config.proto index 8fc3e002..c8a9683c 100644 --- a/config/oidc/config.proto +++ b/config/oidc/config.proto @@ -107,58 +107,45 @@ message OIDCConfig { // Required, but an empty array is allowed. repeated string scopes = 8; - // A secret used to derive cryptographic material for protecting cookies and other data. - // Can be any string. - // Required. - string cryptor_secret = 10 [(validate.rules).string.min_len = 1]; - // A unique identifier of the authservice's browser cookies. Can be any string. // Only needed when multiple services in the same domain are each protected by // their own authservice, in which case each service's authservice should have // a unique value to avoid cookie name conflicts. // Optional. - string cookie_name_prefix = 11; + string cookie_name_prefix = 9; // The configuration for adding ID Tokens as headers to requests forwarded to a service. // Required. - TokenConfig id_token = 12 [(validate.rules).message.required = true]; + TokenConfig id_token = 10 [(validate.rules).message.required = true]; // The configuration for adding Access Tokens as headers to requests forwarded to a service. // Optional. - TokenConfig access_token = 13; - - // The number of seconds a user has to authenticate with the OIDC Provider before their - // authentication flow expires. The timer starts when an unauthenticated user visits - // a service protected by the authservice, keeps running while they are redirected to - // their OIDC Provider to log in, continues to run while they enter their - // username/password and potentially perform 2-factor authentication, and stops - // when the authservice receives the authcode from the OIDC provider's redirect. - // If it takes longer than the timeout for the authcode to be received, then the - // authcode will be rejected by the authservice causing the login to fail, even - // if the user successfully logged in to their OIDC Provider. - // Required. - uint32 timeout = 14 [(validate.rules).uint32.gte = 30]; + TokenConfig access_token = 11; // When specified, the authservice will destroy the authservice session when a request is // made to the configured path. // Optional. - LogoutConfig logout = 15; + LogoutConfig logout = 12; // The Authservice associates obtained OIDC tokens with a session ID in a session store. + // It also stores some temporary information during the login process into the session store, + // which will be removed when the user finishes the login. // This configuration option sets the number of seconds since a user's session with the Authservice has started // until that session should expire. // When configured to `0`, which is the default value, the session will never timeout based on the time // that it was started, but can still timeout due to being idle. // When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never // expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. - uint32 max_absolute_session_timeout = 16; + uint32 max_absolute_session_timeout = 13; // The Authservice associates obtained OIDC tokens with a session ID in a session store. + // It also stores some temporary information during the login process into the session store, + // which will be removed when the user finishes the login. // This configuration option sets the number of seconds since the most recent incoming request from that user // until the user's session with the Authservice should expire. // When configured to `0`, which is the default value, session expiration will not consider idle time, // but can still consider timeout based on maximum absolute time since added. // When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never // expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. - uint32 max_session_idle_timeout = 17; + uint32 max_session_idle_timeout = 14; } diff --git a/docs/README.md b/docs/README.md index fedaf279..aba009e2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -90,14 +90,12 @@ The configuration of an OpenID Connect filter that can be used to retrieve ident | client_id | The OIDC client ID assigned to the filter to be used in the [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Required. | string | | client_secret | The OIDC client secret assigned to the filter to be used in the [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). Required. | string | | scopes | Optional additional scopes passed to the OIDC Provider in the [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest). The `openid` scope is always sent to the OIDC Provider, and does not need to be specified here. Required, but an empty array is allowed. | (slice of) string | -| cryptor_secret | A secret used to derive cryptographic material for protecting cookies and other data. Can be any string. Required. | string | | cookie_name_prefix | A unique identifier of the authservice's browser cookies. Can be any string. Only needed when multiple services in the same domain are each protected by their own authservice, in which case each service's authservice should have a unique value to avoid cookie name conflicts. Optional. | string | | id_token | The configuration for adding ID Tokens as headers to requests forwarded to a service. Required. | TokenConfig | | access_token | The configuration for adding Access Tokens as headers to requests forwarded to a service. Optional. | TokenConfig | -| timeout | The number of seconds a user has to authenticate with the OIDC Provider before their authentication flow expires. The timer starts when an unauthenticated user visits a service protected by the authservice, keeps running while they are redirected to their OIDC Provider to log in, continues to run while they enter their username/password and potentially perform 2-factor authentication, and stops when the authservice receives the authcode from the OIDC provider's redirect. If it takes longer than the timeout for the authcode to be received, then the authcode will be rejected by the authservice causing the login to fail, even if the user successfully logged in to their OIDC Provider. Required. | uint32 | | logout | When specified, the authservice will destroy the authservice session when a request is made to the configured path. Optional. | LogoutConfig | -| max_absolute_session_timeout | The Authservice associates obtained OIDC tokens with a session ID in a session store. This configuration option sets the number of seconds since a user's session with the Authservice has started until that session should expire. When configured to `0`, which is the default value, the session will never timeout based on the time that it was started, but can still timeout due to being idle. When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. | uint32 | -| max_session_idle_timeout | The Authservice associates obtained OIDC tokens with a session ID in a session store. This configuration option sets the number of seconds since the most recent incoming request from that user until the user's session with the Authservice should expire. When configured to `0`, which is the default value, session expiration will not consider idle time, but can still consider timeout based on maximum absolute time since added. When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. | uint32 | +| max_absolute_session_timeout | The Authservice associates obtained OIDC tokens with a session ID in a session store. It also stores some temporary information during the login process into the session store, which will be removed when the user finishes the login. This configuration option sets the number of seconds since a user's session with the Authservice has started until that session should expire. When configured to `0`, which is the default value, the session will never timeout based on the time that it was started, but can still timeout due to being idle. When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. | uint32 | +| max_session_idle_timeout | The Authservice associates obtained OIDC tokens with a session ID in a session store. It also stores some temporary information during the login process into the session store, which will be removed when the user finishes the login. This configuration option sets the number of seconds since the most recent incoming request from that user until the user's session with the Authservice should expire. When configured to `0`, which is the default value, session expiration will not consider idle time, but can still consider timeout based on maximum absolute time since added. When both `max_absolute_session_timeout` and `max_session_idle_timeout` are zero, then sessions will never expire. These settings do not affect how quickly the OIDC tokens contained inside the user's session expire. | uint32 | diff --git a/src/common/session/BUILD b/src/common/session/BUILD index 76cfabaa..aa5444e4 100644 --- a/src/common/session/BUILD +++ b/src/common/session/BUILD @@ -3,58 +3,14 @@ load("//bazel:bazel.bzl", "xx_library") package(default_visibility = ["//visibility:public"]) xx_library( - name = "hkdf", + name = "session_string_generator", srcs = [ - "hkdf_deriver.cc", + "session_string_generator.cc", ], hdrs = [ - "hkdf_deriver.h", - ], - deps = [ - "@com_googlesource_boringssl//:crypto", - ], -) - -xx_library( - name = "gcm_encryptor", - srcs = [ - "gcm_encryptor.cc", - ], - hdrs = [ - "gcm_encryptor.h", - ], - deps = [ - "@com_github_abseil-cpp//absl/types:optional", - "@com_googlesource_boringssl//:crypto", - ], -) - -xx_library( - name = "session_id_generator", - srcs = [ - "session_id_generator.cc", - ], - hdrs = [ - "session_id_generator.h", + "session_string_generator.h", ], deps = [ "//src/common/utilities:random" ], ) - -xx_library( - name = "token_encryptor", - srcs = [ - "token_encryptor.cc", - ], - hdrs = [ - "token_encryptor.h", - ], - deps = [ - ":gcm_encryptor", - ":hkdf", - "//src/common/utilities:random", - "@com_github_abseil-cpp//absl/strings:strings", - "@com_googlesource_boringssl//:crypto", - ], -) diff --git a/src/common/session/gcm_encryptor.cc b/src/common/session/gcm_encryptor.cc deleted file mode 100644 index bbb8db41..00000000 --- a/src/common/session/gcm_encryptor.cc +++ /dev/null @@ -1,126 +0,0 @@ -#include "src/common/session/gcm_encryptor.h" -#include - -#include "openssl/rand.h" - -namespace authservice { -namespace common { -namespace session { - -class GcmEncryptorImpl : public GcmEncryptor { - public: - GcmEncryptorImpl(const std::vector& key, - size_t tag_len = EVP_AEAD_DEFAULT_TAG_LENGTH); - virtual ~GcmEncryptorImpl() override; - - virtual std::vector Seal( - const std::vector& plaintext, - absl::optional> nonce, - const std::vector& aad = {}) override; - virtual absl::optional> Open( - const std::vector& ciphertext, - const std::vector& aad = {}) override; - - private: - bssl::UniquePtr ctx_; -}; - -GcmEncryptorImpl::GcmEncryptorImpl(const std::vector& key, - size_t tag_len) { - const EVP_AEAD* aead_ = nullptr; - if (key.size() == EVP_AEAD_key_length(EVP_aead_aes_128_gcm())) { - aead_ = EVP_aead_aes_128_gcm(); - } else if (key.size() == EVP_AEAD_key_length(EVP_aead_aes_256_gcm())) { - aead_ = EVP_aead_aes_256_gcm(); - } else { - throw std::range_error( - "GCM key is incorrect size, expected 16 or 32 bytes"); - } - ctx_.reset(EVP_AEAD_CTX_new(aead_, key.data(), key.size(), tag_len)); - assert(ctx_); -} - -GcmEncryptorImpl::~GcmEncryptorImpl() { - EVP_AEAD_CTX_cleanup(ctx_.get()); - ctx_.reset(nullptr); -} - -std::vector GcmEncryptorImpl::Seal( - const std::vector& plaintext, - absl::optional> nonce, - const std::vector& aad) { - auto nonce_len = EVP_AEAD_nonce_length(EVP_AEAD_CTX_aead(ctx_.get())); - - std::vector actual_nonce; - if (nonce) { - if (nonce->size() != nonce_len) { - throw std::range_error("GCM nonce is incorrect size"); - } - actual_nonce = *nonce; - } else { - // No nonce supplied, so generate a random one - actual_nonce.resize(nonce_len); - int rc = RAND_bytes(actual_nonce.data(), nonce_len); - assert(rc == 1); - } - - // Create output vector, initially containing the nonce, then reserve maximum - // required space - size_t out_len = - plaintext.size() + EVP_AEAD_max_overhead(EVP_AEAD_CTX_aead(ctx_.get())); - std::vector out(actual_nonce); - out.resize(nonce_len + out_len); - - // Perform the encryption, appending the result to the nonce value - // Result ciphertext will then contain: - // nonce || ciphertext || tag - auto rc = - EVP_AEAD_CTX_seal(ctx_.get(), out.data() + nonce_len, &out_len, out_len, - actual_nonce.data(), nonce_len, plaintext.data(), - plaintext.size(), aad.data(), aad.size()); - assert(rc == 1); - - // Resize down to actual output size - out.resize(nonce_len + out_len); - - return out; -} - -absl::optional> GcmEncryptorImpl::Open( - const std::vector& ciphertext, - const std::vector& aad) { - auto nonce_len = EVP_AEAD_nonce_length(EVP_AEAD_CTX_aead(ctx_.get())); - - // Make sure we have at least enough data to not read past the end when we try - // to access - // the nonce, and ensure we won't underflow when we do 'ciphertext.size() - - // nonce_len' - if (ciphertext.size() < nonce_len) { - return absl::nullopt; - } - - size_t out_len = ciphertext.size(); - std::vector out(out_len); - - auto rc = EVP_AEAD_CTX_open( - ctx_.get(), out.data(), &out_len, out_len, ciphertext.data(), nonce_len, - ciphertext.data() + nonce_len, ciphertext.size() - nonce_len, aad.data(), - aad.size()); - if (rc != 1) { - // Decryption or validation failed in some way - return absl::nullopt; - } - - out.resize(out_len); - - return out; -} - -GcmEncryptorPtr GcmEncryptor::Create(const std::vector& key, - size_t tag_len) { - return std::make_shared(key, tag_len); -} - -} // namespace session -} // namespace common -} // namespace authservice diff --git a/src/common/session/gcm_encryptor.h b/src/common/session/gcm_encryptor.h deleted file mode 100644 index 867fb233..00000000 --- a/src/common/session/gcm_encryptor.h +++ /dev/null @@ -1,58 +0,0 @@ -#ifndef AUTHSERVICE_SRC_COMMON_SESSION_GCM_ENCRYPTOR_H_ -#define AUTHSERVICE_SRC_COMMON_SESSION_GCM_ENCRYPTOR_H_ -#include -#include - -#include "absl/types/optional.h" -#include "openssl/aead.h" - -namespace authservice { -namespace common { -namespace session { - -class GcmEncryptor; -typedef std::shared_ptr GcmEncryptorPtr; - -class GcmEncryptor { - public: - virtual ~GcmEncryptor(){}; - - /** - * GCM encrypt and authenticate some data. - * @param plaintext the data to encrypt and authenticate. - * @param nonce nonce to be used. If none supplied, one will be randomly - * generated. - * @param aad additional authenticated data. - * @return bytes representing the encrypted/authenticated data (nonce || - * ciphertext || tag). - */ - virtual std::vector Seal( - const std::vector& plaintext, - absl::optional> nonce = absl::nullopt, - const std::vector& aad = {}) = 0; - - /** - * GCM decrypt and verify some data. - * @param ciphertext the data (nonce || ciphertext || tag) to be decrypted. - * @param aad additional authenticated data. - * @return bytes representing the decrypted plaintext, or absl::nullopt if - * verification failed. - */ - virtual absl::optional> Open( - const std::vector& ciphertext, - const std::vector& aad = {}) = 0; - - /** - * Create an instance of a GcmEncryptor. - * @param key data of the key used to encrypt/decrypt. - * @param tag_len GCM tag length. - * @return an instance of a GcmEncryptor. - */ - static GcmEncryptorPtr Create(const std::vector& key, - size_t tag_len = EVP_AEAD_DEFAULT_TAG_LENGTH); -}; - -} // namespace session -} // namespace common -} // namespace authservice -#endif // AUTHSERVICE_SRC_COMMON_SESSION_GCM_ENCRYPTOR_H_ \ No newline at end of file diff --git a/src/common/session/hkdf_deriver.cc b/src/common/session/hkdf_deriver.cc deleted file mode 100644 index bee1f1fd..00000000 --- a/src/common/session/hkdf_deriver.cc +++ /dev/null @@ -1,61 +0,0 @@ -#include "src/common/session/hkdf_deriver.h" -#include -#include -#include "openssl/digest.h" -#include "openssl/hkdf.h" - -namespace authservice { -namespace common { -namespace session { - -class HkdfDeriverImpl : public HkdfDeriver { - public: - HkdfDeriverImpl(std::vector secret, HKDFHash hash_alg); - - virtual std::vector Derive( - size_t out_len, const std::vector& salt, - const std::vector& info = {}) override; - - private: - std::vector secret_; - const EVP_MD* hash_alg_; -}; - -HkdfDeriverImpl::HkdfDeriverImpl(std::vector secret, - HKDFHash hash_alg) - : secret_(std::move(secret)) { - switch (hash_alg) { - case HKDFHash::SHA256: - hash_alg_ = EVP_sha256(); - break; - case HKDFHash::SHA384: - hash_alg_ = EVP_sha384(); - break; - case HKDFHash::SHA512: - hash_alg_ = EVP_sha512(); - break; - default: - throw std::range_error("Unsupport hash algorithm"); - } -} - -std::vector HkdfDeriverImpl::Derive( - size_t out_len, const std::vector& salt, - const std::vector& info) { - std::vector output(out_len); - - auto rc = - HKDF(output.data(), out_len, hash_alg_, secret_.data(), secret_.size(), - salt.data(), salt.size(), info.data(), info.size()); - assert(rc == 1); - - return output; -} - -HkdfDeriverPtr HkdfDeriver::Create(const std::vector& secret, - HKDFHash hash_alg) { - return std::make_shared(secret, hash_alg); -} -} // namespace session -} // namespace common -} // namespace authservice diff --git a/src/common/session/hkdf_deriver.h b/src/common/session/hkdf_deriver.h deleted file mode 100644 index 78c6d1d0..00000000 --- a/src/common/session/hkdf_deriver.h +++ /dev/null @@ -1,38 +0,0 @@ -#ifndef AUTHSERVICE_SRC_COMMON_SESSION_HKDF_DERIVER_H_ -#define AUTHSERVICE_SRC_COMMON_SESSION_HKDF_DERIVER_H_ -#include -#include - -namespace authservice { -namespace common { -namespace session { - -class HkdfDeriver; -typedef std::shared_ptr HkdfDeriverPtr; - -enum class HKDFHash { - SHA256, - SHA384, - SHA512, -}; - -class HkdfDeriver { - public: - virtual ~HkdfDeriver(){}; - - virtual std::vector Derive( - size_t out_len, const std::vector& salt, - const std::vector& info = {}) = 0; - - /** - * Create an instance of a HkdfDeriver. - * @return an instance of a HkdfDeriver. - */ - static HkdfDeriverPtr Create(const std::vector& secret, - HKDFHash hash_alg = HKDFHash::SHA256); -}; - -} // namespace session -} // namespace common -} // namespace authservice -#endif // AUTHSERVICE_SRC_COMMON_SESSION_HKDF_DERIVER_H_ \ No newline at end of file diff --git a/src/common/session/session_id_generator.cc b/src/common/session/session_id_generator.cc deleted file mode 100644 index 5a80fac0..00000000 --- a/src/common/session/session_id_generator.cc +++ /dev/null @@ -1,15 +0,0 @@ -#include "session_id_generator.h" -#include "src/common/utilities/random.h" - -namespace authservice { -namespace common { -namespace session { - -std::string SessionIdGenerator::Generate() { - utilities::RandomGenerator generator; - return generator.Generate(64).Str(); -} - -} // namespace utilities -} // namespace common -} // namespace session diff --git a/src/common/session/session_id_generator.h b/src/common/session/session_id_generator.h deleted file mode 100644 index 2e22cef2..00000000 --- a/src/common/session/session_id_generator.h +++ /dev/null @@ -1,28 +0,0 @@ -#ifndef AUTHSERVICE_SESSION_ID_GENERATOR_H -#define AUTHSERVICE_SESSION_ID_GENERATOR_H - -#include -#include - -namespace authservice { -namespace common { -namespace session { - -class SessionIdGenerator; - -typedef std::shared_ptr SessionIdGeneratorPtr; - -class SessionIdGenerator { - public: - /** - * Generate a session ID - * @return The session ID as a string - */ - virtual std::string Generate(); -}; - -} // namespace session -} // namespace common -} // namespace authservice - -#endif //AUTHSERVICE_SESSION_ID_GENERATOR_H diff --git a/src/common/session/session_string_generator.cc b/src/common/session/session_string_generator.cc new file mode 100644 index 00000000..80e381a3 --- /dev/null +++ b/src/common/session/session_string_generator.cc @@ -0,0 +1,27 @@ +#include "session_string_generator.h" +#include "src/common/utilities/random.h" + +namespace authservice { +namespace common { +namespace session { + +std::string SessionStringGenerator::GenerateSessionId() { + return GenerateRandomString(64); +} + +std::string SessionStringGenerator::GenerateNonce() { + return GenerateRandomString(32); +} + +std::string SessionStringGenerator::GenerateState() { + return GenerateRandomString(32); +} + +std::string SessionStringGenerator::GenerateRandomString(int size) { + utilities::RandomGenerator generator; + return generator.Generate(size).Str(); +} + +} // namespace utilities +} // namespace common +} // namespace session diff --git a/src/common/session/session_string_generator.h b/src/common/session/session_string_generator.h new file mode 100644 index 00000000..ec470a22 --- /dev/null +++ b/src/common/session/session_string_generator.h @@ -0,0 +1,31 @@ +#ifndef AUTHSERVICE_SESSION_STRING_GENERATOR_H +#define AUTHSERVICE_SESSION_STRING_GENERATOR_H + +#include +#include + +namespace authservice { +namespace common { +namespace session { + +class SessionStringGenerator; + +typedef std::shared_ptr SessionStringGeneratorPtr; + +class SessionStringGenerator { +public: + virtual std::string GenerateSessionId(); + + virtual std::string GenerateNonce(); + + virtual std::string GenerateState(); + +private: + virtual std::string GenerateRandomString(int size); +}; + +} // namespace session +} // namespace common +} // namespace authservice + +#endif //AUTHSERVICE_SESSION_STRING_GENERATOR_H diff --git a/src/common/session/token_encryptor.cc b/src/common/session/token_encryptor.cc deleted file mode 100644 index 0d803ea1..00000000 --- a/src/common/session/token_encryptor.cc +++ /dev/null @@ -1,122 +0,0 @@ -#include "src/common/session/token_encryptor.h" -#include "absl/strings/escaping.h" -#include "src/common/session/gcm_encryptor.h" -#include "src/common/utilities/random.h" - -namespace authservice { -namespace common { -namespace session { - -namespace { -const size_t NONCE_SIZE = 32; -const size_t DERIVED_KEY_SIZE = 32; -} // namespace - -class TokenEncryptorImpl : public TokenEncryptor { - public: - TokenEncryptorImpl(const std::string& secret, EncryptionAlg enc_alg, - HKDFHash hash_alg); - - std::string Encrypt(const absl::string_view token) override; - absl::optional Decrypt(const std::string& ciphertext) override; - - private: - EncryptionAlg enc_alg_; - HkdfDeriverPtr deriver_; - utilities::RandomGenerator generator_; - - size_t KeySize() const; - - std::vector EncryptInternal( - const absl::string_view token, const std::vector& key) const; -}; - -TokenEncryptorImpl::TokenEncryptorImpl(const std::string& secret, - EncryptionAlg enc_alg, HKDFHash hash_alg) - : enc_alg_(enc_alg) { - // Get the secret from the config and use it and the claim nonce to derive a - // new AES-256 key - std::vector secret_vec(secret.begin(), secret.end()); - deriver_ = HkdfDeriver::Create(secret_vec, hash_alg); -} - -size_t TokenEncryptorImpl::KeySize() const { - switch (enc_alg_) { - case EncryptionAlg::AES128GCM: - return 16; - case EncryptionAlg::AES256GCM: - return 32; - default: - throw std::range_error("Unsupported encryption algorithm"); - } -} - -std::vector TokenEncryptorImpl::EncryptInternal( - const absl::string_view token, const std::vector& key) const { - switch (enc_alg_) { - case EncryptionAlg::AES128GCM: - case EncryptionAlg::AES256GCM: { - // Encrypt the JWT, using the derived key and a random nonce - // Ouput is: gcm_nonce || ciphertext || tag - auto encryptor = GcmEncryptor::Create(key); - std::vector tokenVec(token.begin(), token.end()); - auto encrypted = encryptor->Seal(tokenVec); - return encrypted; - } - default: - throw std::range_error("Unsupported encryption algorithm"); - } -} - -std::string TokenEncryptorImpl::Encrypt(const absl::string_view token) { - auto nonce = generator_.Generate(NONCE_SIZE); - std::vector nonce_vec(nonce.Begin(), nonce.End()); - auto derivedKey = deriver_->Derive(KeySize(), nonce_vec); - - auto encrypted = EncryptInternal(token, derivedKey); - - // Concatenate the claim nonce and the ciphertext - // Result is: derive_nonce || gcm_nonce || ciphertext || tag - std::vector output(nonce_vec); - output.insert(output.end(), encrypted.begin(), encrypted.end()); - - // UrlBase64 encode the final encrypted JWT - return absl::WebSafeBase64Escape(absl::string_view( - reinterpret_cast(output.data()), output.size())); -} - -absl::optional TokenEncryptorImpl::Decrypt( - const std::string& ciphertext) { - // UrlBase64 decode the token - std::string decoded; - if (!absl::WebSafeBase64Unescape(ciphertext, &decoded) || - decoded.size() < NONCE_SIZE) { - return absl::nullopt; - } - - std::vector nonce_vec(decoded.begin(), - decoded.begin() + NONCE_SIZE); - auto derivedKey = deriver_->Derive(DERIVED_KEY_SIZE, nonce_vec); - - // Decrypt the JWT - auto decryptor = GcmEncryptor::Create(derivedKey); - std::vector ciphertext_vec(decoded.begin() + NONCE_SIZE, - decoded.end()); - auto decrypted = decryptor->Open(ciphertext_vec); - - if (!decrypted) { - return absl::nullopt; - } - - return std::string(decrypted->begin(), decrypted->end()); -} - -TokenEncryptorPtr TokenEncryptor::Create(const std::string& secret, - EncryptionAlg enc_alg, - HKDFHash hash_alg) { - return std::make_shared(secret, enc_alg, hash_alg); -} - -} // namespace session -} // namespace common -} // namespace authservice \ No newline at end of file diff --git a/src/common/session/token_encryptor.h b/src/common/session/token_encryptor.h deleted file mode 100644 index 5d351476..00000000 --- a/src/common/session/token_encryptor.h +++ /dev/null @@ -1,61 +0,0 @@ -#ifndef AUTHSERVICE_SRC_COMMON_SESSION_TOKEN_ENCRYPTOR_H_ -#define AUTHSERVICE_SRC_COMMON_SESSION_TOKEN_ENCRYPTOR_H_ -#include -#include -#include "absl/types/optional.h" -#include "src/common/session/hkdf_deriver.h" -#include "absl/strings/string_view.h" - -namespace authservice { -namespace common { -namespace session { - -class TokenEncryptor; -typedef std::shared_ptr TokenEncryptorPtr; - -enum class EncryptionAlg { - AES128GCM, - AES256GCM, -}; - -/** Token encryption utility */ -class TokenEncryptor { - public: - virtual ~TokenEncryptor(){}; - - /** - * Encrypt the given token. - * @param token the token to encrypt and authenticate. - * @param nonce the nonce to use during encryption - * @return base64 string representing the encrypted/authenticated data - */ - virtual std::string Encrypt(const absl::string_view token) = 0; - - /** - * Decrypt the given token. - * @param ciphertext the data (nonce || ciphertext || tag) to be decrypted. - * @param aad additional authenticated data. - * @return plaintext string, or absl::nullopt if verification failed. - */ - virtual absl::optional Decrypt( - const std::string& ciphertext) = 0; - - /** - * Create an instance of a TokenEncryptor. - * @param secret base64 encoded data of the secret used to derive the - * encryption key. - * @param enc_alg encryption algorithm to be used for - * encryption/decryption. - * @param hash_alg hash algorithm to be used for key derivation. - * @return an instance of a TokenEncryptor. - */ - static TokenEncryptorPtr Create( - const std::string& secret, - EncryptionAlg enc_alg = EncryptionAlg::AES256GCM, - HKDFHash hash_alg = HKDFHash::SHA256); -}; - -} // namespace session -} // namespace common -} // namespace authservice -#endif // AUTHSERVICE_SRC_COMMON_SESSION_TOKEN_ENCRYPTOR_H_ \ No newline at end of file diff --git a/src/common/utilities/random.cc b/src/common/utilities/random.cc index f425b65e..66c6d426 100644 --- a/src/common/utilities/random.cc +++ b/src/common/utilities/random.cc @@ -25,14 +25,6 @@ bool Random::operator!=(const Random &rhs) const { return !(*this == rhs); } size_t Random::Size() const { return internal_buffer_.size(); } -std::vector::const_iterator Random::Begin() const { - return internal_buffer_.cbegin(); -} - -std::vector::const_iterator Random::End() const { - return internal_buffer_.cend(); -} - std::string Random::Str() const { return absl::WebSafeBase64Escape( absl::string_view(reinterpret_cast(internal_buffer_.data()), diff --git a/src/common/utilities/random.h b/src/common/utilities/random.h index 5d0e63d2..3ff99c7e 100644 --- a/src/common/utilities/random.h +++ b/src/common/utilities/random.h @@ -37,16 +37,6 @@ class Random { */ size_t Size() const; - /** - * An iterator to the first item of the internal buffer. - * @return A const iterator - */ - std::vector::const_iterator Begin() const; - /** - * An iterator to indicate an iterator has completed or is not valid. - * @return A const iterator - */ - std::vector::const_iterator End() const; /** * Encode a representation to string suitable for use in HTTP requests. * or unique state index. diff --git a/src/filters/filter_chain.cc b/src/filters/filter_chain.cc index 53478dfb..6e5a2edf 100644 --- a/src/filters/filter_chain.cc +++ b/src/filters/filter_chain.cc @@ -53,12 +53,7 @@ std::unique_ptr FilterChainImpl::New() { google::jwt_verify::Jwks::createFrom( filter.oidc().jwks(), google::jwt_verify::Jwks::Type::JWKS)); - auto token_encryptor = common::session::TokenEncryptor::Create( - filter.oidc().cryptor_secret(), - common::session::EncryptionAlg::AES256GCM, - common::session::HKDFHash::SHA512); - - auto session_id_generator = std::make_shared(); + auto session_string_generator = std::make_shared(); auto http = common::http::ptr_t(new common::http::http_impl); @@ -76,7 +71,7 @@ std::unique_ptr FilterChainImpl::New() { } result->AddFilter(filters::FilterPtr(new filters::oidc::OidcFilter( - http, filter.oidc(), token_request_parser, token_encryptor, session_id_generator, oidc_session_store_))); + http, filter.oidc(), token_request_parser, session_string_generator, oidc_session_store_))); } return result; } diff --git a/src/filters/oidc/BUILD b/src/filters/oidc/BUILD index 30163b07..fd8797f8 100644 --- a/src/filters/oidc/BUILD +++ b/src/filters/oidc/BUILD @@ -2,16 +2,6 @@ load("//bazel:bazel.bzl", "xx_library") package(default_visibility = ["//visibility:public"]) -xx_library( - name = "state_cookie_codec", - srcs = ["state_cookie_codec.cc"], - hdrs = ["state_cookie_codec.h"], - deps = [ - "@com_github_abseil-cpp//absl/strings:strings", - "@com_github_abseil-cpp//absl/types:optional", - ], -) - xx_library( name = "token_response", srcs = ["token_response.cc"], @@ -47,11 +37,9 @@ xx_library( deps = [ "//config/oidc:config_cc", "//src/common/http", - "//src/common/session:token_encryptor", - "//src/common/session:session_id_generator", + "//src/common/session:session_string_generator", "//src/common/utilities:random", "//src/filters:filter", - "//src/filters/oidc:state_cookie_codec", "//src/filters/oidc:in_memory_session_store", "//src/common/utilities:time_service", "//src/filters/oidc:token_response", diff --git a/src/filters/oidc/in_memory_session_store.cc b/src/filters/oidc/in_memory_session_store.cc index 7f60bac9..5b9d5b4a 100644 --- a/src/filters/oidc/in_memory_session_store.cc +++ b/src/filters/oidc/in_memory_session_store.cc @@ -8,7 +8,7 @@ namespace oidc { class Session { private: std::shared_ptr token_response_; - absl::optional requested_url_; + std::shared_ptr authorization_state_; uint32_t time_added_; uint32_t time_accessed_; @@ -27,29 +27,29 @@ class Session { return time_accessed_; } - inline void SetTokenResponse(const std::shared_ptr tokenResponse) { - token_response_ = tokenResponse; + inline void SetTokenResponse(std::shared_ptr token_response) { + token_response_ = token_response; } inline std::shared_ptr GetTokenResponse() { return token_response_; } - inline void SetRequestedURL(absl::string_view requestedURL) { - requested_url_ = requestedURL.data(); + inline void SetAuthorizationState(std::shared_ptr authorization_state) { + authorization_state_ = authorization_state; } - inline void ClearRequestedURL() { - requested_url_ = absl::nullopt; + inline std::shared_ptr GetAuthorizationState() { + return authorization_state_; } - inline absl::optional GetRequestedURL() { - return requested_url_; + inline void ClearAuthorizationState() { + authorization_state_ = nullptr; } }; Session::Session(uint32_t time_added) - : token_response_(nullptr), requested_url_(absl::nullopt), time_added_(time_added), + : token_response_(nullptr), authorization_state_(nullptr), time_added_(time_added), time_accessed_(time_added) {} InMemorySessionStore::InMemorySessionStore(std::shared_ptr time_service, @@ -60,18 +60,10 @@ InMemorySessionStore::InMemorySessionStore(std::shared_ptr token_response) { - synchronized(mutex_) { - auto session_optional = FindSession(session_id); - if (session_optional.has_value()) { - auto session = session_optional.value(); - session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - session->SetTokenResponse(token_response); - } else { - auto new_session = std::make_shared(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - new_session->SetTokenResponse(token_response); - session_map_.emplace(session_id.data(), new_session); - } - } + std::function token_response_setter = [&token_response](Session &session) { + session.SetTokenResponse(token_response); + }; + Set(session_id, token_response_setter); } std::shared_ptr InMemorySessionStore::GetTokenResponse(absl::string_view session_id) { @@ -86,41 +78,36 @@ std::shared_ptr InMemorySessionStore::GetTokenResponse(absl::stri } } -void InMemorySessionStore::SetRequestedURL(absl::string_view session_id, absl::string_view requested_url) { - synchronized(mutex_) { - auto session_optional = FindSession(session_id); - if (session_optional.has_value()) { - auto session = session_optional.value(); - session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - session->SetRequestedURL(requested_url); - } else { - auto new_session = std::make_shared(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - new_session->SetRequestedURL(requested_url); - session_map_.emplace(session_id.data(), new_session); - } - } -} +void InMemorySessionStore::RemoveAllExpired() { + auto earliest_time_added_to_keep = + time_service_->GetCurrentTimeInSecondsSinceEpoch() - max_absolute_session_timeout_in_seconds_; + auto earliest_time_idle_to_keep = + time_service_->GetCurrentTimeInSecondsSinceEpoch() - max_session_idle_timeout_in_seconds_; + + bool should_check_absolute_timeout = max_absolute_session_timeout_in_seconds_ > 0; + bool should_check_idle_timeout = max_session_idle_timeout_in_seconds_ > 0; -void InMemorySessionStore::ClearRequestedURL(absl::string_view session_id) { synchronized(mutex_) { - auto session_optional = FindSession(session_id); - if (session_optional.has_value()) { - auto session = session_optional.value(); - session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - session->ClearRequestedURL(); + auto itr = session_map_.begin(); + while (itr != session_map_.end()) { + auto session = itr->second; + bool expired_based_on_time_added = session->GetTimeAdded() < earliest_time_added_to_keep; + bool expired_based_on_idle_time = + session->GetTimeMostRecentlyAccessed() < earliest_time_idle_to_keep; + + if ((should_check_absolute_timeout && expired_based_on_time_added) || + (should_check_idle_timeout && expired_based_on_idle_time)) { + itr = session_map_.erase(itr); + } else { + itr++; + } } } } -absl::optional InMemorySessionStore::GetRequestedURL(absl::string_view session_id) { +void InMemorySessionStore::RemoveSession(absl::string_view session_id) { synchronized(mutex_) { - auto session_optional = FindSession(session_id); - if (!session_optional.has_value()) { - return absl::nullopt; - } - auto session = session_optional.value(); - session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); - return absl::optional(session->GetRequestedURL()); + session_map_.erase(session_id.data()); } } @@ -134,35 +121,48 @@ absl::optional> InMemorySessionStore::FindSession(absl: } } -void InMemorySessionStore::RemoveSession(absl::string_view session_id) { +void InMemorySessionStore::SetAuthorizationState(absl::string_view session_id, + std::shared_ptr authorization_state) { + std::function authorization_state_setter = [&authorization_state](Session &session) { + session.SetAuthorizationState(authorization_state); + }; + Set(session_id, authorization_state_setter); +} + +std::shared_ptr InMemorySessionStore::GetAuthorizationState(absl::string_view session_id) { synchronized(mutex_) { - session_map_.erase(session_id.data()); + auto session_optional = FindSession(session_id); + if (!session_optional.has_value()) { + return nullptr; + } + auto session = session_optional.value(); + session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); + return session->GetAuthorizationState(); } } -void InMemorySessionStore::RemoveAllExpired() { - auto earliest_time_added_to_keep = - time_service_->GetCurrentTimeInSecondsSinceEpoch() - max_absolute_session_timeout_in_seconds_; - auto earliest_time_idle_to_keep = - time_service_->GetCurrentTimeInSecondsSinceEpoch() - max_session_idle_timeout_in_seconds_; - - bool should_check_absolute_timeout = max_absolute_session_timeout_in_seconds_ > 0; - bool should_check_idle_timeout = max_session_idle_timeout_in_seconds_ > 0; - +void InMemorySessionStore::ClearAuthorizationState(absl::string_view session_id) { synchronized(mutex_) { - auto itr = session_map_.begin(); - while (itr != session_map_.end()) { - auto session = itr->second; - bool expired_based_on_time_added = session->GetTimeAdded() < earliest_time_added_to_keep; - bool expired_based_on_idle_time = - session->GetTimeMostRecentlyAccessed() < earliest_time_idle_to_keep; + auto session_optional = FindSession(session_id); + if (session_optional.has_value()) { + auto session = session_optional.value(); + session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); + session->ClearAuthorizationState(); + } + } +} - if ((should_check_absolute_timeout && expired_based_on_time_added) || - (should_check_idle_timeout && expired_based_on_idle_time)) { - itr = session_map_.erase(itr); - } else { - itr++; - } +void InMemorySessionStore::Set(absl::string_view session_id, std::function &lambda) { + synchronized(mutex_) { + auto session_optional = FindSession(session_id); + if (session_optional.has_value()) { + auto session = session_optional.value(); + session->SetTimeMostRecentlyAccessed(time_service_->GetCurrentTimeInSecondsSinceEpoch()); + lambda(*session); + } else { + auto new_session = std::make_shared(time_service_->GetCurrentTimeInSecondsSinceEpoch()); + lambda(*new_session); + session_map_.emplace(session_id.data(), new_session); } } } diff --git a/src/filters/oidc/in_memory_session_store.h b/src/filters/oidc/in_memory_session_store.h index 27cdfc89..633d0f41 100644 --- a/src/filters/oidc/in_memory_session_store.h +++ b/src/filters/oidc/in_memory_session_store.h @@ -18,8 +18,11 @@ class InMemorySessionStore : public SessionStore { uint32_t max_absolute_session_timeout_in_seconds_; uint32_t max_session_idle_timeout_in_seconds_; std::recursive_mutex mutex_; + virtual absl::optional> FindSession(absl::string_view session_id); + virtual void Set(absl::string_view session_id, std::function &lambda); + public: InMemorySessionStore( std::shared_ptr time_service, @@ -28,13 +31,13 @@ class InMemorySessionStore : public SessionStore { virtual void SetTokenResponse(absl::string_view session_id, std::shared_ptr token_response) override; - virtual void SetRequestedURL(absl::string_view session_id, absl::string_view requested_url) override; + virtual std::shared_ptr GetTokenResponse(absl::string_view session_id) override; - virtual void ClearRequestedURL(absl::string_view session_id) override; + virtual void SetAuthorizationState(absl::string_view session_id, std::shared_ptr authorization_state) override; - virtual std::shared_ptr GetTokenResponse(absl::string_view session_id) override; + virtual std::shared_ptr GetAuthorizationState(absl::string_view session_id) override; - virtual absl::optional GetRequestedURL(absl::string_view session_id) override; + virtual void ClearAuthorizationState(absl::string_view session_id) override; virtual void RemoveSession(absl::string_view session_id) override; diff --git a/src/filters/oidc/oidc_filter.cc b/src/filters/oidc/oidc_filter.cc index 6c186467..acece485 100644 --- a/src/filters/oidc/oidc_filter.cc +++ b/src/filters/oidc/oidc_filter.cc @@ -6,11 +6,8 @@ #include "spdlog/spdlog.h" #include "src/common/http/headers.h" #include "src/common/http/http.h" -#include "src/common/utilities/random.h" #include "src/common/utilities/time_service.h" -#include "state_cookie_codec.h" #include -#include namespace beast = boost::beast; // from namespace http = beast::http; // from @@ -36,14 +33,12 @@ const std::map standard_headers = { OidcFilter::OidcFilter(common::http::ptr_t http_ptr, const authservice::config::oidc::OIDCConfig &idp_config, TokenResponseParserPtr parser, - common::session::TokenEncryptorPtr cryptor, - common::session::SessionIdGeneratorPtr session_id_generator, + common::session::SessionStringGeneratorPtr session_string_generator, SessionStorePtr session_store) : http_ptr_(http_ptr), idp_config_(idp_config), parser_(parser), - cryptor_(cryptor), - session_id_generator_(session_id_generator), + session_string_generator_(session_string_generator), session_store_(session_store) { spdlog::trace("{}", __func__); } @@ -170,11 +165,40 @@ google::rpc::Code OidcFilter::RedirectToIdp( const AttributeContext_HttpRequest &httpRequest, absl::optional old_session_id) { if (old_session_id.has_value()) { + // remove old session and regenerate session_id to prevent session fixation attacks session_store_->RemoveSession(old_session_id.value()); } - auto session_id = session_id_generator_->Generate(); - SetRedirectToIdpHeaders(response, session_id); - session_store_->SetRequestedURL(session_id, GetRequestUrl(httpRequest)); + + auto session_id = session_string_generator_->GenerateSessionId(); + auto state = session_string_generator_->GenerateState(); + auto nonce = session_string_generator_->GenerateNonce(); + + std::set scopes = {mandatory_scope_}; + for (const auto &scope : idp_config_.scopes()) { + scopes.insert(scope); + } + + auto callback = common::http::http::ToUrl(idp_config_.callback()); + auto encoded_scopes = absl::StrJoin(scopes, " "); + std::multimap params = { + {"response_type", "code"}, + {"scope", encoded_scopes}, + {"client_id", idp_config_.client_id()}, + {"nonce", nonce}, + {"state", state}, + {"redirect_uri", callback}}; + auto query = common::http::http::EncodeQueryData(params); + + SetStandardResponseHeaders(response); + + auto redirect_location = absl::StrJoin({common::http::http::ToUrl(idp_config_.authorization()), query}, "?"); + SetRedirectHeaders(redirect_location, response); + + session_store_->SetAuthorizationState(session_id.data(), + std::make_shared(state, nonce, GetRequestUrl(httpRequest))); + + SetSessionIdCookie(response, session_id.data()); + return google::rpc::UNAUTHENTICATED; } @@ -231,10 +255,6 @@ std::string OidcFilter::GetCookieName(const std::string &cookie) const { cookie + "-cookie"; } -std::string OidcFilter::GetStateCookieName() const { - return GetCookieName("state"); -} - std::string OidcFilter::GetSessionIdCookieName() const { return GetCookieName("session-id"); } @@ -259,15 +279,6 @@ void OidcFilter::SetCookie( SetHeader(responseHeaders, common::http::headers::SetCookie, cookie_header); } -void OidcFilter::SetEncryptedCookie( - ::google::protobuf::RepeatedPtrField<::envoy::api::v2::core::HeaderValueOption> *responseHeaders, - const std::string &cookie_name, - absl::string_view value_to_be_encrypted, - int64_t timeout -) { - SetCookie(responseHeaders, cookie_name, cryptor_->Encrypt(value_to_be_encrypted), timeout); -} - void OidcFilter::DeleteCookie( ::google::protobuf::RepeatedPtrField<::envoy::api::v2::core::HeaderValueOption> *responseHeaders, const std::string &cookieName @@ -309,45 +320,10 @@ absl::optional OidcFilter::CookieFromHeaders( return absl::nullopt; } -void OidcFilter::SetRedirectToIdpHeaders(::envoy::service::auth::v2::CheckResponse *response, std::string session_id) { - common::utilities::RandomGenerator generator; - auto state = generator.Generate(32).Str(); - auto nonce = generator.Generate(32).Str(); - std::set scopes = {mandatory_scope_}; - for (const auto &scope : idp_config_.scopes()) { - scopes.insert(scope); - } - - auto callback = common::http::http::ToUrl(idp_config_.callback()); - auto encoded_scopes = absl::StrJoin(scopes, " "); - std::multimap params = { - {"response_type", "code"}, - {"scope", encoded_scopes}, - {"client_id", idp_config_.client_id()}, - {"nonce", nonce}, - {"state", state}, - {"redirect_uri", callback}}; - auto query = common::http::http::EncodeQueryData(params); - - SetStandardResponseHeaders(response); - - auto redirect_location = absl::StrJoin({common::http::http::ToUrl(idp_config_.authorization()), query}, "?"); - SetRedirectHeaders(redirect_location, response); - - // Create a secure state cookie that contains the state and nonce. - StateCookieCodec codec; - SetEncryptedCookie(response->mutable_denied_response()->mutable_headers(), GetStateCookieName(), - codec.Encode(state, nonce), idp_config_.timeout()); - - // Set a fresh session id to alleviate Session fixation attack - SetSessionIdCookie(response, session_id); -} - void OidcFilter::SetLogoutHeaders(CheckResponse *response) { SetRedirectHeaders(idp_config_.logout().redirect_to_uri(), response); SetStandardResponseHeaders(response); auto responseHeaders = response->mutable_denied_response()->mutable_headers(); - DeleteCookie(responseHeaders, GetStateCookieName()); DeleteCookie(responseHeaders, GetSessionIdCookieName()); } @@ -499,45 +475,30 @@ google::rpc::Code OidcFilter::RetrieveToken( boost::asio::yield_context yield) { spdlog::trace("{}", __func__); - auto query = RequestQueryString(request); SetStandardResponseHeaders(response); - // Best effort at deleting state cookie for all cases. - auto responseHeaders = response->mutable_denied_response()->mutable_headers(); - DeleteCookie(responseHeaders, GetStateCookieName()); - - // Extract state and nonce from encrypted cookie. - auto encrypted_state_cookie = CookieFromHeaders( - request->attributes().request().http().headers(), GetStateCookieName()); - if (!encrypted_state_cookie.has_value()) { - spdlog::info("{}: missing state cookie", __func__); - return google::rpc::Code::INVALID_ARGUMENT; - } - auto state_cookie = cryptor_->Decrypt(encrypted_state_cookie.value()); - if (!state_cookie.has_value()) { - spdlog::info("{}: invalid state cookie", __func__); - return google::rpc::Code::INVALID_ARGUMENT; - } - StateCookieCodec codec; - auto state_and_nonce = codec.Decode(state_cookie.value()); - if (!state_and_nonce.has_value()) { - spdlog::info("{}: invalid state cookie encoding", __func__); - return google::rpc::Code::INVALID_ARGUMENT; - } - // Extract expected state and authorization code from request + auto query = RequestQueryString(request); auto query_data = common::http::http::DecodeQueryData(query); if (!query_data.has_value()) { spdlog::info("{}: form data is invalid", __func__); return google::rpc::Code::INVALID_ARGUMENT; } - const auto state = query_data->find("state"); - const auto code = query_data->find("code"); - if (state == query_data->end() || code == query_data->end()) { + const auto state_from_request = query_data->find("state"); + const auto code_from_request = query_data->find("code"); + if (state_from_request == query_data->end() || code_from_request == query_data->end()) { spdlog::info("{}: form data does not contain expected state and code parameters", __func__); return google::rpc::Code::INVALID_ARGUMENT; } - if (state->second != state_and_nonce->first) { + + auto authorization_state = session_store_->GetAuthorizationState(session_id); + if (!authorization_state) { + spdlog::info("{}: Missing state, nonce, and original url requested by the user. Cannot redirect.", __func__); + return google::rpc::Code::UNAVAILABLE; + } + + // Compare state from request and session + if (state_from_request->second != authorization_state->GetState()) { spdlog::info("{}: mismatch state", __func__); return google::rpc::Code::INVALID_ARGUMENT; } @@ -552,7 +513,7 @@ google::rpc::Code OidcFilter::RetrieveToken( // Build body auto redirect_uri = common::http::http::ToUrl(idp_config_.callback()); std::multimap params = { - {"code", code->second}, + {"code", code_from_request->second}, {"redirect_uri", redirect_uri}, {"grant_type", "authorization_code"}, }; @@ -569,7 +530,7 @@ google::rpc::Code OidcFilter::RetrieveToken( retrieve_token_response->result_int()); return google::rpc::Code::UNKNOWN; } else { - auto nonce = std::string(state_and_nonce->second.data(), state_and_nonce->second.size()); + auto nonce = authorization_state->GetNonce(); auto token_response = parser_->Parse(idp_config_.client_id(), nonce, retrieve_token_response->body()); if (!token_response) { spdlog::info("{}: Invalid token response", __func__); @@ -586,19 +547,12 @@ google::rpc::Code OidcFilter::RetrieveToken( } } - auto optional_originally_requested_url = session_store_->GetRequestedURL(session_id); - - if (!optional_originally_requested_url.has_value()) { - spdlog::info("{}: Missing original url requested by the user, cannot redirect", __func__); - return google::rpc::Code::UNAVAILABLE; - } - - session_store_->ClearRequestedURL(session_id); + session_store_->ClearAuthorizationState(session_id); spdlog::info("{}: Saving token response to session store", __func__); session_store_->SetTokenResponse(session_id, token_response); - SetRedirectHeaders(optional_originally_requested_url.value(), response); + SetRedirectHeaders(authorization_state->GetRequestedUrl(), response); return google::rpc::Code::UNAUTHENTICATED; } } diff --git a/src/filters/oidc/oidc_filter.h b/src/filters/oidc/oidc_filter.h index dfeb302e..5a9fe189 100644 --- a/src/filters/oidc/oidc_filter.h +++ b/src/filters/oidc/oidc_filter.h @@ -4,11 +4,10 @@ #include "config/oidc/config.pb.h" #include "google/rpc/code.pb.h" #include "src/common/http/http.h" -#include "src/common/session/token_encryptor.h" #include "src/filters/filter.h" #include "src/filters/oidc/token_response.h" #include "src/common/utilities/random.h" -#include "src/common/session/session_id_generator.h" +#include "src/common/session/session_string_generator.h" #include "src/filters/oidc/session_store.h" #include @@ -28,8 +27,7 @@ class OidcFilter final : public filters::Filter { common::http::ptr_t http_ptr_; const authservice::config::oidc::OIDCConfig idp_config_; TokenResponseParserPtr parser_; - common::session::TokenEncryptorPtr cryptor_; - common::session::SessionIdGeneratorPtr session_id_generator_; + common::session::SessionStringGeneratorPtr session_string_generator_; SessionStorePtr session_store_; /** @@ -77,17 +75,6 @@ class OidcFilter final : public filters::Filter { void SetCookie(::google::protobuf::RepeatedPtrField<::envoy::api::v2::core::HeaderValueOption> *responseHeaders, const std::string &cookie_name, absl::string_view value, int64_t timeout); - /** @brief Set cookie. - * - * @param responseHeaders The headers to add to. - * @param cookie_name The key name of the cookie to be set. - * @param value_to_be_encrypted The value of the cookie, which will be encrypted in the cookie. - * @param timeout The lifetime in seconds the cookie is valid for before browsers should not honor this cookie. - */ - void SetEncryptedCookie( - ::google::protobuf::RepeatedPtrField<::envoy::api::v2::core::HeaderValueOption> *responseHeaders, - const std::string &cookie_name, absl::string_view value_to_be_encrypted, int64_t timeout); - /** @brief Extract the requested cookie from the given headers * * @param headers the headers to extract the cookies from @@ -98,13 +85,6 @@ class OidcFilter final : public filters::Filter { const ::google::protobuf::Map<::std::string, ::std::string> &headers, const std::string &cookie); - /** @brief Set IdP redirect parameters - * - * Set IdP redirect parameters so that a requesting agent is forced to - * authenticate the user. - */ - void SetRedirectToIdpHeaders(::envoy::service::auth::v2::CheckResponse *response, std::string session_id); - google::rpc::Code RedirectToIdp( CheckResponse *response, const AttributeContext_HttpRequest &httpRequest, @@ -206,8 +186,7 @@ class OidcFilter final : public filters::Filter { OidcFilter(common::http::ptr_t http_ptr, const authservice::config::oidc::OIDCConfig &idp_config, TokenResponseParserPtr parser, - common::session::TokenEncryptorPtr cryptor, - common::session::SessionIdGeneratorPtr session_id_generator, + common::session::SessionStringGeneratorPtr session_string_generator, SessionStorePtr session_store); google::rpc::Code Process( @@ -221,9 +200,6 @@ class OidcFilter final : public filters::Filter { absl::string_view Name() const override; - /** @brief Get state cookie name. */ - std::string GetStateCookieName() const; - /** @brief Get sessionID cookie name */ std::string GetSessionIdCookieName() const; }; diff --git a/src/filters/oidc/session_store.h b/src/filters/oidc/session_store.h index 81963920..483778e6 100644 --- a/src/filters/oidc/session_store.h +++ b/src/filters/oidc/session_store.h @@ -11,18 +11,42 @@ class SessionStore; typedef std::shared_ptr SessionStorePtr; +class AuthorizationState { +private: + std::string state_; + std::string nonce_; + std::string requested_url_; + +public: + AuthorizationState(absl::string_view state, absl::string_view nonce, absl::string_view requestedUrl) : + state_(state.data()), nonce_(nonce.data()), requested_url_(requestedUrl.data()) {} + +public: + inline std::string &GetState() { + return state_; + } + + inline std::string &GetNonce() { + return nonce_; + } + + inline std::string &GetRequestedUrl() { + return requested_url_; + } +}; + class SessionStore { public: virtual void SetTokenResponse(absl::string_view session_id, std::shared_ptr token_response) = 0; - virtual void SetRequestedURL(absl::string_view session_id, absl::string_view requested_url) = 0; + virtual std::shared_ptr GetTokenResponse(absl::string_view session_id) = 0; - virtual void ClearRequestedURL(absl::string_view session_id) = 0; + virtual void SetAuthorizationState(absl::string_view session_id, std::shared_ptr authorization_state) = 0; - virtual std::shared_ptr GetTokenResponse(absl::string_view session_id) = 0; + virtual std::shared_ptr GetAuthorizationState(absl::string_view session_id) = 0; - virtual absl::optional GetRequestedURL(absl::string_view session_id) = 0; + virtual void ClearAuthorizationState(absl::string_view session_id) = 0; virtual void RemoveSession(absl::string_view session_id) = 0; diff --git a/src/filters/oidc/state_cookie_codec.cc b/src/filters/oidc/state_cookie_codec.cc deleted file mode 100644 index cad2eeaf..00000000 --- a/src/filters/oidc/state_cookie_codec.cc +++ /dev/null @@ -1,27 +0,0 @@ -#include "state_cookie_codec.h" -#include -#include "absl/strings/str_join.h" -#include "absl/strings/str_split.h" -namespace authservice { -namespace filters { -namespace oidc { -namespace { -const char *separator = ";"; -} -std::string StateCookieCodec::Encode(absl::string_view state, - absl::string_view nonce) { - return absl::StrJoin({state, nonce}, separator); -} - -absl::optional> -StateCookieCodec::Decode(absl::string_view value) { - std::vector values = absl::StrSplit(value, separator); - if (values.size() != 2) { - return absl::nullopt; - } - return std::pair(values[0], values[1]); -} - -} // namespace oidc -} // namespace filters -} // namespace authservice \ No newline at end of file diff --git a/src/filters/oidc/state_cookie_codec.h b/src/filters/oidc/state_cookie_codec.h deleted file mode 100644 index 824f771c..00000000 --- a/src/filters/oidc/state_cookie_codec.h +++ /dev/null @@ -1,34 +0,0 @@ -#ifndef AUTHSERVICE_SRC_FILTERS_OIDC_STATE_COOKIE_CODEC_H_ -#define AUTHSERVICE_SRC_FILTERS_OIDC_STATE_COOKIE_CODEC_H_ - -#include -#include "absl/strings/string_view.h" -#include "absl/types/optional.h" -namespace authservice { -namespace filters { -namespace oidc { -/** - * Encoder, Decoder for a set of value to placed into a cookie. - */ -class StateCookieCodec { - public: - /** - * Encode the given state values - * @param state data to be encoded. - * @param nonce data to be encoded. - * @return the encoded value - */ - std::string Encode(absl::string_view state, absl::string_view nonce); - /** - * Decode the given state cookie value into a state and nonce pair. - * @param value the value to decode - * @return the decoded data. - */ - absl::optional> Decode( - absl::string_view value); -}; - -} // namespace oidc -} // namespace filters -} // namespace authservice -#endif // AUTHSERVICE_SRC_FILTERS_OIDC_STATE_COOKIE_CODEC_H_ diff --git a/test/common/session/BUILD b/test/common/session/BUILD index fdf9ab14..8fbaa7d8 100644 --- a/test/common/session/BUILD +++ b/test/common/session/BUILD @@ -3,48 +3,16 @@ cc_library( hdrs = ["mocks.h"], visibility = ["//test:__subpackages__"], deps = [ - "//src/common/session:token_encryptor", - "//src/common/session:session_id_generator" + "//src/common/session:session_string_generator" ], ) cc_test( - name = "hkdf_deriver_test", - srcs = ["hkdf_deriver_test.cc"], + name = "session_string_generator_test", + srcs = ["session_string_generator_test.cc"], deps = [ - "//src/common/session:hkdf", + "//src/common/session:session_string_generator", "@com_google_googletest//:gtest_main", ], linkstatic = select({"@boost//:osx": True, "//conditions:default": False}), # workaround for not being able to figure out how to link dynamically on MacOS ) - -cc_test( - name = "gcm_encryptor_test", - srcs = ["gcm_encryptor_test.cc"], - deps = [ - "//src/common/session:gcm_encryptor", - "@com_google_googletest//:gtest_main", - ], - linkstatic = select({"@boost//:osx": True, "//conditions:default": False}), # workaround for not being able to figure out how to link dynamically on MacOS -) - -cc_test( - name = "session_id_generator_test", - srcs = ["session_id_generator_test.cc"], - deps = [ - "//src/common/session:session_id_generator", - "@com_google_googletest//:gtest_main", - ], - linkstatic = select({"@boost//:osx": True, "//conditions:default": False}), # workaround for not being able to figure out how to link dynamically on MacOS -) - -cc_test( - name = "token_encryptor_test", - srcs = ["token_encryptor_test.cc"], - deps = [ - "//src/common/session:token_encryptor", - "@com_google_googletest//:gtest_main", - "@com_googlesource_boringssl//:crypto", - ], - linkstatic = select({"@boost//:osx": True, "//conditions:default": False}), # workaround for not being able to figure out how to link dynamically on MacOS -) diff --git a/test/common/session/gcm_encryptor_test.cc b/test/common/session/gcm_encryptor_test.cc deleted file mode 100644 index 71cbc7c6..00000000 --- a/test/common/session/gcm_encryptor_test.cc +++ /dev/null @@ -1,52 +0,0 @@ -#include "src/common/session/gcm_encryptor.h" - -#include "gtest/gtest.h" - -namespace authservice { -namespace common { -namespace session { - -namespace { -// Test vectors obtained from NIST: -// https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/cavp-testing-block-cipher-modes -std::vector key = { - 0x26, 0x8e, 0xd1, 0xb5, 0xd7, 0xc9, 0xc7, 0x30, 0x4f, 0x9c, 0xae, - 0x5f, 0xc4, 0x37, 0xb4, 0xcd, 0x3a, 0xeb, 0xe2, 0xec, 0x65, 0xf0, - 0xd8, 0x5c, 0x39, 0x18, 0xd3, 0xd3, 0xb5, 0xbb, 0xa8, 0x9b, -}; -std::vector iv = {0x9e, 0xd9, 0xd8, 0x18, 0x05, 0x64, - 0xe0, 0xe9, 0x45, 0xf5, 0xe5, 0xd4}; -std::vector pt = { - 0xfe, 0x29, 0xa4, 0x0d, 0x8e, 0xbf, 0x57, 0x26, 0x2b, 0xdb, 0x87, - 0x19, 0x1d, 0x01, 0x84, 0x3f, 0x4c, 0xa4, 0xb2, 0xde, 0x97, 0xd8, - 0x82, 0x73, 0x15, 0x4a, 0x0b, 0x7d, 0x9e, 0x2f, 0xdb, 0x80}; -std::vector aad = {}; -std::vector ct = { - 0x79, 0x1a, 0x4a, 0x02, 0x6f, 0x16, 0xf3, 0xa5, 0xea, 0x06, 0x27, - 0x4b, 0xf0, 0x2b, 0xaa, 0xb4, 0x69, 0x86, 0x0a, 0xbd, 0xe5, 0xe6, - 0x45, 0xf3, 0xdd, 0x47, 0x3a, 0x5a, 0xcd, 0xde, 0xec, 0xfc}; -std::vector tag = {0x05, 0xb2, 0xb7, 0x4d, 0xb0, 0x66, - 0x25, 0x50, 0x43, 0x5e, 0xf1, 0x90, - 0x0e, 0x13, 0x6b, 0x15}; -} // namespace - -TEST(GcmEncryptorTest, SealAndOpen) { - auto encryptor = GcmEncryptor::Create(key); - - // Expected output is 'nonce || ciphertext || tag' - std::vector expected; - expected.insert(expected.end(), iv.begin(), iv.end()); - expected.insert(expected.end(), ct.begin(), ct.end()); - expected.insert(expected.end(), tag.begin(), tag.end()); - - auto sealed = encryptor->Seal(pt, iv, aad); - EXPECT_EQ(sealed, expected); - - auto opened = encryptor->Open(sealed, aad); - ASSERT_TRUE(opened); - EXPECT_EQ(*opened, pt); -}; - -} // namespace session -} // namespace common -} // namespace authservice diff --git a/test/common/session/hkdf_deriver_test.cc b/test/common/session/hkdf_deriver_test.cc deleted file mode 100644 index d1ffa29e..00000000 --- a/test/common/session/hkdf_deriver_test.cc +++ /dev/null @@ -1,41 +0,0 @@ -#include "src/common/session/hkdf_deriver.h" -#include "gtest/gtest.h" - -namespace authservice { -namespace common { -namespace session { - -namespace { -// Test vectors obtained from RFC-5869 (Test Case 1) -// https://tools.ietf.org/html/rfc5869 -const std::vector ikm = { - 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, - 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b}; -const std::vector salt = {0x00, 0x01, 0x02, 0x03, 0x04, - 0x05, 0x06, 0x07, 0x08, 0x09, - 0x0a, 0x0b, 0x0c}; -const std::vector info = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, - 0xf5, 0xf6, 0xf7, 0xf8, 0xf9}; -const std::vector l = {0x42}; - -const std::vector prk = { - 0x07, 0x77, 0x09, 0x36, 0x2c, 0x2e, 0x32, 0xdf, 0x0d, 0xdc, 0x3f, - 0x0d, 0xc4, 0x7b, 0xba, 0x63, 0x90, 0xb6, 0xc7, 0x3b, 0xb5, 0x0f, - 0x9c, 0x31, 0x22, 0xec, 0x84, 0x4a, 0xd7, 0xc2, 0xb3, 0xe5}; -const std::vector okm = { - 0x3c, 0xb2, 0x5f, 0x25, 0xfa, 0xac, 0xd5, 0x7a, 0x90, 0x43, 0x4f, - 0x64, 0xd0, 0x36, 0x2f, 0x2a, 0x2d, 0x2d, 0x0a, 0x90, 0xcf, 0x1a, - 0x5a, 0x4c, 0x5d, 0xb0, 0x2d, 0x56, 0xec, 0xc4, 0xc5, 0xbf, 0x34, - 0x00, 0x72, 0x08, 0xd5, 0xb8, 0x87, 0x18, 0x58, 0x65}; -} // namespace - -TEST(HkdfDeriverTest, DeriveKey) { - auto deriver = HkdfDeriver::Create(ikm); - auto result = deriver->Derive(42, salt, info); - - EXPECT_EQ(result, okm); -}; - -} // namespace session -} // namespace common -} // namespace authservice diff --git a/test/common/session/mocks.h b/test/common/session/mocks.h index 903a1268..e99d430e 100644 --- a/test/common/session/mocks.h +++ b/test/common/session/mocks.h @@ -2,22 +2,17 @@ #define AUTHSERVICE_TEST_COMMON_SESSION_MOCKS_H_ #include "gmock/gmock.h" -#include "src/common/session/token_encryptor.h" -#include "src/common/session/session_id_generator.h" +#include "src/common/session/session_string_generator.h" namespace authservice { namespace common { namespace session { -class TokenEncryptorMock final : public TokenEncryptor { +class SessionStringGeneratorMock final : public SessionStringGenerator { public: - MOCK_METHOD1(Encrypt, std::string(const absl::string_view token)); - MOCK_METHOD1(Decrypt, absl::optional(const std::string &ciphertext)); -}; - -class SessionIdGeneratorMock final : public SessionIdGenerator { -public: - MOCK_METHOD(std::string, Generate, ()); + MOCK_METHOD(std::string, GenerateSessionId, ()); + MOCK_METHOD(std::string, GenerateState, ()); + MOCK_METHOD(std::string, GenerateNonce, ()); }; } // namespace session diff --git a/test/common/session/session_id_generator_test.cc b/test/common/session/session_id_generator_test.cc deleted file mode 100644 index e3fc3013..00000000 --- a/test/common/session/session_id_generator_test.cc +++ /dev/null @@ -1,24 +0,0 @@ -#include "src/common/session/session_id_generator.h" -#include "gtest/gtest.h" - -namespace authservice { -namespace common { -namespace session { - -TEST(SessionIdGeneratorTest, Generate) { - int expected_number_of_printable_characters = 86; - - SessionIdGenerator gen; - - auto session_id1 = gen.Generate(); - ASSERT_EQ(expected_number_of_printable_characters, session_id1.length()); - - auto session_id2 = gen.Generate(); - ASSERT_EQ(expected_number_of_printable_characters, session_id2.length()); - - ASSERT_NE(session_id1, session_id2); -} - -} // namespace session -} // namespace common -} // namespace authservice diff --git a/test/common/session/session_string_generator_test.cc b/test/common/session/session_string_generator_test.cc new file mode 100644 index 00000000..9b61cade --- /dev/null +++ b/test/common/session/session_string_generator_test.cc @@ -0,0 +1,52 @@ +#include "src/common/session/session_string_generator.h" +#include "gtest/gtest.h" + +namespace authservice { +namespace common { +namespace session { + +TEST(SessionStringGeneratorTest, Generate) { + int expected_number_of_printable_characters = 86; + + SessionStringGenerator gen; + + auto session_id1 = gen.GenerateSessionId(); + ASSERT_EQ(expected_number_of_printable_characters, session_id1.length()); + + auto session_id2 = gen.GenerateSessionId(); + ASSERT_EQ(expected_number_of_printable_characters, session_id2.length()); + + ASSERT_NE(session_id1, session_id2); +} + +TEST(SessionStringGeneratorTest, GenerateState) { + int expected_number_of_printable_characters = 43; + + SessionStringGenerator gen; + + auto state1 = gen.GenerateState(); + ASSERT_EQ(expected_number_of_printable_characters, state1.length()); + + auto state2 = gen.GenerateState(); + ASSERT_EQ(expected_number_of_printable_characters, state2.length()); + + ASSERT_NE(state1, state2); +} + +TEST(SessionStringGeneratorTest, GenerateNonce) { + int expected_number_of_printable_characters = 43; + + SessionStringGenerator gen; + + auto nonce1 = gen.GenerateNonce(); + ASSERT_EQ(expected_number_of_printable_characters, nonce1.length()); + + auto nonce2 = gen.GenerateNonce(); + ASSERT_EQ(expected_number_of_printable_characters, nonce2.length()); + + ASSERT_NE(nonce1, nonce2); +} + +} // namespace session +} // namespace common +} // namespace authservice diff --git a/test/common/session/token_encryptor_test.cc b/test/common/session/token_encryptor_test.cc deleted file mode 100644 index 1beeafd7..00000000 --- a/test/common/session/token_encryptor_test.cc +++ /dev/null @@ -1,28 +0,0 @@ -#include "src/common/session/token_encryptor.h" -#include "openssl/rand.h" - -#include "gtest/gtest.h" - -namespace authservice { -namespace common { -namespace session { - -TEST(TokenEncryptorTest, SealAndOpen) { - unsigned char key[32]; - unsigned char token[256]; - for (auto i = 0; i < 100; i++) { - ASSERT_EQ(RAND_bytes(key, sizeof(key)), 1); - ASSERT_EQ(RAND_bytes(token, sizeof(token)), 1); - auto encryptor = TokenEncryptor::Create( - std::string(reinterpret_cast(key), sizeof(key))); - auto ciphertext = encryptor->Encrypt( - std::string(reinterpret_cast(token), sizeof(token))); - auto plaintext = encryptor->Decrypt(ciphertext); - ASSERT_TRUE(plaintext.has_value()); - ASSERT_EQ(std::string(reinterpret_cast(token), sizeof(token)), - plaintext); - } -} -} // namespace session -} // namespace common -} // namespace authservice diff --git a/test/common/utilities/random_test.cc b/test/common/utilities/random_test.cc index 62ae2a83..66ef9f09 100644 --- a/test/common/utilities/random_test.cc +++ b/test/common/utilities/random_test.cc @@ -21,9 +21,6 @@ TEST(Random, Rand) { // Test comparators ASSERT_EQ(random_value, parsed_back); ASSERT_FALSE(random_value != *parsed_back); - - // Test values - ASSERT_TRUE(std::equal(random_value.Begin(), random_value.End(), parsed_back->Begin())); } } diff --git a/test/config/getconfig_test.cc b/test/config/getconfig_test.cc index 43e3f608..6790ea30 100644 --- a/test/config/getconfig_test.cc +++ b/test/config/getconfig_test.cc @@ -42,9 +42,7 @@ TEST(GetConfigTest, ReturnsTheConfig) { ASSERT_EQ(oidc.scopes().at(0), "scope"); ASSERT_EQ(oidc.scopes().size(), 1); - ASSERT_EQ(oidc.cryptor_secret(), "some-secret"); ASSERT_EQ(oidc.cookie_name_prefix(), "my-app"); - ASSERT_EQ(oidc.timeout(), 300); ASSERT_EQ(oidc.id_token().preamble(), "Bearer"); ASSERT_EQ(oidc.id_token().header(), "authorization"); diff --git a/test/filters/filter_chain_test.cc b/test/filters/filter_chain_test.cc index eee019b6..2b23057a 100644 --- a/test/filters/filter_chain_test.cc +++ b/test/filters/filter_chain_test.cc @@ -63,7 +63,6 @@ TEST(FilterChainTest, New) { auto configuration = std::unique_ptr(new authservice::config::FilterChain); auto filter_config = configuration->mutable_filters()->Add(); filter_config->mutable_oidc()->set_jwks("some-value"); - filter_config->mutable_oidc()->set_cryptor_secret("some-secret"); FilterChainImpl chain(*configuration); auto instance = chain.New(); diff --git a/test/filters/oidc/BUILD b/test/filters/oidc/BUILD index 8d120c14..c9cae0e2 100644 --- a/test/filters/oidc/BUILD +++ b/test/filters/oidc/BUILD @@ -6,17 +6,6 @@ cc_library( ], ) -cc_test( - name = "state_cookie_codec_test", - srcs = ["state_cookie_codec_test.cc"], - deps = [ - "//src/filters/oidc:state_cookie_codec", - "@com_github_grpc_grpc//:grpc++", - "@com_google_googletest//:gtest_main", - ], - linkstatic = select({"@boost//:osx": True, "//conditions:default": False}), # workaround for not being able to figure out how to link dynamically on MacOS -) - cc_test( name = "token_response_test", srcs = ["token_response_test.cc"], diff --git a/test/filters/oidc/in_memory_session_store_test.cc b/test/filters/oidc/in_memory_session_store_test.cc index e3de955b..c53f019a 100644 --- a/test/filters/oidc/in_memory_session_store_test.cc +++ b/test/filters/oidc/in_memory_session_store_test.cc @@ -73,51 +73,71 @@ TEST_F(InMemorySessionStoreTest, SetTokenResponseAndGetTokenResponse) { ASSERT_EQ(result->GetAccessTokenExpiry(), 99); } -TEST_F(InMemorySessionStoreTest, SetRequestedURLAndClearRequestedURLAndGetRequestedURL) { +TEST_F(InMemorySessionStoreTest, SetAuthorizationStateAndClearAuthorizationStateAndGetAuthorizationState) { InMemorySessionStore in_memory_session_store(time_service_mock_, 42, 128); - auto session_id = std::string("fake_session_id"); - auto other_session_id = "other_session_id"; - auto requested_url = "https://example.com"; - - auto result = in_memory_session_store.GetRequestedURL(session_id); - ASSERT_FALSE(result.has_value()); - - in_memory_session_store.ClearRequestedURL(session_id); // does not crash - ASSERT_FALSE(result.has_value()); - - in_memory_session_store.SetRequestedURL(session_id, requested_url); - // mutate the original to make sure that on the get() we're getting back a copy of the original made at the time of SetRequestedURL() - requested_url = "https://example2.com"; + std::string session_id = "fake_session_id"; + std::string other_session_id = "other_session_id"; + std::string state = "some-state"; + std::string original_state(state); + std::string nonce = "some-nonce"; + std::string original_nonce(nonce); + std::string requested_url = "https://example.com"; + std::string original_requested_url(requested_url); + auto authorization_state = std::make_shared(state, nonce, requested_url); + + auto result = in_memory_session_store.GetAuthorizationState(session_id); + ASSERT_FALSE(result); - result = in_memory_session_store.GetRequestedURL(other_session_id); - ASSERT_FALSE(result.has_value()); + in_memory_session_store.ClearAuthorizationState(session_id); // does not crash + ASSERT_FALSE(result); - result = in_memory_session_store.GetRequestedURL(session_id); - ASSERT_TRUE(result.has_value()); - ASSERT_EQ(result, "https://example.com"); + in_memory_session_store.SetAuthorizationState(session_id, authorization_state); - in_memory_session_store.SetRequestedURL(session_id, requested_url); // overwrite + result = in_memory_session_store.GetAuthorizationState(other_session_id); + ASSERT_FALSE(result); - result = in_memory_session_store.GetRequestedURL(session_id); - ASSERT_TRUE(result.has_value()); - ASSERT_EQ(result, "https://example2.com"); + // mutate original strings that were passed to AuthorizationState constructor, make sure it doesn't change AuthorizationState + nonce += "-modified"; + state += "-modified"; + requested_url += "/modified"; + result = in_memory_session_store.GetAuthorizationState(session_id); + ASSERT_TRUE(result); + ASSERT_EQ(result->GetRequestedUrl(), original_requested_url); + ASSERT_EQ(result->GetState(), original_state); + ASSERT_EQ(result->GetNonce(), original_nonce); + + std::string another_state = "some-other-state"; + std::string another_nonce = "some-other-nonce"; + std::string another_requested_url = "https://other.example.com"; + auto another_authorization_state = std::make_shared(another_state, another_nonce, + another_requested_url); + in_memory_session_store.SetAuthorizationState(session_id, another_authorization_state); // overwrite + + result = in_memory_session_store.GetAuthorizationState(session_id); + ASSERT_TRUE(result); + ASSERT_EQ(result->GetRequestedUrl(), another_requested_url); + ASSERT_EQ(result->GetState(), another_state); + ASSERT_EQ(result->GetNonce(), another_nonce); - in_memory_session_store.ClearRequestedURL(session_id); - ASSERT_FALSE(in_memory_session_store.GetRequestedURL(session_id).has_value()); + in_memory_session_store.ClearAuthorizationState(session_id); + ASSERT_FALSE(in_memory_session_store.GetAuthorizationState(session_id)); } TEST_F(InMemorySessionStoreTest, Remove) { InMemorySessionStore in_memory_session_store(time_service_mock_, 42, 128); auto session_id = std::string("fake_session_id"); auto token_response = CreateTokenResponse(); + auto authorization_state = std::make_shared("state", "nonce", "requested_url"); - in_memory_session_store.SetRequestedURL(session_id, "some-url"); + in_memory_session_store.SetAuthorizationState(session_id, authorization_state); in_memory_session_store.SetTokenResponse(session_id, token_response); ASSERT_TRUE(in_memory_session_store.GetTokenResponse(session_id)); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id).has_value()); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id)); + in_memory_session_store.RemoveSession(session_id); + ASSERT_FALSE(in_memory_session_store.GetTokenResponse(session_id)); - ASSERT_FALSE(in_memory_session_store.GetRequestedURL(session_id).has_value()); + ASSERT_FALSE(in_memory_session_store.GetAuthorizationState(session_id)); in_memory_session_store.RemoveSession("other-session-id"); // ignore non-existent keys without error } @@ -286,62 +306,64 @@ TEST_F(InMemorySessionStoreTest, RemoveAllExpired_UpdatingTokenResponseKeepsSess ASSERT_TRUE(in_memory_session_store.GetTokenResponse(session_id_active)); // last active 40 seconds ago } -TEST_F(InMemorySessionStoreTest, RemoveAllExpired_UpdatingRequestedUrlKeepsSessionActive) { +TEST_F(InMemorySessionStoreTest, RemoveAllExpired_UpdatingAuthorizationStateKeepsSessionActive) { InMemorySessionStore in_memory_session_store(time_service_mock_, 500, 50); + auto authorization_state = std::make_shared("state", "nonce", "requested_url"); // Create two sessions EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(5)); auto session_id_idle = std::string("fake_session_id_idle"); - in_memory_session_store.SetRequestedURL(session_id_idle, "https://example.com"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_idle, authorization_state); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_idle)); auto session_id_active = std::string("fake_session_id_active"); - in_memory_session_store.SetRequestedURL(session_id_active, "https://example.com"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_active, authorization_state); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // Access both at time 30 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(30)); in_memory_session_store.RemoveAllExpired(); - in_memory_session_store.SetRequestedURL(session_id_idle, "https://example.com"); // last active 25 seconds ago - in_memory_session_store.SetRequestedURL(session_id_active, "https://example.com"); // last active 25 seconds ago + in_memory_session_store.SetAuthorizationState(session_id_idle, authorization_state); // last active 25 seconds ago + in_memory_session_store.SetAuthorizationState(session_id_active, authorization_state); // last active 25 seconds ago // Access only one of two at time 50 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(50)); - in_memory_session_store.SetRequestedURL(session_id_active, "https://example.com"); // accessing at time 50 + in_memory_session_store.SetAuthorizationState(session_id_active, authorization_state); // accessing at time 50 // The idle session should be removed at time 90 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(90)); in_memory_session_store.RemoveAllExpired(); - ASSERT_FALSE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); // last active 60 seconds ago - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); // last active 40 seconds ago + ASSERT_FALSE(in_memory_session_store.GetAuthorizationState(session_id_idle)); // last active 60 seconds ago + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // last active 40 seconds ago } -TEST_F(InMemorySessionStoreTest, RemoveAllExpired_ClearRequestedURLKeepsSessionActive) { +TEST_F(InMemorySessionStoreTest, RemoveAllExpired_ClearAuthorizationStateKeepsSessionActive) { InMemorySessionStore in_memory_session_store(time_service_mock_, 500, 50); EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(5)); + auto authorization_state = std::make_shared("state", "nonce", "requested_url"); // Create a session auto session_id_idle = std::string("fake_session_id_idle"); - in_memory_session_store.SetRequestedURL(session_id_idle, "https://example.com"); + in_memory_session_store.SetAuthorizationState(session_id_idle, authorization_state); auto token_response_idle = CreateTokenResponse(); in_memory_session_store.SetTokenResponse(session_id_idle, token_response_idle); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_idle)); // Create another session auto session_id_active = std::string("fake_session_id_active"); - in_memory_session_store.SetRequestedURL(session_id_active, "https://example.com"); + in_memory_session_store.SetAuthorizationState(session_id_active, authorization_state); auto token_response_active = CreateTokenResponse(); in_memory_session_store.SetTokenResponse(session_id_active, token_response_active); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // Access both at time 30 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(30)); in_memory_session_store.RemoveAllExpired(); - in_memory_session_store.ClearRequestedURL(session_id_idle); // last active 25 seconds ago - in_memory_session_store.ClearRequestedURL(session_id_active); // last active 25 seconds ago + in_memory_session_store.ClearAuthorizationState(session_id_idle); // last active 25 seconds ago + in_memory_session_store.ClearAuthorizationState(session_id_active); // last active 25 seconds ago // Access only one of two at time 50 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(50)); - in_memory_session_store.ClearRequestedURL(session_id_active); // accessing at time 50 + in_memory_session_store.ClearAuthorizationState(session_id_active); // accessing at time 50 // The idle session should be removed at time 90 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(90)); @@ -364,69 +386,71 @@ TEST_F(InMemorySessionStoreTest, ASSERT_FALSE(in_memory_session_store.GetTokenResponse(session_id_idle)); } -TEST_F(InMemorySessionStoreTest, RemoveAllExpired_RemovesSessionsOfRequestedURLWhichHaveExceededTheAbsoluteTimeout) { +TEST_F(InMemorySessionStoreTest, RemoveAllExpired_RemovesAuthorizationStatesWhichHaveExceededTheAbsoluteTimeout) { int max_absolute_session_timeout_in_seconds = 190; int max_session_idle_timeout_in_seconds = 0; + auto authorization_state1 = std::make_shared("state1", "nonce1", "requested_url1"); + auto authorization_state2 = std::make_shared("state2", "nonce2", "requested_url2"); InMemorySessionStore in_memory_session_store(time_service_mock_, max_absolute_session_timeout_in_seconds, max_session_idle_timeout_in_seconds); - // Create session of requested URL that will expire EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(5)); auto session_id_will_expire = std::string("fake_session_id_1"); - in_memory_session_store.SetRequestedURL(session_id_will_expire, "https://example1.com"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_will_expire).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_will_expire, authorization_state1); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_will_expire)); - // Create session of requested URL that will not expire EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(20)); auto session_id_will_not_expire = std::string("fake_session_id_2"); - in_memory_session_store.SetRequestedURL(session_id_will_not_expire, "https://example2.com"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_will_not_expire).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_will_not_expire, authorization_state2); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_will_not_expire)); // After 30 seconds, neither should have been cleaned up EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(30)); in_memory_session_store.RemoveAllExpired(); ASSERT_TRUE( - in_memory_session_store.GetRequestedURL(session_id_will_expire).has_value()); // has been in for 25 seconds + in_memory_session_store.GetAuthorizationState(session_id_will_expire)); // has been in for 25 seconds ASSERT_TRUE( - in_memory_session_store.GetRequestedURL(session_id_will_not_expire).has_value()); // has been in for 10 seconds + in_memory_session_store.GetAuthorizationState(session_id_will_not_expire)); // has been in for 10 seconds // After 200 seconds, the older session is cleaned up but the younger one is not EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(200)); in_memory_session_store.RemoveAllExpired(); - ASSERT_FALSE(in_memory_session_store.GetRequestedURL( - session_id_will_expire).has_value()); // has been in 195 seconds, evicted - ASSERT_TRUE(in_memory_session_store.GetRequestedURL( - session_id_will_not_expire).has_value()); // has been in for 180 seconds, not evicted + ASSERT_FALSE(in_memory_session_store.GetAuthorizationState( + session_id_will_expire)); // has been in 195 seconds, evicted + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState( + session_id_will_not_expire)); // has been in for 180 seconds, not evicted } TEST_F(InMemorySessionStoreTest, - RemoveAllExpired_RemovesSessionsOfRequestedUrlsWhichHaveExceededTheMaxIdleSessionTimeout) { + RemoveAllExpired_RemovesAuthorizationStatesWhichHaveExceededTheMaxIdleSessionTimeout) { InMemorySessionStore in_memory_session_store(time_service_mock_, 500, 50); + auto authorization_state_idle = std::make_shared("state1", "nonce1", "requested_url1"); + auto authorization_state_active = std::make_shared("state2", "nonce2", "requested_url2"); // Create two sessions EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(5)); auto session_id_idle = std::string("fake_session_id_idle"); - in_memory_session_store.SetRequestedURL(session_id_idle, "https://example.com?1"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_idle, authorization_state_idle); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_idle)); auto session_id_active = std::string("fake_session_id_active"); - in_memory_session_store.SetRequestedURL(session_id_active, "https://example.com?2"); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); + in_memory_session_store.SetAuthorizationState(session_id_active, authorization_state_active); + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // Access both at time 30 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(30)); in_memory_session_store.RemoveAllExpired(); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); // last active 25 seconds ago - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); // last active 25 seconds ago + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_idle)); // last active 25 seconds ago + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // last active 25 seconds ago // Access only one of two at time 50 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(50)); - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); // accessing at time 50 + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // accessing at time 50 // The idle session should be removed at time 90 EXPECT_CALL(*time_service_mock_, GetCurrentTimeInSecondsSinceEpoch()).WillRepeatedly(Return(90)); in_memory_session_store.RemoveAllExpired(); - ASSERT_FALSE(in_memory_session_store.GetRequestedURL(session_id_idle).has_value()); // last active 60 seconds ago - ASSERT_TRUE(in_memory_session_store.GetRequestedURL(session_id_active).has_value()); // last active 40 seconds ago + ASSERT_FALSE(in_memory_session_store.GetAuthorizationState(session_id_idle)); // last active 60 seconds ago + ASSERT_TRUE(in_memory_session_store.GetAuthorizationState(session_id_active)); // last active 40 seconds ago } // When the concurrency code is incorrect, this test will occasionally fail. @@ -485,7 +509,7 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForTokenResponseOperations) { // When the concurrency code is incorrect, this test will occasionally fail. // If it fails, there is definitely something wrong with the concurrency code. -TEST_F(InMemorySessionStoreTest, ThreadSafetyForClearRequestedURL) { +TEST_F(InMemorySessionStoreTest, ThreadSafetyForClearAuthorizationState) { InMemorySessionStore in_memory_session_store(time_service_mock_, 0, 0); std::vector threads; @@ -498,8 +522,9 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForClearRequestedURL) { for (int i = 0; i < thread_count; ++i) { threads.emplace_back([iterations, &in_memory_session_store]() { for (int j = 1; j < iterations + 1; ++j) { - in_memory_session_store.SetRequestedURL("session_id", "https://foo.com"); - in_memory_session_store.ClearRequestedURL("session_id"); + auto authorization_state = std::make_shared("state", "nonce", "requested_url"); + in_memory_session_store.SetAuthorizationState("session_id", authorization_state); + in_memory_session_store.ClearAuthorizationState("session_id"); } }); } @@ -518,7 +543,7 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForClearRequestedURL) { // When the concurrency code is incorrect, this test will occasionally fail. // If it fails, there is definitely something wrong with the concurrency code. -TEST_F(InMemorySessionStoreTest, ThreadSafetyForGetRequestedURL) { +TEST_F(InMemorySessionStoreTest, ThreadSafetyForGetAuthorizationState) { InMemorySessionStore in_memory_session_store(time_service_mock_, 0, 0); std::vector threads; @@ -531,8 +556,9 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForGetRequestedURL) { for (int i = 0; i < thread_count; ++i) { threads.emplace_back([iterations, &in_memory_session_store]() { for (int j = 1; j < iterations + 1; ++j) { - in_memory_session_store.SetRequestedURL("session_id", "https://foo.com"); - in_memory_session_store.GetRequestedURL("session_id"); + auto authorization_state = std::make_shared("state", "nonce", "requested_url"); + in_memory_session_store.SetAuthorizationState("session_id", authorization_state); + in_memory_session_store.GetAuthorizationState("session_id"); } }); } @@ -551,7 +577,7 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForGetRequestedURL) { // When the concurrency code is incorrect, this test will occasionally fail. // If it fails, there is definitely something wrong with the concurrency code. -TEST_F(InMemorySessionStoreTest, ThreadSafetyForSetRequestedURL) { +TEST_F(InMemorySessionStoreTest, ThreadSafetyForSetAuthorizationState) { InMemorySessionStore in_memory_session_store(time_service_mock_, 0, 0); std::vector threads; @@ -567,12 +593,13 @@ TEST_F(InMemorySessionStoreTest, ThreadSafetyForSetRequestedURL) { int unique_number = (i * iterations) + j; auto key = std::string("session_id_") + std::to_string(unique_number); auto unique_url = std::string("https://example.com") + std::to_string(unique_number); + auto authorization_state = std::make_shared("state", "nonce", unique_url); - in_memory_session_store.SetRequestedURL(key, unique_url); + in_memory_session_store.SetAuthorizationState(key, authorization_state); - auto retrieved_optional_url = in_memory_session_store.GetRequestedURL(key); - ASSERT_TRUE(retrieved_optional_url.has_value()); - ASSERT_EQ(retrieved_optional_url.value(), unique_url); + auto retrieved_optional_authorization_state = in_memory_session_store.GetAuthorizationState(key); + ASSERT_TRUE(retrieved_optional_authorization_state); + ASSERT_EQ(retrieved_optional_authorization_state->GetRequestedUrl(), unique_url); } }); } diff --git a/test/filters/oidc/oidc_filter_test.cc b/test/filters/oidc/oidc_filter_test.cc index 3bb9d416..5b3332af 100644 --- a/test/filters/oidc/oidc_filter_test.cc +++ b/test/filters/oidc/oidc_filter_test.cc @@ -68,8 +68,7 @@ class OidcFilterTest : public ::testing::Test { authservice::config::oidc::OIDCConfig config_; std::string callback_host_; std::shared_ptr parser_mock_; - std::shared_ptr cryptor_mock_; - std::shared_ptr session_id_generator_mock_; + std::shared_ptr session_string_generator_mock_; std::shared_ptr session_store_; std::shared_ptr test_token_response_; ::envoy::service::auth::v2::CheckRequest request_; @@ -100,19 +99,16 @@ class OidcFilterTest : public ::testing::Test { config_.mutable_callback()->set_path("/callback"); config_.set_client_id("example-app"); config_.set_client_secret("ZXhhbXBsZS1hcHAtc2VjcmV0"); - config_.set_cryptor_secret("xxx123"); config_.set_cookie_name_prefix("cookie-prefix"); config_.mutable_id_token()->set_header("authorization"); config_.mutable_id_token()->set_preamble("Bearer"); - config_.set_timeout(300); std::stringstream callback_host; callback_host << config_.callback().hostname() << ':' << std::dec << config_.callback().port(); callback_host_ = callback_host.str(); parser_mock_ = std::make_shared(); - cryptor_mock_ = std::make_shared(); - session_id_generator_mock_ = std::make_shared(); + session_string_generator_mock_ = std::make_shared(); session_store_ = std::static_pointer_cast(std::make_shared( std::make_shared(), 1000, 1000) ); @@ -137,43 +133,34 @@ class OidcFilterTest : public ::testing::Test { void SetExpiredAccessTokenResponseInSessionStore(); - void AssertRequestedUrlHasBeenStored(const std::string &session_id, std::string expected_requested_url); + void AssertRequestedUrlAndStateAndNonceHaveBeenStored(absl::string_view session_id, absl::string_view expected_requested_url, absl::string_view expected_state, absl::string_view expected_nonce); + + void MockSessionGenerator(absl::string_view session_id, absl::string_view state, absl::string_view nonce); }; TEST_F(OidcFilterTest, Constructor) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); } TEST_F(OidcFilterTest, Name) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); ASSERT_EQ(filter.Name().compare("oidc"), 0); } -TEST_F(OidcFilterTest, GetStateCookieName) { - config_.clear_cookie_name_prefix(); - OidcFilter filter1(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); - ASSERT_EQ(filter1.GetStateCookieName(), "__Host-authservice-state-cookie"); - - config_.set_cookie_name_prefix("my-prefix"); - OidcFilter filter2(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); - ASSERT_EQ(filter2.GetStateCookieName(), - "__Host-my-prefix-authservice-state-cookie"); -} - TEST_F(OidcFilterTest, GetSessionIdCookieName) { config_.clear_cookie_name_prefix(); - OidcFilter filter1(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter1(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); ASSERT_EQ(filter1.GetSessionIdCookieName(), "__Host-authservice-session-id-cookie"); config_.set_cookie_name_prefix("my-prefix"); - OidcFilter filter2(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter2(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); ASSERT_EQ(filter2.GetSessionIdCookieName(), "__Host-my-prefix-authservice-session-id-cookie"); } TEST_F(OidcFilterTest, NoHttpHeader) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); ::envoy::service::auth::v2::CheckRequest request; ::envoy::service::auth::v2::CheckResponse response; @@ -192,41 +179,35 @@ TEST_F(OidcFilterTest, NoHttpSchema) { */ TEST_F(OidcFilterTest, NoAuthorization) { - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session123")); - - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + std::string session_id = "session123"; + std::string state = "some-state"; + std::string nonce = "some-nonce"; + MockSessionGenerator(session_id, state, nonce); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto status = filter.Process(&request_, &response_); ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); ASSERT_EQ(response_.denied_response().status().code(), ::envoy::type::StatusCode::Found); - AssertRequestedUrlHasBeenStored("session123", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(session_id, requested_url_, state, nonce); ASSERT_THAT( response_.denied_response().headers(), ContainsHeaders({ { common::http::headers::Location, - MatchesRegex( - "^https://acme-idp\\.tld/" - "authorization\\?client_id=example-app&nonce=[A-Za-z0-9_-]{43}&" - "redirect_uri=https%3A%2F%2Fme\\.tld%2Fcallback&response_type=code&" - "scope=openid&state=[A-Za-z0-9_-]{43}$") + StrEq("https://acme-idp.tld/" + "authorization?client_id=example-app&nonce=" + nonce + + "&redirect_uri=https%3A%2F%2Fme.tld%2Fcallback&response_type=code&" + "scope=openid&state=" + state) }, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, { common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-session-id-cookie=session123; " + StrEq("__Host-cookie-prefix-authservice-session-id-cookie=" + session_id + "; " "HttpOnly; Path=/; " "SameSite=Lax; Secure") } @@ -235,21 +216,23 @@ TEST_F(OidcFilterTest, NoAuthorization) { } TEST_F(OidcFilterTest, NoAuthorization_WithoutPathOrQueryParameters) { - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session123")); + auto session_id = "session123"; + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(session_id, state, nonce); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->clear_query(); httpRequest->clear_path(); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); filter.Process(&request_, &response_); - AssertRequestedUrlHasBeenStored("session123", "https://example.com"); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(session_id, "https://example.com", state, nonce); } TEST_F(OidcFilterTest, AlreadyHasUnexpiredIdTokenShouldSendRequestToAppWithAuthorizationHeaderContainingIdToken) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); session_store_->SetTokenResponse("session123", test_token_response_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); @@ -271,15 +254,18 @@ TEST_F(OidcFilterTest, AlreadyHasUnexpiredIdTokenShouldSendRequestToAppWithAutho TEST_F(OidcFilterTest, ShouldRedirectToIdpToAuthenticateAgain_WhenAccessTokenIsMissing_GivenTheAccessTokenHeaderHasBeenConfigured) { EnableAccessTokens(config_); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); + TokenResponse token_response(test_id_token_jwt_); token_response.SetAccessTokenExpiry(2906139022); //Feb 2, 2062 token_response.SetAccessToken(nullptr); - session_store_->SetTokenResponse("session123", std::make_shared(token_response)); - - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); - - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + session_store_->SetTokenResponse(old_session_id, std::make_shared(token_response)); + + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, @@ -287,7 +273,7 @@ TEST_F(OidcFilterTest, ShouldRedirectToIdpToAuthenticateAgain_WhenAccessTokenIsM auto status = filter.Process(&request_, &response_); - AssertRequestedUrlHasBeenStored("session456", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); // We expect to be redirected to authenticate ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); @@ -300,45 +286,43 @@ TEST_F(OidcFilterTest, ShouldRedirectToIdpToAuthenticateAgain_WhenAccessTokenIsM {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, { common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure") + StrEq("__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure") } }) ); // Old token should be deleted - ASSERT_FALSE(session_store_->GetTokenResponse("session123")); + ASSERT_FALSE(session_store_->GetTokenResponse(old_session_id)); } TEST_F(OidcFilterTest, ExpiredAccessToken_ShouldRedirectToIdpToAuthenticateAgain_WhenTheAccessTokenHeaderHasBeenConfigured_GivenThereIsNoRefreshToken) { EnableAccessTokens(config_); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); + TokenResponse token_response(test_id_token_jwt_); // id token, not expired token_response.SetAccessTokenExpiry(1); // already expired token_response.SetAccessToken("fake_access_token"); - session_store_->SetTokenResponse("session123", std::make_shared(token_response)); + session_store_->SetTokenResponse(old_session_id, std::make_shared(token_response)); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); auto status = filter.Process(&request_, &response_); // We expect to be redirected to authenticate ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); - AssertRequestedUrlHasBeenStored("session456", requested_url_); - ASSERT_FALSE(session_store_->GetTokenResponse("session123")); // Old token should be deleted + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); + ASSERT_FALSE(session_store_->GetTokenResponse(old_session_id)); ASSERT_THAT( response_.denied_response().headers(), @@ -346,14 +330,8 @@ TEST_F(OidcFilterTest, ExpiredAccessToken_ShouldRedirectToIdpToAuthenticateAgain {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); } @@ -381,7 +359,7 @@ TEST_F(OidcFilterTest, ExpiredAccessTokenShouldRefreshTheTokenResponse_WhenTheAc EXPECT_CALL(*parser_mock_, ParseRefreshTokenResponse(_, _)) .WillOnce(::testing::Return(test_refresh_token_response)); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( @@ -409,21 +387,24 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_AndGeneratesNewSessi EnableAccessTokens(config_); auto mocked_http = new common::http::http_mock(); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); auto status = filter.Process(&request_, &response_); ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); - AssertRequestedUrlHasBeenStored("session456", requested_url_); - ASSERT_FALSE(session_store_->GetRequestedURL("session123").has_value()); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); + ASSERT_FALSE(session_store_->GetAuthorizationState(old_session_id)); ASSERT_THAT( response_.denied_response().headers(), @@ -431,14 +412,8 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_AndGeneratesNewSessi {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); } @@ -455,18 +430,22 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenFailingToParseTh EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)).WillOnce(Return(ByMove(std::move(raw_http_token_response_from_idp)))); EXPECT_CALL(*parser_mock_, ParseRefreshTokenResponse(_, _)).WillOnce(::testing::Return(nullptr)); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); + + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); auto status = filter.Process(&request_, &response_); - AssertRequestedUrlHasBeenStored("session456", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); ASSERT_THAT( @@ -475,19 +454,12 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenFailingToParseTh {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); - auto stored_token_response = session_store_->GetTokenResponse("session123"); - ASSERT_FALSE(stored_token_response); + ASSERT_FALSE(session_store_->GetTokenResponse(old_session_id)); } TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenFailingToEstablishHttpConnectionToIDP) { @@ -498,18 +470,21 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenFailingToEstabli auto mocked_http = new common::http::http_mock(); EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)).WillOnce(Return(ByMove(nullptr))); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); auto status = filter.Process(&request_, &response_); ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); - AssertRequestedUrlHasBeenStored("session456", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); ASSERT_THAT( response_.denied_response().headers(), @@ -517,19 +492,12 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenFailingToEstabli {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); - auto stored_token_response = session_store_->GetTokenResponse("session123"); - ASSERT_FALSE(stored_token_response); + ASSERT_FALSE(session_store_->GetTokenResponse(old_session_id)); } TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenIDPReturnsUnsuccessfulHttpResponseCode) { @@ -543,42 +511,39 @@ TEST_F(OidcFilterTest, Process_RedirectsUsersToAuthenticate_WhenIDPReturnsUnsucc raw_http_token_response_from_idp->result(beast::http::status::bad_request); EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)).WillOnce(Return(ByMove(std::move(raw_http_token_response_from_idp)))); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); // The redirect to IDP requires a state/nonce cookie. EXPECT_CALL(*parser_mock_, ParseRefreshTokenResponse(_, _)).Times(0); // we want the code to return before attempting to parse the bad response - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); + + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); auto jwt_status = test_id_token_jwt_.parseFromString(test_id_token_jwt_string_); ASSERT_EQ(jwt_status, google::jwt_verify::Status::Ok); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); auto status = filter.Process(&request_, &response_); ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); - AssertRequestedUrlHasBeenStored("session456", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); ASSERT_THAT( response_.denied_response().headers(), ContainsHeaders({ {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); - auto stored_token_response = session_store_->GetTokenResponse("session123"); - ASSERT_FALSE(stored_token_response); + ASSERT_FALSE(session_store_->GetTokenResponse(old_session_id)); } TEST_F(OidcFilterTest, Process_PermitsTheRequestToContinue_GivenTheAccessTokenIsExpired_ButGivenTheAccessTokenHeaderHasNotBeenConfigured) { @@ -591,7 +556,7 @@ TEST_F(OidcFilterTest, Process_PermitsTheRequestToContinue_GivenTheAccessTokenIs httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto status = filter.Process(&request_, &response_); @@ -612,7 +577,7 @@ TEST_F(OidcFilterTest, ShouldPermitTheRequestToContinue_WhenTokenResponseWithAcc token_response.SetAccessToken("fake_access_token"); session_store_->SetTokenResponse("session123", std::make_shared(token_response)); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); @@ -643,35 +608,33 @@ TEST_F(OidcFilterTest, ExpiredIdTokenShouldRedirectToIdpToAuthenticateAgainWhenT token_response.SetAccessToken("expected_access_token"); token_response.SetAccessTokenExpiry(10000000000); // access token not expired, Sat 20 Nov 2286 - session_store_->SetTokenResponse("session123", std::make_shared(token_response)); + auto old_session_id = std::string("session123"); + auto new_session_id = std::string("session456"); + auto state = "some-state"; + auto nonce = "some-nonce"; + MockSessionGenerator(new_session_id, state, nonce); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).WillOnce(Return("encrypted")); - EXPECT_CALL(*session_id_generator_mock_, Generate()).WillOnce(Return("session456")); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + "__Host-cookie-prefix-authservice-session-id-cookie=" + old_session_id}); + + session_store_->SetTokenResponse(old_session_id, std::make_shared(token_response)); auto status = filter.Process(&request_, &response_); // We expect to be redirected to authenticate because the id_token is expired ASSERT_EQ(status, google::rpc::Code::UNAUTHENTICATED); - AssertRequestedUrlHasBeenStored("session456", requested_url_); + AssertRequestedUrlAndStateAndNonceHaveBeenStored(new_session_id, requested_url_, state, nonce); ASSERT_THAT( response_.denied_response().headers(), ContainsHeaders({ {common::http::headers::Location, StartsWith(common::http::http::ToUrl(config_.authorization()))}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=encrypted; " - "HttpOnly; Max-Age=300; Path=/; " - "SameSite=Lax; Secure") - }, {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-session-id-cookie=session456; HttpOnly; Path=/; SameSite=Lax; Secure")} + "__Host-cookie-prefix-authservice-session-id-cookie=" + new_session_id + "; HttpOnly; Path=/; SameSite=Lax; Secure")} }) ); } @@ -679,7 +642,7 @@ TEST_F(OidcFilterTest, ExpiredIdTokenShouldRedirectToIdpToAuthenticateAgainWhenT TEST_F(OidcFilterTest, AlreadyHasUnexpiredTokensShouldSendRequestToAppWithHeadersContainingBothTokensWhenTheAccessTokenHeaderHasBeenConfigured) { EnableAccessTokens(config_); session_store_->SetTokenResponse("session123", test_token_response_); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, @@ -701,12 +664,11 @@ TEST_F(OidcFilterTest, LogoutWithCookies) { session_store_->SetTokenResponse("session123", test_token_response_); config_.mutable_logout()->set_path("/logout"); config_.mutable_logout()->set_redirect_to_uri("https://redirect-uri"); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=state; " "__Host-cookie-prefix-authservice-session-id-cookie=session123" }); httpRequest->set_path("/logout"); @@ -725,8 +687,6 @@ TEST_F(OidcFilterTest, LogoutWithCookies) { {common::http::headers::Location, StrEq("https://redirect-uri")}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-state-cookie=deleted; HttpOnly; Max-Age=0; Path=/; SameSite=Lax; Secure")}, {common::http::headers::SetCookie, StrEq( "__Host-cookie-prefix-authservice-session-id-cookie=deleted; HttpOnly; Max-Age=0; Path=/; SameSite=Lax; Secure")} }) @@ -736,7 +696,7 @@ TEST_F(OidcFilterTest, LogoutWithCookies) { TEST_F(OidcFilterTest, LogoutWithNoCookies) { config_.mutable_logout()->set_path("/logout"); config_.mutable_logout()->set_redirect_to_uri("https://redirect-uri"); - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_path("/logout"); @@ -752,8 +712,6 @@ TEST_F(OidcFilterTest, LogoutWithNoCookies) { {common::http::headers::Location, StrEq("https://redirect-uri")}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - {common::http::headers::SetCookie, StrEq( - "__Host-cookie-prefix-authservice-state-cookie=deleted; HttpOnly; Max-Age=0; Path=/; SameSite=Lax; Secure")}, {common::http::headers::SetCookie, StrEq( "__Host-cookie-prefix-authservice-session-id-cookie=deleted; HttpOnly; Max-Age=0; Path=/; SameSite=Lax; Secure")} }) @@ -781,30 +739,18 @@ TEST_F(OidcFilterTest, RetrieveToken_RedirectsUser_WithAccessTokenHeaderNameConf AssertRetrieveToken(config_, config_.callback().hostname()); } -TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenOriginallyRequestedUrlCannotBeFound) { +TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenAuthorizationStateInfoCannotBeFoundInSession) { + std::string session_id = "session123"; auto oidcConfig = config_; auto callback_host_on_request = callback_host_; - EXPECT_CALL(*parser_mock_, Parse(oidcConfig.client_id(), ::testing::_, ::testing::_)) - .WillOnce(::testing::Return(test_token_response_)); auto mocked_http = new common::http::http_mock(); - auto raw_http = common::http::response_t( - new beast::http::response()); - raw_http->result(beast::http::status::ok); - EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)) - .WillOnce(Return(ByMove(std::move(raw_http)))); - OidcFilter filter(common::http::ptr_t(mocked_http), oidcConfig, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), oidcConfig, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_on_request); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce(Return( - absl::optional("expectedstate;expectednonce"))); - std::vector parts = {oidcConfig.callback().path().c_str(), - "code=value&state=expectedstate"}; + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); + std::vector parts = {oidcConfig.callback().path().c_str(), "code=value&state=some-state-value"}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); @@ -812,10 +758,17 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenOriginallyRequestedUrlCann } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenTokenResponseIsMissingAccessToken) { + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + EnableAccessTokens(config_); google::jwt_verify::Jwt jwt = {}; auto token_response = std::make_shared(jwt); - EXPECT_CALL(*parser_mock_, Parse(config_.client_id(), ::testing::_, ::testing::_)) + EXPECT_CALL(*parser_mock_, Parse(config_.client_id(), nonce, ::testing::_)) .WillOnce(::testing::Return(token_response)); auto mocked_http = new common::http::http_mock(); auto raw_http = common::http::response_t( @@ -823,153 +776,47 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenTokenResponseIsMissingAcce raw_http->result(beast::http::status::ok); EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)) .WillOnce(Return(ByMove(std::move(raw_http)))); - ASSERT_FALSE(session_store_->GetTokenResponse("session123")); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + ASSERT_FALSE(session_store_->GetTokenResponse(session_id)); + OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, session_string_generator_mock_, session_store_); - auto httpRequest = - request_.mutable_attributes()->mutable_request()->mutable_http(); + auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce(Return( - absl::optional("expectedstate;expectednonce"))); - EXPECT_CALL(*cryptor_mock_, Encrypt(_)).Times(0); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; + std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); - ASSERT_FALSE(session_store_->GetTokenResponse("session123")); + ASSERT_FALSE(session_store_->GetTokenResponse(session_id)); ASSERT_THAT( response_.denied_response().headers(), ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, - }) - ); -} - -TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingStateCookie) { - auto mocked_http = new common::http::http_mock(); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); - auto httpRequest = - request_.mutable_attributes()->mutable_request()->mutable_http(); - httpRequest->set_host(callback_host_); - httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; - httpRequest->set_path(absl::StrJoin(parts, "?")); - auto code = filter.Process(&request_, &response_); - ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); - - ASSERT_THAT( - response_.denied_response().headers(), - ContainsHeaders({ - {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, - {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, - }) - ); -} - -TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenInvalidStateCookie) { - auto mocked_http = new common::http::http_mock(); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); - auto httpRequest = - request_.mutable_attributes()->mutable_request()->mutable_http(); - httpRequest->set_host(callback_host_); - httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=invalid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("invalid")) - .WillOnce(Return(absl::nullopt)); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; - httpRequest->set_path(absl::StrJoin(parts, "?")); - auto code = filter.Process(&request_, &response_); - ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); - - ASSERT_THAT( - response_.denied_response().headers(), - ContainsHeaders({ - {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, - {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, - }) - ); -} - -TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenInvalidStateCookieFormat) { - auto mocked_http = new common::http::http_mock(); - OidcFilter filter(common::http::ptr_t(mocked_http), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); - auto httpRequest = - request_.mutable_attributes()->mutable_request()->mutable_http(); - httpRequest->set_host(callback_host_); - httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce( - Return(absl::optional("invalidformat"))); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; - httpRequest->set_path(absl::StrJoin(parts, "?")); - auto code = filter.Process(&request_, &response_); - ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); - - ASSERT_THAT( - response_.denied_response().headers(), - ContainsHeaders({ - {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, - {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingCode) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); - std::vector parts = {config_.callback().path().c_str(), - "key=value&state=expectedstate"}; + std::vector parts = {config_.callback().path().c_str(), "key=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); @@ -979,18 +826,12 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingCode) { ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingState) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); @@ -1010,28 +851,27 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingState) { ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenUnexpectedState) { - OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=unexpectedstate"}; + std::vector parts = {config_.callback().path().c_str(), "code=value&state=unexpectedstate"}; httpRequest->set_path(absl::StrJoin(parts, "?")); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); @@ -1041,35 +881,30 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenUnexpectedState) { ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenBrokenPipe) { + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + auto *http_mock = new common::http::http_mock(); auto raw_http = common::http::response_t(); EXPECT_CALL(*http_mock, Post(_, _, _, _, _)) .WillOnce(Return(ByMove(std::move(raw_http)))); - OidcFilter filter(common::http::ptr_t(http_mock), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(http_mock), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce(Return( - absl::optional("expectedstate;expectednonce"))); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); + std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INTERNAL); @@ -1079,38 +914,33 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenBrokenPipe) { ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenInvalidResponse) { - EXPECT_CALL(*parser_mock_, Parse(config_.client_id(), ::testing::_, ::testing::_)) + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + + EXPECT_CALL(*parser_mock_, Parse(config_.client_id(), nonce, ::testing::_)) .WillOnce(::testing::Return(nullptr)); auto *http_mock = new common::http::http_mock(); auto raw_http = common::http::response_t( (new beast::http::response())); EXPECT_CALL(*http_mock, Post(_, _, _, _, _)) .WillOnce(Return(ByMove(std::move(raw_http)))); - OidcFilter filter(common::http::ptr_t(http_mock), config_, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(http_mock), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce(Return( - absl::optional("expectedstate;expectednonce"))); - std::vector parts = {config_.callback().path().c_str(), - "code=value&state=expectedstate"}; + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); + std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); @@ -1120,20 +950,23 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenInvalidResponse) { ContainsHeaders({ {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; " - "SameSite=Lax; Secure"), - }, }) ); } -void OidcFilterTest::AssertRequestedUrlHasBeenStored(const std::string &session_id, std::string expected_requested_url) { - auto stored_originally_requested_url = session_store_->GetRequestedURL(session_id); - ASSERT_TRUE(stored_originally_requested_url.has_value()); - ASSERT_EQ(stored_originally_requested_url.value(), expected_requested_url); +void OidcFilterTest::AssertRequestedUrlAndStateAndNonceHaveBeenStored(absl::string_view session_id, absl::string_view expected_requested_url, absl::string_view expected_state, absl::string_view expected_nonce) { + auto authorization_state = session_store_->GetAuthorizationState(session_id.data()); + ASSERT_TRUE(authorization_state); + + ASSERT_EQ(authorization_state->GetRequestedUrl(), expected_requested_url.data()); + ASSERT_EQ(authorization_state->GetState(), expected_state.data()); + ASSERT_EQ(authorization_state->GetNonce(), expected_nonce.data()); +} + +void OidcFilterTest::MockSessionGenerator(absl::string_view session_id, absl::string_view state, absl::string_view nonce) { + EXPECT_CALL(*session_string_generator_mock_, GenerateSessionId()).WillOnce(Return(session_id.data())); + EXPECT_CALL(*session_string_generator_mock_, GenerateState()).WillOnce(Return(state.data())); + EXPECT_CALL(*session_string_generator_mock_, GenerateNonce()).WillOnce(Return(nonce.data())); } void OidcFilterTest::SetExpiredAccessTokenResponseInSessionStore() { @@ -1149,10 +982,14 @@ void OidcFilterTest::EnableAccessTokens(config::oidc::OIDCConfig &oidcConfig) { } void OidcFilterTest::AssertRetrieveToken(config::oidc::OIDCConfig &oidcConfig, std::string callback_host_on_request) { - auto originally_requested_url = std::string("https://example.com/summary"); - session_store_->SetRequestedURL("session123", originally_requested_url); - - EXPECT_CALL(*parser_mock_, Parse(oidcConfig.client_id(), ::testing::_, ::testing::_)) + std::string session_id = "session123"; + std::string state = "expectedstate"; + std::string nonce = "expectednonce"; + std::string requested_url = "https://example.com/summary"; + auto authorization_state = std::make_shared(state, nonce, requested_url); + session_store_->SetAuthorizationState(session_id, authorization_state); + + EXPECT_CALL(*parser_mock_, Parse(oidcConfig.client_id(), nonce, ::testing::_)) .WillOnce(::testing::Return(test_token_response_)); auto mocked_http = new common::http::http_mock(); auto raw_http = common::http::response_t( @@ -1160,43 +997,31 @@ void OidcFilterTest::AssertRetrieveToken(config::oidc::OIDCConfig &oidcConfig, s raw_http->result(beast::http::status::ok); EXPECT_CALL(*mocked_http, Post(_, _, _, _, _)) .WillOnce(Return(ByMove(std::move(raw_http)))); - OidcFilter filter(common::http::ptr_t(mocked_http), oidcConfig, parser_mock_, cryptor_mock_, session_id_generator_mock_, session_store_); + OidcFilter filter(common::http::ptr_t(mocked_http), oidcConfig, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_on_request); httpRequest->mutable_headers()->insert( - {common::http::headers::Cookie, - "__Host-cookie-prefix-authservice-state-cookie=valid; " - "__Host-cookie-prefix-authservice-session-id-cookie=session123"}); - EXPECT_CALL(*cryptor_mock_, Decrypt("valid")) - .WillOnce(Return( - absl::optional("expectedstate;expectednonce"))); - std::vector parts = {oidcConfig.callback().path().c_str(), - "code=value&state=expectedstate"}; + {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); + std::vector parts = {oidcConfig.callback().path().c_str(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::UNAUTHENTICATED); - auto stored_token_response = session_store_->GetTokenResponse("session123"); + auto stored_token_response = session_store_->GetTokenResponse(session_id); ASSERT_TRUE(stored_token_response); ASSERT_EQ(stored_token_response->IDToken().jwt_, test_id_token_jwt_string_); ASSERT_EQ(stored_token_response->AccessToken(), "expected_access_token"); ASSERT_EQ(stored_token_response->GetAccessTokenExpiry(), 10000000000); - ASSERT_FALSE(session_store_->GetRequestedURL("session123").has_value()); + ASSERT_FALSE(session_store_->GetAuthorizationState(session_id)); ASSERT_THAT( response_.denied_response().headers(), ContainsHeaders({ - {common::http::headers::Location, StartsWith(originally_requested_url)}, + {common::http::headers::Location, StartsWith(requested_url)}, {common::http::headers::CacheControl, StrEq(common::http::headers::CacheControlDirectives::NoCache)}, {common::http::headers::Pragma, StrEq(common::http::headers::PragmaDirectives::NoCache)}, - { - common::http::headers::SetCookie, - StrEq("__Host-cookie-prefix-authservice-state-cookie=deleted; " - "HttpOnly; Max-Age=0; Path=/; SameSite=Lax; " - "Secure") - } }) ); } diff --git a/test/filters/oidc/state_cookie_codec_test.cc b/test/filters/oidc/state_cookie_codec_test.cc deleted file mode 100644 index 4d9ae3f4..00000000 --- a/test/filters/oidc/state_cookie_codec_test.cc +++ /dev/null @@ -1,30 +0,0 @@ -#include "src/filters/oidc/state_cookie_codec.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -namespace authservice { -namespace filters { -namespace oidc { -TEST(StateCookieCodecTest, Encode) { - StateCookieCodec codec; - auto encoded = codec.Encode("mystate", "mynonce"); - ASSERT_STREQ(encoded.c_str(), "mystate;mynonce"); -} - -TEST(StateCookieCodecTest, Decode) { - StateCookieCodec codec; - auto decoded = codec.Decode("mystate;mynonce"); - ASSERT_TRUE(decoded.has_value()); - ASSERT_EQ(decoded->first, absl::string_view("mystate")); - ASSERT_EQ(decoded->second, absl::string_view("mynonce")); - - // Too many values - decoded = codec.Decode("too;many;values"); - ASSERT_FALSE(decoded.has_value()); - // Not enough values - decoded = codec.Decode("NotEnough"); - ASSERT_FALSE(decoded.has_value()); -} -} // namespace oidc -} // namespace filters -} // namespace authservice diff --git a/test/fixtures/valid-config.json b/test/fixtures/valid-config.json index 3d8b4d67..691122ba 100644 --- a/test/fixtures/valid-config.json +++ b/test/fixtures/valid-config.json @@ -42,7 +42,6 @@ "scopes": [ "scope" ], - "cryptor_secret": "some-secret", "cookie_name_prefix": "my-app", "id_token": { "preamble": "Bearer", @@ -51,7 +50,6 @@ "access_token": { "header": "x-access-token" }, - "timeout": 300, "logout": { "path": "/logout", "redirect_to_uri": "https://logout-redirect" From 70f531b414618b72bbf6c28cf15e70c571680054 Mon Sep 17 00:00:00 2001 From: Andrew Chang Date: Wed, 26 Feb 2020 11:41:29 -0800 Subject: [PATCH 2/2] Fix a memory management mistake in oidc_filter_test.cc - Remove an unused include in in_memory_session_store_test.cc - Add a new target in the Makefile to help run focused tests from the command line [Issue #69] Signed-off-by: Ryan Richard --- Makefile | 9 +++++++- .../oidc/in_memory_session_store_test.cc | 1 - test/filters/oidc/oidc_filter_test.cc | 21 +++++++++---------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 449e08fe..296879fb 100644 --- a/Makefile +++ b/Makefile @@ -39,7 +39,14 @@ run: bazel run $(BAZEL_FLAGS) $(TARGET) test: - bazel test $(BAZEL_FLAGS) --strategy=TestRunner=standalone --test_output=all //test/... + bazel test $(BAZEL_FLAGS) --strategy=TestRunner=standalone --test_output=all --cache_test_results=no //test/... + +# Only run tests whose name matches a filter +# Usage examples: +# make filter-test FILTER=*RetrieveToken* +# make filter-test FILTER=OidcFilterTest.* +filter-test: + bazel test $(BAZEL_FLAGS) --strategy=TestRunner=standalone --test_output=all --cache_test_results=no //test/... --test_arg='--gtest_filter=$(FILTER)' coverage: bazel coverage $(BAZEL_FLAGS) --instrumentation_filter=//src/ //... diff --git a/test/filters/oidc/in_memory_session_store_test.cc b/test/filters/oidc/in_memory_session_store_test.cc index c53f019a..ec0026a9 100644 --- a/test/filters/oidc/in_memory_session_store_test.cc +++ b/test/filters/oidc/in_memory_session_store_test.cc @@ -1,6 +1,5 @@ #include #include -#include #include "gtest/gtest.h" #include "src/filters/oidc/in_memory_session_store.h" #include "test/common/utilities/mocks.h" diff --git a/test/filters/oidc/oidc_filter_test.cc b/test/filters/oidc/oidc_filter_test.cc index 5b3332af..ab16671f 100644 --- a/test/filters/oidc/oidc_filter_test.cc +++ b/test/filters/oidc/oidc_filter_test.cc @@ -259,12 +259,12 @@ TEST_F(OidcFilterTest, ShouldRedirectToIdpToAuthenticateAgain_WhenAccessTokenIsM auto state = "some-state"; auto nonce = "some-nonce"; MockSessionGenerator(new_session_id, state, nonce); - + TokenResponse token_response(test_id_token_jwt_); token_response.SetAccessTokenExpiry(2906139022); //Feb 2, 2062 token_response.SetAccessToken(nullptr); session_store_->SetTokenResponse(old_session_id, std::make_shared(token_response)); - + OidcFilter filter(common::http::ptr_t(), config_, parser_mock_, session_string_generator_mock_, session_store_); auto httpRequest = request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->mutable_headers()->insert( @@ -750,7 +750,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenAuthorizationStateInfoCann httpRequest->set_host(callback_host_on_request); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - std::vector parts = {oidcConfig.callback().path().c_str(), "code=value&state=some-state-value"}; + std::vector parts = {oidcConfig.callback().path(), "code=value&state=some-state-value"}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); @@ -784,7 +784,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenTokenResponseIsMissingAcce httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; + std::vector parts = {config_.callback().path(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); @@ -813,7 +813,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingCode) { request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); - std::vector parts = {config_.callback().path().c_str(), "key=value&state=" + state}; + std::vector parts = {config_.callback().path(), "key=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); @@ -836,8 +836,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenMissingState) { request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); - std::vector parts = {config_.callback().path().c_str(), - "code=value"}; + std::vector parts = {config_.callback().path().c_str(), "code=value"}; httpRequest->set_path(absl::StrJoin(parts, "?")); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, @@ -868,7 +867,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenUnexpectedState) { request_.mutable_attributes()->mutable_request()->mutable_http(); httpRequest->set_host(callback_host_); httpRequest->set_path(config_.callback().path()); - std::vector parts = {config_.callback().path().c_str(), "code=value&state=unexpectedstate"}; + std::vector parts = {config_.callback().path(), "code=value&state=unexpectedstate"}; httpRequest->set_path(absl::StrJoin(parts, "?")); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); @@ -904,7 +903,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenBrokenPipe) { httpRequest->set_path(config_.callback().path()); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; + std::vector parts = {config_.callback().path(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INTERNAL); @@ -940,7 +939,7 @@ TEST_F(OidcFilterTest, RetrieveToken_ReturnsError_WhenInvalidResponse) { httpRequest->set_path(config_.callback().path()); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - std::vector parts = {config_.callback().path().c_str(), "code=value&state=" + state}; + std::vector parts = {config_.callback().path(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_); ASSERT_EQ(code, google::rpc::Code::INVALID_ARGUMENT); @@ -1002,7 +1001,7 @@ void OidcFilterTest::AssertRetrieveToken(config::oidc::OIDCConfig &oidcConfig, s httpRequest->set_host(callback_host_on_request); httpRequest->mutable_headers()->insert( {common::http::headers::Cookie, "__Host-cookie-prefix-authservice-session-id-cookie=" + session_id}); - std::vector parts = {oidcConfig.callback().path().c_str(), "code=value&state=" + state}; + std::vector parts = {oidcConfig.callback().path(), "code=value&state=" + state}; httpRequest->set_path(absl::StrJoin(parts, "?")); auto code = filter.Process(&request_, &response_);