From ffd36629821be22da7a468d57958e2ca70f256e8 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 10 Jun 2015 19:29:27 -0700 Subject: [PATCH] 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 --- registry/auth/basic/access.go | 16 ++- registry/auth/basic/access_test.go | 20 +++- registry/auth/basic/htpasswd.go | 166 ++++++++------------------- registry/auth/basic/htpasswd_test.go | 85 ++++++++++++++ registry/handlers/app.go | 1 + 5 files changed, 164 insertions(+), 124 deletions(-) create mode 100644 registry/auth/basic/htpasswd_test.go diff --git a/registry/auth/basic/access.go b/registry/auth/basic/access.go index 11e4ae5a9..f7d5e79b7 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 3bc994373..1976b32e2 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 f50805e78..dd9bb1acf 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 000000000..5cc861264 --- /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 2f37aa530..08c1c004d 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