diff --git a/source/extensions/filters/http/jwt_authn/verifier.cc b/source/extensions/filters/http/jwt_authn/verifier.cc index 130f7a7fba4c9..138a50e95a9d5 100644 --- a/source/extensions/filters/http/jwt_authn/verifier.cc +++ b/source/extensions/filters/http/jwt_authn/verifier.cc @@ -25,6 +25,8 @@ struct CompletionState { bool is_completed_{false}; // number of completed inner verifier for an any/all verifier. std::size_t number_completed_children_{0}; + // A valid error for a RequireAny + Status any_valid_error_{Status::Ok}; }; class ContextImpl : public Verifier::Context { @@ -297,10 +299,20 @@ class AnyVerifierImpl : public BaseGroupVerifierImpl { if (completion_state.is_completed_) { return; } + // For RequireAny: usually it returns the error from the last provider. + // But if a Jwt is not for a provider, its auth returns JwtMissed or JwtUnknownIssuer. + // Such error should not be used as the final error if there are other valid errors. + if (status != Status::Ok && status != Status::JwtMissed && status != Status::JwtUnknownIssuer) { + completion_state.any_valid_error_ = status; + } if (++completion_state.number_completed_children_ == verifiers_.size() || Status::Ok == status) { completion_state.is_completed_ = true; - completeWithStatus(status, context); + Status final_status = status; + if (status != Status::Ok && completion_state.any_valid_error_ != Status::Ok) { + final_status = completion_state.any_valid_error_; + } + completeWithStatus(final_status, context); } } }; 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 137b6f855704e..10ca0909555ed 100644 --- a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc +++ b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc @@ -351,6 +351,51 @@ TEST_F(GroupVerifierTest, TestRequiresAnyLastAuthOk) { // Test requires any with both auth returning error. Requires any returns the error last received // back to the caller. TEST_F(GroupVerifierTest, TestRequiresAnyAllAuthFailed) { + TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_); + auto mock_auth = std::make_unique(); + createSyncMockAuthsAndVerifier(StatusMap{{"example_provider", Status::JwtMissed}, + {"other_provider", Status::JwtHeaderBadKid}}); + + // onComplete with failure status, not payload + EXPECT_CALL(mock_cb_, setPayload(_)).Times(0); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtHeaderBadKid)).Times(1); + auto headers = Http::TestRequestHeaderMapImpl{ + {"example-auth-userinfo", ""}, + {"other-auth-userinfo", ""}, + }; + context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); + verifier_->verify(context_); + EXPECT_FALSE(headers.has("example-auth-userinfo")); + EXPECT_FALSE(headers.has("other-auth-userinfo")); +} + +// Test requires any with both auth returning errors, last error is JwtMissed. +// Usually the final error is from the last one. +// But if a token is not for a provider, that provider auth will either return +// JwtMissed or JwtUnknownIssuer, such error should not be used for the final +// error in Any case +TEST_F(GroupVerifierTest, TestRequiresAnyLastIsJwtMissed) { + TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_); + auto mock_auth = std::make_unique(); + createSyncMockAuthsAndVerifier(StatusMap{{"example_provider", Status::JwtHeaderBadKid}, + {"other_provider", Status::JwtMissed}}); + + // onComplete with failure status, not payload + EXPECT_CALL(mock_cb_, setPayload(_)).Times(0); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtHeaderBadKid)).Times(1); + auto headers = Http::TestRequestHeaderMapImpl{ + {"example-auth-userinfo", ""}, + {"other-auth-userinfo", ""}, + }; + context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); + verifier_->verify(context_); + EXPECT_FALSE(headers.has("example-auth-userinfo")); + EXPECT_FALSE(headers.has("other-auth-userinfo")); +} + +// Test requires any with both auth returning errors: last error is +// JwtUnknownIssuer +TEST_F(GroupVerifierTest, TestRequiresAnyLastIsJwtUnknownIssuer) { TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_); auto mock_auth = std::make_unique(); createSyncMockAuthsAndVerifier(StatusMap{{"example_provider", Status::JwtHeaderBadKid}, @@ -358,7 +403,7 @@ TEST_F(GroupVerifierTest, TestRequiresAnyAllAuthFailed) { // onComplete with failure status, not payload EXPECT_CALL(mock_cb_, setPayload(_)).Times(0); - EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)).Times(1); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtHeaderBadKid)).Times(1); auto headers = Http::TestRequestHeaderMapImpl{ {"example-auth-userinfo", ""}, {"other-auth-userinfo", ""},