From d08cb25c7dee87b45b6993db81d0f21b62a1b542 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Tue, 30 Apr 2024 15:10:28 +0200 Subject: [PATCH 1/2] Prevent deprecation of `AppServerOrSAMLIdPServiceProvider` from being a breaking change --- lib/auth/assist/assistv1/service.go | 2 +- lib/auth/auth_with_roles.go | 2 +- lib/auth/grpcserver.go | 2 +- lib/services/unified_resource.go | 34 ++++++++++++++++++--- lib/services/unified_resource_test.go | 44 +++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 7 deletions(-) diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index cd680dc9e4ccb..45c4667a44f9a 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -374,7 +374,7 @@ func assembleSearchResponse(ctx context.Context, a *Service, documents []*ai.Doc resources = append(resources, resource) } - paginated, err := services.MakePaginatedResources(types.KindUnifiedResource, resources, nil /* requestable map */) + paginated, err := services.MakePaginatedResources(ctx, types.KindUnifiedResource, resources, nil /* requestable map */) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 0083bbe051de1..cea920ecda365 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -1423,7 +1423,7 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L } } - paginatedResources, err := services.MakePaginatedResources(types.KindUnifiedResource, unifiedResources, resourceAccess.requestableMap) + paginatedResources, err := services.MakePaginatedResources(ctx, types.KindUnifiedResource, unifiedResources, resourceAccess.requestableMap) if err != nil { return nil, trace.Wrap(err, "making paginated unified resources") } diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index 9bfc312bc7daa..ccf3a9c511412 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -4310,7 +4310,7 @@ func (g *GRPCServer) ListResources(ctx context.Context, req *authpb.ListResource return nil, trace.Wrap(err) } - paginatedResources, err := services.MakePaginatedResources(req.ResourceType, resp.Resources, nil /* requestable map */) + paginatedResources, err := services.MakePaginatedResources(ctx, req.ResourceType, resp.Resources, nil /* requestable map */) if err != nil { return nil, trace.Wrap(err, "making paginated resources") } diff --git a/lib/services/unified_resource.go b/lib/services/unified_resource.go index 7346d65303a11..95e8a09fcfd37 100644 --- a/lib/services/unified_resource.go +++ b/lib/services/unified_resource.go @@ -25,6 +25,7 @@ import ( "sync" "time" + "github.com/coreos/go-semver/semver" "github.com/google/btree" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" @@ -33,6 +34,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" + "github.com/gravitational/teleport/api/metadata" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/backend" "github.com/gravitational/teleport/lib/utils" @@ -742,7 +744,7 @@ const ( ) // MakePaginatedResource converts a resource into a paginated proto representation. -func MakePaginatedResource(requestType string, r types.ResourceWithLabels, requiresRequest bool) (*proto.PaginatedResource, error) { +func MakePaginatedResource(ctx context.Context, requestType string, r types.ResourceWithLabels, requiresRequest bool) (*proto.PaginatedResource, error) { var protoResource *proto.PaginatedResource resourceKind := requestType if requestType == types.KindUnifiedResource { @@ -850,7 +852,31 @@ func MakePaginatedResource(requestType string, r types.ResourceWithLabels, requi return nil, trace.BadParameter("%s has invalid type %T", resourceKind, resource) } - protoResource = &proto.PaginatedResource{Resource: &proto.PaginatedResource_SAMLIdPServiceProvider{SAMLIdPServiceProvider: serviceProvider}, RequiresRequest: requiresRequest} + // TODO(gzdunek): DELETE IN 17.0 + // This is needed to maintain backward compatibility between v16 server and v15 client. + clientVersion, versionExists := metadata.ClientVersionFromContext(ctx) + isClientNotSupportingSAMLIdPServiceProviderResource := versionExists && semver.New(clientVersion).Major < 16 + + if isClientNotSupportingSAMLIdPServiceProviderResource { + protoResource = &proto.PaginatedResource{ + Resource: &proto.PaginatedResource_AppServerOrSAMLIdPServiceProvider{ + //nolint:staticcheck // SA1019. TODO(gzdunek): DELETE IN 17.0 + AppServerOrSAMLIdPServiceProvider: &types.AppServerOrSAMLIdPServiceProviderV1{ + Resource: &types.AppServerOrSAMLIdPServiceProviderV1_SAMLIdPServiceProvider{ + SAMLIdPServiceProvider: serviceProvider, + }, + }, + }, + RequiresRequest: requiresRequest, + } + } else { + protoResource = &proto.PaginatedResource{ + Resource: &proto.PaginatedResource_SAMLIdPServiceProvider{ + SAMLIdPServiceProvider: serviceProvider, + }, + RequiresRequest: requiresRequest, + } + } default: return nil, trace.NotImplemented("resource type %s doesn't support pagination", resource.GetKind()) } @@ -859,11 +885,11 @@ func MakePaginatedResource(requestType string, r types.ResourceWithLabels, requi } // MakePaginatedResources converts a list of resources into a list of paginated proto representations. -func MakePaginatedResources(requestType string, resources []types.ResourceWithLabels, requestableMap map[string]struct{}) ([]*proto.PaginatedResource, error) { +func MakePaginatedResources(ctx context.Context, requestType string, resources []types.ResourceWithLabels, requestableMap map[string]struct{}) ([]*proto.PaginatedResource, error) { paginatedResources := make([]*proto.PaginatedResource, 0, len(resources)) for _, r := range resources { _, requiresRequest := requestableMap[r.GetName()] - protoResource, err := MakePaginatedResource(requestType, r, requiresRequest) + protoResource, err := MakePaginatedResource(ctx, requestType, r, requiresRequest) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/services/unified_resource_test.go b/lib/services/unified_resource_test.go index c6132ba0aff95..48163761f039d 100644 --- a/lib/services/unified_resource_test.go +++ b/lib/services/unified_resource_test.go @@ -30,9 +30,12 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/defaults" + apimetadata "github.com/gravitational/teleport/api/metadata" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/types/header" "github.com/gravitational/teleport/lib/backend/memory" @@ -382,6 +385,47 @@ func TestUnifiedResourceWatcher_DeleteEvent(t *testing.T) { }, 5*time.Second, 10*time.Millisecond, "Timed out waiting for unified resources to be deleted") } +func Test_PaginatedResourcesSAMLIdPServiceProviderCompatibility(t *testing.T) { + samlApp, err := types.NewSAMLIdPServiceProvider( + types.Metadata{ + Name: "sp1", + }, + types.SAMLIdPServiceProviderSpecV1{ + EntityDescriptor: newTestEntityDescriptor("sp1"), + EntityID: "sp1", + }, + ) + require.NoError(t, err) + + // for a v15 client, expect AppServerOrSAMLIdPServiceProvider response + v15ctx := metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{apimetadata.VersionKey: "15.0.0"})) + v15response, err := services.MakePaginatedResources(v15ctx, types.KindUnifiedResource, []types.ResourceWithLabels{samlApp}, map[string]struct{}{}) + require.NoError(t, err) + require.Equal(t, + &proto.PaginatedResource{ + Resource: &proto.PaginatedResource_AppServerOrSAMLIdPServiceProvider{ + //nolint:staticcheck // SA1019. TODO(gzdunek): DELETE IN 17.0 (with the entire test) + AppServerOrSAMLIdPServiceProvider: &types.AppServerOrSAMLIdPServiceProviderV1{ + Resource: &types.AppServerOrSAMLIdPServiceProviderV1_SAMLIdPServiceProvider{ + SAMLIdPServiceProvider: samlApp.(*types.SAMLIdPServiceProviderV1), + }, + }}}, + v15response[0], + ) + + // for a v16 client, expect SAMLIdPServiceProvider response + v16ctx := metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{apimetadata.VersionKey: "16.0.0"})) + v16response, err := services.MakePaginatedResources(v16ctx, types.KindUnifiedResource, []types.ResourceWithLabels{samlApp}, map[string]struct{}{}) + require.NoError(t, err) + require.Equal(t, + &proto.PaginatedResource{ + Resource: &proto.PaginatedResource_SAMLIdPServiceProvider{ + SAMLIdPServiceProvider: samlApp.(*types.SAMLIdPServiceProviderV1), + }}, + v16response[0], + ) +} + func newTestEntityDescriptor(entityID string) string { return fmt.Sprintf(testEntityDescriptor, entityID) } From c72098d2a956f09a0c29a597a0a8df5be4aa3096 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Wed, 8 May 2024 12:48:35 +0200 Subject: [PATCH 2/2] Improve version check --- lib/services/unified_resource.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/services/unified_resource.go b/lib/services/unified_resource.go index 95e8a09fcfd37..7be133a1f0837 100644 --- a/lib/services/unified_resource.go +++ b/lib/services/unified_resource.go @@ -855,7 +855,13 @@ func MakePaginatedResource(ctx context.Context, requestType string, r types.Reso // TODO(gzdunek): DELETE IN 17.0 // This is needed to maintain backward compatibility between v16 server and v15 client. clientVersion, versionExists := metadata.ClientVersionFromContext(ctx) - isClientNotSupportingSAMLIdPServiceProviderResource := versionExists && semver.New(clientVersion).Major < 16 + isClientNotSupportingSAMLIdPServiceProviderResource := false + if versionExists { + version, err := semver.NewVersion(clientVersion) + if err == nil && version.Major < 16 { + isClientNotSupportingSAMLIdPServiceProviderResource = true + } + } if isClientNotSupportingSAMLIdPServiceProviderResource { protoResource = &proto.PaginatedResource{