diff --git a/registry/auth/basic/access.go b/registry/auth/basic/access.go index 11e4ae5a..f7d5e79b 100644 --- a/registry/auth/basic/access.go +++ b/registry/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, diff --git a/registry/auth/basic/access_test.go b/registry/auth/basic/access_test.go index 3bc99437..1976b32e 100644 --- a/registry/auth/basic/access_test.go +++ b/registry/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]) + } } } diff --git a/registry/auth/basic/htpasswd.go b/registry/auth/basic/htpasswd.go index f50805e7..dd9bb1ac 100644 --- a/registry/auth/basic/htpasswd.go +++ b/registry/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 } diff --git a/registry/auth/basic/htpasswd_test.go b/registry/auth/basic/htpasswd_test.go new file mode 100644 index 00000000..5cc86126 --- /dev/null +++ b/registry/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) + } + } + +} diff --git a/registry/handlers/app.go b/registry/handlers/app.go index 2f37aa53..08c1c004 100644 --- a/registry/handlers/app.go +++ b/registry/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