From a10a53cf9df2c7c83337ddab1c4bea7cff4e087f Mon Sep 17 00:00:00 2001 From: Trent Clarke Date: Wed, 15 Nov 2023 17:10:19 +1100 Subject: [PATCH] [v14] Grant the Okta service the right to write Users Manual backport of #33630 to v14 --- lib/auth/auth_with_roles.go | 105 ++++++ lib/auth/auth_with_roles_okta_rbac_test.go | 405 +++++++++++++++++++++ lib/authz/permissions.go | 2 +- 3 files changed, 511 insertions(+), 1 deletion(-) create mode 100644 lib/auth/auth_with_roles_okta_rbac_test.go diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index c19345c9a07e1..62957f81f8286 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2839,6 +2839,10 @@ func (a *ServerWithRoles) DeleteUser(ctx context.Context, user string) error { return trace.Wrap(err) } + if err := checkOktaAccessByName(&a.context, a.authServer, user, types.VerbDelete); err != nil { + return trace.Wrap(err) + } + return a.authServer.DeleteUser(ctx, user) } @@ -3421,6 +3425,11 @@ func (a *ServerWithRoles) CreateUser(ctx context.Context, user types.User) error if err := a.action(apidefaults.Namespace, types.KindUser, types.VerbCreate); err != nil { return trace.Wrap(err) } + + if err := checkOktaOrigin(&a.context, user); err != nil { + return trace.Wrap(err) + } + return a.authServer.CreateUser(ctx, user) } @@ -3431,6 +3440,14 @@ func (a *ServerWithRoles) UpdateUser(ctx context.Context, user types.User) error return trace.Wrap(err) } + if err := checkOktaOrigin(&a.context, user); err != nil { + return trace.Wrap(err) + } + + if err := checkOktaAccessByName(&a.context, a.authServer, user.GetName(), types.VerbUpdate); err != nil { + return trace.Wrap(err) + } + return a.authServer.UpdateUser(ctx, user) } @@ -3439,6 +3456,14 @@ func (a *ServerWithRoles) UpsertUser(u types.User) error { return trace.Wrap(err) } + if err := checkOktaOrigin(&a.context, u); err != nil { + return trace.Wrap(err) + } + + if err := checkOktaAccessByName(&a.context, a.authServer, u.GetName(), types.VerbUpdate); err != nil { + return trace.Wrap(err) + } + createdBy := u.GetCreatedBy() if createdBy.IsEmpty() { u.SetCreatedBy(types.CreatedBy{ @@ -3464,6 +3489,19 @@ func (a *ServerWithRoles) CompareAndSwapUser(ctx context.Context, new, existing return trace.Wrap(err) } + if err := checkOktaOrigin(&a.context, new); err != nil { + return trace.Wrap(err) + } + + // Checking the `existing` origin should be enough to assert that okta has + // write access to the user, because if the backend record says something + // different then the `CompareAndSwap()` will fail anyway, and this way we + // save ourselves a backend user lookup. + + if err := checkOktaAccess(&a.context, existing, types.VerbUpdate); err != nil { + return trace.Wrap(err) + } + return a.authServer.CompareAndSwapUser(ctx, new, existing) } @@ -6946,3 +6984,70 @@ func verbsToReplaceResourceWithOrigin(stored types.ResourceWithOrigin) []string } return verbs } + +// checkOktaOrigin checks that the supplied user has an appropriate origin label +// set. In this case "appropriate" means having the Okta origin set if and only +// if the supplied auth context has the build-in Okta role. An auth context +// without the Okta role may supply any origin value *other than* okta +// (including nil). +// Returns an error if the user origin value is "inappropriate". +func checkOktaOrigin(authzCtx *authz.Context, user types.User) error { + isOktaService := authz.HasBuiltinRole(*authzCtx, string(types.RoleOkta)) + hasOktaOrigin := user.Origin() == types.OriginOkta + + switch { + case isOktaService && !hasOktaOrigin: + return trace.BadParameter(`Okta service must supply "okta" origin`) + + case !isOktaService && hasOktaOrigin: + return trace.BadParameter(`Must be Okta service to set "okta" origin`) + + default: + return nil + } +} + +// checkOktaAccessByName gates access to update operations on user records +// based on the origin label on the supplied user record. +func checkOktaAccessByName(authzCtx *authz.Context, users services.UsersService, existingUsername string, verb string) error { + existingUser, err := users.GetUser(existingUsername, false) + if err != nil && !trace.IsNotFound(err) { + return trace.Wrap(err) + } + + return checkOktaAccess(authzCtx, existingUser, verb) +} + +// checkOktaAccess gates access to update operations on user records based +// on the origin label on the supplied user record. +// +// A nil `existingUser` is interpreted as there being no matching existing +// user in the cluster; if there is no user then there is no user to +// overwrite, so access is granted +func checkOktaAccess(authzCtx *authz.Context, existingUser types.User, verb string) error { + // We base or decision to allow write access to a resource on the Origin + // label. If there is no existing user, then there can be no label to block + // access, so anyone can do anything. + if existingUser == nil { + return nil + } + + if !authz.HasBuiltinRole(*authzCtx, string(types.RoleOkta)) { + // The only thing a non-okta service caller is allowed to do to an + // Okta-origin user is delete it + if (existingUser.Origin() == types.OriginOkta) && (verb != types.VerbDelete) { + return trace.BadParameter("Okta origin may not be changed") + } + return nil + } + + // An okta-service caller only has rights over the user if they have an + // "Origin: Okta" label + if existingUser.Origin() == types.OriginOkta { + return nil + } + + // If we get to here, we have exhausted all possible ways that the caller + // may be allowed to modify a user, so they get AccessDenied by default. + return trace.AccessDenied("Okta service may only %s Okta users", verb) +} diff --git a/lib/auth/auth_with_roles_okta_rbac_test.go b/lib/auth/auth_with_roles_okta_rbac_test.go new file mode 100644 index 0000000000000..a6881bb673006 --- /dev/null +++ b/lib/auth/auth_with_roles_okta_rbac_test.go @@ -0,0 +1,405 @@ +// Copyright 2023 Gravitational, Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +import ( + "context" + "testing" + + "github.com/gravitational/trace" + "github.com/stretchr/testify/require" + + "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/authz" +) + +func newOktaUser(t *testing.T) types.User { + t.Helper() + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + + user.SetOrigin(types.OriginOkta) + + return user +} + +// TestOktaServiceUserCRUD() asserts that user operations involving Okta-origin +// users obey the following rules: +// +// 1. Only the Teleport Okta service may create an Okta-origin user. +// 2. Only the Teleport Okta service may modify an Okta-origin user. +// 3. Anyone with User RW can delete an Okta-origin user. +// +// TODO(tcsc): DELETE IN 16.0.0 (or when user management is excised from ServerWithRoles) +func TestOktaServiceUserCRUD(t *testing.T) { + // Given an RBAC-checking `ServerWithRoles` configured with the built-in + // Okta Role... + ctx := context.Background() + + srv, err := NewTestAuthServer(TestAuthServerConfig{Dir: t.TempDir()}) + require.NoError(t, err) + + oktaAuthContext, err := srv.Authorizer.Authorize(authz.ContextWithUser(ctx, TestBuiltin(types.RoleOkta).I)) + require.NoError(t, err) + + authWithOktaRole := &ServerWithRoles{ + authServer: srv.AuthServer, + alog: srv.AuditLog, + context: *oktaAuthContext, + } + t.Cleanup(func() { authWithOktaRole.Close() }) + + botAuthContext, err := srv.Authorizer.Authorize(authz.ContextWithUser(ctx, TestBuiltin(types.RoleAdmin).I)) + require.NoError(t, err) + + authWithBotRole := &ServerWithRoles{ + authServer: srv.AuthServer, + alog: srv.AuditLog, + context: *botAuthContext, + } + t.Cleanup(func() { authWithBotRole.Close() }) + + t.Run("create", func(t *testing.T) { + t.Run("okta service creating okta users is allowed", func(t *testing.T) { + user := newOktaUser(t) + err := authWithOktaRole.CreateUser(ctx, user) + require.NoError(t, err) + }) + + t.Run("okta service creating non-okta users in an error", func(t *testing.T) { + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + + err = authWithOktaRole.CreateUser(ctx, user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service creating an okta user is an error", func(t *testing.T) { + user := newOktaUser(t) + + err := authWithBotRole.CreateUser(ctx, user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + }) + + t.Run("update", func(t *testing.T) { + t.Run("okta service updating okta user is allowed", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + // When I (as the Okta service) modify the user and attempt to update the backend record... + user.SetTraits(map[string][]string{"foo": {"bar", "baz"}}) + + // Expect the operation to succeed + err = authWithOktaRole.UpdateUser(ctx, user) + require.NoError(t, err) + }) + + t.Run("okta service updating non-okta user is an error", func(t *testing.T) { + // Given an existing non-okta user + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + // When I (as the Okta service) attempt modify that user + user.SetOrigin(types.OriginOkta) + err = authWithOktaRole.UpdateUser(ctx, user) + + // Expect the attempt to fail + require.Error(t, err) + require.Truef(t, trace.IsAccessDenied(err), "Expected access denied, got %T: %s", err, err.Error()) + }) + + t.Run("okta service removing okta origin is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + // When I (as the Okta service) attempt reset the user origin + user.SetOrigin(types.OriginDynamic) + + // Expect the attempt to fail + err = authWithOktaRole.UpdateUser(ctx, user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("okta service updating a non-existent user is an error", func(t *testing.T) { + // Given an okta user not present in the user DB + user := newOktaUser(t) + + // when I try to update that user, an error is returned rather than + // having the whole system crash + err = authWithOktaRole.UpdateUser(ctx, user) + require.Error(t, err) + }) + + t.Run("non-okta service removing okta origin is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + // When I (as a non-Okta service) attempt reset the user origin + user.SetOrigin(types.OriginDynamic) + + // Expect the attempt to fail + err = authWithBotRole.UpdateUser(ctx, user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service updating okta user is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + // When I (as a non-Okta service) attempt modify that user + user.SetTraits(map[string][]string{"foo": {"bar", "baz"}}) + err = authWithBotRole.UpdateUser(ctx, user) + + // Expect the attempt to fail + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter got %T: %s", err, err.Error()) + }) + }) + + t.Run("upsert", func(t *testing.T) { + t.Run("okta service creating non-okta user is an error", func(t *testing.T) { + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + + err = authWithOktaRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("okta service creating okta user is allowed", func(t *testing.T) { + user := newOktaUser(t) + err = authWithOktaRole.CreateUser(ctx, user) + require.NoError(t, err) + }) + + t.Run("okta service updating okta user is allowed", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + user.SetTraits(map[string][]string{"foo": {"bar", "baz"}}) + + err = authWithOktaRole.UpsertUser(user) + require.NoError(t, err) + }) + + t.Run("okta service updating non-okta user is an error", func(t *testing.T) { + // Given an existing non-okta user + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + user.SetOrigin(types.OriginOkta) + + err = authWithOktaRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsAccessDenied(err), "Expected access denied, got %T: %s", err, err.Error()) + }) + + t.Run("okta service removing the okta origin is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + user.SetOrigin(types.OriginDynamic) + + err = authWithOktaRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service creating okta user is an error", func(t *testing.T) { + user := newOktaUser(t) + err = authWithBotRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service removing okta origin is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + user.SetOrigin(types.OriginDynamic) + + err = authWithBotRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service updating an okta user is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + user.AddRole(teleport.PresetAccessRoleName) + + err = authWithBotRole.UpsertUser(user) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + }) + + t.Run("compare and swap", func(t *testing.T) { + t.Run("okta service updating Okta user is allowed", func(t *testing.T) { + // Given an existing okta user + existing := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, existing) + require.NoError(t, err) + + modified, err := srv.AuthServer.GetUser(existing.GetName(), false) + require.NoError(t, err) + modified.SetTraits(map[string][]string{"foo": {"bar", "baz"}}) + + err = authWithOktaRole.CompareAndSwapUser(ctx, modified, existing) + require.NoError(t, err) + }) + + t.Run("okta service updating non-Okta user is an error", func(t *testing.T) { + // Given an existing non-okta existing + existing, err := types.NewUser(t.Name()) + require.NoError(t, err) + err = srv.AuthServer.CreateUser(ctx, existing) + require.NoError(t, err) + + modified, err := srv.AuthServer.GetUser(existing.GetName(), false) + require.NoError(t, err) + modified.SetOrigin(types.OriginOkta) + + err = authWithOktaRole.CompareAndSwapUser(ctx, modified, existing) + require.Error(t, err) + require.Truef(t, trace.IsAccessDenied(err), "Expected access denied, got %T: %s", err, err.Error()) + }) + + t.Run("okta service removing Okta origin is an error", func(t *testing.T) { + // Given an existing okta existing + existing := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, existing) + require.NoError(t, err) + + modified, err := srv.AuthServer.GetUser(existing.GetName(), false) + require.NoError(t, err) + modified.SetOrigin(types.OriginDynamic) + + err = authWithOktaRole.CompareAndSwapUser(ctx, modified, existing) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("okta service updating a non-existent user is an error", func(t *testing.T) { + // Given an okta user not present in the user DB + user := newOktaUser(t) + + // when I try to update that user, an error is returned rather than + // having the whole system crash + err := authWithOktaRole.CompareAndSwapUser(ctx, user, user) + require.Error(t, err) + }) + + t.Run("non-okta service removing okta origin is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + original, err := srv.AuthServer.GetUser(user.GetName(), false) + require.NoError(t, err) + + modified, err := srv.AuthServer.GetUser(user.GetName(), false) + require.NoError(t, err) + modified.SetOrigin(types.OriginDynamic) + + err = authWithBotRole.CompareAndSwapUser(ctx, modified, original) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service updating an okta user is an error", func(t *testing.T) { + // Given an existing okta user + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + original, err := srv.AuthServer.GetUser(user.GetName(), false) + require.NoError(t, err) + + modified, err := srv.AuthServer.GetUser(user.GetName(), false) + require.NoError(t, err) + modified.AddRole(teleport.PresetAccessRoleName) + + err = authWithBotRole.CompareAndSwapUser(ctx, modified, original) + require.Error(t, err) + require.Truef(t, trace.IsBadParameter(err), "Expected bad parameter, got %T: %s", err, err.Error()) + }) + }) + + t.Run("delete", func(t *testing.T) { + t.Run("okta service deleting Okta user is allowed", func(t *testing.T) { + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + err = authWithOktaRole.DeleteUser(ctx, user.GetName()) + require.NoError(t, err) + + _, err = srv.AuthServer.GetUser(user.GetName(), false) + require.True(t, trace.IsNotFound(err), "Expected not found, got %s", err.Error()) + }) + + t.Run("okta service deleting non-Okta user is an error", func(t *testing.T) { + user, err := types.NewUser(t.Name()) + require.NoError(t, err) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + err = authWithOktaRole.DeleteUser(ctx, user.GetName()) + require.Error(t, err) + require.Truef(t, trace.IsAccessDenied(err), "Expected access denied, got %T: %s", err, err.Error()) + }) + + t.Run("non-okta service deleting Okta user is allowed", func(t *testing.T) { + user := newOktaUser(t) + err = srv.AuthServer.CreateUser(ctx, user) + require.NoError(t, err) + + err = authWithBotRole.DeleteUser(ctx, user.GetName()) + require.NoError(t, err) + + _, err = srv.AuthServer.GetUser(user.GetName(), false) + require.True(t, trace.IsNotFound(err), "Expected not found, got %s", err.Error()) + }) + }) +} diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index c09c4aeacb15d..959e658afb6b6 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -919,7 +919,7 @@ func definitionForBuiltinRole(clusterName string, recConfig types.SessionRecordi types.NewRule(types.KindEvent, services.RW()), types.NewRule(types.KindAppServer, services.RW()), types.NewRule(types.KindClusterNetworkingConfig, services.RO()), - types.NewRule(types.KindUser, services.RO()), + types.NewRule(types.KindUser, services.RW()), types.NewRule(types.KindUserGroup, services.RW()), types.NewRule(types.KindOktaImportRule, services.RO()), types.NewRule(types.KindOktaAssignment, services.RW()),