jwt_authn: fix a bug where JWT with wrong issuer is allowed in allow_…

…missing case (#15194) [jwt] When allow_missing is used inside RequiresAny, the requests with JWT with wrong issuer are accepted. This is a bug, allow_missing should only allow requests without any JWT. This change fixed the above issue by preserving JwtUnknownIssuer in allow_missing case. Signed-off-by: Wayne Zhang <qiwzhang@google.com>
envoyproxy · Feb 26, 2021 · ea39e3c · ea39e3c
1 parent d3ff1c6
commit ea39e3c
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<std::string>{"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_);