From 7fabab35a5c98cf37fc8c7f29cad63eb6f32ab45 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 7 Dec 2023 14:37:53 -0700 Subject: [PATCH 1/4] Revert "log error when entity descriptor cant be fetched in saml (#34896)" This reverts commit 27b79a2684b43ee0d768b6f98f6da9bfe645e126. --- lib/services/saml.go | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/lib/services/saml.go b/lib/services/saml.go index 7588914a75002..83536fce51463 100644 --- a/lib/services/saml.go +++ b/lib/services/saml.go @@ -41,28 +41,6 @@ import ( "github.com/gravitational/teleport/lib/utils" ) -func setEntityDescriptorFromURL(sc types.SAMLConnector) error { - if sc.GetEntityDescriptorURL() == "" { - return nil - } - - resp, err := http.Get(sc.GetEntityDescriptorURL()) - if err != nil { - return trace.WrapWithMessage(err, "unable to fetch entity descriptor from %v for SAML connector %v", sc.GetEntityDescriptorURL(), sc.GetName()) - } - if resp.StatusCode != http.StatusOK { - return trace.BadParameter("status code %v when fetching from %v for SAML connector %v", resp.StatusCode, sc.GetEntityDescriptorURL(), sc.GetName()) - } - defer resp.Body.Close() - body, err := utils.ReadAtMost(resp.Body, teleport.MaxHTTPResponseSize) - if err != nil { - return trace.Wrap(err) - } - sc.SetEntityDescriptor(string(body)) - log.Debugf("[SAML] Successfully fetched entity descriptor from %v for connector %v", sc.GetEntityDescriptorURL(), sc.GetName()) - return nil -} - // ValidateSAMLConnector validates the SAMLConnector and sets default values. // If a remote to fetch roles is specified, roles will be validated to exist. func ValidateSAMLConnector(sc types.SAMLConnector, rg RoleGetter) error { @@ -70,8 +48,21 @@ func ValidateSAMLConnector(sc types.SAMLConnector, rg RoleGetter) error { return trace.Wrap(err) } - if err := setEntityDescriptorFromURL(sc); err != nil { - log.Errorf("error loading entity descriptor from URL: %s", err) + if sc.GetEntityDescriptorURL() != "" { + resp, err := http.Get(sc.GetEntityDescriptorURL()) + if err != nil { + return trace.WrapWithMessage(err, "unable to fetch entity descriptor from %v for SAML connector %v", sc.GetEntityDescriptorURL(), sc.GetName()) + } + if resp.StatusCode != http.StatusOK { + return trace.BadParameter("status code %v when fetching from %v for SAML connector %v", resp.StatusCode, sc.GetEntityDescriptorURL(), sc.GetName()) + } + defer resp.Body.Close() + body, err := utils.ReadAtMost(resp.Body, teleport.MaxHTTPResponseSize) + if err != nil { + return trace.Wrap(err) + } + sc.SetEntityDescriptor(string(body)) + log.Debugf("[SAML] Successfully fetched entity descriptor from %v for connector %v", sc.GetEntityDescriptorURL(), sc.GetName()) } if sc.GetEntityDescriptor() != "" { From c8dce7372a73ad930049f68dd47c8a3b8119f125 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 7 Dec 2023 15:27:36 -0700 Subject: [PATCH 2/4] Fix so that individual SSO Connector failures don't prevent other connectors from working --- lib/auth/clt.go | 6 +++--- lib/auth/grpcserver.go | 6 +++--- lib/services/identity.go | 8 +++++--- lib/services/local/users.go | 27 +++++++++++++++------------ 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/auth/clt.go b/lib/auth/clt.go index 728abdd19430d..1877efb58d30c 100644 --- a/lib/auth/clt.go +++ b/lib/auth/clt.go @@ -528,7 +528,7 @@ type IdentityService interface { UpsertOIDCConnector(ctx context.Context, connector types.OIDCConnector) (types.OIDCConnector, error) // GetOIDCConnector returns OIDC connector information by id GetOIDCConnector(ctx context.Context, id string, withSecrets bool) (types.OIDCConnector, error) - // GetOIDCConnectors gets OIDC connectors list + // GetOIDCConnectors gets valid OIDC connectors list GetOIDCConnectors(ctx context.Context, withSecrets bool) ([]types.OIDCConnector, error) // DeleteOIDCConnector deletes OIDC connector by ID DeleteOIDCConnector(ctx context.Context, connectorID string) error @@ -547,7 +547,7 @@ type IdentityService interface { UpsertSAMLConnector(ctx context.Context, connector types.SAMLConnector) (types.SAMLConnector, error) // GetSAMLConnector returns SAML connector information by id GetSAMLConnector(ctx context.Context, id string, withSecrets bool) (types.SAMLConnector, error) - // GetSAMLConnectors gets SAML connectors list + // GetSAMLConnectors gets valid SAML connectors list GetSAMLConnectors(ctx context.Context, withSecrets bool) ([]types.SAMLConnector, error) // DeleteSAMLConnector deletes SAML connector by ID DeleteSAMLConnector(ctx context.Context, connectorID string) error @@ -564,7 +564,7 @@ type IdentityService interface { UpdateGithubConnector(ctx context.Context, connector types.GithubConnector) (types.GithubConnector, error) // UpsertGithubConnector creates or updates a Github connector. UpsertGithubConnector(ctx context.Context, connector types.GithubConnector) (types.GithubConnector, error) - // GetGithubConnectors returns all configured Github connectors + // GetGithubConnectors returns valid Github connectors GetGithubConnectors(ctx context.Context, withSecrets bool) ([]types.GithubConnector, error) // GetGithubConnector returns the specified Github connector GetGithubConnector(ctx context.Context, id string, withSecrets bool) (types.GithubConnector, error) diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index e900a52179783..09bf5511b4431 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -3039,7 +3039,7 @@ func (g *GRPCServer) GetOIDCConnector(ctx context.Context, req *types.ResourceWi return connector, nil } -// GetOIDCConnectors retrieves all OIDC connectors. +// GetOIDCConnectors retrieves valid OIDC connectors, errors from individual connectors are not forwarded. func (g *GRPCServer) GetOIDCConnectors(ctx context.Context, req *types.ResourcesWithSecretsRequest) (*types.OIDCConnectorV3List, error) { auth, err := g.authenticate(ctx) if err != nil { @@ -3183,7 +3183,7 @@ func (g *GRPCServer) GetSAMLConnector(ctx context.Context, req *types.ResourceWi return samlConnectorV2, nil } -// GetSAMLConnectors retrieves all SAML connectors. +// GetSAMLConnectors retrieves valid SAML connectors, errors from individual connectors are not forwarded. func (g *GRPCServer) GetSAMLConnectors(ctx context.Context, req *types.ResourcesWithSecretsRequest) (*types.SAMLConnectorV2List, error) { auth, err := g.authenticate(ctx) if err != nil { @@ -3329,7 +3329,7 @@ func (g *GRPCServer) GetGithubConnector(ctx context.Context, req *types.Resource return githubConnectorV3, nil } -// GetGithubConnectors retrieves all Github connectors. +// GetGithubConnectors retrieves valid GitHub connectors, errors from individual connectors are not forwarded. func (g *GRPCServer) GetGithubConnectors(ctx context.Context, req *types.ResourcesWithSecretsRequest) (*types.GithubConnectorV3List, error) { auth, err := g.authenticate(ctx) if err != nil { diff --git a/lib/services/identity.go b/lib/services/identity.go index f8e7d5a72a709..d637b84436c94 100644 --- a/lib/services/identity.go +++ b/lib/services/identity.go @@ -181,7 +181,8 @@ type Identity interface { // GetOIDCConnector returns OIDC connector data, withSecrets adds or removes client secret from return results GetOIDCConnector(ctx context.Context, id string, withSecrets bool) (types.OIDCConnector, error) - // GetOIDCConnectors returns registered connectors, withSecrets adds or removes client secret from return results + // GetOIDCConnectors returns valid registered connectors, withSecrets adds or removes client secret from return + // results. Invalid Connectors are simply logged but errors are not forwarded. GetOIDCConnectors(ctx context.Context, withSecrets bool) ([]types.OIDCConnector, error) // CreateOIDCAuthRequest creates new auth request @@ -203,7 +204,8 @@ type Identity interface { // GetSAMLConnector returns OIDC connector data, withSecrets adds or removes secrets from return results GetSAMLConnector(ctx context.Context, id string, withSecrets bool) (types.SAMLConnector, error) - // GetSAMLConnectors returns registered connectors, withSecrets adds or removes secret from return results + // GetSAMLConnectors returns valid registered connectors, withSecrets adds or removes secret from return results. + // Invalid Connectors are simply logged but errors are not forwarded. GetSAMLConnectors(ctx context.Context, withSecrets bool) ([]types.SAMLConnector, error) // CreateSAMLAuthRequest creates new auth request @@ -225,7 +227,7 @@ type Identity interface { // UpsertGithubConnector creates or updates a Github connector. UpsertGithubConnector(ctx context.Context, connector types.GithubConnector) (types.GithubConnector, error) - // GetGithubConnectors returns all configured Github connectors + // GetGithubConnectors returns valid Github connectors, nvalid Connectors are simply logged but errors are not forwarded. GetGithubConnectors(ctx context.Context, withSecrets bool) ([]types.GithubConnector, error) // GetGithubConnector returns a Github connector by its name diff --git a/lib/services/local/users.go b/lib/services/local/users.go index fd7b8c9d2c686..1877f35ef2f99 100644 --- a/lib/services/local/users.go +++ b/lib/services/local/users.go @@ -1246,17 +1246,18 @@ func (s *IdentityService) GetOIDCConnectors(ctx context.Context, withSecrets boo if err != nil { return nil, trace.Wrap(err) } - connectors := make([]types.OIDCConnector, len(result.Items)) - for i, item := range result.Items { + var connectors []types.OIDCConnector + for _, item := range result.Items { conn, err := services.UnmarshalOIDCConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - return nil, trace.Wrap(err) + logrus.Errorf("error loading OIDC Connector: %s", err) + continue } if !withSecrets { conn.SetClientSecret("") conn.SetGoogleServiceAccount("") } - connectors[i] = conn + connectors = append(connectors, conn) } return connectors, nil } @@ -1411,11 +1412,12 @@ func (s *IdentityService) GetSAMLConnectors(ctx context.Context, withSecrets boo if err != nil { return nil, trace.Wrap(err) } - connectors := make([]types.SAMLConnector, len(result.Items)) - for i, item := range result.Items { + var connectors []types.SAMLConnector + for _, item := range result.Items { conn, err := services.UnmarshalSAMLConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - return nil, trace.Wrap(err) + logrus.Errorf("error loading SAML Connector: %s", err) + continue } if !withSecrets { keyPair := conn.GetSigningKeyPair() @@ -1424,7 +1426,7 @@ func (s *IdentityService) GetSAMLConnectors(ctx context.Context, withSecrets boo conn.SetSigningKeyPair(keyPair) } } - connectors[i] = conn + connectors = append(connectors, conn) } return connectors, nil } @@ -1600,16 +1602,17 @@ func (s *IdentityService) GetGithubConnectors(ctx context.Context, withSecrets b if err != nil { return nil, trace.Wrap(err) } - connectors := make([]types.GithubConnector, len(result.Items)) - for i, item := range result.Items { + var connectors []types.GithubConnector + for _, item := range result.Items { connector, err := services.UnmarshalGithubConnector(item.Value, services.WithRevision(item.Revision)) if err != nil { - return nil, trace.Wrap(err) + logrus.Errorf("error loading GitHub Connector: %s", err) + continue } if !withSecrets { connector.SetClientSecret("") } - connectors[i] = connector + connectors = append(connectors, connector) } return connectors, nil } From b9c4e27e6dd4345928ce6c31d2d5a81e71418d29 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 8 Dec 2023 10:59:46 -0700 Subject: [PATCH 3/4] Apply PR feedback for better logging and docs --- lib/services/identity.go | 2 +- lib/services/local/users.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/services/identity.go b/lib/services/identity.go index d637b84436c94..a93bb27d35d29 100644 --- a/lib/services/identity.go +++ b/lib/services/identity.go @@ -227,7 +227,7 @@ type Identity interface { // UpsertGithubConnector creates or updates a Github connector. UpsertGithubConnector(ctx context.Context, connector types.GithubConnector) (types.GithubConnector, error) - // GetGithubConnectors returns valid Github connectors, nvalid Connectors are simply logged but errors are not forwarded. + // GetGithubConnectors returns valid Github connectors, invalid Connectors are simply logged but errors are not forwarded. GetGithubConnectors(ctx context.Context, withSecrets bool) ([]types.GithubConnector, error) // GetGithubConnector returns a Github connector by its name diff --git a/lib/services/local/users.go b/lib/services/local/users.go index 1877f35ef2f99..a2ee18a7054d9 100644 --- a/lib/services/local/users.go +++ b/lib/services/local/users.go @@ -1250,7 +1250,7 @@ func (s *IdentityService) GetOIDCConnectors(ctx context.Context, withSecrets boo for _, item := range result.Items { conn, err := services.UnmarshalOIDCConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - logrus.Errorf("error loading OIDC Connector: %s", err) + logrus.WithError(err).Errorf("Error unmarshaling OIDC Connector: %s", item.Key) continue } if !withSecrets { @@ -1416,7 +1416,7 @@ func (s *IdentityService) GetSAMLConnectors(ctx context.Context, withSecrets boo for _, item := range result.Items { conn, err := services.UnmarshalSAMLConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - logrus.Errorf("error loading SAML Connector: %s", err) + logrus.WithError(err).Errorf("Error unmarshaling SAML Connector: %s", item.Key) continue } if !withSecrets { @@ -1606,7 +1606,7 @@ func (s *IdentityService) GetGithubConnectors(ctx context.Context, withSecrets b for _, item := range result.Items { connector, err := services.UnmarshalGithubConnector(item.Value, services.WithRevision(item.Revision)) if err != nil { - logrus.Errorf("error loading GitHub Connector: %s", err) + logrus.WithError(err).Errorf("Error unmarshaling GitHub Connector: %s", item.Key) continue } if !withSecrets { From 8368bd163d32e16bb926b2df378c000e38895bef Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 8 Dec 2023 11:09:20 -0700 Subject: [PATCH 4/4] Formatting update after feedback --- lib/services/local/users.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/services/local/users.go b/lib/services/local/users.go index a2ee18a7054d9..3e1bb9c61cfd0 100644 --- a/lib/services/local/users.go +++ b/lib/services/local/users.go @@ -1250,7 +1250,10 @@ func (s *IdentityService) GetOIDCConnectors(ctx context.Context, withSecrets boo for _, item := range result.Items { conn, err := services.UnmarshalOIDCConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - logrus.WithError(err).Errorf("Error unmarshaling OIDC Connector: %s", item.Key) + logrus. + WithError(err). + WithField("key", item.Key). + Errorf("Error unmarshaling OIDC Connector") continue } if !withSecrets { @@ -1416,7 +1419,10 @@ func (s *IdentityService) GetSAMLConnectors(ctx context.Context, withSecrets boo for _, item := range result.Items { conn, err := services.UnmarshalSAMLConnector(item.Value, services.WithExpires(item.Expires), services.WithRevision(item.Revision)) if err != nil { - logrus.WithError(err).Errorf("Error unmarshaling SAML Connector: %s", item.Key) + logrus. + WithError(err). + WithField("key", item.Key). + Errorf("Error unmarshaling SAML Connector") continue } if !withSecrets { @@ -1606,7 +1612,10 @@ func (s *IdentityService) GetGithubConnectors(ctx context.Context, withSecrets b for _, item := range result.Items { connector, err := services.UnmarshalGithubConnector(item.Value, services.WithRevision(item.Revision)) if err != nil { - logrus.WithError(err).Errorf("Error unmarshaling GitHub Connector: %s", item.Key) + logrus. + WithError(err). + WithField("key", item.Key). + Errorf("Error unmarshaling GitHub Connector") continue } if !withSecrets {