Skip to content

Commit

Permalink
Correctly clean up headers used for payload from JWT authentication (i…
Browse files Browse the repository at this point in the history 
…stio#1879)

* Correctly clean up headers used for payload from JWT authentication

* Clang
  • Loading branch information
@diemtvu
diemtvu committed Jul 30, 2018
1 parent 074bcab commit a3c572e
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 40 deletions.
18 changes: 3 additions & 15 deletions src/envoy/http/jwt_auth/http_filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,13 @@ JwtVerificationFilter::JwtVerificationFilter(Upstream::ClusterManager& cm,

JwtVerificationFilter::~JwtVerificationFilter() {}

void JwtVerificationFilter::onDestroy() {
ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__);
jwt_auth_.onDestroy();
}
void JwtVerificationFilter::onDestroy() { 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);

Expand All @@ -59,8 +50,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;
Expand All @@ -82,15 +73,13 @@ 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;
}
return FilterDataStatus::Continue;
}

FilterTrailersStatus JwtVerificationFilter::decodeTrailers(HeaderMap&) {
ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__);
if (state_ == Calling) {
return FilterTrailersStatus::StopIteration;
}
Expand All @@ -99,7 +88,6 @@ FilterTrailersStatus JwtVerificationFilter::decodeTrailers(HeaderMap&) {

void JwtVerificationFilter::setDecoderFilterCallbacks(
StreamDecoderFilterCallbacks& callbacks) {
ENVOY_LOG(debug, "Called JwtVerificationFilter : {}", __func__);
decoder_callbacks_ = &callbacks;
}

Expand Down
2 changes: 1 addition & 1 deletion src/envoy/http/jwt_auth/integration_test/envoy.conf.jwk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"cluster": "example_issuer"
}
},
"forward_payload_header": "sec-istio-auth-userinfo"
"forward_payload_header": "test-jwt-payload-output"
}
]
}
Expand Down
3 changes: 2 additions & 1 deletion src/envoy/http/jwt_auth/integration_test/envoy_allow_missing_or_failed_jwt.conf.jwk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 5 additions & 4 deletions src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@
namespace Envoy {

namespace {
// The HTTP header key for the JWT verification result
// 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(
"sec-istio-auth-userinfo");
"test-jwt-payload-output");
// {"iss":"https://example.com","sub":"[email protected]","aud":"example_service","exp":2001001001}
const std::string kJwtVerificationResult =
"eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz"
Expand Down Expand Up @@ -260,7 +261,7 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, RSASuccess1) {

auto expected_headers = BaseRequestHeaders();
expected_headers.addCopy(
"sec-istio-auth-userinfo",
kJwtVerificationResultHeaderKey,
"eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz"
"dEBleGFtcGxlLmNvbSIsImF1ZCI6ImV4YW1wbGVfc2VydmljZSIs"
"ImV4cCI6MjAwMTAwMTAwMX0");
Expand All @@ -282,7 +283,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"
Expand Down
18 changes: 11 additions & 7 deletions src/envoy/http/jwt_auth/jwt_authenticator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ 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<std::unique_ptr<JwtTokenExtractor::Token>> tokens;
store_.token_extractor().Extract(headers, &tokens);
Expand Down Expand Up @@ -195,16 +205,10 @@ 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());
}
headers_->addCopy(key, jwt_->PayloadStrBase64Url());
}

if (!issuer_item.jwt_config().forward()) {
Expand Down
23 changes: 11 additions & 12 deletions src/envoy/http/jwt_auth/jwt_authenticator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"(
{
Expand All @@ -108,7 +110,7 @@ const char kExampleConfig[] = R"(
"seconds": 600
}
},
"forward_payload_header": "sec-istio-auth-userinfo"
"forward_payload_header": "test-output"
}
]
}
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit a3c572e

Please sign in to comment.