From 04d4f0dca44df180013f57b5ee2f8a6d026863dc Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 25 Jul 2018 12:09:44 -0700 Subject: [PATCH 1/2] Correctly clean up headers used for payload from JWT authentication --- src/envoy/http/jwt_auth/http_filter.cc | 14 ++--------- .../jwt_auth/integration_test/envoy.conf.jwk | 2 +- ...envoy_allow_missing_or_failed_jwt.conf.jwk | 3 ++- .../http_filter_integration_test.cc | 10 ++++---- src/envoy/http/jwt_auth/jwt_authenticator.cc | 20 ++++++++-------- .../http/jwt_auth/jwt_authenticator_test.cc | 23 +++++++++---------- 6 files changed, 32 insertions(+), 40 deletions(-) diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 1b7c35b621a..709045a3ed9 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -32,21 +32,14 @@ JwtVerificationFilter::JwtVerificationFilter(Upstream::ClusterManager& cm, JwtVerificationFilter::~JwtVerificationFilter() {} void JwtVerificationFilter::onDestroy() { - ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__); jwt_auth_.onDestroy(); } FilterHeadersStatus JwtVerificationFilter::decodeHeaders(HeaderMap& headers, bool) { - ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__); state_ = Calling; stopped_ = false; - // Sanitize the JWT verification result in the HTTP headers - // TODO (lei-tang): when the JWT verification result is in a configurable - // header, need to sanitize based on the configuration. - headers.remove(JwtAuth::JwtAuthenticator::JwtPayloadKey()); - // Verify the JWT token, onDone() will be called when completed. jwt_auth_.Verify(headers, this); @@ -59,8 +52,8 @@ FilterHeadersStatus JwtVerificationFilter::decodeHeaders(HeaderMap& headers, } void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { - ENVOY_LOG(debug, "Called JwtVerificationFilter : check complete {}", - int(status)); + ENVOY_LOG(debug, "JwtVerificationFilter::onDone with status {}", + JwtAuth::StatusToString(status)); // This stream has been reset, abort the callback. if (state_ == Responded) { return; @@ -82,7 +75,6 @@ void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { } FilterDataStatus JwtVerificationFilter::decodeData(Buffer::Instance&, bool) { - ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__); if (state_ == Calling) { return FilterDataStatus::StopIterationAndWatermark; } @@ -90,7 +82,6 @@ FilterDataStatus JwtVerificationFilter::decodeData(Buffer::Instance&, bool) { } FilterTrailersStatus JwtVerificationFilter::decodeTrailers(HeaderMap&) { - ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__); if (state_ == Calling) { return FilterTrailersStatus::StopIteration; } @@ -99,7 +90,6 @@ FilterTrailersStatus JwtVerificationFilter::decodeTrailers(HeaderMap&) { void JwtVerificationFilter::setDecoderFilterCallbacks( StreamDecoderFilterCallbacks& callbacks) { - ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__); decoder_callbacks_ = &callbacks; } diff --git a/src/envoy/http/jwt_auth/integration_test/envoy.conf.jwk b/src/envoy/http/jwt_auth/integration_test/envoy.conf.jwk index 21a8d136fc2..0262d21357e 100644 --- a/src/envoy/http/jwt_auth/integration_test/envoy.conf.jwk +++ b/src/envoy/http/jwt_auth/integration_test/envoy.conf.jwk @@ -46,7 +46,7 @@ "cluster": "example_issuer" } }, - "forward_payload_header": "sec-istio-auth-userinfo" + "forward_payload_header": "test-jwt-payload-output" } ] } diff --git a/src/envoy/http/jwt_auth/integration_test/envoy_allow_missing_or_failed_jwt.conf.jwk b/src/envoy/http/jwt_auth/integration_test/envoy_allow_missing_or_failed_jwt.conf.jwk index 91746d58809..172fa3f7a2c 100644 --- a/src/envoy/http/jwt_auth/integration_test/envoy_allow_missing_or_failed_jwt.conf.jwk +++ b/src/envoy/http/jwt_auth/integration_test/envoy_allow_missing_or_failed_jwt.conf.jwk @@ -45,7 +45,8 @@ "uri": "http://example.com/foobar_cert", "cluster": "example_issuer" } - } + }, + "forward_payload_header": "test-jwt-payload-output" } ], "allow_missing_or_failed": true diff --git a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc index d606ee1b9c5..11a306f2b1e 100644 --- a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc +++ b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc @@ -19,9 +19,9 @@ namespace Envoy { namespace { -// The HTTP header key for the JWT verification result -const Http::LowerCaseString kJwtVerificationResultHeaderKey( - "sec-istio-auth-userinfo"); +// The HTTP header key for the JWT verification result. Should be the same as the one define for +// forward_payload_header in envoy.conf.jwk +const Http::LowerCaseString kJwtVerificationResultHeaderKey("test-jwt-payload-output"); // {"iss":"https://example.com","sub":"test@example.com","aud":"example_service","exp":2001001001} const std::string kJwtVerificationResult = "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" @@ -260,7 +260,7 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, RSASuccess1) { auto expected_headers = BaseRequestHeaders(); expected_headers.addCopy( - "sec-istio-auth-userinfo", + kJwtVerificationResultHeaderKey, "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" "dEBleGFtcGxlLmNvbSIsImF1ZCI6ImV4YW1wbGVfc2VydmljZSIs" "ImV4cCI6MjAwMTAwMTAwMX0"); @@ -282,7 +282,7 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, ES256Success1) { "T9ubWvRvNGGYOTuJ8T17Db68Qk3T8UNTK5lzfR_mw"; auto expected_headers = BaseRequestHeaders(); - expected_headers.addCopy("sec-istio-auth-userinfo", + expected_headers.addCopy(kJwtVerificationResultHeaderKey, "eyJpc3MiOiJo" "dHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtc" "GxlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbX" diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index b535d31f9a3..28e09e61ff0 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -59,6 +59,15 @@ void JwtAuthenticator::Verify(HeaderMap& headers, headers_ = &headers; callback_ = callback; + // Sanitize the JWT verification result in the HTTP headers + for (const auto& rule : store_.config().rules()) { + if (!rule.forward_payload_header().empty()) { + ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", rule.forward_payload_header()); + const LowerCaseString key(rule.forward_payload_header()); + headers.remove(key); + } + } + ENVOY_LOG(debug, "Jwt authentication starts"); std::vector> tokens; store_.token_extractor().Extract(headers, &tokens); @@ -195,16 +204,9 @@ void JwtAuthenticator::VerifyKey(const PubkeyCacheItem& issuer_item) { return; } - // TODO(lei-tang): remove this backward compatibility. - // Tracking issue: https://github.com/istio/istio/issues/4744 - headers_->addReferenceKey(kJwtPayloadKey, jwt_->PayloadStrBase64Url()); - if (!issuer_item.jwt_config().forward_payload_header().empty()) { - const LowerCaseString key( - issuer_item.jwt_config().forward_payload_header()); - if (key.get() != kJwtPayloadKey.get()) { - headers_->addCopy(key, jwt_->PayloadStrBase64Url()); - } + const LowerCaseString key(issuer_item.jwt_config().forward_payload_header()); + headers_->addCopy(key, jwt_->PayloadStrBase64Url()); } if (!issuer_item.jwt_config().forward()) { diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index b5ec6ca974f..ae090943251 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -88,6 +88,8 @@ const std::string kPublicKey = " \"kid\": \"b3319a147514df7ee5e4bcdee51350cc890cc89e\"" "}]}"; +// Keep this same as forward_payload_header field in the config below. +const char kOutputHeadersKey[] = "test-output"; // A good JSON config. const char kExampleConfig[] = R"( { @@ -108,7 +110,7 @@ const char kExampleConfig[] = R"( "seconds": 600 } }, - "forward_payload_header": "sec-istio-auth-userinfo" + "forward_payload_header": "test-output" } ] } @@ -319,7 +321,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTandCache) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" "aWNlIn0"); @@ -350,7 +352,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoAlg) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" "aWNlIn0"); @@ -383,7 +385,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoKid) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" "aWNlIn0"); @@ -409,7 +411,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cDovL2V4YW1wbG" "Vfc2VydmljZS8ifQ"); @@ -435,7 +437,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService1) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cHM6Ly9leGFtcG" "xlX3NlcnZpY2UxLyJ9"); @@ -461,7 +463,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService2) { auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_("sec-istio-auth-userinfo"), + EXPECT_EQ(headers.get_(kOutputHeadersKey), "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cDovL2V4YW1wbG" "Vfc2VydmljZTIifQ"); @@ -728,11 +730,8 @@ TEST_F(JwtAuthenticatorTest, TestNoForwardPayloadHeader) { })); auth_->Verify(headers, &mock_cb); - // Test when forward_payload_header is not set, the output should still - // contain the sec-istio-auth-userinfo header for backward compatibility. - EXPECT_TRUE(headers.has("sec-istio-auth-userinfo")); - // In addition, the sec-istio-auth-userinfo header should be the only header - EXPECT_EQ(headers.size(), 1); + // Test when forward_payload_header is not set, nothing added to headers. + EXPECT_EQ(headers.size(), 0); } TEST_F(JwtAuthenticatorTest, TestInlineJwks) { From 8e83a0b65c9445a2216bdde0c0f2b65d2701baab Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 25 Jul 2018 12:24:17 -0700 Subject: [PATCH 2/2] Clang --- src/envoy/http/jwt_auth/http_filter.cc | 4 +--- .../integration_test/http_filter_integration_test.cc | 7 ++++--- src/envoy/http/jwt_auth/jwt_authenticator.cc | 6 ++++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 709045a3ed9..405cc611bab 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -31,9 +31,7 @@ JwtVerificationFilter::JwtVerificationFilter(Upstream::ClusterManager& cm, JwtVerificationFilter::~JwtVerificationFilter() {} -void JwtVerificationFilter::onDestroy() { - jwt_auth_.onDestroy(); -} +void JwtVerificationFilter::onDestroy() { jwt_auth_.onDestroy(); } FilterHeadersStatus JwtVerificationFilter::decodeHeaders(HeaderMap& headers, bool) { diff --git a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc index 11a306f2b1e..a5ae802d76d 100644 --- a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc +++ b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc @@ -19,9 +19,10 @@ namespace Envoy { namespace { -// The HTTP header key for the JWT verification result. Should be the same as the one define for -// forward_payload_header in envoy.conf.jwk -const Http::LowerCaseString kJwtVerificationResultHeaderKey("test-jwt-payload-output"); +// The HTTP header key for the JWT verification result. Should be the same as +// the one define for forward_payload_header in envoy.conf.jwk +const Http::LowerCaseString kJwtVerificationResultHeaderKey( + "test-jwt-payload-output"); // {"iss":"https://example.com","sub":"test@example.com","aud":"example_service","exp":2001001001} const std::string kJwtVerificationResult = "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index 28e09e61ff0..56b21356136 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -62,7 +62,8 @@ void JwtAuthenticator::Verify(HeaderMap& headers, // Sanitize the JWT verification result in the HTTP headers for (const auto& rule : store_.config().rules()) { if (!rule.forward_payload_header().empty()) { - ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", rule.forward_payload_header()); + ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", + rule.forward_payload_header()); const LowerCaseString key(rule.forward_payload_header()); headers.remove(key); } @@ -205,7 +206,8 @@ void JwtAuthenticator::VerifyKey(const PubkeyCacheItem& issuer_item) { } if (!issuer_item.jwt_config().forward_payload_header().empty()) { - const LowerCaseString key(issuer_item.jwt_config().forward_payload_header()); + const LowerCaseString key( + issuer_item.jwt_config().forward_payload_header()); headers_->addCopy(key, jwt_->PayloadStrBase64Url()); }