jwt_authn: return a valid error message for require_any

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);
     }
   }
 };

test/extensions/filters/http/jwt_authn/group_verifier_test.cc

@@ -351,14 +351,59 @@ 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<MockAuthenticator>();
+  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<MockAuthenticator>();
+  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<MockAuthenticator>();
   createSyncMockAuthsAndVerifier(StatusMap{{"example_provider", Status::JwtHeaderBadKid},
                                            {"other_provider", Status::JwtUnknownIssuer}});
 
   // 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", ""},