diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index fe9e813b94991..1c08fb66e6d17 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -58,6 +58,7 @@ Bug Fixes * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream. * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. +* jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted. * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. * sni: as the server name in sni should be case-insensitive, envoy will convert the server name as lower case first before any other process inside envoy. diff --git a/source/extensions/filters/http/jwt_authn/verifier.cc b/source/extensions/filters/http/jwt_authn/verifier.cc index cb920c1a5477c..83361c8edd7b8 100644 --- a/source/extensions/filters/http/jwt_authn/verifier.cc +++ b/source/extensions/filters/http/jwt_authn/verifier.cc @@ -301,17 +301,16 @@ class AnyVerifierImpl : public BaseGroupVerifierImpl { // Then wait for all children to be done. if (++completion_state.number_completed_children_ == verifiers_.size()) { // Aggregate all children status into a final status. - // JwtMissing should be treated differently than other failure status - // since it simply means there is not Jwt token for the required provider. - // If there is a failure status other than JwtMissing in the children, - // it should be used as the final status. + // JwtMissed and JwtUnknownIssuer should be treated differently than other errors. + // JwtMissed means not Jwt token for the required provider. + // JwtUnknownIssuer means wrong issuer for the required provider. Status final_status = Status::JwtMissed; for (const auto& it : verifiers_) { - // If a Jwt is extracted from a location not specified by the required provider, - // the authenticator returns JwtUnknownIssuer. It should be treated the same as - // JwtMissed. + // Prefer errors which are not JwtMissed nor JwtUnknownIssuer. + // Prefer JwtUnknownIssuer between JwtMissed and JwtUnknownIssuer. Status child_status = context.getCompletionState(it.get()).status_; - if (child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) { + if ((child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) || + final_status == Status::JwtMissed) { final_status = child_status; } } diff --git a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc index 08cea40b92513..ba198bb922a5f 100644 --- a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc +++ b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc @@ -197,7 +197,7 @@ TEST_F(SingleAllowMissingInOrListTest, BadJwt) { } TEST_F(SingleAllowMissingInOrListTest, MissingIssToken) { - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, ES256WithoutIssToken}}; context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); verifier_->verify(context_); @@ -471,6 +471,15 @@ TEST_F(AllowMissingInOrListTest, OtherGoodJwt) { EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader)); } +TEST_F(AllowMissingInOrListTest, WrongIssuer) { + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); + auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, OtherGoodToken}}; + context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); + verifier_->verify(context_); + // x-other JWT should be ignored. + EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader)); +} + TEST_F(AllowMissingInOrListTest, BadAndGoodJwts) { EXPECT_CALL(mock_cb_, onComplete(Status::JwtVerificationFail)); auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, NonExistKidToken}, @@ -589,7 +598,7 @@ TEST_F(AllowMissingInAndOfOrListTest, TwoGoodJwts) { } TEST_F(AllowMissingInAndOfOrListTest, GoodAndBadJwts) { - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); // Use the token with example.com issuer for x-other. auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, GoodToken}, {kOtherHeader, GoodToken}}; diff --git a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc index ab14d6243cb09..0ccbe3aac770f 100644 --- a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc +++ b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc @@ -582,8 +582,8 @@ TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButFailed) { callbacks_["other_provider"](Status::JwtExpired); } -// Test RequiresAny with two providers and allow_missing, but OK -TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) { +// Test RequiresAny with two providers and allow_missing, but one returns JwtUnknownIssuer +TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButUnknownIssuer) { TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_); proto_config_.mutable_rules(0) ->mutable_requires() @@ -592,7 +592,7 @@ TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) { ->mutable_allow_missing(); createAsyncMockAuthsAndVerifier(std::vector{"example_provider", "other_provider"}); - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); auto headers = Http::TestRequestHeaderMapImpl{}; context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);