remove client secret encryption option

constant time compare for client secret verification will be kept

Signed-off-by: Rui Yang <ruiya@vmware.com>

server/handlers.go

@@ -17,7 +17,6 @@ import (
 
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/gorilla/mux"
-	"golang.org/x/crypto/bcrypt"
 	jose "gopkg.in/square/go-jose.v2"
 
 	"github.com/dexidp/dex/connector"
@@ -682,21 +681,14 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if s.hashClientSecret {
+	if subtle.ConstantTimeCompare([]byte(client.Secret), []byte(clientSecret)) != 1 {
-		if err := bcrypt.CompareHashAndPassword([]byte(clientSecret), []byte(client.Secret)); err != nil {
+		if clientSecret == "" {
-			s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized)
+			s.logger.Infof("missing client_secret on token request for client: %s", client.ID)
-			return
+		} else {
-		}
+			s.logger.Infof("invalid client_secret on token request for client: %s", client.ID)
-	} else {
-		if subtle.ConstantTimeCompare([]byte(client.Secret), []byte(clientSecret)) != 1 {
-			if clientSecret == "" {
-				s.logger.Infof("missing client_secret on token request for client: %s", client.ID)
-			} else {
-				s.logger.Infof("invalid client_secret on token request for client: %s", client.ID)
-			}
-			s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized)
-			return
 		}
+		s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized)
+		return
 	}
 
 	grantType := r.PostFormValue("grant_type")

server/server.go

@@ -77,9 +77,6 @@ type Config struct {
 	// If enabled, the connectors selection page will always be shown even if there's only one
 	AlwaysShowLoginScreen bool
 
-	// If enabled, the client secret is expected to be encrypted
-	HashClientSecret bool
-
 	RotateKeysAfter        time.Duration // Defaults to 6 hours.
 	IDTokensValidFor       time.Duration // Defaults to 24 hours
 	AuthRequestsValidFor   time.Duration // Defaults to 24 hours
@@ -154,9 +151,6 @@ type Server struct {
 	// If enabled, show the connector selection screen even if there's only one
 	alwaysShowLogin bool
 
-	// If enabled, the client secret is expected to be encrypted
-	hashClientSecret bool
-
 	// Used for password grant
 	passwordConnector string
 
@@ -196,23 +190,6 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
 		return nil, errors.New("server: storage cannot be nil")
 	}
 
-	if c.HashClientSecret {
-		clients, err := c.Storage.ListClients()
-		if err != nil {
-			return nil, fmt.Errorf("server: failed to list clients")
-		}
-
-		for _, client := range clients {
-			if client.Secret == "" {
-				return nil, fmt.Errorf("server: client secret can't be empty")
-			}
-
-			if err = checkCost([]byte(client.Secret)); err != nil {
-				return nil, fmt.Errorf("server: failed to check cost of client secret: %v", err)
-			}
-		}
-	}
-
 	if len(c.SupportedResponseTypes) == 0 {
 		c.SupportedResponseTypes = []string{responseTypeCode}
 	}
@@ -256,7 +233,6 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy)
 		deviceRequestsValidFor: value(c.DeviceRequestsValidFor, 5*time.Minute),
 		skipApproval:           c.SkipApprovalScreen,
 		alwaysShowLogin:        c.AlwaysShowLoginScreen,
-		hashClientSecret:       c.HashClientSecret,
 		now:                    now,
 		templates:              tmpls,
 		passwordConnector:      c.PasswordConnector,

server/server_test.go

@@ -1637,163 +1637,3 @@ func TestOAuth2DeviceFlow(t *testing.T) {
 		}()
 	}
 }
-
-func TestClientSecretEncryption(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	httpServer, s := newTestServer(ctx, t, func(c *Config) {
-		c.HashClientSecret = true
-	})
-	defer httpServer.Close()
-
-	clientID := "testclient"
-	clientSecret := "testclientsecret"
-	hash, err := bcrypt.GenerateFromPassword([]byte(clientSecret), bcrypt.DefaultCost)
-	if err != nil {
-		t.Fatalf("failed to bcrypt: %s", err)
-	}
-
-	// Query server's provider metadata.
-	p, err := oidc.NewProvider(ctx, httpServer.URL)
-	if err != nil {
-		t.Fatalf("failed to get provider: %v", err)
-	}
-
-	var (
-		// If the OAuth2 client didn't get a response, we need
-		// to print the requests the user saw.
-		gotCode           bool
-		reqDump, respDump []byte // Auth step, not token.
-		state             = "a_state"
-	)
-	defer func() {
-		if !gotCode {
-			t.Errorf("never got a code in callback\n%s\n%s", reqDump, respDump)
-		}
-	}()
-
-	// Setup OAuth2 client.
-	var oauth2Config *oauth2.Config
-
-	requestedScopes := []string{oidc.ScopeOpenID, "email", "profile", "groups", "offline_access"}
-
-	// Create the OAuth2 config.
-	oauth2Config = &oauth2.Config{
-		ClientID:     clientID,
-		ClientSecret: string(hash),
-		Endpoint:     p.Endpoint(),
-		Scopes:       requestedScopes,
-	}
-
-	oauth2Client := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/callback" {
-			// User is visiting app first time. Redirect to dex.
-			http.Redirect(w, r, oauth2Config.AuthCodeURL(state), http.StatusSeeOther)
-			return
-		}
-
-		// User is at '/callback' so they were just redirected _from_ dex.
-		q := r.URL.Query()
-
-		// Grab code, exchange for token.
-		if code := q.Get("code"); code != "" {
-			gotCode = true
-			token, err := oauth2Config.Exchange(ctx, code)
-			if err != nil {
-				t.Errorf("failed to exchange code for token: %v", err)
-				return
-			}
-
-			oidcConfig := &oidc.Config{SkipClientIDCheck: true}
-
-			idToken, ok := token.Extra("id_token").(string)
-			if !ok {
-				t.Errorf("no id token found")
-				return
-			}
-			if _, err := p.Verifier(oidcConfig).Verify(ctx, idToken); err != nil {
-				t.Errorf("failed to verify id token: %v", err)
-				return
-			}
-		}
-
-		w.WriteHeader(http.StatusOK)
-	}))
-
-	oauth2Config.RedirectURL = oauth2Client.URL + "/callback"
-
-	defer oauth2Client.Close()
-
-	// Regester the client above with dex.
-	client := storage.Client{
-		ID:           clientID,
-		Secret:       clientSecret,
-		RedirectURIs: []string{oauth2Client.URL + "/callback"},
-	}
-	if err := s.storage.CreateClient(client); err != nil {
-		t.Fatalf("failed to create client: %v", err)
-	}
-
-	// Login!
-	//
-	//   1. First request to client, redirects to dex.
-	//   2. Dex "logs in" the user, redirects to client with "code".
-	//   3. Client exchanges "code" for "token" (id_token, refresh_token, etc.).
-	//   4. Test is run with OAuth2 token response.
-	//
-	resp, err := http.Get(oauth2Client.URL + "/login")
-	if err != nil {
-		t.Fatalf("get failed: %v", err)
-	}
-	defer resp.Body.Close()
-
-	if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil {
-		t.Fatal(err)
-	}
-	if respDump, err = httputil.DumpResponse(resp, true); err != nil {
-		t.Fatal(err)
-	}
-}
-
-func TestClientSecretEncryptionCost(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	clientID := "testclient"
-	clientSecret := "testclientsecret"
-	hash, err := bcrypt.GenerateFromPassword([]byte(clientSecret), 5)
-	if err != nil {
-		t.Fatalf("failed to bcrypt: %s", err)
-	}
-
-	// Register the client above with dex.
-	client := storage.Client{
-		ID:     clientID,
-		Secret: string(hash),
-	}
-
-	config := Config{
-		Storage: memory.New(logger),
-		Web: WebConfig{
-			Dir: "../web",
-		},
-		Logger:             logger,
-		PrometheusRegistry: prometheus.NewRegistry(),
-		HashClientSecret:   true,
-	}
-
-	err = config.Storage.CreateClient(client)
-	if err != nil {
-		t.Fatalf("failed to create client: %v", err)
-	}
-
-	_, err = newServer(ctx, config, staticRotationStrategy(testKey))
-	if err == nil {
-		t.Error("constructing server should have failed")
-	}
-
-	if !strings.Contains(err.Error(), "failed to check cost") {
-		t.Error("should have failed with cost error")
-	}
-}