From 8756bf7c06bf5d9d4e6b313478f301482c235c12 Mon Sep 17 00:00:00 2001 From: sushmith <6890568+bsushmith@users.noreply.github.com> Date: Thu, 12 Oct 2023 15:55:54 +0530 Subject: [PATCH] fix(gcloudiam): add check for duplicate roles (#94) --- internal/server/services.go | 2 +- pkg/evaluator/expression_test.go | 1 - plugins/providers/gcloudiam/config.go | 10 ++ plugins/providers/gcloudiam/config_test.go | 147 +++++++++++++++++++ plugins/providers/gcloudiam/provider.go | 5 +- plugins/providers/gcloudiam/provider_test.go | 61 +++++--- 6 files changed, 203 insertions(+), 23 deletions(-) diff --git a/internal/server/services.go b/internal/server/services.go index 2ee12be3b..06e274d47 100644 --- a/internal/server/services.go +++ b/internal/server/services.go @@ -107,7 +107,7 @@ func InitServices(deps ServiceDeps) (*Services, error) { metabase.NewProvider(domain.ProviderTypeMetabase, deps.Crypto, deps.Logger), grafana.NewProvider(domain.ProviderTypeGrafana, deps.Crypto), tableau.NewProvider(domain.ProviderTypeTableau, deps.Crypto), - gcloudiam.NewProvider(domain.ProviderTypeGCloudIAM, deps.Crypto), + gcloudiam.NewProvider(domain.ProviderTypeGCloudIAM, deps.Crypto, deps.Logger), noop.NewProvider(domain.ProviderTypeNoOp, deps.Logger), gcs.NewProvider(domain.ProviderTypeGCS, deps.Crypto), dataplex.NewProvider(domain.ProviderTypePolicyTag, deps.Crypto), diff --git a/pkg/evaluator/expression_test.go b/pkg/evaluator/expression_test.go index ee6c12852..3cd0d5a42 100644 --- a/pkg/evaluator/expression_test.go +++ b/pkg/evaluator/expression_test.go @@ -164,7 +164,6 @@ func TestEvaluate(t *testing.T) { }, expectedResult: true, }, - { expression: "len(Split($user.email_id, '@')[0]) > 2", params: map[string]interface{}{ diff --git a/plugins/providers/gcloudiam/config.go b/plugins/providers/gcloudiam/config.go index 2a08c63fa..ec53595e1 100644 --- a/plugins/providers/gcloudiam/config.go +++ b/plugins/providers/gcloudiam/config.go @@ -127,6 +127,16 @@ func (c *Config) parseAndValidate() error { if len(rc.Roles) == 0 { validationErrors = append(validationErrors, ErrRolesShouldNotBeEmpty) } + + // check for duplicates in roles + rolesMap := make(map[string]bool, 0) + for _, role := range rc.Roles { + if val, ok := rolesMap[role.ID]; ok && val { + validationErrors = append(validationErrors, fmt.Errorf("duplicate role: %q", role.ID)) + } else { + rolesMap[role.ID] = true + } + } } if len(validationErrors) > 0 { diff --git a/plugins/providers/gcloudiam/config_test.go b/plugins/providers/gcloudiam/config_test.go index faefc401d..fc6978b2b 100644 --- a/plugins/providers/gcloudiam/config_test.go +++ b/plugins/providers/gcloudiam/config_test.go @@ -3,6 +3,8 @@ package gcloudiam_test import ( "encoding/base64" "errors" + "github.com/goto/guardian/domain" + "github.com/goto/guardian/pkg/crypto" "testing" "github.com/goto/guardian/mocks" @@ -93,3 +95,148 @@ func TestCredentials(t *testing.T) { }) }) } + +func TestConfig_ParseAndValidate(t *testing.T) { + + t.Run("should return error if resource config is nil", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: nil, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.EqualError(t, actualErr, "empty resource config") + }) + + t.Run("should return error if service account key is not base64", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: "test-service-account-key", + ResourceName: "projects/test-project", + }, + Resources: nil, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.NotNil(t, actualErr) + }) + t.Run("should return error if duplicate resource type is present", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-roleA", Name: "test role A", Permissions: []interface{}{"test-permission-a"}}, + }, + }, + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-roleB", Name: "test role B", Permissions: []interface{}{"test-permission-b"}}, + }}, + }, + } + expectedErr := "duplicate resource type: \"project\"" + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Equal(t, expectedErr, actualErr.Error()) + }) + + t.Run("should return error for invalid resource type", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + {Type: "invalid-resource-type"}, + }, + } + expectedErr := "invalid resource type: \"invalid-resource-type\"\ngcloud_iam provider should not have empty roles" + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Equal(t, expectedErr, actualErr.Error()) + }) + + t.Run("should return nil if config is valid", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-role", Name: "test role", Permissions: []interface{}{"test-permission"}}, + }, + }, + }, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Nil(t, actualErr) + }) + + t.Run("should return error if duplicate role is configured", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-role1", Name: "test role 1", Permissions: []interface{}{"test-permission-1"}}, + {ID: "test-role2", Name: "test role 2", Permissions: []interface{}{"test-permission-2"}}, + {ID: "test-role1", Name: "test role 1", Permissions: []interface{}{"test-permission-11"}}, + {ID: "test-role3", Name: "test role 3", Permissions: []interface{}{"test-permission-3"}}, + }, + }, + }, + } + + expectedErr := "duplicate role: \"test-role1\"" + + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.EqualError(t, actualErr, expectedErr) + }) + +} + +func getBase64EncodedString() string { + return base64.StdEncoding.EncodeToString([]byte("test-service-account-key")) +} diff --git a/plugins/providers/gcloudiam/provider.go b/plugins/providers/gcloudiam/provider.go index 9691686e0..81148c77e 100644 --- a/plugins/providers/gcloudiam/provider.go +++ b/plugins/providers/gcloudiam/provider.go @@ -6,6 +6,7 @@ import ( "github.com/goto/guardian/core/provider" "github.com/goto/guardian/domain" + "github.com/goto/salt/log" "github.com/mitchellh/mapstructure" "golang.org/x/net/context" "google.golang.org/api/iam/v1" @@ -34,13 +35,15 @@ type Provider struct { typeName string Clients map[string]GcloudIamClient crypto encryptor + logger log.Logger } -func NewProvider(typeName string, crypto encryptor) *Provider { +func NewProvider(typeName string, crypto encryptor, logger log.Logger) *Provider { return &Provider{ typeName: typeName, Clients: map[string]GcloudIamClient{}, crypto: crypto, + logger: logger, } } diff --git a/plugins/providers/gcloudiam/provider_test.go b/plugins/providers/gcloudiam/provider_test.go index 8d0433a0e..48959fc71 100644 --- a/plugins/providers/gcloudiam/provider_test.go +++ b/plugins/providers/gcloudiam/provider_test.go @@ -3,6 +3,7 @@ package gcloudiam_test import ( "encoding/base64" "errors" + "github.com/goto/salt/log" "testing" "github.com/goto/guardian/domain" @@ -18,7 +19,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -91,7 +93,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -181,7 +184,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -229,7 +233,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -278,7 +283,8 @@ func TestGetType(t *testing.T) { t.Run("should return provider type name", func(t *testing.T) { expectedTypeName := domain.ProviderTypeMetabase crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider(expectedTypeName, crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider(expectedTypeName, crypto, l) actualTypeName := p.GetType() @@ -289,7 +295,8 @@ func TestGetType(t *testing.T) { func TestGetResources(t *testing.T) { t.Run("should error when credentials are invalid", func(t *testing.T) { crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) pc := &domain.ProviderConfig{ Type: domain.ProviderTypeGCloudIAM, URN: "test-project-id", @@ -308,7 +315,8 @@ func TestGetResources(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -517,7 +525,8 @@ func TestGetResources(t *testing.T) { func TestGrantAccess(t *testing.T) { t.Run("should return error if credentials is invalid", func(t *testing.T) { crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) pc := &domain.ProviderConfig{ Credentials: "invalid-credentials", @@ -541,7 +550,8 @@ func TestGrantAccess(t *testing.T) { t.Run("should return error if resource type is unknown", func(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) crypto.On("Decrypt", "c2VydmljZS1hY2NvdW50LWtleS1qc29u").Return(`{"type":"service_account"}`, nil) // tests the newIamClient when p.Clients is not initialised in the provider config expectedError := errors.New("invalid resource type") @@ -614,7 +624,8 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -662,11 +673,12 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedRole := "role-1" expectedAccountType := "user" expectedAccountID := "test@email.com" expectedPermission := "roles/bigquery.admin" - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -714,7 +726,8 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -758,7 +771,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -829,7 +843,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -881,7 +896,8 @@ func TestRevokeAccess(t *testing.T) { expectedPermission := "roles/bigquery.admin" expectedAccountType := "user" expectedAccountID := "test@email.com" - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -928,7 +944,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -973,7 +990,8 @@ func TestGetRoles(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -996,6 +1014,7 @@ func TestGetRoles(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedRoles := []*domain.Role{ { ID: "role-1", @@ -1004,7 +1023,7 @@ func TestGetRoles(t *testing.T) { }, } - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -1038,9 +1057,10 @@ func TestGetPermissions(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedPermissions := []interface{}{"roles/bigquery.admin"} - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -1073,7 +1093,8 @@ func TestGetAccountTypes(t *testing.T) { t.Run("should return the supported account types: user, serviceAccount", func(t *testing.T) { expectedAccountTypes := []string{"user", "serviceAccount", "group"} crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) actualAccountTypes := p.GetAccountTypes()