Refactor Basic Authentication package

This change refactors the basic authentication implementation to better follow
Go coding standards. Many types are no longer exported. The parser is now a
separate function from the authentication code. The standard functions
(*http.Request).BasicAuth/SetBasicAuth are now used where appropriate.

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

cmd/registry/config.yml

@@ -26,10 +26,6 @@ storage:
     maintenance:
         uploadpurging:
             enabled: false
-auth:
-    basic:
-       realm: test-realm
-       path: /tmp/registry-dev/.htpasswd
 http:
     addr: :5000
     secret: asecretforlocaldevelopment

registry/auth/basic/access.go

@@ -15,23 +15,20 @@ import (
 	"golang.org/x/net/context"
 )
 
+var (
+	// ErrInvalidCredential is returned when the auth token does not authenticate correctly.
+	ErrInvalidCredential = errors.New("invalid authorization credential")
+
+	// ErrAuthenticationFailure returned when authentication failure to be presented to agent.
+	ErrAuthenticationFailure = errors.New("authentication failured")
+)
+
 type accessController struct {
 	realm    string
 	htpasswd *htpasswd
 }
 
-type challenge struct {
-	realm string
-	err   error
-}
-
 var _ auth.AccessController = &accessController{}
-var (
-	// ErrPasswordRequired Returned when no auth token is given.
-	ErrPasswordRequired = errors.New("authorization credential required")
-	// ErrInvalidCredential is returned when the auth token does not authenticate correctly.
-	ErrInvalidCredential = errors.New("invalid authorization credential")
-)
 
 func newAccessController(options map[string]interface{}) (auth.AccessController, error) {
 	realm, present := options["realm"]
@@ -53,28 +50,29 @@ func (ac *accessController) Authorized(ctx context.Context, accessRecords ...aut
 		return nil, err
 	}
 
-	authHeader := req.Header.Get("Authorization")
-	if authHeader == "" {
-		challenge := challenge{
-			realm: ac.realm,
-		}
-		return nil, &challenge
-	}
-
-	user, pass, ok := req.BasicAuth()
+	username, password, ok := req.BasicAuth()
 	if !ok {
-		return nil, errors.New("Invalid Authorization header")
-	}
-
-	if res, _ := ac.htpasswd.AuthenticateUser(user, pass); !res {
-		challenge := challenge{
+		return nil, &challenge{
 			realm: ac.realm,
+			err:   ErrInvalidCredential,
 		}
-		challenge.err = ErrInvalidCredential
-		return nil, &challenge
 	}
 
-	return auth.WithUser(ctx, auth.UserInfo{Name: user}), nil
+	if err := ac.htpasswd.authenticateUser(ctx, username, password); err != nil {
+		ctxu.GetLogger(ctx).Errorf("error authenticating user %q: %v", username, err)
+		return nil, &challenge{
+			realm: ac.realm,
+			err:   ErrAuthenticationFailure,
+		}
+	}
+
+	return auth.WithUser(ctx, auth.UserInfo{Name: username}), nil
+}
+
+// challenge implements the auth.Challenge interface.
+type challenge struct {
+	realm string
+	err   error
 }
 
 func (ch *challenge) ServeHTTP(w http.ResponseWriter, r *http.Request) {

registry/auth/basic/access_test.go

@@ -1,14 +1,13 @@
 package basic
 
 import (
-	"encoding/base64"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
+	"github.com/docker/distribution/context"
 	"github.com/docker/distribution/registry/auth"
-	"golang.org/x/net/context"
 )
 
 func TestBasicAccessController(t *testing.T) {
@@ -33,6 +32,7 @@ func TestBasicAccessController(t *testing.T) {
 		"realm": testRealm,
 		"path":  tempFile.Name(),
 	}
+	ctx := context.Background()
 
 	accessController, err := newAccessController(options)
 	if err != nil {
@@ -44,7 +44,7 @@ func TestBasicAccessController(t *testing.T) {
 	var userNumber = 0
 
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		ctx := context.WithValue(nil, "http.request", r)
+		ctx := context.WithRequest(ctx, r)
 		authCtx, err := accessController.Authorized(ctx)
 		if err != nil {
 			switch err := err.(type) {
@@ -87,13 +87,14 @@ func TestBasicAccessController(t *testing.T) {
 
 	for i := 0; i < len(testUsers); i++ {
 		userNumber = i
-		req, _ = http.NewRequest("GET", server.URL, nil)
-		sekrit := testUsers[i] + ":" + testPasswords[i]
-		credential := "Basic " + base64.StdEncoding.EncodeToString([]byte(sekrit))
+		req, err := http.NewRequest("GET", server.URL, nil)
+		if err != nil {
+			t.Fatalf("error allocating new request: %v", err)
+		}
+
+		req.SetBasicAuth(testUsers[i], testPasswords[i])
 
-		req.Header.Set("Authorization", credential)
 		resp, err = client.Do(req)
-
 		if err != nil {
 			t.Fatalf("unexpected error during GET: %v", err)
 		}
@@ -101,7 +102,7 @@ func TestBasicAccessController(t *testing.T) {
 
 		// Request should be authorized
 		if resp.StatusCode != http.StatusNoContent {
-			t.Fatalf("unexpected non-success response status: %v != %v for %s %s %s", resp.StatusCode, http.StatusNoContent, testUsers[i], testPasswords[i], credential)
+			t.Fatalf("unexpected non-success response status: %v != %v for %s %s", resp.StatusCode, http.StatusNoContent, testUsers[i], testPasswords[i])
 		}
 	}
 

registry/auth/basic/htpasswd.go

@@ -1,54 +1,66 @@
 package basic
 
 import (
+	"bufio"
 	"crypto/sha1"
 	"encoding/base64"
-	"encoding/csv"
-	"errors"
+	"io"
 	"os"
 	"regexp"
 	"strings"
 
+	"github.com/docker/distribution/context"
 	"golang.org/x/crypto/bcrypt"
 )
 
-// ErrAuthenticationFailure A generic error message for authentication failure to be presented to agent.
-var ErrAuthenticationFailure = errors.New("Bad username or password")
-
-// 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.
 type htpasswd struct {
 	path string
-	reader *csv.Reader
 }
 
-// AuthType Represents a particular hash function used in the htpasswd file.
-type AuthType int
+// authType represents a particular hash function used in the htpasswd file.
+type authType int
 
 const (
-	// PlainText Plain-text password storage (htpasswd -p)
-	PlainText AuthType = iota
-	// SHA1 sha hashed password storage (htpasswd -s)
-	SHA1
-	// ApacheMD5 apr iterated md5 hashing (htpasswd -m)
-	ApacheMD5
-	// BCrypt BCrypt adapative password hashing (htpasswd -B)
-	BCrypt
-	// Crypt System crypt() hashes.  (htpasswd -d)
-	Crypt
+	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
+	}
+	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 {
+func (at authType) String() string {
 	switch at {
-	case PlainText:
+	case authTypePlainText:
 		return "plaintext"
-	case SHA1:
+	case authTypeSHA1:
 		return "sha1"
-	case ApacheMD5:
+	case authTypeApacheMD5:
 		return "md5"
-	case BCrypt:
+	case authTypeBCrypt:
 		return "bcrypt"
-	case Crypt:
+	case authTypeCrypt:
 		return "system crypt"
 	}
 	return "unknown"
@@ -59,83 +71,80 @@ func newHTPasswd(htpath string) *htpasswd {
 	return &htpasswd{path: htpath}
 }
 
-var bcryptPrefixRegexp = regexp.MustCompile(`^\$2[ab]?y\$`)
-
-// GetAuthCredentialType Inspect an htpasswd file credential and guess the encryption algorithm used.
-func GetAuthCredentialType(cred string) AuthType {
-	if strings.HasPrefix(cred, "{SHA}") {
-		return SHA1
-	}
-	if strings.HasPrefix(cred, "$apr1$") {
-		return ApacheMD5
-	}
-	if bcryptPrefixRegexp.MatchString(cred) {
-		return BCrypt
-	}
-	// There's just not a great way to distinguish between these next two...
-	if len(cred) == 13 {
-		return Crypt
-	}
-	return PlainText
-}
-
-// AuthenticateUser Check a given user:password credential against the receiving HTPasswd's file.
-func (htpasswd *htpasswd) AuthenticateUser(user string, pwd string) (bool, error) {
-
+// 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)
 	if err != nil {
-		return false, err
+		return err
+	}
+	defer in.Close()
+
+	for _, entry := range parseHTPasswd(ctx, in) {
+		if entry.username != username {
+			continue // wrong entry
 		}
 
-	// Parse the contents of the standard .htpasswd until we hit the end or find a match.
-	reader := csv.NewReader(in)
-	reader.Comma = ':'
-	reader.Comment = '#'
-	reader.TrimLeadingSpace = true
-	for entry, readerr := reader.Read(); entry != nil || readerr != nil; entry, readerr = reader.Read() {
-		if readerr != nil {
-			return false, readerr
+		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
 			}
-		if len(entry) == 0 {
+
+			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
+}
+
+// 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{}
+	scanner := bufio.NewScanner(rd)
+	for scanner.Scan() {
+		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)
 			continue
 		}
-		if entry[0] == user {
-			credential := entry[1]
-			credType := GetAuthCredentialType(credential)
-			switch credType {
-			case SHA1:
-				{
-					sha := sha1.New()
-					sha.Write([]byte(pwd))
-					hash := base64.StdEncoding.EncodeToString(sha.Sum(nil))
-					return entry[1][5:] == hash, nil
+
+		entries = append(entries, htpasswdEntry{
+			username: t[:i],
+			password: t[i+1:],
+		})
 	}
-			case ApacheMD5:
-				{
-					return false, errors.New(ApacheMD5.String() + " htpasswd hash function not yet supported")
-				}
-			case BCrypt:
-				{
-					err := bcrypt.CompareHashAndPassword([]byte(credential), []byte(pwd))
-					if err != nil {
-						return false, err
-					}
-					return true, nil
-				}
-			case Crypt:
-				{
-					return false, errors.New(Crypt.String() + " htpasswd hash function not yet supported")
-				}
-			case PlainText:
-				{
-					if pwd == credential {
-						return true, nil
-					}
-					return false, ErrAuthenticationFailure
-				}
-			}
-		}
-	}
-	return false, ErrAuthenticationFailure
+
+	return entries
 }