From a88a53274fd4b8e04c44d05efe258d0b66390de9 Mon Sep 17 00:00:00 2001 From: Barak Michener Date: Mon, 27 Apr 2015 17:01:55 -0400 Subject: [PATCH] security: Lazily create the security directories. Fixes #2755, may find new instances for #2741 revert the kv integration test fix nits amend security mention of GUEST --- Documentation/security_api.md | 2 +- etcdserver/security/security.go | 31 ++++++------ etcdserver/security/security_requests.go | 26 +++++++++- etcdserver/security/security_test.go | 2 +- integration/v2_http_kv_test.go | 64 ++++++++++++------------ 5 files changed, 74 insertions(+), 51 deletions(-) diff --git a/Documentation/security_api.md b/Documentation/security_api.md index 3e28c29b1..ccb63ac84 100644 --- a/Documentation/security_api.md +++ b/Documentation/security_api.md @@ -19,7 +19,7 @@ Each role has exact one associated Permission List. An permission list exists fo The special static ROOT (named `root`) role has a full permissions on all key-value resources, the permission to manage user resources and settings resources. Only the ROOT role has the permission to manage user resources and modify settings resources. The ROOT role is built-in and does not need to be created. -There is also a special GUEST role, named 'guest'. These are the permissions given to unauthenticated requests to etcd. This role will be created when security is enabled, unless it already exists, and by default allows access to the full keyspace due to backward compatibility. (etcd did not previously authenticate any actions.). This role can be modified by a ROOT role holder at any time. +There is also a special GUEST role, named 'guest'. These are the permissions given to unauthenticated requests to etcd. This role will be created automatically, and by default allows access to the full keyspace due to backward compatability. (etcd did not previously authenticate any actions.). This role can be modified by a ROOT role holder at any time, to reduce the capabilities of unauthenticated users. #### Permissions diff --git a/etcdserver/security/security.go b/etcdserver/security/security.go index c0877a06b..9ab9991db 100644 --- a/etcdserver/security/security.go +++ b/etcdserver/security/security.go @@ -68,9 +68,9 @@ type doer interface { } type Store struct { - server doer - timeout time.Duration - enabled bool + server doer + timeout time.Duration + ensuredOnce bool } type User struct { @@ -116,14 +116,17 @@ func NewStore(server doer, timeout time.Duration) *Store { server: server, timeout: timeout, } - s.ensureSecurityDirectories() - s.enabled = s.detectSecurity() return s } func (s *Store) AllUsers() ([]string, error) { resp, err := s.requestResource("/users/", false) if err != nil { + if e, ok := err.(*etcderr.Error); ok { + if e.ErrorCode == etcderr.EcodeKeyNotFound { + return []string{}, nil + } + } return nil, err } var nodes []string @@ -239,16 +242,20 @@ func (s *Store) UpdateUser(user User) (User, error) { } func (s *Store) AllRoles() ([]string, error) { + nodes := []string{GuestRoleName, RootRoleName} resp, err := s.requestResource("/roles/", false) if err != nil { + if e, ok := err.(*etcderr.Error); ok { + if e.ErrorCode == etcderr.EcodeKeyNotFound { + return nodes, nil + } + } return nil, err } - var nodes []string for _, n := range resp.Event.Node.Nodes { _, role := path.Split(n.Key) nodes = append(nodes, role) } - nodes = append(nodes, RootRoleName) sort.Strings(nodes) return nodes, nil } @@ -349,18 +356,14 @@ func (s *Store) UpdateRole(role Role) (Role, error) { } func (s *Store) SecurityEnabled() bool { - return s.enabled + return s.detectSecurity() } func (s *Store) EnableSecurity() error { if s.SecurityEnabled() { return securityErr("already enabled") } - err := s.ensureSecurityDirectories() - if err != nil { - return err - } - _, err = s.GetUser("root") + _, err := s.GetUser("root") if err != nil { return securityErr("No root user available, please create one") } @@ -375,7 +378,6 @@ func (s *Store) EnableSecurity() error { } err = s.enableSecurity() if err == nil { - s.enabled = true log.Printf("security: enabled security") } else { log.Printf("error enabling security: %v", err) @@ -389,7 +391,6 @@ func (s *Store) DisableSecurity() error { } err := s.disableSecurity() if err == nil { - s.enabled = false log.Printf("security: disabled security") } else { log.Printf("error disabling security: %v", err) diff --git a/etcdserver/security/security_requests.go b/etcdserver/security/security_requests.go index 7e8da4ddd..42e59f735 100644 --- a/etcdserver/security/security_requests.go +++ b/etcdserver/security/security_requests.go @@ -26,6 +26,9 @@ import ( ) func (s *Store) ensureSecurityDirectories() error { + if s.ensuredOnce { + return nil + } for _, res := range []string{StorePermsPrefix, StorePermsPrefix + "/users/", StorePermsPrefix + "/roles/"} { ctx, cancel := context.WithTimeout(context.Background(), s.timeout) defer cancel() @@ -47,15 +50,26 @@ func (s *Store) ensureSecurityDirectories() error { return err } } - _, err := s.createResource("/enabled", false) + ctx, cancel := context.WithTimeout(context.Background(), s.timeout) + defer cancel() + pe := false + rr := etcdserverpb.Request{ + Method: "PUT", + Path: StorePermsPrefix + "/enabled", + Val: "false", + PrevExist: &pe, + } + _, err := s.server.Do(ctx, rr) if err != nil { if e, ok := err.(*etcderr.Error); ok { if e.ErrorCode == etcderr.EcodeNodeExist { + s.ensuredOnce = true return nil } } return err } + s.ensuredOnce = true return nil } @@ -75,7 +89,7 @@ func (s *Store) detectSecurity() bool { value, err := s.requestResource("/enabled", false) if err != nil { if e, ok := err.(*etcderr.Error); ok { - if e.ErrorCode == etcderr.EcodeNodeExist { + if e.ErrorCode == etcderr.EcodeKeyNotFound { return false } } @@ -111,6 +125,10 @@ func (s *Store) createResource(res string, value interface{}) (etcdserver.Respon return s.setResource(res, value, false) } func (s *Store) setResource(res string, value interface{}, prevexist bool) (etcdserver.Response, error) { + err := s.ensureSecurityDirectories() + if err != nil { + return etcdserver.Response{}, err + } ctx, cancel := context.WithTimeout(context.Background(), s.timeout) defer cancel() data, err := json.Marshal(value) @@ -128,6 +146,10 @@ func (s *Store) setResource(res string, value interface{}, prevexist bool) (etcd } func (s *Store) deleteResource(res string) (etcdserver.Response, error) { + err := s.ensureSecurityDirectories() + if err != nil { + return etcdserver.Response{}, err + } ctx, cancel := context.WithTimeout(context.Background(), s.timeout) defer cancel() pex := true diff --git a/etcdserver/security/security_test.go b/etcdserver/security/security_test.go index 1c7fc55d9..e5e33567d 100644 --- a/etcdserver/security/security_test.go +++ b/etcdserver/security/security_test.go @@ -236,7 +236,7 @@ func TestAllRoles(t *testing.T) { }, }, } - expected := []string{"animal", "human", "root"} + expected := []string{"animal", "guest", "human", "root"} s := Store{d, time.Second, false} out, err := s.AllRoles() diff --git a/integration/v2_http_kv_test.go b/integration/v2_http_kv_test.go index 3bc6b313c..596c0ff46 100644 --- a/integration/v2_http_kv_test.go +++ b/integration/v2_http_kv_test.go @@ -53,19 +53,19 @@ func TestV2Set(t *testing.T) { "/v2/keys/foo/bar", v, http.StatusCreated, - `{"action":"set","node":{"key":"/foo/bar","value":"bar","modifiedIndex":8,"createdIndex":8}}`, + `{"action":"set","node":{"key":"/foo/bar","value":"bar","modifiedIndex":4,"createdIndex":4}}`, }, { "/v2/keys/foodir?dir=true", url.Values{}, http.StatusCreated, - `{"action":"set","node":{"key":"/foodir","dir":true,"modifiedIndex":9,"createdIndex":9}}`, + `{"action":"set","node":{"key":"/foodir","dir":true,"modifiedIndex":5,"createdIndex":5}}`, }, { "/v2/keys/fooempty", url.Values(map[string][]string{"value": {""}}), http.StatusCreated, - `{"action":"set","node":{"key":"/fooempty","value":"","modifiedIndex":10,"createdIndex":10}}`, + `{"action":"set","node":{"key":"/fooempty","value":"","modifiedIndex":6,"createdIndex":6}}`, }, } @@ -214,12 +214,12 @@ func TestV2CAS(t *testing.T) { }, { "/v2/keys/cas/foo", - url.Values(map[string][]string{"value": {"YYY"}, "prevIndex": {"8"}}), + url.Values(map[string][]string{"value": {"YYY"}, "prevIndex": {"4"}}), http.StatusOK, map[string]interface{}{ "node": map[string]interface{}{ "value": "YYY", - "modifiedIndex": float64(9), + "modifiedIndex": float64(5), }, "action": "compareAndSwap", }, @@ -231,8 +231,8 @@ func TestV2CAS(t *testing.T) { map[string]interface{}{ "errorCode": float64(101), "message": "Compare failed", - "cause": "[10 != 9]", - "index": float64(9), + "cause": "[10 != 5]", + "index": float64(5), }, }, { @@ -281,7 +281,7 @@ func TestV2CAS(t *testing.T) { map[string]interface{}{ "errorCode": float64(101), "message": "Compare failed", - "cause": "[bad_value != ZZZ] [100 != 10]", + "cause": "[bad_value != ZZZ] [100 != 6]", }, }, { @@ -291,12 +291,12 @@ func TestV2CAS(t *testing.T) { map[string]interface{}{ "errorCode": float64(101), "message": "Compare failed", - "cause": "[100 != 10]", + "cause": "[100 != 6]", }, }, { "/v2/keys/cas/foo", - url.Values(map[string][]string{"value": {"XXX"}, "prevValue": {"bad_value"}, "prevIndex": {"10"}}), + url.Values(map[string][]string{"value": {"XXX"}, "prevValue": {"bad_value"}, "prevIndex": {"6"}}), http.StatusPreconditionFailed, map[string]interface{}{ "errorCode": float64(101), @@ -446,7 +446,7 @@ func TestV2CAD(t *testing.T) { map[string]interface{}{ "errorCode": float64(101), "message": "Compare failed", - "cause": "[100 != 8]", + "cause": "[100 != 4]", }, }, { @@ -458,12 +458,12 @@ func TestV2CAD(t *testing.T) { }, }, { - "/v2/keys/foo?prevIndex=8", + "/v2/keys/foo?prevIndex=4", http.StatusOK, map[string]interface{}{ "node": map[string]interface{}{ "key": "/foo", - "modifiedIndex": float64(10), + "modifiedIndex": float64(6), }, "action": "compareAndDelete", }, @@ -491,7 +491,7 @@ func TestV2CAD(t *testing.T) { map[string]interface{}{ "node": map[string]interface{}{ "key": "/foovalue", - "modifiedIndex": float64(11), + "modifiedIndex": float64(7), }, "action": "compareAndDelete", }, @@ -529,7 +529,7 @@ func TestV2Unique(t *testing.T) { http.StatusCreated, map[string]interface{}{ "node": map[string]interface{}{ - "key": "/foo/8", + "key": "/foo/4", "value": "XXX", }, "action": "create", @@ -541,7 +541,7 @@ func TestV2Unique(t *testing.T) { http.StatusCreated, map[string]interface{}{ "node": map[string]interface{}{ - "key": "/foo/9", + "key": "/foo/5", "value": "XXX", }, "action": "create", @@ -553,7 +553,7 @@ func TestV2Unique(t *testing.T) { http.StatusCreated, map[string]interface{}{ "node": map[string]interface{}{ - "key": "/bar/10", + "key": "/bar/6", "value": "XXX", }, "action": "create", @@ -615,8 +615,8 @@ func TestV2Get(t *testing.T) { map[string]interface{}{ "key": "/foo/bar", "dir": true, - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), }, }, }, @@ -634,14 +634,14 @@ func TestV2Get(t *testing.T) { map[string]interface{}{ "key": "/foo/bar", "dir": true, - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), "nodes": []interface{}{ map[string]interface{}{ "key": "/foo/bar/zar", "value": "XXX", - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), }, }, }, @@ -709,8 +709,8 @@ func TestV2QuorumGet(t *testing.T) { map[string]interface{}{ "key": "/foo/bar", "dir": true, - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), }, }, }, @@ -728,14 +728,14 @@ func TestV2QuorumGet(t *testing.T) { map[string]interface{}{ "key": "/foo/bar", "dir": true, - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), "nodes": []interface{}{ map[string]interface{}{ "key": "/foo/bar/zar", "value": "XXX", - "createdIndex": float64(8), - "modifiedIndex": float64(8), + "createdIndex": float64(4), + "modifiedIndex": float64(4), }, }, }, @@ -781,7 +781,7 @@ func TestV2Watch(t *testing.T) { "node": map[string]interface{}{ "key": "/foo/bar", "value": "XXX", - "modifiedIndex": float64(8), + "modifiedIndex": float64(4), }, "action": "set", } @@ -802,7 +802,7 @@ func TestV2WatchWithIndex(t *testing.T) { var body map[string]interface{} c := make(chan bool, 1) go func() { - resp, _ := tc.Get(fmt.Sprintf("%s%s", u, "/v2/keys/foo/bar?wait=true&waitIndex=9")) + resp, _ := tc.Get(fmt.Sprintf("%s%s", u, "/v2/keys/foo/bar?wait=true&waitIndex=5")) body = tc.ReadBodyJSON(resp) c <- true }() @@ -839,7 +839,7 @@ func TestV2WatchWithIndex(t *testing.T) { "node": map[string]interface{}{ "key": "/foo/bar", "value": "XXX", - "modifiedIndex": float64(9), + "modifiedIndex": float64(5), }, "action": "set", }