Harden basic auth implementation

After consideration, the basic authentication implementation has been
simplified to only support bcrypt entries in an htpasswd file. This greatly
increases the security of the implementation by reducing the possibility of
timing attacks and other problems trying to detect the password hash type.

Also, the htpasswd file is only parsed at startup, ensuring that the file can
be edited and not effect ongoing requests. Newly added passwords take effect on
restart. Subsequently, password hash entries are now stored in a map.

Test cases have been modified accordingly.

Signed-off-by: Stephen J Day <stephen.day@docker.com>

docs/auth/basic/access.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"os"
 
 	ctxu "github.com/docker/distribution/context"
 	"github.com/docker/distribution/registry/auth"
@@ -41,7 +42,18 @@ func newAccessController(options map[string]interface{}) (auth.AccessController,
 		return nil, fmt.Errorf(`"path" must be set for basic access controller`)
 	}
 
-	return &accessController{realm: realm.(string), htpasswd: newHTPasswd(path.(string))}, nil
+	f, err := os.Open(path.(string))
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	h, err := newHTPasswd(f)
+	if err != nil {
+		return nil, err
+	}
+
+	return &accessController{realm: realm.(string), htpasswd: h}, nil
 }
 
 func (ac *accessController) Authorized(ctx context.Context, accessRecords ...auth.Access) (context.Context, error) {
@@ -58,7 +70,7 @@ func (ac *accessController) Authorized(ctx context.Context, accessRecords ...aut
 		}
 	}
 
-	if err := ac.htpasswd.authenticateUser(ctx, username, password); err != nil {
+	if err := ac.htpasswd.authenticateUser(username, password); err != nil {
 		ctxu.GetLogger(ctx).Errorf("error authenticating user %q: %v", username, err)
 		return nil, &challenge{
 			realm: ac.realm,

docs/auth/basic/access_test.go

@@ -11,7 +11,6 @@ import (
 )
 
 func TestBasicAccessController(t *testing.T) {
-
 	testRealm := "The-Shire"
 	testUsers := []string{"bilbo", "frodo", "MiShil", "DeokMan"}
 	testPasswords := []string{"baggins", "baggins", "새주", "공주님"}
@@ -85,6 +84,11 @@ func TestBasicAccessController(t *testing.T) {
 		t.Fatalf("unexpected non-fail response status: %v != %v", resp.StatusCode, http.StatusUnauthorized)
 	}
 
+	nonbcrypt := map[string]struct{}{
+		"bilbo":   struct{}{},
+		"DeokMan": struct{}{},
+	}
+
 	for i := 0; i < len(testUsers); i++ {
 		userNumber = i
 		req, err := http.NewRequest("GET", server.URL, nil)
@@ -100,9 +104,17 @@ func TestBasicAccessController(t *testing.T) {
 		}
 		defer resp.Body.Close()
 
-		// Request should be authorized
-		if resp.StatusCode != http.StatusNoContent {
-			t.Fatalf("unexpected non-success response status: %v != %v for %s %s", resp.StatusCode, http.StatusNoContent, testUsers[i], testPasswords[i])
+		if _, ok := nonbcrypt[testUsers[i]]; ok {
+			// these are not allowed.
+			// Request should be authorized
+			if resp.StatusCode != http.StatusUnauthorized {
+				t.Fatalf("unexpected non-success response status: %v != %v for %s %s", resp.StatusCode, http.StatusUnauthorized, testUsers[i], testPasswords[i])
+			}
+		} else {
+			// Request should be authorized
+			if resp.StatusCode != http.StatusNoContent {
+				t.Fatalf("unexpected non-success response status: %v != %v for %s %s", resp.StatusCode, http.StatusNoContent, testUsers[i], testPasswords[i])
+			}
 		}
 	}
 

docs/auth/basic/htpasswd.go

@@ -2,149 +2,79 @@ package basic
 
 import (
 	"bufio"
-	"crypto/sha1"
-	"encoding/base64"
+	"fmt"
 	"io"
-	"os"
-	"regexp"
 	"strings"
 
-	"github.com/docker/distribution/context"
 	"golang.org/x/crypto/bcrypt"
 )
 
-// htpasswd holds a path to a system .htpasswd file and the machinery to parse it.
+// htpasswd holds a path to a system .htpasswd file and the machinery to parse
+// it. Only bcrypt hash entries are supported.
 type htpasswd struct {
-	path string
+	entries map[string][]byte // maps username to password byte slice.
 }
 
-// authType represents a particular hash function used in the htpasswd file.
-type authType int
-
-const (
-	authTypePlainText authType = iota // Plain-text password storage (htpasswd -p)
-	authTypeSHA1                      // sha hashed password storage (htpasswd -s)
-	authTypeApacheMD5                 // apr iterated md5 hashing (htpasswd -m)
-	authTypeBCrypt                    // BCrypt adapative password hashing (htpasswd -B)
-	authTypeCrypt                     // System crypt() hashes.  (htpasswd -d)
-)
-
-var bcryptPrefixRegexp = regexp.MustCompile(`^\$2[ab]?y\$`)
-
-// detectAuthCredentialType inspects the credential and resolves the encryption scheme.
-func detectAuthCredentialType(cred string) authType {
-	if strings.HasPrefix(cred, "{SHA}") {
-		return authTypeSHA1
+// newHTPasswd parses the reader and returns an htpasswd or an error.
+func newHTPasswd(rd io.Reader) (*htpasswd, error) {
+	entries, err := parseHTPasswd(rd)
+	if err != nil {
+		return nil, err
 	}
-	if strings.HasPrefix(cred, "$apr1$") {
-		return authTypeApacheMD5
-	}
-	if bcryptPrefixRegexp.MatchString(cred) {
-		return authTypeBCrypt
-	}
-	// There's just not a great way to distinguish between these next two...
-	if len(cred) == 13 {
-		return authTypeCrypt
-	}
-	return authTypePlainText
-}
 
-// String Returns a text representation of the AuthType
-func (at authType) String() string {
-	switch at {
-	case authTypePlainText:
-		return "plaintext"
-	case authTypeSHA1:
-		return "sha1"
-	case authTypeApacheMD5:
-		return "md5"
-	case authTypeBCrypt:
-		return "bcrypt"
-	case authTypeCrypt:
-		return "system crypt"
-	}
-	return "unknown"
-}
-
-// NewHTPasswd Create a new HTPasswd with the given path to .htpasswd file.
-func newHTPasswd(htpath string) *htpasswd {
-	return &htpasswd{path: htpath}
+	return &htpasswd{entries: entries}, nil
 }
 
 // AuthenticateUser checks a given user:password credential against the
-// receiving HTPasswd's file. If the check passes, nil is returned. Note that
-// this parses the htpasswd file on each request so ensure that updates are
-// available.
-func (htpasswd *htpasswd) authenticateUser(ctx context.Context, username string, password string) error {
-	// Open the file.
-	in, err := os.Open(htpasswd.path)
+// receiving HTPasswd's file. If the check passes, nil is returned.
+func (htpasswd *htpasswd) authenticateUser(username string, password string) error {
+	credentials, ok := htpasswd.entries[username]
+	if !ok {
+		// timing attack paranoia
+		bcrypt.CompareHashAndPassword([]byte{}, []byte(password))
+
+		return ErrAuthenticationFailure
+	}
+
+	err := bcrypt.CompareHashAndPassword([]byte(credentials), []byte(password))
 	if err != nil {
-		return err
-	}
-	defer in.Close()
-
-	for _, entry := range parseHTPasswd(ctx, in) {
-		if entry.username != username {
-			continue // wrong entry
-		}
-
-		switch t := detectAuthCredentialType(entry.password); t {
-		case authTypeSHA1:
-			sha := sha1.New()
-			sha.Write([]byte(password))
-			hash := base64.StdEncoding.EncodeToString(sha.Sum(nil))
-
-			if entry.password[5:] != hash {
-				return ErrAuthenticationFailure
-			}
-
-			return nil
-		case authTypeBCrypt:
-			err := bcrypt.CompareHashAndPassword([]byte(entry.password), []byte(password))
-			if err != nil {
-				return ErrAuthenticationFailure
-			}
-
-			return nil
-		case authTypePlainText:
-			if password != entry.password {
-				return ErrAuthenticationFailure
-			}
-
-			return nil
-		default:
-			context.GetLogger(ctx).Errorf("unsupported basic authentication type: %v", t)
-		}
+		return ErrAuthenticationFailure
 	}
 
-	return ErrAuthenticationFailure
+	return nil
 }
 
-// htpasswdEntry represents a line in an htpasswd file.
-type htpasswdEntry struct {
-	username string // username, plain text
-	password string // stores hashed passwd
-}
-
-// parseHTPasswd parses the contents of htpasswd. Bad entries are skipped and
-// logged, so this may return empty. This will read all the entries in the
-// file, whether or not they are needed.
-func parseHTPasswd(ctx context.Context, rd io.Reader) []htpasswdEntry {
-	entries := []htpasswdEntry{}
+// parseHTPasswd parses the contents of htpasswd. This will read all the
+// entries in the file, whether or not they are needed. An error is returned
+// if an syntax errors are encountered or if the reader fails.
+func parseHTPasswd(rd io.Reader) (map[string][]byte, error) {
+	entries := map[string][]byte{}
 	scanner := bufio.NewScanner(rd)
+	var line int
 	for scanner.Scan() {
+		line++ // 1-based line numbering
 		t := strings.TrimSpace(scanner.Text())
-		i := strings.Index(t, ":")
-		if i < 0 || i >= len(t) {
-			context.GetLogger(ctx).Errorf("bad entry in htpasswd: %q", t)
+
+		if len(t) < 1 {
 			continue
 		}
 
-		entries = append(entries, htpasswdEntry{
-			username: t[:i],
-			password: t[i+1:],
-		})
+		// lines that *begin* with a '#' are considered comments
+		if t[0] == '#' {
+			continue
+		}
+
+		i := strings.Index(t, ":")
+		if i < 0 || i >= len(t) {
+			return nil, fmt.Errorf("htpasswd: invalid entry at line %d: %q", line, scanner.Text())
+		}
+
+		entries[t[:i]] = []byte(t[i+1:])
 	}
 
-	return entries
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	return entries, nil
 }

docs/auth/basic/htpasswd_test.go

@@ -0,0 +1,85 @@
+package basic
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+)
+
+func TestParseHTPasswd(t *testing.T) {
+
+	for _, tc := range []struct {
+		desc    string
+		input   string
+		err     error
+		entries map[string][]byte
+	}{
+		{
+			desc: "basic example",
+			input: `
+# This is a comment in a basic example.
+bilbo:{SHA}5siv5c0SHx681xU6GiSx9ZQryqs=
+frodo:$2y$05$926C3y10Quzn/LnqQH86VOEVh/18T6RnLaS.khre96jLNL/7e.K5W
+MiShil:$2y$05$0oHgwMehvoe8iAWS8I.7l.KoECXrwVaC16RPfaSCU5eVTFrATuMI2
+DeokMan:공주님
+`,
+			entries: map[string][]byte{
+				"bilbo":   []byte("{SHA}5siv5c0SHx681xU6GiSx9ZQryqs="),
+				"frodo":   []byte("$2y$05$926C3y10Quzn/LnqQH86VOEVh/18T6RnLaS.khre96jLNL/7e.K5W"),
+				"MiShil":  []byte("$2y$05$0oHgwMehvoe8iAWS8I.7l.KoECXrwVaC16RPfaSCU5eVTFrATuMI2"),
+				"DeokMan": []byte("공주님"),
+			},
+		},
+		{
+			desc: "ensures comments are filtered",
+			input: `
+# asdf:asdf
+`,
+		},
+		{
+			desc: "ensure midline hash is not comment",
+			input: `
+asdf:as#df
+`,
+			entries: map[string][]byte{
+				"asdf": []byte("as#df"),
+			},
+		},
+		{
+			desc: "ensure midline hash is not comment",
+			input: `
+# A valid comment
+valid:entry
+asdf
+`,
+			err: fmt.Errorf(`htpasswd: invalid entry at line 4: "asdf"`),
+		},
+	} {
+
+		entries, err := parseHTPasswd(strings.NewReader(tc.input))
+		if err != tc.err {
+			if tc.err == nil {
+				t.Fatalf("%s: unexpected error: %v", tc.desc, err)
+			} else {
+				if err.Error() != tc.err.Error() { // use string equality here.
+					t.Fatalf("%s: expected error not returned: %v != %v", tc.desc, err, tc.err)
+				}
+			}
+		}
+
+		if tc.err != nil {
+			continue // don't test output
+		}
+
+		// allow empty and nil to be equal
+		if tc.entries == nil {
+			tc.entries = map[string][]byte{}
+		}
+
+		if !reflect.DeepEqual(entries, tc.entries) {
+			t.Fatalf("%s: entries not parsed correctly: %v != %v", tc.desc, entries, tc.entries)
+		}
+	}
+
+}

docs/handlers/app.go

@@ -147,6 +147,7 @@ func NewApp(ctx context.Context, configuration configuration.Configuration) *App
 			panic(fmt.Sprintf("unable to configure authorization (%s): %v", authType, err))
 		}
 		app.accessController = accessController
+		ctxu.GetLogger(app).Debugf("configured %q access controller", authType)
 	}
 
 	return app