From ab17165352a9b04ac69c158efe7fdac20197cdc0 Mon Sep 17 00:00:00 2001 From: rob boll Date: Mon, 23 Nov 2015 16:10:08 -0500 Subject: [PATCH] v2http: refactor http basic auth refactor http basic auth code to combine basic auth extraction and validation --- etcdserver/api/v2http/client_auth.go | 54 ++++++++--------- etcdserver/api/v2http/client_auth_test.go | 71 +++++++++++++++++++++++ 2 files changed, 99 insertions(+), 26 deletions(-) diff --git a/etcdserver/api/v2http/client_auth.go b/etcdserver/api/v2http/client_auth.go index cf1585bed..0907c438a 100644 --- a/etcdserver/api/v2http/client_auth.go +++ b/etcdserver/api/v2http/client_auth.go @@ -37,6 +37,25 @@ func hasWriteRootAccess(sec auth.Store, r *http.Request) bool { return hasRootAccess(sec, r) } +func userFromBasicAuth(sec auth.Store, r *http.Request) *auth.User { + username, password, ok := r.BasicAuth() + if !ok { + plog.Warningf("auth: malformed basic auth encoding") + return nil + } + user, err := sec.GetUser(username) + if err != nil { + return nil + } + + ok = sec.CheckPassword(user, password) + if !ok { + plog.Warningf("auth: incorrect password for user: %s", username) + return nil + } + return &user +} + func hasRootAccess(sec auth.Store, r *http.Request) bool { if sec == nil { // No store means no auth available, eg, tests. @@ -45,26 +64,18 @@ func hasRootAccess(sec auth.Store, r *http.Request) bool { if !sec.AuthEnabled() { return true } - username, password, ok := r.BasicAuth() - if !ok { - return false - } - rootUser, err := sec.GetUser(username) - if err != nil { + + rootUser := userFromBasicAuth(sec, r) + if rootUser == nil { return false } - ok = sec.CheckPassword(rootUser, password) - if !ok { - plog.Warningf("auth: wrong password for user %s", username) - return false - } for _, role := range rootUser.Roles { if role == auth.RootRoleName { return true } } - plog.Warningf("auth: user %s does not have the %s role for resource %s.", username, auth.RootRoleName, r.URL.Path) + plog.Warningf("auth: user %s does not have the %s role for resource %s.", rootUser.User, auth.RootRoleName, r.URL.Path) return false } @@ -80,21 +91,12 @@ func hasKeyPrefixAccess(sec auth.Store, r *http.Request, key string, recursive b plog.Warningf("auth: no authorization provided, checking guest access") return hasGuestAccess(sec, r, key) } - username, password, ok := r.BasicAuth() - if !ok { - plog.Warningf("auth: malformed basic auth encoding") - return false - } - user, err := sec.GetUser(username) - if err != nil { - plog.Warningf("auth: no such user: %s.", username) - return false - } - authAsUser := sec.CheckPassword(user, password) - if !authAsUser { - plog.Warningf("auth: incorrect password for user: %s.", username) + + user := userFromBasicAuth(sec, r) + if user == nil { return false } + writeAccess := r.Method != "GET" && r.Method != "HEAD" for _, roleName := range user.Roles { role, err := sec.GetRole(roleName) @@ -109,7 +111,7 @@ func hasKeyPrefixAccess(sec auth.Store, r *http.Request, key string, recursive b return true } } - plog.Warningf("auth: invalid access for user %s on key %s.", username, key) + plog.Warningf("auth: invalid access for user %s on key %s.", user.User, key) return false } diff --git a/etcdserver/api/v2http/client_auth_test.go b/etcdserver/api/v2http/client_auth_test.go index 734e19ac0..5afbd750b 100644 --- a/etcdserver/api/v2http/client_auth_test.go +++ b/etcdserver/api/v2http/client_auth_test.go @@ -440,6 +440,14 @@ func mustAuthRequest(method, username, password string) *http.Request { return req } +func unauthedRequest(method string) *http.Request { + req, err := http.NewRequest(method, "path", strings.NewReader("")) + if err != nil { + panic("Cannot make request: " + err.Error()) + } + return req +} + func TestPrefixAccess(t *testing.T) { var table = []struct { key string @@ -701,3 +709,66 @@ func TestPrefixAccess(t *testing.T) { } } } + +func TestUserFromBasicAuth(t *testing.T) { + sec := &mockAuthStore{ + users: map[string]*auth.User{ + "user": { + User: "user", + Roles: []string{"root"}, + Password: "password", + }, + }, + roles: map[string]*auth.Role{ + "root": { + Role: "root", + }, + }, + } + + var table = []struct { + username string + req *http.Request + userExists bool + }{ + { + // valid user, valid pass + username: "user", + req: mustAuthRequest("GET", "user", "password"), + userExists: true, + }, + { + // valid user, bad pass + username: "user", + req: mustAuthRequest("GET", "user", "badpass"), + userExists: false, + }, + { + // valid user, no pass + username: "user", + req: mustAuthRequest("GET", "user", ""), + userExists: false, + }, + { + // missing user + username: "missing", + req: mustAuthRequest("GET", "missing", "badpass"), + userExists: false, + }, + { + // no basic auth + req: unauthedRequest("GET"), + userExists: false, + }, + } + + for i, tt := range table { + user := userFromBasicAuth(sec, tt.req) + if tt.userExists == (user == nil) { + t.Errorf("#%d: userFromBasicAuth doesn't match (expected %v)", i, tt.userExists) + } + if user != nil && (tt.username != user.User) { + t.Errorf("#%d: userFromBasicAuth username doesn't match (expected %s, got %s)", i, tt.username, user.User) + } + } +}