From ec3ff8bbd35968ac0095324788fb925611b5331d Mon Sep 17 00:00:00 2001 From: Yimin Chen Date: Tue, 25 Apr 2023 19:52:47 -0700 Subject: [PATCH] Block non admin role to access system services (#4227) * Block non admin role to access system services --- common/authorization/default_authorizer.go | 50 ++++- .../authorization/default_authorizer_test.go | 184 +++++++++--------- 2 files changed, 129 insertions(+), 105 deletions(-) diff --git a/common/authorization/default_authorizer.go b/common/authorization/default_authorizer.go index a5f8d817bb0..428ed3c0e8a 100644 --- a/common/authorization/default_authorizer.go +++ b/common/authorization/default_authorizer.go @@ -34,6 +34,11 @@ type ( } ) +const ( + operatorServicePrefix = "/temporal.api.operatorservice.v1.OperatorService/" + adminServicePrefix = "/temporal.server.api.adminservice.v1.AdminService/" +) + var _ Authorizer = (*defaultAuthorizer)(nil) // NewDefaultAuthorizer creates a default authorizer @@ -44,6 +49,16 @@ func NewDefaultAuthorizer() Authorizer { var resultAllow = Result{Decision: DecisionAllow} var resultDeny = Result{Decision: DecisionDeny} +// Authorize determines if an API call by given claims should be allowed or denied. +// Rules: +// +// Health check APIs are allowed to everyone. +// System Admin is allowed to access all APIs on all namespaces. +// System Writer is allowed to access non admin APIs on all namespaces. +// System Reader is allowed to access readonly APIs on all namespaces. +// Namespace Admin is allowed to access all APIs on their namespaces. +// Namespace Writer is allowed to access non admin APIs on their namespaces. +// Namespace Reader is allowed to access non admin readonly APIs on their namespaces. func (a *defaultAuthorizer) Authorize(_ context.Context, claims *Claims, target *CallTarget) (Result, error) { // APIs that are essentially read-only health checks with no sensitive information are // always allowed @@ -54,27 +69,46 @@ func (a *defaultAuthorizer) Authorize(_ context.Context, claims *Claims, target if claims == nil { return resultDeny, nil } - // Check system level permissions - if claims.System >= RoleWriter { + // System Admin is allowed for everything + if claims.System >= RoleAdmin { + return resultAllow, nil + } + + // admin service means admin / operator service + isAdminService := strings.HasPrefix(target.APIName, adminServicePrefix) || strings.HasPrefix(target.APIName, operatorServicePrefix) + + // System Writer is allowed for non admin service APIs + if claims.System >= RoleWriter && !isAdminService { return resultAllow, nil } api := ApiName(target.APIName) readOnlyNamespaceAPI := IsReadOnlyNamespaceAPI(api) readOnlyGlobalAPI := IsReadOnlyGlobalAPI(api) + // System Reader is allowed for all read only APIs if claims.System >= RoleReader && (readOnlyNamespaceAPI || readOnlyGlobalAPI) { return resultAllow, nil } - role, found := claims.Namespaces[strings.ToLower(target.Namespace)] + // Below are for non system roles. + role, found := claims.Namespaces[target.Namespace] if !found || role == RoleUndefined { return resultDeny, nil } - if role >= RoleWriter { - return resultAllow, nil - } - if role >= RoleReader && readOnlyNamespaceAPI { - return resultAllow, nil + + if isAdminService { + // for admin service APIs, only RoleAdmin of given namespace can access + if role >= RoleAdmin { + return resultAllow, nil + } + } else { + // for non admin service APIs + if role >= RoleWriter { + return resultAllow, nil + } + if role >= RoleReader && readOnlyNamespaceAPI { + return resultAllow, nil + } } return resultDeny, nil diff --git a/common/authorization/default_authorizer_test.go b/common/authorization/default_authorizer_test.go index 9c7770417b6..1320529996a 100644 --- a/common/authorization/default_authorizer_test.go +++ b/common/authorization/default_authorizer_test.go @@ -32,17 +32,31 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" - "go.temporal.io/server/common/config" ) var ( - claimsNone = Claims{} - claimsNamespaceOnly = Claims{ + claimsNone = Claims{} + claimsNamespaceAdmin = Claims{ + Namespaces: map[string]Role{ + testNamespace: RoleAdmin, + }, + } + claimsNamespaceWriter = Claims{ Namespaces: map[string]Role{ testNamespace: RoleWriter, }, } + claimsNamespaceReader = Claims{ + Namespaces: map[string]Role{ + testNamespace: RoleReader, + }, + } + claimsBarAdmin = Claims{ + Namespaces: map[string]Role{ + "bar": RoleAdmin, + }, + } claimsSystemAdmin = Claims{ System: RoleAdmin, } @@ -52,19 +66,6 @@ var ( claimsSystemReader = Claims{ System: RoleReader, } - claimsSystemReaderNamespaceUndefined = Claims{ - System: RoleReader, - Namespaces: map[string]Role{ - "bar": RoleUndefined, - }, - } - claimsSystemUndefinedNamespaceReader = Claims{ - System: RoleUndefined, - Namespaces: map[string]Role{ - "bar": RoleReader, - }, - } - targetFooBar = CallTarget{ APIName: "Foo", Namespace: "bar", @@ -89,6 +90,18 @@ var ( APIName: "/temporal.api.workflowservice.v1.WorkflowService/GetSystemInfo", Namespace: "", } + targetStartWorkflow = CallTarget{ + Namespace: testNamespace, + APIName: "/temporal.api.workflowservice.v1.WorkflowService/StartWorkflowExecution", + } + targetAdminAPI = CallTarget{ + Namespace: testNamespace, + APIName: "/temporal.server.api.adminservice.v1.AdminService/AddSearchAttributes", + } + targetAdminReadonlyAPI = CallTarget{ + Namespace: testNamespace, + APIName: "/temporal.server.api.adminservice.v1.AdminService/GetSearchAttributes", + } ) type ( @@ -116,87 +129,64 @@ func (s *defaultAuthorizerSuite) TearDownTest() { s.controller.Finish() } -func (s *defaultAuthorizerSuite) TestSystemAdminAuthZ() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemAdmin, &targetFooBar) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemWriterAuthZ() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemWriter, &targetFooBar) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemReaderAuthZ() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemReader, &targetFooBar) - s.NoError(err) - s.Equal(DecisionDeny, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemReaderBarUndefinedAuthZ() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemReaderNamespaceUndefined, &targetFooBar) - s.NoError(err) - s.Equal(DecisionDeny, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemUndefinedNamespaceReaderAuthZ() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemUndefinedNamespaceReader, &targetFooBar) - s.NoError(err) - s.Equal(DecisionDeny, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemUndefinedNamespaceCaseMismatch() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemUndefinedNamespaceReader, &targetFooBAR) - s.NoError(err) - s.Equal(DecisionDeny, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemUndefinedNamespaceReaderListNamespaces() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemUndefinedNamespaceReader, &targetListNamespaces) - s.NoError(err) - s.Equal(DecisionDeny, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemUndefinedNamespaceReaderDescribeNamespace() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemUndefinedNamespaceReader, &targetDescribeNamespace) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemWriterDescribeNamespace() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemWriter, &targetDescribeNamespace) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemWriterListNamespaces() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemWriter, &targetListNamespaces) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemAdminDescribeNamespace() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemAdmin, &targetDescribeNamespace) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestSystemAdminListNamespaces() { - result, err := s.authorizer.Authorize(context.TODO(), &claimsSystemAdmin, &targetListNamespaces) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestNamespaceOnly() { - // don't need any system-level claims to do namespace-level apis - result, err := s.authorizer.Authorize(context.TODO(), &claimsNamespaceOnly, startWorkflowExecutionTarget) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) -} -func (s *defaultAuthorizerSuite) TestHealthChecks() { - // all health checks should work all the time - for _, claims := range []*Claims{ - nil, - &claimsNone, - &claimsNamespaceOnly, - } { - for _, target := range []*CallTarget{ - &targetGrpcHealthCheck, - &targetGetSystemInfo, - } { - result, err := s.authorizer.Authorize(context.TODO(), claims, target) - s.NoError(err) - s.Equal(DecisionAllow, result.Decision) - } +func (s *defaultAuthorizerSuite) TestAuthorize() { + testCases := []struct { + Name string + Claims Claims + Target CallTarget + Decision Decision + }{ + // SystemAdmin is allowed on everything + {"SystemAdminOnFooBar", claimsSystemAdmin, targetFooBar, DecisionAllow}, + {"SystemAdminOnAdminAPI", claimsSystemAdmin, targetAdminAPI, DecisionAllow}, + {"SystemAdminOnReadonlyAPI", claimsSystemAdmin, targetAdminReadonlyAPI, DecisionAllow}, + {"SystemAdminOnStartWorkflow", claimsSystemAdmin, targetStartWorkflow, DecisionAllow}, + + // SystemWriter is allowed on all read only APIs and non-admin APIs on every namespaces + {"SystemWriterOnFooBar", claimsSystemWriter, targetFooBar, DecisionAllow}, + {"SystemWriterOnAdminAPI", claimsSystemWriter, targetAdminAPI, DecisionDeny}, + {"SystemWriterOnReadonlyAPI", claimsSystemWriter, targetAdminReadonlyAPI, DecisionAllow}, + {"SystemWriterOnStartWorkflow", claimsSystemWriter, targetStartWorkflow, DecisionAllow}, + + // SystemReader is allowed on all read only APIs and blocked + {"SystemReaderOnFooBar", claimsSystemReader, targetFooBar, DecisionDeny}, + {"SystemReaderOnAdminAPI", claimsSystemReader, targetAdminAPI, DecisionDeny}, + {"SystemReaderOnReadonlyAPI", claimsSystemReader, targetAdminReadonlyAPI, DecisionAllow}, + {"SystemReaderOnStartWorkflow", claimsSystemReader, targetStartWorkflow, DecisionDeny}, + + // NamespaceAdmin is allowed on admin service to their own namespaces (test-namespace) + {"NamespaceAdminOnAdminAPI", claimsNamespaceAdmin, targetAdminAPI, DecisionAllow}, + {"NamespaceAdminOnReadonlyAPI", claimsNamespaceAdmin, targetAdminReadonlyAPI, DecisionAllow}, + {"NamespaceAdminOnStartWorkflow", claimsNamespaceAdmin, targetStartWorkflow, DecisionAllow}, + {"NamespaceAdminOnFooBar", claimsNamespaceAdmin, targetFooBar, DecisionDeny}, // namespace mismatch + + {"BarAdminOnFooBar", claimsBarAdmin, targetFooBar, DecisionAllow}, + {"BarAdminOnFooBAR", claimsBarAdmin, targetFooBAR, DecisionDeny}, // namespace case mismatch + + // NamespaceWriter is not allowed on admin APIs + {"NamespaceWriterOnAdminAPI", claimsNamespaceWriter, targetAdminAPI, DecisionDeny}, + {"NamespaceWriterOnReadonlyAPI", claimsNamespaceWriter, targetAdminReadonlyAPI, DecisionDeny}, + {"NamespaceWriterOnStartWorkflow", claimsNamespaceWriter, targetStartWorkflow, DecisionAllow}, + {"NamespaceWriterOnFooBar", claimsNamespaceWriter, targetFooBar, DecisionDeny}, // namespace mismatch + + // NamespaceReader is allowed on read-only APIs on non admin service + {"NamespaceReaderOnAdminAPI", claimsNamespaceReader, targetAdminAPI, DecisionDeny}, + {"NamespaceReaderOnReadonlyAPI", claimsNamespaceReader, targetAdminReadonlyAPI, DecisionDeny}, + {"NamespaceReaderOnStartWorkflow", claimsNamespaceReader, targetStartWorkflow, DecisionDeny}, + {"NamespaceReaderOnFooBar", claimsNamespaceReader, targetFooBar, DecisionDeny}, // namespace mismatch + {"NamespaceReaderOnListWorkflow", claimsNamespaceReader, targetGetSystemInfo, DecisionAllow}, + + // healthcheck allowed to everyone + {"RoleNoneOnGetSystemInfo", claimsNone, targetGetSystemInfo, DecisionAllow}, + {"NamespaceReaderOnGetSystemInfo", claimsNamespaceReader, targetGetSystemInfo, DecisionAllow}, + {"RoleNoneOnHealthCheck", claimsNone, targetGrpcHealthCheck, DecisionAllow}, + {"NamespaceReaderOnHealthCheck", claimsNamespaceReader, targetGrpcHealthCheck, DecisionAllow}, + } + + for _, tt := range testCases { + result, err := s.authorizer.Authorize(context.TODO(), &tt.Claims, &tt.Target) + s.NoError(err) + s.Equal(tt.Decision, result.Decision, "Failed case: %v", tt.Name) } }