From 427c457801637e3d5b8a0e1f88cc1095a3f193c9 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 8 Jun 2015 18:56:48 -0700 Subject: [PATCH] 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 --- docs/auth/basic/access.go | 54 +++++---- docs/auth/basic/access_test.go | 19 +-- docs/auth/basic/htpasswd.go | 203 +++++++++++++++++---------------- 3 files changed, 142 insertions(+), 134 deletions(-) diff --git a/docs/auth/basic/access.go b/docs/auth/basic/access.go index 24f4009f..11e4ae5a 100644 --- a/docs/auth/basic/access.go +++ b/docs/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) { diff --git a/docs/auth/basic/access_test.go b/docs/auth/basic/access_test.go index 62699a63..3bc99437 100644 --- a/docs/auth/basic/access_test.go +++ b/docs/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]) } } diff --git a/docs/auth/basic/htpasswd.go b/docs/auth/basic/htpasswd.go index cc305ff1..f50805e7 100644 --- a/docs/auth/basic/htpasswd.go +++ b/docs/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 + path string } -// 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 + } + + 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) + } } - // 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 - } - if len(entry) == 0 { + 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 - } - 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 - } - } - } + + entries = append(entries, htpasswdEntry{ + username: t[:i], + password: t[i+1:], + }) } - return false, ErrAuthenticationFailure + + return entries }