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

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

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)

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

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()

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",
 	}