From 95bc5111e5e83fad92c0324d3c83c5bba3001da9 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Wed, 2 Sep 2020 15:46:30 +0200 Subject: [PATCH 1/4] Switch over from roleCache+middleware to roleManager --- go.mod | 2 +- go.sum | 3 +++ pkg/proto/v0/accounts.pb.micro_test.go | 36 -------------------------- pkg/server/http/server.go | 26 +++++++++++-------- pkg/service/v0/accounts.go | 17 +++++++----- pkg/service/v0/option.go | 8 +++--- pkg/service/v0/service.go | 29 +++++++++------------ 7 files changed, 45 insertions(+), 76 deletions(-) diff --git a/go.mod b/go.mod index ae595565b..76f9e634e 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/olekukonko/tablewriter v0.0.4 github.com/onsi/ginkgo v1.10.1 // indirect github.com/onsi/gomega v1.7.0 // indirect - github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b + github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0 // indirect github.com/restic/calens v0.2.0 diff --git a/go.sum b/go.sum index c7c96e591..b92f6bd0f 100644 --- a/go.sum +++ b/go.sum @@ -601,6 +601,7 @@ github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO github.com/hashicorp/mdns v1.0.0/go.mod h1:tL+uN++7HEJ6SQLQ2/p+z2pH24WQKWjBPkE0mNTz8vQ= github.com/hashicorp/memberlist v0.1.3/go.mod h1:ajVTdAv/9Im8oMAAj5G31PhhMCZJV2pPBoIllUwCN7I= github.com/hashicorp/serf v0.8.2/go.mod h1:6hOLApaqBFA1NXqRQAsxw9QxuDEvNxSQRwA/JwenrHc= +github.com/haya14busa/goverage v0.0.0-20180129164344-eec3514a20b5 h1:FdBGmSkD2QpQzRWup//SGObvWf2nq89zj9+ta9OvI3A= github.com/haya14busa/goverage v0.0.0-20180129164344-eec3514a20b5/go.mod h1:0YZ2wQSuwviXXXGUiK6zXzskyBLAbLXhamxzcFHSLoM= github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= @@ -840,6 +841,8 @@ github.com/ovh/go-ovh v0.0.0-20181109152953-ba5adb4cf014/go.mod h1:joRatxRJaZBsY github.com/owncloud/ocis-pkg/v2 v2.4.0/go.mod h1:FSzIvhx9HcZcq4jgNaDowNvM7PTX/XCyoMvyfzidUpE= github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b h1:PRw0b4abdrDKloh417qPsS5lkB/bjJ3Rc4txzHx/hBg= github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw= +github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a h1:6iW1JWmAl5TFDhxGnPhL7VwolrcQsHWvXdZt+GremZI= +github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw= github.com/owncloud/ocis-settings v0.3.2-0.20200828091056-47af10a0e872/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA= github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe h1:kiU5lz12R0LNJE1/zI2vxesZPWm6BvSO7hvZC8yOoAc= github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA= diff --git a/pkg/proto/v0/accounts.pb.micro_test.go b/pkg/proto/v0/accounts.pb.micro_test.go index 7386e0cd1..56c6e331d 100644 --- a/pkg/proto/v0/accounts.pb.micro_test.go +++ b/pkg/proto/v0/accounts.pb.micro_test.go @@ -170,8 +170,6 @@ func init() { log.Fatalf("Could not create new service") } - hdlr.Client = mockClient{} - err = proto.RegisterAccountsServiceHandler(service.Server(), hdlr) if err != nil { log.Fatal("could not register the Accounts handler") @@ -1189,37 +1187,3 @@ func TestAccountUpdateReadOnlyField(t *testing.T) { cleanUp(t) } - -type mockClient struct{} - -func (c mockClient) Init(option ...client.Option) error { - return nil -} - -func (c mockClient) Options() client.Options { - return client.Options{} -} - -func (c mockClient) NewMessage(topic string, msg interface{}, opts ...client.MessageOption) client.Message { - return nil -} - -func (c mockClient) NewRequest(service, endpoint string, req interface{}, reqOpts ...client.RequestOption) client.Request { - return nil -} - -func (c mockClient) Call(ctx context.Context, req client.Request, rsp interface{}, opts ...client.CallOption) error { - return nil -} - -func (c mockClient) Stream(ctx context.Context, req client.Request, opts ...client.CallOption) (client.Stream, error) { - return nil, nil -} - -func (c mockClient) Publish(ctx context.Context, msg client.Message, opts ...client.PublishOption) error { - return nil -} - -func (c mockClient) String() string { - return "ClientMock" -} diff --git a/pkg/server/http/server.go b/pkg/server/http/server.go index f11337c82..64fef9dd0 100644 --- a/pkg/server/http/server.go +++ b/pkg/server/http/server.go @@ -30,9 +30,21 @@ func Server(opts ...Option) http.Service { http.Flags(options.Flags...), ) - roleCache := roles.NewCache(roles.Size(1024), roles.TTL(time.Hour*24*7)) - - handler, err := svc.New(svc.Logger(options.Logger), svc.Config(options.Config), svc.RoleCache(&roleCache)) + // TODO this won't work with a registry other than mdns. Look into Micro's client initialization. + // https://github.com/owncloud/ocis-proxy/issues/38 + rs := settings.NewRoleService("com.owncloud.api.settings", mclient.DefaultClient) + roleManager := roles.NewManager( + roles.CacheSize(1024), + roles.CacheTTL(time.Hour*24*7), + roles.Logger(options.Logger), + roles.RoleService(rs), + ) + handler, err := svc.New( + svc.Logger(options.Logger), + svc.Config(options.Config), + svc.RoleManager(&roleManager), + svc.RoleService(rs), + ) if err != nil { options.Logger.Fatal().Err(err).Msg("could not initialize service handler") } @@ -48,14 +60,6 @@ func Server(opts ...Option) http.Service { account.Logger(options.Logger), account.JWTSecret(options.Config.TokenManager.JWTSecret)), ) - // TODO this won't work with a registry other than mdns. Look into Micro's client initialization. - // https://github.com/owncloud/ocis-proxy/issues/38 - rs := settings.NewRoleService("com.owncloud.api.settings", mclient.DefaultClient) - mux.Use(middleware.Roles( - options.Logger, - rs, - &roleCache, - )) mux.Use(middleware.Version( options.Name, diff --git a/pkg/service/v0/accounts.go b/pkg/service/v0/accounts.go index 98ebd3c6b..1b25c2027 100644 --- a/pkg/service/v0/accounts.go +++ b/pkg/service/v0/accounts.go @@ -19,7 +19,7 @@ import ( merrors "github.com/micro/go-micro/v2/errors" "github.com/owncloud/ocis-accounts/pkg/proto/v0" "github.com/owncloud/ocis-accounts/pkg/provider" - "github.com/owncloud/ocis-pkg/v2/middleware" + "github.com/owncloud/ocis-pkg/v2/roles" settings "github.com/owncloud/ocis-settings/pkg/proto/v0" settings_svc "github.com/owncloud/ocis-settings/pkg/service/v0" "github.com/rs/zerolog" @@ -159,14 +159,18 @@ func (s Service) passwordIsValid(hash string, pwd string) (ok bool) { func (s Service) hasAccountManagementPermissions(ctx context.Context) bool { // get roles from context - roleIDs, ok := middleware.ReadRoleIDsFromContext(ctx) + roleIDs, ok := roles.ReadRoleIDsFromContext(ctx) if !ok { - // if there were no roleIDs on the request we have to assume that the request didn't come from the proxy + /** + * FIXME: with this we are skipping permission checks on all requests that are coming in without roleIDs in the + * metadata context. This is a huge security impairment, as that's the case not only for grpc requests but also + * for unauthenticated http requests and http requests coming in without hitting the ocis-proxy first. + */ return true } // check if permission is present in roles of the authenticated account - return s.RoleCache.FindPermissionByID(roleIDs, AccountManagementPermissionID) != nil + return s.RoleManager.FindPermissionByID(ctx, roleIDs, AccountManagementPermissionID) != nil } // ListAccounts implements the AccountsServiceHandler interface @@ -346,9 +350,8 @@ func (s Service) CreateAccount(ctx context.Context, in *proto.CreateAccountReque return } - // TODO: All users for now as create Account request does not have any role field - rs := settings.NewRoleService("com.owncloud.api.settings", s.Client) - _, err = rs.AssignRoleToUser(ctx, &settings.AssignRoleToUserRequest{ + // TODO: assign user role to all new users for now, as create Account request does not have any role field + _, err = s.RoleService.AssignRoleToUser(ctx, &settings.AssignRoleToUserRequest{ AccountUuid: acc.Id, RoleId: settings_svc.BundleUUIDRoleUser, }) diff --git a/pkg/service/v0/option.go b/pkg/service/v0/option.go index e8274e230..559f992a2 100644 --- a/pkg/service/v0/option.go +++ b/pkg/service/v0/option.go @@ -15,7 +15,7 @@ type Options struct { Logger log.Logger Config *config.Config RoleService settings.RoleService - RoleCache *roles.Cache + RoleManager *roles.Manager } func newOptions(opts ...Option) Options { @@ -49,9 +49,9 @@ func RoleService(val settings.RoleService) Option { } } -// RoleCache provides a function to set the roles cache option. -func RoleCache(val *roles.Cache) Option { +// RoleManager provides a function to set the roles manager option. +func RoleManager(val *roles.Manager) Option { return func(o *Options) { - o.RoleCache = val + o.RoleManager = val } } diff --git a/pkg/service/v0/service.go b/pkg/service/v0/service.go index 5bdd5aa60..1e967ff46 100644 --- a/pkg/service/v0/service.go +++ b/pkg/service/v0/service.go @@ -17,8 +17,6 @@ import ( "github.com/blevesearch/bleve/analysis/analyzer/standard" "github.com/blevesearch/bleve/analysis/token/lowercase" "github.com/blevesearch/bleve/analysis/tokenizer/unicode" - mclient "github.com/micro/go-micro/v2/client" - mgrpc "github.com/micro/go-micro/v2/client/grpc" "github.com/owncloud/ocis-accounts/pkg/config" "github.com/owncloud/ocis-accounts/pkg/proto/v0" "github.com/owncloud/ocis-pkg/v2/log" @@ -33,10 +31,7 @@ func New(opts ...Option) (s *Service, err error) { logger := options.Logger cfg := options.Config roleService := options.RoleService - if roleService == nil { - roleService = settings.NewRoleService("com.owncloud.api.settings", mgrpc.NewClient()) - } - roleCache := options.RoleCache + roleManager := options.RoleManager // read all user and group records accountsDir := filepath.Join(cfg.Server.AccountsDataPath, "accounts") @@ -327,11 +322,11 @@ func New(opts ...Option) (s *Service, err error) { indexMapping.TypeField = "BleveType" s = &Service{ - id: cfg.GRPC.Namespace + "." + cfg.Server.Name, - log: logger, - Config: cfg, - Client: mgrpc.NewClient(), - RoleCache: roleCache, + id: cfg.GRPC.Namespace + "." + cfg.Server.Name, + log: logger, + Config: cfg, + RoleService: roleService, + RoleManager: roleManager, } indexDir := filepath.Join(cfg.Server.AccountsDataPath, "index.bleve") @@ -368,12 +363,12 @@ func assignRoleToUser(accountID, roleID string, rs settings.RoleService, logger // Service implements the AccountsServiceHandler interface type Service struct { - id string - log log.Logger - Config *config.Config - index bleve.Index - Client mclient.Client - RoleCache *roles.Cache + id string + log log.Logger + Config *config.Config + index bleve.Index + RoleService settings.RoleService + RoleManager *roles.Manager } func cleanupID(id string) (string, error) { From a25f27965211fb440f6cc0c24a628ba055df1b49 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Wed, 2 Sep 2020 15:51:02 +0200 Subject: [PATCH 2/4] Add changelog and update to ocis-pkg/v2@master --- changelog/unreleased/use-roles-manager.md | 6 ++++++ go.mod | 2 +- go.sum | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/use-roles-manager.md diff --git a/changelog/unreleased/use-roles-manager.md b/changelog/unreleased/use-roles-manager.md new file mode 100644 index 000000000..ab2fd3786 --- /dev/null +++ b/changelog/unreleased/use-roles-manager.md @@ -0,0 +1,6 @@ +Change: We make use of the roles manager to enforce permission checks + +The roles cache and its cache update middleware have been replaced with a roles manager in ocis-pkg/v2. We've switched +over to the new roles manager implementation, to prepare for permission checks on grpc requests as well. + +https://github.com/owncloud/ocis-accounts/pull/108 diff --git a/go.mod b/go.mod index 76f9e634e..61be784ba 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/olekukonko/tablewriter v0.0.4 github.com/onsi/ginkgo v1.10.1 // indirect github.com/onsi/gomega v1.7.0 // indirect - github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a + github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0 // indirect github.com/restic/calens v0.2.0 diff --git a/go.sum b/go.sum index b92f6bd0f..b0578a779 100644 --- a/go.sum +++ b/go.sum @@ -843,6 +843,8 @@ github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b h1:PRw0b4ab github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw= github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a h1:6iW1JWmAl5TFDhxGnPhL7VwolrcQsHWvXdZt+GremZI= github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw= +github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada h1:iVknnuT/z8QCAeBpHEcbI/AiQ9FOBvF5aOAFT3TIM+I= +github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw= github.com/owncloud/ocis-settings v0.3.2-0.20200828091056-47af10a0e872/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA= github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe h1:kiU5lz12R0LNJE1/zI2vxesZPWm6BvSO7hvZC8yOoAc= github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA= From 23ea3966555d035566340dce12f68361a6719fc6 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Wed, 2 Sep 2020 15:57:14 +0200 Subject: [PATCH 3/4] Switch to new ocis commit id --- .drone.star | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.drone.star b/.drone.star index cf20a38ca..080c991dd 100644 --- a/.drone.star +++ b/.drone.star @@ -1,7 +1,7 @@ def main(ctx): before = [ testing(ctx), - UITests(ctx, 'account-management-permission-checks', '9c24a3388e80f373f094c2b122594039c167c318', 'master', 'e0746d8d3a5879d2c0cd4aaf30c07ee98ab2b945') + UITests(ctx, 'account-management-permission-checks', 'cbc3ac599589db685ed2f007df6e979c00bdcf29', 'master', 'e0746d8d3a5879d2c0cd4aaf30c07ee98ab2b945') ] stages = [ From 2293ea260642f322472763861e9be8e85bf0274a Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Wed, 2 Sep 2020 16:14:59 +0200 Subject: [PATCH 4/4] Add ocis-pkg PR url to changelog --- changelog/unreleased/use-roles-manager.md | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog/unreleased/use-roles-manager.md b/changelog/unreleased/use-roles-manager.md index ab2fd3786..3b3f68842 100644 --- a/changelog/unreleased/use-roles-manager.md +++ b/changelog/unreleased/use-roles-manager.md @@ -4,3 +4,4 @@ The roles cache and its cache update middleware have been replaced with a roles over to the new roles manager implementation, to prepare for permission checks on grpc requests as well. https://github.com/owncloud/ocis-accounts/pull/108 +https://github.com/owncloud/ocis-pkg/pull/60