From a58a7e45863bd76e22700a1fdee5fae89218ca90 Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Thu, 9 Dec 2021 16:01:22 -0800 Subject: [PATCH 1/4] remove empty principals logic --- authz/rbac_translator.go | 8 +------- authz/rbac_translator_test.go | 26 -------------------------- authz/sdk_end2end_test.go | 30 +++--------------------------- 3 files changed, 4 insertions(+), 60 deletions(-) diff --git a/authz/rbac_translator.go b/authz/rbac_translator.go index 010fc89a6e2..aaeb3aa5728 100644 --- a/authz/rbac_translator.go +++ b/authz/rbac_translator.go @@ -157,19 +157,13 @@ func parsePrincipalNames(principalNames []string) []*v3rbacpb.Principal { } func parsePeer(source peer) *v3rbacpb.Principal { - if source.Principals == nil { + if source.Principals == nil || len(source.Principals) == 0 { return &v3rbacpb.Principal{ Identifier: &v3rbacpb.Principal_Any{ Any: true, }, } } - if len(source.Principals) == 0 { - return &v3rbacpb.Principal{ - Identifier: &v3rbacpb.Principal_Authenticated_{ - Authenticated: &v3rbacpb.Principal_Authenticated{}, - }} - } return principalOr(parsePrincipalNames(source.Principals)) } diff --git a/authz/rbac_translator_test.go b/authz/rbac_translator_test.go index e22ab62ce26..64a3484ef8c 100644 --- a/authz/rbac_translator_test.go +++ b/authz/rbac_translator_test.go @@ -205,32 +205,6 @@ func TestTranslatePolicy(t *testing.T) { }, }, }, - "empty principal field": { - authzPolicy: `{ - "name": "authz", - "allow_rules": [{ - "name": "allow_authenticated", - "source": {"principals":[]} - }] - }`, - wantPolicies: []*v3rbacpb.RBAC{ - { - Action: v3rbacpb.RBAC_ALLOW, - Policies: map[string]*v3rbacpb.Policy{ - "authz_allow_authenticated": { - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Authenticated_{ - Authenticated: &v3rbacpb.Principal_Authenticated{}, - }}, - }, - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - }, - }, - }, - }, - }, "unknown field": { authzPolicy: `{"random": 123}`, wantErr: "failed to unmarshal policy", diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go index 839faaa7608..ea976714a09 100644 --- a/authz/sdk_end2end_test.go +++ b/authz/sdk_end2end_test.go @@ -261,30 +261,6 @@ var sdkTests = map[string]struct { wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), }, "DeniesRPCRequestWithPrincipalsFieldOnUnauthenticatedConnection": { - authzPolicy: `{ - "name": "authz", - "allow_rules": - [ - { - "name": "allow_TestServiceCalls", - "source": { - "principals": - [ - "foo" - ] - }, - "request": { - "paths": - [ - "/grpc.testing.TestService/*" - ] - } - } - ] - }`, - wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), - }, - "DeniesRPCRequestWithEmptyPrincipalsOnUnauthenticatedConnection": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -292,7 +268,7 @@ var sdkTests = map[string]struct { { "name": "allow_authenticated", "source": { - "principals": [] + "principals": ["*"] } } ] @@ -394,7 +370,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection( { "name": "allow_authenticated", "source": { - "principals": [] + "principals": ["*"] } } ] @@ -446,7 +422,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection { "name": "allow_authenticated", "source": { - "principals": [] + "principals": ["*"] } } ] From 5f0a981aaaad1ede84b9d6cc3c5ddd78d76cb88a Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Mon, 13 Dec 2021 16:24:03 -0800 Subject: [PATCH 2/4] Update test --- authz/sdk_end2end_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go index ea976714a09..72912a01d0d 100644 --- a/authz/sdk_end2end_test.go +++ b/authz/sdk_end2end_test.go @@ -362,7 +362,7 @@ func (s) TestSDKStaticPolicyEnd2End(t *testing.T) { } } -func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection(t *testing.T) { +func (s) TestSDKAllowsRPCRequestWithPrincipalsFieldOnTLSAuthenticatedConnection(t *testing.T) { authzPolicy := `{ "name": "authz", "allow_rules": @@ -370,7 +370,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection( { "name": "allow_authenticated", "source": { - "principals": ["*"] + "principals": ["*", ""] } } ] @@ -414,7 +414,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection( } } -func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection(t *testing.T) { +func (s) TestSDKAllowsRPCRequestWithPrincipalsFieldOnMTLSAuthenticatedConnection(t *testing.T) { authzPolicy := `{ "name": "authz", "allow_rules": @@ -422,7 +422,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection { "name": "allow_authenticated", "source": { - "principals": ["*"] + "principals": ["*", ""] } } ] From 9c7af6970d521f8c319b152931a2e6f889a871e3 Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Mon, 13 Dec 2021 16:38:25 -0800 Subject: [PATCH 3/4] minor formatting --- authz/sdk_end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go index 72912a01d0d..b3d449d5dfb 100644 --- a/authz/sdk_end2end_test.go +++ b/authz/sdk_end2end_test.go @@ -268,7 +268,7 @@ var sdkTests = map[string]struct { { "name": "allow_authenticated", "source": { - "principals": ["*"] + "principals": ["*", ""] } } ] From 080042cdbe803eb7edf3450807b6ec2e147ec75f Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Tue, 28 Dec 2021 12:13:11 -0800 Subject: [PATCH 4/4] resolving comments --- authz/rbac_translator.go | 2 +- authz/rbac_translator_test.go | 40 +++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/authz/rbac_translator.go b/authz/rbac_translator.go index aaeb3aa5728..75bbdb44d49 100644 --- a/authz/rbac_translator.go +++ b/authz/rbac_translator.go @@ -157,7 +157,7 @@ func parsePrincipalNames(principalNames []string) []*v3rbacpb.Principal { } func parsePeer(source peer) *v3rbacpb.Principal { - if source.Principals == nil || len(source.Principals) == 0 { + if len(source.Principals) == 0 { return &v3rbacpb.Principal{ Identifier: &v3rbacpb.Principal_Any{ Any: true, diff --git a/authz/rbac_translator_test.go b/authz/rbac_translator_test.go index 64a3484ef8c..e8e2f76b5ed 100644 --- a/authz/rbac_translator_test.go +++ b/authz/rbac_translator_test.go @@ -205,6 +205,46 @@ func TestTranslatePolicy(t *testing.T) { }, }, }, + "allow authenticated": { + authzPolicy: `{ + "name": "authz", + "allow_rules": [ + { + "name": "allow_authenticated", + "source": { + "principals":["*", ""] + } + }] + }`, + wantPolicies: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "authz_allow_authenticated": { + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{ + Ids: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Authenticated_{ + Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{ + MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: ".+"}}, + }}, + }}, + {Identifier: &v3rbacpb.Principal_Authenticated_{ + Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{ + MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: ""}, + }}, + }}, + }, + }}}, + }, + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + }, + }, + }, + }, + }, "unknown field": { authzPolicy: `{"random": 123}`, wantErr: "failed to unmarshal policy",