feat(registry): enhance authentication checks in htpasswd implementation (#4758)

registry/auth/htpasswd/access.go

@@ -35,6 +35,9 @@ type accessController struct {
 	modtime  time.Time
 	mu       sync.Mutex
 	htpasswd *htpasswd
+
+	// overrideDummyHash allows overriding the dummy-hash for testing.
+	overrideDummyHash []byte
 }
 
 var _ auth.AccessController = &accessController{}
@@ -53,7 +56,13 @@ func newAccessController(options map[string]any) (auth.AccessController, error)
 	if err := createHtpasswdFile(path); err != nil {
 		return nil, err
 	}
-	return &accessController{realm: realm.(string), path: path}, nil
+	var dummyHash []byte
+	if hash, ok := options["overrideDummyHash"]; ok {
+		// override dummy hash for testing
+		dummyHash = hash.([]byte)
+	}
+
+	return &accessController{realm: realm.(string), path: path, overrideDummyHash: dummyHash}, nil
 }
 
 func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.Access) (*auth.Grant, error) {
@@ -83,7 +92,7 @@ func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.
 		}
 		defer f.Close()
 
-		h, err := newHTPasswd(f)
+		h, err := newHTPasswd(f, ac.overrideDummyHash)
 		if err != nil {
 			ac.mu.Unlock()
 			return nil, err
@@ -93,11 +102,10 @@ func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.
 	localHTPasswd := ac.htpasswd
 	ac.mu.Unlock()
 
-	if err := localHTPasswd.authenticateUser(username, password); err != nil {
-		dcontext.GetLogger(req.Context()).Errorf("error authenticating user %q: %v", username, err)
+	if err := localHTPasswd.authenticateUser(req.Context(), username, password); err != nil {
 		return nil, &challenge{
 			realm: ac.realm,
-			err:   auth.ErrAuthenticationFailure,
+			err:   err,
 		}
 	}
 

registry/auth/htpasswd/access_test.go

@@ -2,10 +2,10 @@ package htpasswd
 
 import (
 	"bytes"
-	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/distribution/distribution/v3/registry/auth"
@@ -13,37 +13,33 @@ import (
 
 func TestBasicAccessController(t *testing.T) {
 	testRealm := "The-Shire"
-	testUsers := []string{"bilbo", "frodo", "MiShil", "DeokMan"}
-	testPasswords := []string{"baggins", "baggins", "새주", "공주님"}
+	testUsers := []string{"bilbo", "frodo", "MiShil", "DeokMan", "nonexistent"}
+	testPasswords := []string{"baggins", "baggins", "새주", "공주님", "nonexistent"}
 	testHtpasswdContent := `bilbo:{SHA}5siv5c0SHx681xU6GiSx9ZQryqs=
 							frodo:$2y$05$926C3y10Quzn/LnqQH86VOEVh/18T6RnLaS.khre96jLNL/7e.K5W
 							MiShil:$2y$05$0oHgwMehvoe8iAWS8I.7l.KoECXrwVaC16RPfaSCU5eVTFrATuMI2
 							DeokMan:공주님`
 
-	tempFile, err := os.CreateTemp("", "htpasswd-test")
+	tempFile := filepath.Join(t.TempDir(), "htpasswd")
+	err := os.WriteFile(tempFile, []byte(testHtpasswdContent), 0600)
 	if err != nil {
-		t.Fatal("could not create temporary htpasswd file")
-	}
-	if _, err = tempFile.WriteString(testHtpasswdContent); err != nil {
 		t.Fatal("could not write temporary htpasswd file")
 	}
 
-	options := map[string]any{
+	accessCtrl, err := newAccessController(map[string]any{
 		"realm": testRealm,
-		"path":  tempFile.Name(),
-	}
+		"path":  tempFile,
 
-	accessController, err := newAccessController(options)
+		"overrideDummyHash": []byte("$2a$05$/vyFmJBPzsrsp6EC53biLulrw8zVjsWqpw26Hb.wfMyrHmRdh2orW"), // hash of "nonexistent"
+	})
 	if err != nil {
 		t.Fatal("error creating access controller")
 	}
 
-	tempFile.Close()
-
 	userNumber := 0
 
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		grant, err := accessController.Authorized(r)
+		grant, err := accessCtrl.Authorized(r)
 		if err != nil {
 			switch err := err.(type) {
 			case auth.Challenge:
@@ -83,8 +79,9 @@ func TestBasicAccessController(t *testing.T) {
 	}
 
 	nonbcrypt := map[string]struct{}{
-		"bilbo":   {},
-		"DeokMan": {},
+		"bilbo":       {},
+		"DeokMan":     {},
+		"nonexistent": {},
 	}
 
 	for i := range testUsers {
@@ -115,30 +112,48 @@ func TestBasicAccessController(t *testing.T) {
 			}
 		}
 	}
+
+	for i := range len(testUsers) {
+		userNumber = i
+		req, err := http.NewRequest(http.MethodGet, server.URL, nil)
+		if err != nil {
+			t.Fatalf("error allocating new request: %v", err)
+		}
+
+		invalidPassword := testPasswords[i] + "invalid"
+		req.SetBasicAuth(testUsers[i], invalidPassword)
+
+		resp, err = client.Do(req)
+		if err != nil {
+			t.Fatalf("unexpected error during GET: %v", err)
+		}
+		defer resp.Body.Close()
+
+		if resp.StatusCode != http.StatusUnauthorized {
+			t.Fatalf("unexpected non-success response status: %v != %v for %s %s", resp.StatusCode, http.StatusUnauthorized, testUsers[i], invalidPassword)
+		}
+	}
 }
 
 func TestCreateHtpasswdFile(t *testing.T) {
-	tempFile, err := os.CreateTemp("", "htpasswd-test")
-	if err != nil {
+	tempFile := filepath.Join(t.TempDir(), "htpasswd")
+	if err := os.WriteFile(tempFile, []byte{}, 0600); err != nil {
 		t.Fatalf("could not create temporary htpasswd file %v", err)
 	}
-	defer tempFile.Close()
 	options := map[string]any{
 		"realm": "/auth/htpasswd",
-		"path":  tempFile.Name(),
+		"path":  tempFile,
 	}
 	// Ensure file is not populated
 	if _, err := newAccessController(options); err != nil {
 		t.Fatalf("error creating access controller %v", err)
 	}
-	content, err := io.ReadAll(tempFile)
-	if err != nil {
+	if content, err := os.ReadFile(tempFile); err != nil {
 		t.Fatalf("failed to read file %v", err)
-	}
-	if !bytes.Equal([]byte{}, content) {
+	} else if !bytes.Equal([]byte{}, content) {
 		t.Fatalf("htpasswd file should not be populated %v", string(content))
 	}
-	if err := os.Remove(tempFile.Name()); err != nil {
+	if err := os.Remove(tempFile); err != nil {
 		t.Fatalf("failed to remove temp file %v", err)
 	}
 
@@ -146,11 +161,9 @@ func TestCreateHtpasswdFile(t *testing.T) {
 	if _, err := newAccessController(options); err != nil {
 		t.Fatalf("error creating access controller %v", err)
 	}
-	content, err = os.ReadFile(tempFile.Name())
-	if err != nil {
+	if content, err := os.ReadFile(tempFile); err != nil {
 		t.Fatalf("failed to read file %v", err)
-	}
-	if !bytes.HasPrefix(content, []byte("docker:$2a$")) {
+	} else if !bytes.HasPrefix(content, []byte("docker:$2a$")) {
 		t.Fatalf("failed to find default user in file %s", string(content))
 	}
 }

registry/auth/htpasswd/htpasswd.go

@@ -2,46 +2,63 @@ package htpasswd
 
 import (
 	"bufio"
+	"context"
+	"errors"
 	"fmt"
 	"io"
 	"strings"
 
+	"github.com/distribution/distribution/v3/internal/dcontext"
 	"github.com/distribution/distribution/v3/registry/auth"
 
 	"golang.org/x/crypto/bcrypt"
 )
 
+// dummyBcryptHash is used for non-existing users to prevent timing attacks.
+const dummyBcryptHash = "$2a$05$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+
 // htpasswd holds a path to a system .htpasswd file and the machinery to parse
 // it. Only bcrypt hash entries are supported.
 type htpasswd struct {
 	entries map[string][]byte // maps username to password byte slice.
+
+	// dummyHash is used for non-existing users to prevent timing attacks.
+	dummyHash []byte
 }
 
 // newHTPasswd parses the reader and returns an htpasswd or an error.
-func newHTPasswd(rd io.Reader) (*htpasswd, error) {
+func newHTPasswd(rd io.Reader, dummyHash []byte) (*htpasswd, error) {
 	entries, err := parseHTPasswd(rd)
 	if err != nil {
 		return nil, err
 	}
-
-	return &htpasswd{entries: entries}, nil
+	if dummyHash == nil {
+		dummyHash = []byte(dummyBcryptHash)
+	}
+	return &htpasswd{
+		entries:   entries,
+		dummyHash: dummyHash,
+	}, nil
 }
 
 // AuthenticateUser checks a given user:password credential against the
 // receiving HTPasswd's file. If the check passes, nil is returned.
-func (htpasswd *htpasswd) authenticateUser(username string, password string) error {
+func (htpasswd *htpasswd) authenticateUser(ctx context.Context, username string, password string) error {
 	credentials, ok := htpasswd.entries[username]
 	if !ok {
-		// timing attack paranoia
-		if err := bcrypt.CompareHashAndPassword([]byte{}, []byte(password)); err != nil {
-			return auth.ErrAuthenticationFailure
-		}
-
+		// timing attack paranoia, always compare the hash even if the user is not found.
+		_ = bcrypt.CompareHashAndPassword(htpasswd.dummyHash, []byte(password))
 		return auth.ErrAuthenticationFailure
 	}
 
-	err := bcrypt.CompareHashAndPassword(credentials, []byte(password))
-	if err != nil {
+	if err := bcrypt.CompareHashAndPassword(credentials, []byte(password)); err != nil {
+		logger := dcontext.GetLogger(ctx).WithError(err).WithField("username", username)
+		if !errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
+			logger.Error("error authenticating user")
+		} else {
+			logger.Info("user failed to authenticate")
+		}
+		// Always return a generic "ErrAuthenticationFailure", not the underlying error.
 		return auth.ErrAuthenticationFailure
 	}