Better errors for invalid OIDC connectors, fixes #2690

Invalid or inaccessible OIDC connectors were preventing
all other logins to work, as they were grabbing mutex
and making a network call.

Besides, the error was not informative as the user
saw "web headers timeout" error in the browser.

This fix makes sure that:

* Connector errors are isolated to the connector name
and other flows are not blocked
* Error is presented to user in a reasonable way and
provides instructions
* Administrator can see the error in the audit logs.

lib/auth/oidc.go

@@ -17,6 +17,7 @@ limitations under the License.
 package auth
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -36,38 +37,103 @@ import (
 	"github.com/gravitational/trace"
 )
 
+func (s *AuthServer) getOrCreateOIDCClient(conn services.OIDCConnector) (*oidc.Client, error) {
+	client, err := s.getOIDCClient(conn)
+	if err == nil {
+		return client, nil
+	}
+	if !trace.IsNotFound(err) {
+		return nil, trace.Wrap(err)
+	}
+	return s.createOIDCClient(conn)
+}
+
 func (s *AuthServer) getOIDCClient(conn services.OIDCConnector) (*oidc.Client, error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	config := oidc.ClientConfig{
-		RedirectURL: conn.GetRedirectURL(),
-		Credentials: oidc.ClientCredentials{
-			ID:     conn.GetClientID(),
-			Secret: conn.GetClientSecret(),
-		},
-		// open id notifies provider that we are using OIDC scopes
-		Scope: utils.Deduplicate(append([]string{"openid", "email"}, conn.GetScope()...)),
+	clientPack, ok := s.oidcClients[conn.GetName()]
+	if !ok {
+		return nil, trace.NotFound("connector %v is not found", conn.GetName())
 	}
 
-	clientPack, ok := s.oidcClients[conn.GetName()]
+	config := oidcConfig(conn)
 	if ok && oidcConfigsEqual(clientPack.config, config) {
 		return clientPack.client, nil
 	}
+
 	delete(s.oidcClients, conn.GetName())
+	return nil, trace.NotFound("connector %v has updated the configuration and is invalidated", conn.GetName())
 
+}
+
+func (s *AuthServer) createOIDCClient(conn services.OIDCConnector) (*oidc.Client, error) {
+	config := oidcConfig(conn)
 	client, err := oidc.NewClient(config)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	client.SyncProviderConfig(conn.GetIssuerURL())
+	ctx, cancel := context.WithTimeout(context.Background(), defaults.WebHeadersTimeout)
+	defer cancel()
+
+	go func() {
+		defer cancel()
+		client.SyncProviderConfig(conn.GetIssuerURL())
+	}()
+
+	select {
+	case <-ctx.Done():
+	case <-s.closeCtx.Done():
+		return nil, trace.ConnectionProblem(nil, "auth server is shutting down")
+	}
+
+	// Canceled is expected in case if sync provider config finishes faster
+	// than the deadline
+	if ctx.Err() != nil && ctx.Err() != context.Canceled {
+		var err error
+		if ctx.Err() == context.DeadlineExceeded {
+			err = trace.ConnectionProblem(err,
+				"failed to reach out to oidc connector %v, most likely URL %q is not valid or not accessible, check configuration and try to re-create the connector",
+				conn.GetName(), conn.GetIssuerURL())
+		} else {
+			err = trace.ConnectionProblem(err,
+				"unknown problem with connector %v, most likely URL %q is not valid or not accessible, check configuration and try to re-create the connector",
+				conn.GetName(), conn.GetIssuerURL())
+		}
+		s.EmitAuditEvent(events.UserSSOLoginFailure, events.EventFields{
+			events.LoginMethod:        events.LoginMethodOIDC,
+			events.AuthAttemptSuccess: false,
+			events.AuthAttemptErr:     trace.Unwrap(ctx.Err()).Error(),
+			events.AuthAttemptMessage: err.Error(),
+		})
+		// return user-friendly error hiding the actual error in the event
+		// logs for security purposes
+		return nil, trace.ConnectionProblem(nil,
+			"failed to login with %v, please contact your system administrator to check audit logs",
+			conn.GetDisplay())
+	}
+
+	s.lock.Lock()
+	defer s.lock.Unlock()
 
 	s.oidcClients[conn.GetName()] = &oidcClient{client: client, config: config}
 
 	return client, nil
 }
 
+func oidcConfig(conn services.OIDCConnector) oidc.ClientConfig {
+	return oidc.ClientConfig{
+		RedirectURL: conn.GetRedirectURL(),
+		Credentials: oidc.ClientCredentials{
+			ID:     conn.GetClientID(),
+			Secret: conn.GetClientSecret(),
+		},
+		// open id notifies provider that we are using OIDC scopes
+		Scope: utils.Deduplicate(append([]string{"openid", "email"}, conn.GetScope()...)),
+	}
+}
+
 func (s *AuthServer) UpsertOIDCConnector(connector services.OIDCConnector) error {
 	return s.Identity.UpsertOIDCConnector(connector)
 }
@@ -81,7 +147,7 @@ func (s *AuthServer) CreateOIDCAuthRequest(req services.OIDCAuthRequest) (*servi
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	oidcClient, err := s.getOIDCClient(connector)
+	oidcClient, err := s.getOrCreateOIDCClient(connector)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -176,7 +242,7 @@ func (a *AuthServer) validateOIDCAuthCallback(q url.Values) (*OIDCAuthResponse,
 		return nil, trace.Wrap(err)
 	}
 
-	oidcClient, err := a.getOIDCClient(connector)
+	oidcClient, err := a.getOrCreateOIDCClient(connector)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}

lib/defaults/defaults.go

@@ -97,6 +97,11 @@ const (
 	// DefaultThrottleTimeout is a timemout used to throttle failed auth servers
 	DefaultThrottleTimeout = 10 * time.Second
 
+	// WebHeadersTimeout is a timeout that is set for web requests
+	// before browsers raise "Timeout waiting web headers" error in
+	// the browser
+	WebHeadersTimeout = 10 * time.Second
+
 	// DefaultIdleConnectionDuration indicates for how long Teleport will hold
 	// the SSH connection open if there are no reads/writes happening over it.
 	// 15 minutes default is compliant with PCI DSS standards

lib/web/apiserver.go

@@ -690,7 +690,13 @@ func (h *Handler) oidcLoginWeb(w http.ResponseWriter, r *http.Request, p httprou
 			CheckUser:         true,
 		})
 	if err != nil {
-		return nil, trace.Wrap(err)
+		// redirect to an error page
+		pathToError := url.URL{
+			Path:     "/web/msg/error/login_failed",
+			RawQuery: url.Values{"details": []string{err.Error()}}.Encode(),
+		}
+		http.Redirect(w, r, pathToError.String(), http.StatusFound)
+		return nil, nil
 	}
 	http.Redirect(w, r, response.RedirectURL, http.StatusFound)
 	return nil, nil