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>
This commit is contained in:
Stephen J Day 2015-06-10 19:29:27 -07:00
parent ffe56ebe41
commit ffd3662982
5 changed files with 164 additions and 124 deletions

View file

@ -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,

View file

@ -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])
}
}
}

View file

@ -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
}

View file

@ -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)
}
}
}

View file

@ -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