From f32ce91dd33d26b8d4dea2b9a724fb47456c77d2 Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Mon, 21 Nov 2022 15:21:16 +1100 Subject: [PATCH 1/3] auth: Remove Upsert/Delete from SAMLService interface Remove the Upsert and Delete methods from the SAMLService interface. It was intended for these to be part of the SAML "plugin", however they are needed for the operator tests to be able to create and delete the SAML connectors. The methods are back to being implemented directly in the `auth.Server`. This does not cause any real issues with the rest in the enterprise repo as the upsert/delete logic is just manipulating the local backend with no actual SAML logic in it. The `Get*` methods had already been kept in the `auth.Server` for the same reason. This narrows the `SAMLService` interface to just creating the SAML auth request and validating the response that comes back from the SAML identity provider. This is the core of the SAML-specific logic. --- lib/auth/saml.go | 105 ++++++++++++++++++----------------------------- 1 file changed, 41 insertions(+), 64 deletions(-) diff --git a/lib/auth/saml.go b/lib/auth/saml.go index b1d3b15a7699f..2a0b4a3a1bd01 100644 --- a/lib/auth/saml.go +++ b/lib/auth/saml.go @@ -44,46 +44,68 @@ import ( "github.com/gravitational/teleport/lib/utils" ) +// ErrSAMLNotImplemented is the error returned by the SAML methods when not +// using the Enterprise edition of Teleport. +var ErrSAMLNotImplemented = trace.AccessDenied("SAML is only available in enterprise subscriptions") + // SAMLService are the methods that the auth server delegates to a plugin for -// implementing the SAML connector. The Identity service contains a few more -// methods for getting connectors and requests from the backend, but there is -// no specific logic related to SAML in those methods. +// implementing the SAML connector. These are the core functions of SAML +// authentication - the connector CRUD operations and Get methods are +// implemeneted in auth.Server and provide no connector-specific logic. type SAMLService interface { - // UpsertSAMLConnector updates or creates SAML connector - UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) error - // DeleteSAMLConnector deletes SAML connector by ID - DeleteSAMLConnector(ctx context.Context, connectorID string) error // CreateSAMLAuthRequest creates SAML AuthnRequest CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRequest) (*types.SAMLAuthRequest, error) // ValidateSAMLResponse validates SAML auth response ValidateSAMLResponse(ctx context.Context, re string, connectorID string) (*SAMLAuthResponse, error) } -// UpsertSAMLConnector delegates the method call to the samlAuthService if present, -// or returns a NotImplemented error if not present. +// UpsertSAMLConnector creates or updates a SAML connector. func (a *Server) UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) error { - if a.samlAuthService == nil { - return trace.NotImplemented("SAML is only available in enterprise subscriptions") + if err := a.Services.UpsertSAMLConnector(ctx, connector); err != nil { + return trace.Wrap(err) + } + if err := a.emitter.EmitAuditEvent(ctx, &apievents.SAMLConnectorCreate{ + Metadata: apievents.Metadata{ + Type: events.SAMLConnectorCreatedEvent, + Code: events.SAMLConnectorCreatedCode, + }, + UserMetadata: ClientUserMetadata(ctx), + ResourceMetadata: apievents.ResourceMetadata{ + Name: connector.GetName(), + }, + }); err != nil { + log.WithError(err).Warn("Failed to emit SAML connector create event.") } - return trace.Wrap(a.samlAuthService.UpsertSAMLConnector(ctx, connector)) + return nil } -// DeleteSAMLConnector delegates the method call to the samlAuthService if present, -// or returns a NotImplemented error if not present. +// DeleteSAMLConnector deletes a SAML connector. func (a *Server) DeleteSAMLConnector(ctx context.Context, connectorID string) error { - if a.samlAuthService == nil { - return trace.NotImplemented("SAML is only available in enterprise subscriptions") + if err := a.Services.DeleteSAMLConnector(ctx, connectorID); err != nil { + return trace.Wrap(err) + } + if err := a.emitter.EmitAuditEvent(ctx, &apievents.SAMLConnectorDelete{ + Metadata: apievents.Metadata{ + Type: events.SAMLConnectorDeletedEvent, + Code: events.SAMLConnectorDeletedCode, + }, + UserMetadata: ClientUserMetadata(ctx), + ResourceMetadata: apievents.ResourceMetadata{ + Name: connectorID, + }, + }); err != nil { + log.WithError(err).Warn("Failed to emit SAML connector delete event.") } - return trace.Wrap(a.samlAuthService.DeleteSAMLConnector(ctx, connectorID)) + return nil } // CreateSAMLAuthRequest delegates the method call to the samlAuthService if present, // or returns a NotImplemented error if not present. func (a *Server) CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRequest) (*types.SAMLAuthRequest, error) { if a.samlAuthService == nil { - return nil, trace.NotImplemented("SAML is only available in enterprise subscriptions") + return nil, ErrSAMLNotImplemented } rq, err := a.samlAuthService.CreateSAMLAuthRequest(ctx, req) @@ -94,7 +116,7 @@ func (a *Server) CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRe // or returns a NotImplemented error if not present. func (a *Server) ValidateSAMLResponse(ctx context.Context, re string, connectorID string) (*SAMLAuthResponse, error) { if a.samlAuthService == nil { - return nil, trace.NotImplemented("SAML is only available in enterprise subscriptions") + return nil, ErrSAMLNotImplemented } resp, err := a.samlAuthService.ValidateSAMLResponse(ctx, re, connectorID) @@ -140,51 +162,6 @@ type samlProvider struct { // ErrSAMLNoRoles results from not mapping any roles from SAML claims. var ErrSAMLNoRoles = trace.AccessDenied("No roles mapped from claims. The mappings may contain typos.") -// UpsertSAMLConnector creates or updates a SAML connector. -func (sas *SAMLAuthService) UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) error { - if err := services.ValidateSAMLConnector(connector, sas.auth); err != nil { - return trace.Wrap(err) - } - if err := sas.auth.Services.UpsertSAMLConnector(ctx, connector); err != nil { - return trace.Wrap(err) - } - if err := sas.emitter.EmitAuditEvent(ctx, &apievents.SAMLConnectorCreate{ - Metadata: apievents.Metadata{ - Type: events.SAMLConnectorCreatedEvent, - Code: events.SAMLConnectorCreatedCode, - }, - UserMetadata: ClientUserMetadata(ctx), - ResourceMetadata: apievents.ResourceMetadata{ - Name: connector.GetName(), - }, - }); err != nil { - log.WithError(err).Warn("Failed to emit SAML connector create event.") - } - - return nil -} - -// DeleteSAMLConnector deletes a SAML connector by name. -func (sas *SAMLAuthService) DeleteSAMLConnector(ctx context.Context, connectorName string) error { - if err := sas.auth.Services.DeleteSAMLConnector(ctx, connectorName); err != nil { - return trace.Wrap(err) - } - if err := sas.emitter.EmitAuditEvent(ctx, &apievents.SAMLConnectorDelete{ - Metadata: apievents.Metadata{ - Type: events.SAMLConnectorDeletedEvent, - Code: events.SAMLConnectorDeletedCode, - }, - UserMetadata: ClientUserMetadata(ctx), - ResourceMetadata: apievents.ResourceMetadata{ - Name: connectorName, - }, - }); err != nil { - log.WithError(err).Warn("Failed to emit SAML connector delete event.") - } - - return nil -} - func (sas *SAMLAuthService) CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRequest) (*types.SAMLAuthRequest, error) { connector, provider, err := sas.getSAMLConnectorAndProvider(ctx, req) if err != nil { From 7461a667c221c69c7033104243c6633fd2d567c1 Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Wed, 23 Nov 2022 07:20:08 +1100 Subject: [PATCH 2/3] pr: Rename and wrap `ErrSAMLNotImplemented` Rename error variable to be more explicit, and ensure it is wrapped when returned from a function. Addresses: https://github.com/gravitational/teleport/pull/18634/files#r1029449563 --- lib/auth/saml.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/auth/saml.go b/lib/auth/saml.go index 2a0b4a3a1bd01..8fa28be562e05 100644 --- a/lib/auth/saml.go +++ b/lib/auth/saml.go @@ -44,9 +44,9 @@ import ( "github.com/gravitational/teleport/lib/utils" ) -// ErrSAMLNotImplemented is the error returned by the SAML methods when not +// ErrSAMLRequiresEnterprise is the error returned by the SAML methods when not // using the Enterprise edition of Teleport. -var ErrSAMLNotImplemented = trace.AccessDenied("SAML is only available in enterprise subscriptions") +var ErrSAMLRequiresEnterprise = trace.AccessDenied("SAML is only available in enterprise subscriptions") // SAMLService are the methods that the auth server delegates to a plugin for // implementing the SAML connector. These are the core functions of SAML @@ -61,6 +61,12 @@ type SAMLService interface { // UpsertSAMLConnector creates or updates a SAML connector. func (a *Server) UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) error { + // Validate the SAML connector here, because even though Services.UpsertSAMLConnector + // also validates, it does not have a RoleGetter to use to validate the roles, so + // has to pass `nil` for the second argument. + if err := services.ValidateSAMLConnector(connector, a); err != nil { + return trace.Wrap(err) + } if err := a.Services.UpsertSAMLConnector(ctx, connector); err != nil { return trace.Wrap(err) } @@ -105,7 +111,7 @@ func (a *Server) DeleteSAMLConnector(ctx context.Context, connectorID string) er // or returns a NotImplemented error if not present. func (a *Server) CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRequest) (*types.SAMLAuthRequest, error) { if a.samlAuthService == nil { - return nil, ErrSAMLNotImplemented + return nil, trace.Wrap(ErrSAMLRequiresEnterprise) } rq, err := a.samlAuthService.CreateSAMLAuthRequest(ctx, req) @@ -116,7 +122,7 @@ func (a *Server) CreateSAMLAuthRequest(ctx context.Context, req types.SAMLAuthRe // or returns a NotImplemented error if not present. func (a *Server) ValidateSAMLResponse(ctx context.Context, re string, connectorID string) (*SAMLAuthResponse, error) { if a.samlAuthService == nil { - return nil, ErrSAMLNotImplemented + return nil, trace.Wrap(ErrSAMLRequiresEnterprise) } resp, err := a.samlAuthService.ValidateSAMLResponse(ctx, re, connectorID) From 550a86477fffc9f83412f8da3e2298f29f37d619 Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Wed, 23 Nov 2022 22:56:20 +1100 Subject: [PATCH 3/3] pr: Wrap ErrRequiresEnterprise in ErrSAMLRequiresEnterprise --- lib/auth/auth_with_roles.go | 2 +- lib/auth/saml.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 08482c2af37c2..22b302ab04fa4 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2822,7 +2822,7 @@ func (a *ServerWithRoles) UpsertSAMLConnector(ctx context.Context, connector typ return trace.Wrap(err) } if !modules.GetModules().Features().SAML { - return trace.AccessDenied("SAML is only available in enterprise subscriptions") + return trace.Wrap(ErrSAMLRequiresEnterprise) } return a.authServer.UpsertSAMLConnector(ctx, connector) } diff --git a/lib/auth/saml.go b/lib/auth/saml.go index 8fa28be562e05..1b6082251c512 100644 --- a/lib/auth/saml.go +++ b/lib/auth/saml.go @@ -46,7 +46,7 @@ import ( // ErrSAMLRequiresEnterprise is the error returned by the SAML methods when not // using the Enterprise edition of Teleport. -var ErrSAMLRequiresEnterprise = trace.AccessDenied("SAML is only available in enterprise subscriptions") +var ErrSAMLRequiresEnterprise = fmt.Errorf("SAML: %w", ErrRequiresEnterprise) // SAMLService are the methods that the auth server delegates to a plugin for // implementing the SAML connector. These are the core functions of SAML