Fix a bug in origin authenticator that wrongly treats empty origin methods as pass

src/envoy/http/authn/origin_authenticator.cc

@@ -31,34 +31,19 @@ OriginAuthenticator::OriginAuthenticator(FilterContext* filter_context,
     : AuthenticatorBase(filter_context), policy_(policy) {}
 
 bool OriginAuthenticator::run(Payload* payload) {
-  bool success = false;
-
-  if (policy_.origins_size() == 0) {
-    switch (policy_.principal_binding()) {
-      case iaapi::PrincipalBinding::USE_ORIGIN:
-        // Validation should reject policy that have rule to USE_ORIGIN but
-        // does not provide any origin method so this code should
-        // never reach. However, it's ok to treat it as authentication
-        // fails.
-        ENVOY_LOG(warn,
-                  "Principal is binded to origin, but no method specified in "
-                  "policy {}",
-                  policy_.DebugString());
-        break;
-      case iaapi::PrincipalBinding::USE_PEER:
-        // On the other hand, it's ok to have no (origin) methods if
-        // rule USE_SOURCE
-        success = true;
-        break;
-      default:
-        // Should never come here.
-        ENVOY_LOG(error, "Invalid binding value for policy {}",
-                  policy_.DebugString());
-        break;
-    }
+  if (policy_.origins_size() == 0 &&
+      policy_.principal_binding() == iaapi::PrincipalBinding::USE_ORIGIN) {
+    // Validation should reject policy that have rule to USE_ORIGIN but
+    // does not provide any origin method so this code should
+    // never reach. However, it's ok to treat it as authentication
+    // fails.
+    ENVOY_LOG(warn,
+              "Principal is binded to origin, but no method specified in "
+              "policy {}",
+              policy_.DebugString());
+    return false;
   }
 
-  bool triggered = false;
   const char* request_path = nullptr;
   if (filter_context()->headerMap().Path() != nullptr) {
     request_path = filter_context()->headerMap().Path()->value().c_str();
@@ -69,6 +54,8 @@ bool OriginAuthenticator::run(Payload* payload) {
               "validation");
   }
 
+  bool triggered = false;
+  bool triggered_success = false;
   for (const auto& method : policy_.origins()) {
     const auto& jwt = method.jwt();
 
@@ -79,20 +66,22 @@ bool OriginAuthenticator::run(Payload* payload) {
       triggered = true;
       if (validateJwt(jwt, payload)) {
         ENVOY_LOG(debug, "JWT validation succeeded");
-        success = true;
+        triggered_success = true;
         break;
       }
     }
   }
 
-  if (success) {
+  // returns true if no jwt was triggered, or triggered and success.
+  if (!triggered || triggered_success) {
     filter_context()->setOriginResult(payload);
     filter_context()->setPrincipal(policy_.principal_binding());
+    ENVOY_LOG(debug, "Origin authenticator succeeded");
+    return true;
   }
 
-  // If none of the JWT triggered, origin authentication will be ignored, as if
-  // it is not defined.
-  return !triggered || success;
+  ENVOY_LOG(debug, "Origin authenticator failed");
+  return false;
 }
 
 }  // namespace AuthN

src/envoy/http/authn/origin_authenticator_test.cc

@@ -41,6 +41,14 @@ namespace Istio {
 namespace AuthN {
 namespace {
 
+const char kZeroOriginMethodPolicyBindPeer[] = R"(
+  principal_binding: USE_PEER
+)";
+
+const char kZeroOriginMethodPolicyBindOrigin[] = R"(
+  principal_binding: USE_ORIGIN
+)";
+
 const char kSingleOriginMethodPolicy[] = R"(
   principal_binding: USE_ORIGIN
   origins {
@@ -209,6 +217,36 @@ TEST_P(OriginAuthenticatorTest, Empty) {
                                       filter_context_.authenticationResult()));
 }
 
+// It should fail if the binding is USE_ORIGIN but origin methods are empty.
+TEST_P(OriginAuthenticatorTest, ZeroMethodFail) {
+  ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
+      kZeroOriginMethodPolicyBindOrigin, &policy_));
+  createAuthenticator();
+  EXPECT_FALSE(authenticator_->run(payload_));
+}
+
+// It should pass if the binding is USE_PEER and origin methods are empty.
+TEST_P(OriginAuthenticatorTest, ZeroMethodPass) {
+  ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
+      kZeroOriginMethodPolicyBindPeer, &policy_));
+  createAuthenticator();
+
+  Result expected_result = TestUtilities::AuthNResultFromString(R"(
+      origin {
+        user: "bar"
+        presenter: "istio.io"
+      }
+    )");
+  if (set_peer_) {
+    expected_result.set_principal("bar");
+    expected_result.set_peer_user("bar");
+  }
+
+  EXPECT_TRUE(authenticator_->run(&jwt_extra_payload_));
+  EXPECT_TRUE(TestUtility::protoEqual(expected_result,
+                                      filter_context_.authenticationResult()));
+}
+
 TEST_P(OriginAuthenticatorTest, SingleMethodPass) {
   ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(kSingleOriginMethodPolicy,
                                                     &policy_));