From 1ebf8a3409aec6ec4ec8f47d81607c35db626edb Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Fri, 15 Nov 2024 17:02:53 +0100 Subject: [PATCH] Respect windows_desktop_labels for dynamic desktops (#48941) * Respect windows_desktop_labels for dynamic desktops * fix imports * Fix test * add access check test --- .../dynamicwindowsv1/service.go | 47 ++++++++++++++- .../dynamicwindowsv1/service_test.go | 58 +++++++++++++++++-- lib/services/role.go | 1 + 3 files changed, 99 insertions(+), 7 deletions(-) diff --git a/lib/auth/dynamicwindows/dynamicwindowsv1/service.go b/lib/auth/dynamicwindows/dynamicwindowsv1/service.go index 5a42eefe8edca..b8ca1c8ac09f5 100644 --- a/lib/auth/dynamicwindows/dynamicwindowsv1/service.go +++ b/lib/auth/dynamicwindows/dynamicwindowsv1/service.go @@ -29,6 +29,7 @@ import ( dynamicwindowspb "github.com/gravitational/teleport/api/gen/proto/go/teleport/dynamicwindows/v1" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/authz" + "github.com/gravitational/teleport/lib/services" ) // Service implements the teleport.trust.v1.TrustService RPC service. @@ -108,6 +109,9 @@ func (s *Service) GetDynamicWindowsDesktop(ctx context.Context, request *dynamic if err != nil { return nil, trace.Wrap(err) } + if err := checkAccess(auth, d); err != nil { + return nil, trace.Wrap(err) + } desktop, ok := d.(*types.DynamicWindowsDesktopV1) if !ok { @@ -136,6 +140,9 @@ func (s *Service) ListDynamicWindowsDesktops(ctx context.Context, request *dynam NextPageToken: next, } for _, d := range desktops { + if err := checkAccess(auth, d); err != nil { + continue + } desktop, ok := d.(*types.DynamicWindowsDesktopV1) if !ok { return nil, trace.BadParameter("unexpected type %T", d) @@ -158,6 +165,9 @@ func (s *Service) CreateDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbCreate); err != nil { return nil, trace.Wrap(err) } + if err := checkAccess(auth, req.GetDesktop()); err != nil { + return nil, trace.Wrap(err) + } d, err := s.backend.CreateDynamicWindowsDesktop(ctx, types.DynamicWindowsDesktop(req.Desktop)) if err != nil { return nil, trace.Wrap(err) @@ -171,6 +181,10 @@ func (s *Service) CreateDynamicWindowsDesktop(ctx context.Context, req *dynamicw return createdDesktop, nil } +func checkAccess(auth *authz.Context, desktop types.DynamicWindowsDesktop) error { + return auth.Checker.CheckAccess(desktop, services.AccessState{MFAVerified: true}) +} + // UpdateDynamicWindowsDesktop updates an existing dynamic Windows desktop. func (s *Service) UpdateDynamicWindowsDesktop(ctx context.Context, req *dynamicwindowspb.UpdateDynamicWindowsDesktopRequest) (*types.DynamicWindowsDesktopV1, error) { auth, err := s.authorizer.Authorize(ctx) @@ -183,7 +197,17 @@ func (s *Service) UpdateDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbUpdate); err != nil { return nil, trace.Wrap(err) } - d, err := s.backend.UpdateDynamicWindowsDesktop(ctx, req.Desktop) + d, err := s.cache.GetDynamicWindowsDesktop(ctx, req.GetDesktop().GetName()) + if err != nil { + return nil, trace.Wrap(err) + } + if err := checkAccess(auth, d); err != nil { + return nil, trace.Wrap(err) + } + if err := checkAccess(auth, req.GetDesktop()); err != nil { + return nil, trace.Wrap(err) + } + d, err = s.backend.UpdateDynamicWindowsDesktop(ctx, req.Desktop) if err != nil { return nil, trace.Wrap(err) } @@ -208,7 +232,19 @@ func (s *Service) UpsertDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbCreate, types.VerbUpdate); err != nil { return nil, trace.Wrap(err) } - d, err := s.backend.UpsertDynamicWindowsDesktop(ctx, req.Desktop) + d, err := s.cache.GetDynamicWindowsDesktop(ctx, req.GetDesktop().GetName()) + if !trace.IsNotFound(err) { + if err != nil { + return nil, trace.Wrap(err) + } + if err := checkAccess(auth, d); err != nil { + return nil, trace.Wrap(err) + } + } + if err := checkAccess(auth, req.GetDesktop()); err != nil { + return nil, trace.Wrap(err) + } + d, err = s.backend.UpsertDynamicWindowsDesktop(ctx, req.Desktop) if err != nil { return nil, trace.Wrap(err) } @@ -233,6 +269,13 @@ func (s *Service) DeleteDynamicWindowsDesktop(ctx context.Context, req *dynamicw if err := auth.CheckAccessToKind(types.KindDynamicWindowsDesktop, types.VerbDelete); err != nil { return nil, trace.Wrap(err) } + d, err := s.cache.GetDynamicWindowsDesktop(ctx, req.GetName()) + if err != nil { + return nil, trace.Wrap(err) + } + if err := checkAccess(auth, d); err != nil { + return nil, trace.Wrap(err) + } if err := s.backend.DeleteDynamicWindowsDesktop(ctx, req.GetName()); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go b/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go index 1c474f7192be9..8ae55acf040a0 100644 --- a/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go +++ b/lib/auth/dynamicwindows/dynamicwindowsv1/service_test.go @@ -37,6 +37,43 @@ import ( "github.com/gravitational/teleport/lib/utils" ) +func TestFailedAccessCheck(t *testing.T) { + t.Parallel() + checker := fakeChecker{ + allowedVerbs: []string{types.VerbRead, types.VerbList, types.VerbCreate, types.VerbUpdate}, + } + s := newService(t, authz.AdminActionAuthMFAVerified, &checker) + desktop, err := types.NewDynamicWindowsDesktopV1("test2", nil, types.DynamicWindowsDesktopSpecV1{Addr: "addr"}) + require.NoError(t, err) + req := dynamicwindowsv1.CreateDynamicWindowsDesktopRequest{ + Desktop: desktop, + } + _, err = s.CreateDynamicWindowsDesktop(context.Background(), &req) + require.NoError(t, err) + checker.failAccess = true + testCases := []string{ + "CreateDynamicWindowsDesktop", + "UpdateDynamicWindowsDesktop", + "UpsertDynamicWindowsDesktop", + "DeleteDynamicWindowsDesktop", + "GetDynamicWindowsDesktop", + } + for _, tt := range testCases { + t.Run(fmt.Sprintf("%s failed access check", tt), func(t *testing.T) { + err := callMethod(s, tt) + require.True(t, trace.IsAccessDenied(err)) + }) + } + t.Run("ListDynamicWindowsDesktops failed access check", func(t *testing.T) { + req := dynamicwindowsv1.ListDynamicWindowsDesktopsRequest{ + PageSize: 10, + } + resp, err := s.ListDynamicWindowsDesktops(context.Background(), &req) + require.NoError(t, err) + require.Empty(t, resp.Desktops) + }) +} + func TestServiceAccess(t *testing.T) { t.Parallel() @@ -87,7 +124,7 @@ func TestServiceAccess(t *testing.T) { for _, state := range tt.allowedStates { for _, verbs := range utils.Combinations(tt.allowedVerbs) { t.Run(fmt.Sprintf("%v,allowed:%v,verbs:%v", tt.name, stateToString(state), verbs), func(t *testing.T) { - service := newService(t, state, fakeChecker{allowedVerbs: verbs}) + service := newService(t, state, &fakeChecker{allowedVerbs: verbs}) err := callMethod(service, tt.name) // expect access denied except with full set of verbs. if len(verbs) == len(tt.allowedVerbs) { @@ -105,7 +142,7 @@ func TestServiceAccess(t *testing.T) { t.Run(fmt.Sprintf("%v,disallowed:%v", tt.name, stateToString(state)), func(t *testing.T) { // it is enough to test against tt.allowedVerbs, // this is the only different data point compared to the test cases above. - service := newService(t, state, fakeChecker{allowedVerbs: tt.allowedVerbs}) + service := newService(t, state, &fakeChecker{allowedVerbs: tt.allowedVerbs}) err := callMethod(service, tt.name) require.True(t, trace.IsAccessDenied(err)) }) @@ -157,16 +194,19 @@ func callMethod(service *Service, method string) error { if desc.MethodName == method { _, err := desc.Handler(service, context.Background(), func(arg any) error { switch arg := arg.(type) { + case *dynamicwindowsv1.GetDynamicWindowsDesktopRequest: + arg.Name = "test2" + case *dynamicwindowsv1.CreateDynamicWindowsDesktopRequest: arg.Desktop, _ = types.NewDynamicWindowsDesktopV1("test", nil, types.DynamicWindowsDesktopSpecV1{ Addr: "test", }) case *dynamicwindowsv1.UpdateDynamicWindowsDesktopRequest: - arg.Desktop, _ = types.NewDynamicWindowsDesktopV1("test", nil, types.DynamicWindowsDesktopSpecV1{ + arg.Desktop, _ = types.NewDynamicWindowsDesktopV1("test2", nil, types.DynamicWindowsDesktopSpecV1{ Addr: "test", }) case *dynamicwindowsv1.UpsertDynamicWindowsDesktopRequest: - arg.Desktop, _ = types.NewDynamicWindowsDesktopV1("test", nil, types.DynamicWindowsDesktopSpecV1{ + arg.Desktop, _ = types.NewDynamicWindowsDesktopV1("test2", nil, types.DynamicWindowsDesktopSpecV1{ Addr: "test", }) } @@ -180,10 +220,11 @@ func callMethod(service *Service, method string) error { type fakeChecker struct { allowedVerbs []string + failAccess bool services.AccessChecker } -func (f fakeChecker) CheckAccessToRule(_ services.RuleContext, _ string, resource string, verb string) error { +func (f *fakeChecker) CheckAccessToRule(_ services.RuleContext, _ string, resource string, verb string) error { if resource == types.KindDynamicWindowsDesktop { if slices.Contains(f.allowedVerbs, verb) { return nil @@ -193,6 +234,13 @@ func (f fakeChecker) CheckAccessToRule(_ services.RuleContext, _ string, resourc return trace.AccessDenied("access denied to rule=%v/verb=%v", resource, verb) } +func (f *fakeChecker) CheckAccess(r services.AccessCheckable, state services.AccessState, matchers ...services.RoleMatcher) error { + if f.failAccess { + return trace.AccessDenied("denied") + } + return nil +} + func newService(t *testing.T, authState authz.AdminActionAuthState, checker services.AccessChecker) *Service { t.Helper() diff --git a/lib/services/role.go b/lib/services/role.go index 16b1c79287e87..60891a725e3bd 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -349,6 +349,7 @@ func validateRoleExpressions(r types.Role) error { {"db_labels", types.KindDatabase}, {"db_service_labels", types.KindDatabaseService}, {"windows_desktop_labels", types.KindWindowsDesktop}, + {"windows_desktop_labels", types.KindDynamicWindowsDesktop}, {"group_labels", types.KindUserGroup}, } { labelMatchers, err := r.GetLabelMatchers(condition.condition, labels.kind)