From 18169e896cf654d9f8e0cd8354c791047c0e5d41 Mon Sep 17 00:00:00 2001 From: Yicheng Qin Date: Tue, 4 Aug 2015 14:07:39 -0700 Subject: [PATCH] etcdhttp: fix access check for multiple roles in auth Check access for multiple roles should go through all roles. --- etcdserver/etcdhttp/client_auth.go | 7 +- etcdserver/etcdhttp/client_auth_test.go | 142 ++++++++++++++++-------- 2 files changed, 101 insertions(+), 48 deletions(-) diff --git a/etcdserver/etcdhttp/client_auth.go b/etcdserver/etcdhttp/client_auth.go index c1a34aebb..99a0d0647 100644 --- a/etcdserver/etcdhttp/client_auth.go +++ b/etcdserver/etcdhttp/client_auth.go @@ -97,9 +97,12 @@ func hasKeyPrefixAccess(sec auth.Store, r *http.Request, key string, recursive b continue } if recursive { - return role.HasRecursiveAccess(key, writeAccess) + if role.HasRecursiveAccess(key, writeAccess) { + return true + } + } else if role.HasKeyAccess(key, writeAccess) { + return true } - return role.HasKeyAccess(key, writeAccess) } plog.Warningf("auth: invalid access for user %s on key %s.", username, key) return false diff --git a/etcdserver/etcdhttp/client_auth_test.go b/etcdserver/etcdhttp/client_auth_test.go index d7cf4accf..b9890527b 100644 --- a/etcdserver/etcdhttp/client_auth_test.go +++ b/etcdserver/etcdhttp/client_auth_test.go @@ -38,7 +38,7 @@ func mustJSONRequest(t *testing.T, method string, p string, body string) *http.R type mockAuthStore struct { user *auth.User - role *auth.Role + roles map[string]*auth.Role err error enabled bool } @@ -59,13 +59,15 @@ func (s *mockAuthStore) UpdateUser(user auth.User) (auth.User, error) { return * func (s *mockAuthStore) AllRoles() ([]string, error) { return []string{"awesome", "guest", "root"}, s.err } -func (s *mockAuthStore) GetRole(name string) (auth.Role, error) { return *s.role, s.err } -func (s *mockAuthStore) CreateRole(role auth.Role) error { return s.err } -func (s *mockAuthStore) DeleteRole(name string) error { return s.err } -func (s *mockAuthStore) UpdateRole(role auth.Role) (auth.Role, error) { return *s.role, s.err } -func (s *mockAuthStore) AuthEnabled() bool { return s.enabled } -func (s *mockAuthStore) EnableAuth() error { return s.err } -func (s *mockAuthStore) DisableAuth() error { return s.err } +func (s *mockAuthStore) GetRole(name string) (auth.Role, error) { return *s.roles[name], s.err } +func (s *mockAuthStore) CreateRole(role auth.Role) error { return s.err } +func (s *mockAuthStore) DeleteRole(name string) error { return s.err } +func (s *mockAuthStore) UpdateRole(role auth.Role) (auth.Role, error) { + return *s.roles[role.Role], s.err +} +func (s *mockAuthStore) AuthEnabled() bool { return s.enabled } +func (s *mockAuthStore) EnableAuth() error { return s.err } +func (s *mockAuthStore) DisableAuth() error { return s.err } func TestAuthFlow(t *testing.T) { enableMapMu.Lock() @@ -159,8 +161,10 @@ func TestAuthFlow(t *testing.T) { { req: mustJSONRequest(t, "GET", "roles/manager", ""), store: mockAuthStore{ - role: &auth.Role{ - Role: "manager", + roles: map[string]*auth.Role{ + "manager": { + Role: "manager", + }, }, }, wcode: http.StatusOK, @@ -181,8 +185,10 @@ func TestAuthFlow(t *testing.T) { { req: mustJSONRequest(t, "PUT", "roles/manager", `{"role":"manager","revoke":{"kv":{"read":["foo"],"write":[]}}}`), store: mockAuthStore{ - role: &auth.Role{ - Role: "manager", + roles: map[string]*auth.Role{ + "manager": { + Role: "manager", + }, }, }, wcode: http.StatusOK, @@ -223,8 +229,10 @@ func TestAuthFlow(t *testing.T) { Password: goodPassword, Roles: []string{"root"}, }, - role: &auth.Role{ - Role: "root", + roles: map[string]*auth.Role{ + "root": { + Role: "root", + }, }, }, wcode: http.StatusOK, @@ -279,8 +287,10 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"root"}, }, - role: &auth.Role{ - Role: "root", + roles: map[string]*auth.Role{ + "root": { + Role: "root", + }, }, enabled: true, }, @@ -297,12 +307,14 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"foorole"}, }, - role: &auth.Role{ - Role: "foorole", - Permissions: auth.Permissions{ - KV: auth.RWPermission{ - Read: []string{"/foo"}, - Write: []string{"/foo"}, + roles: map[string]*auth.Role{ + "foorole": { + Role: "foorole", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo"}, + Write: []string{"/foo"}, + }, }, }, }, @@ -321,12 +333,14 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"foorole"}, }, - role: &auth.Role{ - Role: "foorole", - Permissions: auth.Permissions{ - KV: auth.RWPermission{ - Read: []string{"/foo*"}, - Write: []string{"/foo*"}, + roles: map[string]*auth.Role{ + "foorole": { + Role: "foorole", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo*"}, + Write: []string{"/foo*"}, + }, }, }, }, @@ -345,12 +359,14 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"foorole"}, }, - role: &auth.Role{ - Role: "foorole", - Permissions: auth.Permissions{ - KV: auth.RWPermission{ - Read: []string{"/foo*"}, - Write: []string{"/foo*"}, + roles: map[string]*auth.Role{ + "foorole": { + Role: "foorole", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo*"}, + Write: []string{"/foo*"}, + }, }, }, }, @@ -381,12 +397,14 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"foorole"}, }, - role: &auth.Role{ - Role: "guest", - Permissions: auth.Permissions{ - KV: auth.RWPermission{ - Read: []string{"/foo*"}, - Write: []string{"/foo*"}, + roles: map[string]*auth.Role{ + "guest": { + Role: "guest", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo*"}, + Write: []string{"/foo*"}, + }, }, }, }, @@ -405,12 +423,14 @@ func TestPrefixAccess(t *testing.T) { Password: goodPassword, Roles: []string{"foorole"}, }, - role: &auth.Role{ - Role: "guest", - Permissions: auth.Permissions{ - KV: auth.RWPermission{ - Read: []string{"/foo*"}, - Write: []string{"/foo*"}, + roles: map[string]*auth.Role{ + "guest": { + Role: "guest", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo*"}, + Write: []string{"/foo*"}, + }, }, }, }, @@ -420,6 +440,36 @@ func TestPrefixAccess(t *testing.T) { hasKeyPrefixAccess: false, hasRecursiveAccess: false, }, + // check access for multiple roles + { + key: "/foo", + req: mustAuthRequest("GET", "user", "good"), + store: &mockAuthStore{ + user: &auth.User{ + User: "user", + Password: goodPassword, + Roles: []string{"role1", "role2"}, + }, + roles: map[string]*auth.Role{ + "role1": { + Role: "role1", + }, + "role2": { + Role: "role2", + Permissions: auth.Permissions{ + KV: auth.RWPermission{ + Read: []string{"/foo"}, + Write: []string{"/foo"}, + }, + }, + }, + }, + enabled: true, + }, + hasRoot: false, + hasKeyPrefixAccess: true, + hasRecursiveAccess: false, + }, } for i, tt := range table {