backport retain JwtUnknownIssuer error for allow_missing (#15199)

Signed-off-by: Asra Ali <[email protected]>
envoyproxy · Feb 26, 2021 · d6a4496 · d6a4496
1 parent a7d9915
commit d6a4496
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 16 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-1.17.1-dev
+1.17.1
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -1,5 +1,5 @@
-1.17.1 (Pending)
-================
+1.17.1 (February 25, 2021)
+==========================
 
 Incompatible Behavior Changes
 -----------------------------
@@ -13,6 +13,7 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
+* 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.
 * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration.
 
 Removed Config or Runtime

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