From fe8085b8866a97624bd04769054803c897e13e44 Mon Sep 17 00:00:00 2001 From: Rui Yang Date: Mon, 17 May 2021 10:16:50 -0400 Subject: [PATCH] remove client secret encryption option constant time compare for client secret verification will be kept Signed-off-by: Rui Yang --- server/handlers.go | 22 ++---- server/server.go | 24 ------- server/server_test.go | 160 ------------------------------------------ 3 files changed, 7 insertions(+), 199 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 6d3f9e7e..dbdbf6dc 100644 --- a/server/handlers.go +++ b/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 err := bcrypt.CompareHashAndPassword([]byte(clientSecret), []byte(client.Secret)); err != nil { - s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized) - return - } - } 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 + 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 } grantType := r.PostFormValue("grant_type") diff --git a/server/server.go b/server/server.go index 5909d5c3..e6782e4e 100644 --- a/server/server.go +++ b/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, diff --git a/server/server_test.go b/server/server_test.go index 76abbab9..87ca6c17 100644 --- a/server/server_test.go +++ b/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") - } -}