From be7895d97f00dfe73c282d85e99d051d2ff38311 Mon Sep 17 00:00:00 2001 From: Lei Tang <32078630+lei-tang@users.noreply.github.com> Date: Mon, 4 Mar 2019 16:39:04 -0800 Subject: [PATCH 1/4] Add the support of bypassing JWT authn for CORS requests --- src/envoy/http/jwt_auth/jwt_authenticator.cc | 40 ++++++++++++------- .../http/jwt_auth/jwt_authenticator_test.cc | 13 ++++++ 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index b309b6b169a..22ecd96f3fd 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -25,9 +25,12 @@ namespace { // The HTTP header to pass verified token payload. const LowerCaseString kJwtPayloadKey("sec-istio-auth-userinfo"); +// The CORS OPTIONS HTTP method +const LowerCaseString kOptionsHttpMethod("options"); + // Extract host and path from a URI -void ExtractUriHostPath(const std::string& uri, std::string* host, - std::string* path) { +void ExtractUriHostPath(const std::string &uri, std::string *host, + std::string *path) { // Example: // uri = "https://example.com/certs" // pos : ^ @@ -49,13 +52,13 @@ void ExtractUriHostPath(const std::string& uri, std::string* host, } // namespace -JwtAuthenticator::JwtAuthenticator(Upstream::ClusterManager& cm, - JwtAuthStore& store) +JwtAuthenticator::JwtAuthenticator(Upstream::ClusterManager &cm, + JwtAuthStore &store) : cm_(cm), store_(store) {} // Verify a JWT token. -void JwtAuthenticator::Verify(HeaderMap& headers, - JwtAuthenticator::Callbacks* callback) { +void JwtAuthenticator::Verify(HeaderMap &headers, + JwtAuthenticator::Callbacks *callback) { headers_ = &headers; callback_ = callback; @@ -119,7 +122,7 @@ void JwtAuthenticator::Verify(HeaderMap& headers, FetchPubkey(issuer); } -void JwtAuthenticator::FetchPubkey(PubkeyCacheItem* issuer) { +void JwtAuthenticator::FetchPubkey(PubkeyCacheItem *issuer) { uri_ = issuer->jwt_config().remote_jwks().http_uri().uri(); std::string host, path; ExtractUriHostPath(uri_, &host, &path); @@ -130,7 +133,7 @@ void JwtAuthenticator::FetchPubkey(PubkeyCacheItem* issuer) { message->headers().insertPath().value(path); message->headers().insertHost().value(host); - const auto& cluster = issuer->jwt_config().remote_jwks().http_uri().cluster(); + const auto &cluster = issuer->jwt_config().remote_jwks().http_uri().cluster(); if (cm_.get(cluster) == nullptr) { DoneWithStatus(Status::FAILED_FETCH_PUBKEY); return; @@ -141,7 +144,7 @@ void JwtAuthenticator::FetchPubkey(PubkeyCacheItem* issuer) { std::move(message), *this, Http::AsyncClient::RequestOptions()); } -void JwtAuthenticator::onSuccess(MessagePtr&& response) { +void JwtAuthenticator::onSuccess(MessagePtr &&response) { request_ = nullptr; uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); if (status_code == 200) { @@ -149,7 +152,7 @@ void JwtAuthenticator::onSuccess(MessagePtr&& response) { std::string body; if (response->body()) { auto len = response->body()->length(); - body = std::string(static_cast(response->body()->linearize(len)), + body = std::string(static_cast(response->body()->linearize(len)), len); } else { ENVOY_LOG(debug, "fetch pubkey [uri = {}]: body is empty", uri_); @@ -177,7 +180,7 @@ void JwtAuthenticator::onDestroy() { } // Handle the public key fetch done event. -void JwtAuthenticator::OnFetchPubkeyDone(const std::string& pubkey) { +void JwtAuthenticator::OnFetchPubkeyDone(const std::string &pubkey) { auto issuer = store_.pubkey_cache().LookupByIssuer(jwt_->Iss()); Status status = issuer->SetRemoteJwks(pubkey); if (status != Status::OK) { @@ -188,7 +191,7 @@ void JwtAuthenticator::OnFetchPubkeyDone(const std::string& pubkey) { } // Verify with a specific public key. -void JwtAuthenticator::VerifyKey(const PubkeyCacheItem& issuer_item) { +void JwtAuthenticator::VerifyKey(const PubkeyCacheItem &issuer_item) { JwtAuth::Verifier v; if (!v.Verify(*jwt_, *issuer_item.pubkey())) { DoneWithStatus(v.GetStatus()); @@ -214,11 +217,20 @@ bool JwtAuthenticator::OkToBypass() { return true; } + // Per the spec + // http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS + // pre-flight requests shouldn't include user credentials. + if (headers_->Method() && + LowerCaseString(kOptionsHttpMethod) == + LowerCaseString(headers_->Method()->value().c_str())) { + return true; + } + // TODO: use bypass field return false; } -void JwtAuthenticator::DoneWithStatus(const Status& status) { +void JwtAuthenticator::DoneWithStatus(const Status &status) { ENVOY_LOG(debug, "Jwt authentication completed with: {}", JwtAuth::StatusToString(status)); ENVOY_LOG(debug, @@ -232,7 +244,7 @@ void JwtAuthenticator::DoneWithStatus(const Status& status) { callback_ = nullptr; } -const LowerCaseString& JwtAuthenticator::JwtPayloadKey() { +const LowerCaseString &JwtAuthenticator::JwtPayloadKey() { return kJwtPayloadKey; } diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index 05bf3ad2fc3..73efffa960e 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -530,6 +530,19 @@ TEST_F(JwtAuthenticatorTest, TestMissingJwtWhenAllowMissingOrFailedIsTrue) { auth_->Verify(headers, &mock_cb_); } +TEST_F(JwtAuthenticatorTest, TestMissingJwtWhenHttpMethodIsCORS) { + // In this test, when JWT is missing, the status should still be OK + // because CORS preflight requests shouldn't include user credentials. + EXPECT_CALL(mock_cm_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(mock_cb_, onDone(_)).WillOnce(Invoke([](const Status &status) { + ASSERT_EQ(status, Status::OK); + })); + + auto cors_headers = + TestHeaderMapImpl{{":method", "OPTIONS"}, {":path", "/any/cors-path"}}; + auth_->Verify(cors_headers, &mock_cb_); +} + TEST_F(JwtAuthenticatorTest, TestInValidJwtWhenAllowMissingOrFailedIsTrue) { // In this test, when JWT is invalid, the status should still be OK // because allow_missing_or_failed is true. From 2ec9a8847b1ad7956e7b675519fc36ff25cc75e4 Mon Sep 17 00:00:00 2001 From: Lei Tang <32078630+lei-tang@users.noreply.github.com> Date: Tue, 5 Mar 2019 11:09:23 -0800 Subject: [PATCH 2/4] Bail out earlier for CORS preflight requests --- src/envoy/http/jwt_auth/jwt_authenticator.cc | 20 ++++++++++--------- .../http/jwt_auth/jwt_authenticator_test.cc | 15 ++++++++++++++ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index 22ecd96f3fd..263b7e23691 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -62,6 +62,17 @@ void JwtAuthenticator::Verify(HeaderMap &headers, headers_ = &headers; callback_ = callback; + // Per the spec + // http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS + // pre-flight requests shouldn't include user credentials. + if (headers_->Method() && + LowerCaseString(kOptionsHttpMethod) == + LowerCaseString(headers_->Method()->value().c_str())) { + ENVOY_LOG(debug, "CORS preflight requests are passed through."); + DoneWithStatus(Status::OK); + return; + } + ENVOY_LOG(debug, "Jwt authentication starts"); std::vector> tokens; store_.token_extractor().Extract(headers, &tokens); @@ -217,15 +228,6 @@ bool JwtAuthenticator::OkToBypass() { return true; } - // Per the spec - // http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS - // pre-flight requests shouldn't include user credentials. - if (headers_->Method() && - LowerCaseString(kOptionsHttpMethod) == - LowerCaseString(headers_->Method()->value().c_str())) { - return true; - } - // TODO: use bypass field return false; } diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index 73efffa960e..50d2f3546fb 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -543,6 +543,21 @@ TEST_F(JwtAuthenticatorTest, TestMissingJwtWhenHttpMethodIsCORS) { auth_->Verify(cors_headers, &mock_cb_); } +TEST_F(JwtAuthenticatorTest, TestInvalidJWTWhenHttpMethodIsCORS) { + // In this test, when JWT is invalid, the status should still be OK + // because CORS preflight requests are passed through. + EXPECT_CALL(mock_cm_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(mock_cb_, onDone(_)).WillOnce(Invoke([](const Status &status) { + ASSERT_EQ(status, Status::OK); + })); + + std::string token = "invalidToken"; + auto cors_headers = TestHeaderMapImpl{{":method", "OPTIONS"}, + {":path", "/any/cors-path"}, + {"Authorization", "Bearer " + token}}; + auth_->Verify(cors_headers, &mock_cb_); +} + TEST_F(JwtAuthenticatorTest, TestInValidJwtWhenAllowMissingOrFailedIsTrue) { // In this test, when JWT is invalid, the status should still be OK // because allow_missing_or_failed is true. From 9fa92e36af8be0970f017cf77f5be182d39d7751 Mon Sep 17 00:00:00 2001 From: Lei Tang <32078630+lei-tang@users.noreply.github.com> Date: Tue, 5 Mar 2019 12:35:22 -0800 Subject: [PATCH 3/4] Use OPTIONS constant value from Envoy --- src/envoy/http/jwt_auth/jwt_authenticator.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index 263b7e23691..10b5ffc2e4c 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -25,9 +25,6 @@ namespace { // The HTTP header to pass verified token payload. const LowerCaseString kJwtPayloadKey("sec-istio-auth-userinfo"); -// The CORS OPTIONS HTTP method -const LowerCaseString kOptionsHttpMethod("options"); - // Extract host and path from a URI void ExtractUriHostPath(const std::string &uri, std::string *host, std::string *path) { @@ -66,9 +63,9 @@ void JwtAuthenticator::Verify(HeaderMap &headers, // http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS // pre-flight requests shouldn't include user credentials. if (headers_->Method() && - LowerCaseString(kOptionsHttpMethod) == + LowerCaseString(Http::Headers::get().MethodValues.Options) == LowerCaseString(headers_->Method()->value().c_str())) { - ENVOY_LOG(debug, "CORS preflight requests are passed through."); + ENVOY_LOG(debug, "CORS preflight request is passed through."); DoneWithStatus(Status::OK); return; } From 9c275d40d8ebb00ebd5a4fe34040aa2f467264c1 Mon Sep 17 00:00:00 2001 From: Lei Tang <32078630+lei-tang@users.noreply.github.com> Date: Tue, 5 Mar 2019 14:18:40 -0800 Subject: [PATCH 4/4] Remove changing to lowercase --- src/envoy/http/jwt_auth/jwt_authenticator.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index 10b5ffc2e4c..e5605ac89d2 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -62,9 +62,8 @@ void JwtAuthenticator::Verify(HeaderMap &headers, // Per the spec // http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS // pre-flight requests shouldn't include user credentials. - if (headers_->Method() && - LowerCaseString(Http::Headers::get().MethodValues.Options) == - LowerCaseString(headers_->Method()->value().c_str())) { + if (headers_->Method() && Http::Headers::get().MethodValues.Options == + headers_->Method()->value().c_str()) { ENVOY_LOG(debug, "CORS preflight request is passed through."); DoneWithStatus(Status::OK); return;