diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..ab766f2e8138 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -8,6 +8,9 @@ minor_behavior_changes: bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* +- area: jwt_authn + change: | + Fixed JWT extractor, which concatenated headers with a comma, resultig in invalid tokens. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/filters/http/jwt_authn/extractor.cc b/source/extensions/filters/http/jwt_authn/extractor.cc index 805af897a612..da5083026b82 100644 --- a/source/extensions/filters/http/jwt_authn/extractor.cc +++ b/source/extensions/filters/http/jwt_authn/extractor.cc @@ -253,20 +253,21 @@ ExtractorImpl::extract(const Http::RequestHeaderMap& headers) const { for (const auto& location_it : header_locations_) { const auto& location_spec = location_it.second; ENVOY_LOG(debug, "extract {}", location_it.first); - const auto result = - Http::HeaderUtility::getAllOfHeaderAsString(headers, location_spec->header_); - if (result.result().has_value()) { - auto value_str = result.result().value(); - if (!location_spec->value_prefix_.empty()) { - const auto pos = value_str.find(location_spec->value_prefix_); - if (pos == absl::string_view::npos) { - // value_prefix not found anywhere in value_str, so skip - continue; + auto header_value = headers.get(location_spec->header_); + if (!header_value.empty()) { + for (size_t i = 0; i < header_value.size(); i++) { + auto value_str = header_value[i]->value().getStringView(); + if (!location_spec->value_prefix_.empty()) { + const auto pos = value_str.find(location_spec->value_prefix_); + if (pos == absl::string_view::npos) { + // value_prefix not found anywhere in value_str, so skip + continue; + } + value_str = extractJWT(value_str, pos + location_spec->value_prefix_.length()); } - value_str = extractJWT(value_str, pos + location_spec->value_prefix_.length()); + tokens.push_back(std::make_unique( + std::string(value_str), location_spec->issuer_checker_, location_spec->header_)); } - tokens.push_back(std::make_unique( - std::string(value_str), location_spec->issuer_checker_, location_spec->header_)); } } diff --git a/test/extensions/filters/http/jwt_authn/extractor_test.cc b/test/extensions/filters/http/jwt_authn/extractor_test.cc index b639a0701619..3cad76fe54a9 100644 --- a/test/extensions/filters/http/jwt_authn/extractor_test.cc +++ b/test/extensions/filters/http/jwt_authn/extractor_test.cc @@ -143,6 +143,30 @@ TEST_F(ExtractorTest, TestDefaultHeaderLocationWithValidJWT) { EXPECT_TRUE(tokens[0]->isIssuerAllowed("issuer1")); } +TEST_F(ExtractorTest, TestDuplicatedHeadersWithDuplicatedTokenPrefixes) { + auto headers = TestRequestHeaderMapImpl{{"Authorization", absl::StrCat("Bearer ", GoodToken)}, + {"Authorization", absl::StrCat("Bearer ", GoodToken)}}; + auto tokens = extractor_->extract(headers); + EXPECT_EQ(tokens.size(), 2); + + // Only the issue1 is using default header location. + EXPECT_EQ(tokens[0]->token(), GoodToken); + EXPECT_TRUE(tokens[0]->isIssuerAllowed("issuer1")); + EXPECT_EQ(tokens[1]->token(), GoodToken); + EXPECT_TRUE(tokens[1]->isIssuerAllowed("issuer1")); +} + +TEST_F(ExtractorTest, TestDuplicatedHeadersWithUniqueTokenPrefixes) { + auto headers = TestRequestHeaderMapImpl{{"Authorization", absl::StrCat("Bearer ", GoodToken)}, + {"Authorization", absl::StrCat("Basic ", "basic")}}; + auto tokens = extractor_->extract(headers); + EXPECT_EQ(tokens.size(), 1); + + // Only the issue1 is using default header location. + EXPECT_EQ(tokens[0]->token(), GoodToken); + EXPECT_TRUE(tokens[0]->isIssuerAllowed("issuer1")); +} + // Test extracting JWT as Bearer token from the default header location: "Authorization" - // using an actual (correctly-formatted) JWT but token is invalid, like: GoodToken + // chars_after_space expected to get all token include characters after the space: @@ -203,12 +227,13 @@ TEST_F(ExtractorTest, TestCustomHeaderToken) { EXPECT_FALSE(headers.has(Http::LowerCaseString("token-header"))); } -// Make sure a double custom header concatenates the token +// Make sure a double custom header does not concatenate the token TEST_F(ExtractorTest, TestDoubleCustomHeaderToken) { auto headers = TestRequestHeaderMapImpl{{"token-header", "jwt_token"}, {"token-header", "foo"}}; auto tokens = extractor_->extract(headers); - EXPECT_EQ(tokens.size(), 1); - EXPECT_EQ(tokens[0]->token(), "jwt_token,foo"); + EXPECT_EQ(tokens.size(), 2); + EXPECT_EQ(tokens[0]->token(), "jwt_token"); + EXPECT_EQ(tokens[1]->token(), "foo"); } // Test extracting token from the custom header: "prefix-header"