From 5d90b32d9dec9e82dd2f5181d3748c57e68e97ac Mon Sep 17 00:00:00 2001
From: Ashitha Santhosh <55257063+ashithasantosh@users.noreply.github.com>
Date: Thu, 9 Dec 2021 15:37:33 -0800
Subject: [PATCH] authz: fix regex expression match (#5035)

* Fixes regex expression matching.

* Adds tests

* Updates FulMatchWithRegex and regex string for presence match.

* Add tests for FullMatchWithRegex

* Update regex to allow whitespace characters
---
 authz/rbac_translator.go        |  8 ++++---
 authz/rbac_translator_test.go   |  2 +-
 authz/sdk_end2end_test.go       | 40 ++++++++++++++++++++++++++++-----
 internal/grpcutil/regex.go      | 11 +++++----
 internal/grpcutil/regex_test.go | 12 ++++++++++
 5 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/authz/rbac_translator.go b/authz/rbac_translator.go
index ba49b0c52..010fc89a6 100644
--- a/authz/rbac_translator.go
+++ b/authz/rbac_translator.go
@@ -94,7 +94,8 @@ func getStringMatcher(value string) *v3matcherpb.StringMatcher {
 	switch {
 	case value == "*":
 		return &v3matcherpb.StringMatcher{
-			MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{},
+			MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{
+				SafeRegex: &v3matcherpb.RegexMatcher{Regex: ".+"}},
 		}
 	case strings.HasSuffix(value, "*"):
 		prefix := strings.TrimSuffix(value, "*")
@@ -117,8 +118,9 @@ func getHeaderMatcher(key, value string) *v3routepb.HeaderMatcher {
 	switch {
 	case value == "*":
 		return &v3routepb.HeaderMatcher{
-			Name:                 key,
-			HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{},
+			Name: key,
+			HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{
+				SafeRegexMatch: &v3matcherpb.RegexMatcher{Regex: ".+"}},
 		}
 	case strings.HasSuffix(value, "*"):
 		prefix := strings.TrimSuffix(value, "*")
diff --git a/authz/rbac_translator_test.go b/authz/rbac_translator_test.go
index 9b88362ea..e22ab62ce 100644
--- a/authz/rbac_translator_test.go
+++ b/authz/rbac_translator_test.go
@@ -127,7 +127,7 @@ func TestTranslatePolicy(t *testing.T) {
 									Ids: []*v3rbacpb.Principal{
 										{Identifier: &v3rbacpb.Principal_Authenticated_{
 											Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{
-												MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{},
+												MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: ".+"}},
 											}},
 										}},
 									},
diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go
index 79fa379bc..839faaa76 100644
--- a/authz/sdk_end2end_test.go
+++ b/authz/sdk_end2end_test.go
@@ -95,8 +95,7 @@ var sdkTests = map[string]struct {
 						"request": {
 							"paths":
 							[
-								"/grpc.testing.TestService/UnaryCall",
-								"/grpc.testing.TestService/StreamingInputCall"
+								"/grpc.testing.TestService/*"
 							],
 							"headers":
 							[
@@ -122,11 +121,11 @@ var sdkTests = map[string]struct {
 				"allow_rules":
 				[
 					{
-						"name": "allow_TestServiceCalls",
+						"name": "allow_all",
 						"request": {
 							"paths":
 							[
-								"/grpc.testing.TestService/*"
+								"*"
 							]
 						}
 					}
@@ -134,11 +133,11 @@ var sdkTests = map[string]struct {
 				"deny_rules":
 				[
 					{
-						"name": "deny_TestServiceCalls",
+						"name": "deny_all",
 						"request": {
 							"paths":
 							[
-								"/grpc.testing.TestService/*"
+								"*"
 							]
 						}
 					}
@@ -300,6 +299,35 @@ var sdkTests = map[string]struct {
 			}`,
 		wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"),
 	},
+	"DeniesRPCRequestNoMatchInAllowFailsPresenceMatch": {
+		authzPolicy: `{
+				"name": "authz",
+				"allow_rules":
+				[
+					{
+						"name": "allow_TestServiceCalls",
+						"request": {
+							"paths":
+							[
+								"/grpc.testing.TestService/*"
+							],
+							"headers":
+							[
+								{
+									"key": "key-abc",
+									"values":
+									[
+										"*"
+									]
+								}
+							]
+						}
+					}
+				]
+			}`,
+		md:         metadata.Pairs("key-abc", ""),
+		wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"),
+	},
 }
 
 func (s) TestSDKStaticPolicyEnd2End(t *testing.T) {
diff --git a/internal/grpcutil/regex.go b/internal/grpcutil/regex.go
index 2810a8ba2..7a092b2b8 100644
--- a/internal/grpcutil/regex.go
+++ b/internal/grpcutil/regex.go
@@ -20,9 +20,12 @@ package grpcutil
 
 import "regexp"
 
-// FullMatchWithRegex returns whether the full string matches the regex provided.
-func FullMatchWithRegex(re *regexp.Regexp, string string) bool {
+// FullMatchWithRegex returns whether the full text matches the regex provided.
+func FullMatchWithRegex(re *regexp.Regexp, text string) bool {
+	if len(text) == 0 {
+		return re.MatchString(text)
+	}
 	re.Longest()
-	rem := re.FindString(string)
-	return len(rem) == len(string)
+	rem := re.FindString(text)
+	return len(rem) == len(text)
 }
diff --git a/internal/grpcutil/regex_test.go b/internal/grpcutil/regex_test.go
index 1b2299858..4c12804fe 100644
--- a/internal/grpcutil/regex_test.go
+++ b/internal/grpcutil/regex_test.go
@@ -48,6 +48,18 @@ func TestFullMatchWithRegex(t *testing.T) {
 			string:   "ab",
 			want:     true,
 		},
+		{
+			name:     "match all",
+			regexStr: ".*",
+			string:   "",
+			want:     true,
+		},
+		{
+			name:     "matches non-empty strings",
+			regexStr: ".+",
+			string:   "",
+			want:     false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {