Merge pull request #10907 from spzala/emptyrole10905

etcdserver: do not allow creating empty role

CHANGELOG-3.4.md

@@ -355,6 +355,9 @@ See [security doc](https://github.com/etcd-io/etcd/blob/master/Documentation/op-
   - Now, WAL directory that contains only `lost+found` or a file that's not suffixed with `.wal` is considered non-initialized.
 - Fix [`ETCD_CONFIG_FILE` env variable parsing in `etcd`](https://github.com/etcd-io/etcd/pull/10762).
 - Fix [race condition in `rafthttp` transport pause/resume](https://github.com/etcd-io/etcd/pull/10826).
+- Fix [server crash from creating an empty role](https://github.com/etcd-io/etcd/pull/10907).
+  - Previously, creating a role with an empty name crashed etcd server with an error code `Unavailable`.
+  - Now, creating a role with an empty name is not allowed with an error code `InvalidArgument`.
 
 ### API
 

Documentation/dev-guide/api_reference_v3.md

@@ -11,14 +11,14 @@ This is a generated documentation. Please read the proto files for more.
 | AuthEnable | AuthEnableRequest | AuthEnableResponse | AuthEnable enables authentication. |
 | AuthDisable | AuthDisableRequest | AuthDisableResponse | AuthDisable disables authentication. |
 | Authenticate | AuthenticateRequest | AuthenticateResponse | Authenticate processes an authenticate request. |
-| UserAdd | AuthUserAddRequest | AuthUserAddResponse | UserAdd adds a new user. |
+| UserAdd | AuthUserAddRequest | AuthUserAddResponse | UserAdd adds a new user. User name cannot be empty. |
 | UserGet | AuthUserGetRequest | AuthUserGetResponse | UserGet gets detailed user information. |
 | UserList | AuthUserListRequest | AuthUserListResponse | UserList gets a list of all users. |
 | UserDelete | AuthUserDeleteRequest | AuthUserDeleteResponse | UserDelete deletes a specified user. |
 | UserChangePassword | AuthUserChangePasswordRequest | AuthUserChangePasswordResponse | UserChangePassword changes the password of a specified user. |
 | UserGrantRole | AuthUserGrantRoleRequest | AuthUserGrantRoleResponse | UserGrant grants a role to a specified user. |
 | UserRevokeRole | AuthUserRevokeRoleRequest | AuthUserRevokeRoleResponse | UserRevokeRole revokes a role of specified user. |
-| RoleAdd | AuthRoleAddRequest | AuthRoleAddResponse | RoleAdd adds a new role. |
+| RoleAdd | AuthRoleAddRequest | AuthRoleAddResponse | RoleAdd adds a new role. Role name cannot be empty. |
 | RoleGet | AuthRoleGetRequest | AuthRoleGetResponse | RoleGet gets detailed role information. |
 | RoleList | AuthRoleListRequest | AuthRoleListResponse | RoleList gets lists of all roles. |
 | RoleDelete | AuthRoleDeleteRequest | AuthRoleDeleteResponse | RoleDelete deletes a specified role. |

Documentation/dev-guide/apispec/swagger/rpc.swagger.json

@@ -101,7 +101,7 @@
         "tags": [
           "Auth"
         ],
-        "summary": "RoleAdd adds a new role.",
+        "summary": "RoleAdd adds a new role. Role name cannot be empty.",
         "operationId": "RoleAdd",
         "parameters": [
           {
@@ -263,7 +263,7 @@
         "tags": [
           "Auth"
         ],
-        "summary": "UserAdd adds a new user.",
+        "summary": "UserAdd adds a new user. User name cannot be empty.",
         "operationId": "UserAdd",
         "parameters": [
           {

auth/store.go

@@ -57,6 +57,7 @@ var (
 	ErrUserNotFound         = errors.New("auth: user not found")
 	ErrRoleAlreadyExist     = errors.New("auth: role already exists")
 	ErrRoleNotFound         = errors.New("auth: role not found")
+	ErrRoleEmpty            = errors.New("auth: role name is empty")
 	ErrAuthFailed           = errors.New("auth: authentication failed, invalid user ID or password")
 	ErrPermissionDenied     = errors.New("auth: permission denied")
 	ErrRoleNotGranted       = errors.New("auth: role is not granted to the user")
@@ -796,6 +797,10 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete
 }
 
 func (as *authStore) RoleAdd(r *pb.AuthRoleAddRequest) (*pb.AuthRoleAddResponse, error) {
+	if len(r.Name) == 0 {
+		return nil, ErrRoleEmpty
+	}
+
 	tx := as.be.BatchTx()
 	tx.Lock()
 	defer tx.Unlock()

auth/store_test.go

@@ -269,6 +269,12 @@ func TestRoleAdd(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+
+	// add a role with empty name
+	_, err = as.RoleAdd(&pb.AuthRoleAddRequest{Name: ""})
+	if err != ErrRoleEmpty {
+		t.Fatal(err)
+	}
 }
 
 func TestUserGrant(t *testing.T) {

clientv3/integration/role_test.go

@@ -40,4 +40,9 @@ func TestRoleError(t *testing.T) {
 	if err != rpctypes.ErrRoleAlreadyExist {
 		t.Fatalf("expected %v, got %v", rpctypes.ErrRoleAlreadyExist, err)
 	}
+
+	_, err = authapi.RoleAdd(context.TODO(), "")
+	if err != rpctypes.ErrRoleEmpty {
+		t.Fatalf("expected %v, got %v", rpctypes.ErrRoleEmpty, err)
+	}
 }

etcdserver/api/v3rpc/rpctypes/error.go

@@ -54,6 +54,7 @@ var (
 	ErrGRPCUserNotFound         = status.New(codes.FailedPrecondition, "etcdserver: user name not found").Err()
 	ErrGRPCRoleAlreadyExist     = status.New(codes.FailedPrecondition, "etcdserver: role name already exists").Err()
 	ErrGRPCRoleNotFound         = status.New(codes.FailedPrecondition, "etcdserver: role name not found").Err()
+	ErrGRPCRoleEmpty            = status.New(codes.InvalidArgument, "etcdserver: role name is empty").Err()
 	ErrGRPCAuthFailed           = status.New(codes.InvalidArgument, "etcdserver: authentication failed, invalid user ID or password").Err()
 	ErrGRPCPermissionDenied     = status.New(codes.PermissionDenied, "etcdserver: permission denied").Err()
 	ErrGRPCRoleNotGranted       = status.New(codes.FailedPrecondition, "etcdserver: role is not granted to the user").Err()
@@ -110,6 +111,7 @@ var (
 		ErrorDesc(ErrGRPCUserNotFound):         ErrGRPCUserNotFound,
 		ErrorDesc(ErrGRPCRoleAlreadyExist):     ErrGRPCRoleAlreadyExist,
 		ErrorDesc(ErrGRPCRoleNotFound):         ErrGRPCRoleNotFound,
+		ErrorDesc(ErrGRPCRoleEmpty):            ErrGRPCRoleEmpty,
 		ErrorDesc(ErrGRPCAuthFailed):           ErrGRPCAuthFailed,
 		ErrorDesc(ErrGRPCPermissionDenied):     ErrGRPCPermissionDenied,
 		ErrorDesc(ErrGRPCRoleNotGranted):       ErrGRPCRoleNotGranted,
@@ -168,6 +170,7 @@ var (
 	ErrUserNotFound         = Error(ErrGRPCUserNotFound)
 	ErrRoleAlreadyExist     = Error(ErrGRPCRoleAlreadyExist)
 	ErrRoleNotFound         = Error(ErrGRPCRoleNotFound)
+	ErrRoleEmpty            = Error(ErrGRPCRoleEmpty)
 	ErrAuthFailed           = Error(ErrGRPCAuthFailed)
 	ErrPermissionDenied     = Error(ErrGRPCPermissionDenied)
 	ErrRoleNotGranted       = Error(ErrGRPCRoleNotGranted)

etcdserver/api/v3rpc/util.go

@@ -69,6 +69,7 @@ var toGRPCErrorMap = map[error]error{
 	auth.ErrUserNotFound:         rpctypes.ErrGRPCUserNotFound,
 	auth.ErrRoleAlreadyExist:     rpctypes.ErrGRPCRoleAlreadyExist,
 	auth.ErrRoleNotFound:         rpctypes.ErrGRPCRoleNotFound,
+	auth.ErrRoleEmpty:            rpctypes.ErrGRPCRoleEmpty,
 	auth.ErrAuthFailed:           rpctypes.ErrGRPCAuthFailed,
 	auth.ErrPermissionDenied:     rpctypes.ErrGRPCPermissionDenied,
 	auth.ErrRoleNotGranted:       rpctypes.ErrGRPCRoleNotGranted,

etcdserver/etcdserverpb/rpc.proto

@@ -264,7 +264,7 @@ service Auth {
     };
   }
 
-  // UserAdd adds a new user.
+  // UserAdd adds a new user. User name cannot be empty.
   rpc UserAdd(AuthUserAddRequest) returns (AuthUserAddResponse) {
       option (google.api.http) = {
         post: "/v3/auth/user/add"
@@ -320,7 +320,7 @@ service Auth {
     };
   }
 
-  // RoleAdd adds a new role.
+  // RoleAdd adds a new role. Role name cannot be empty.
   rpc RoleAdd(AuthRoleAddRequest) returns (AuthRoleAddResponse) {
       option (google.api.http) = {
         post: "/v3/auth/role/add"