diff --git a/CHANGELOG-3.4.md b/CHANGELOG-3.4.md index 5e3cee031..c33b5217f 100644 --- a/CHANGELOG-3.4.md +++ b/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 diff --git a/Documentation/dev-guide/api_reference_v3.md b/Documentation/dev-guide/api_reference_v3.md index 04bacabbc..c59074511 100644 --- a/Documentation/dev-guide/api_reference_v3.md +++ b/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. | diff --git a/Documentation/dev-guide/apispec/swagger/rpc.swagger.json b/Documentation/dev-guide/apispec/swagger/rpc.swagger.json index c7cacf595..7619423be 100644 --- a/Documentation/dev-guide/apispec/swagger/rpc.swagger.json +++ b/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": [ { diff --git a/auth/store.go b/auth/store.go index c90f47746..a35e84178 100644 --- a/auth/store.go +++ b/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() diff --git a/auth/store_test.go b/auth/store_test.go index a7cae29a8..bb9716995 100644 --- a/auth/store_test.go +++ b/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) { diff --git a/clientv3/integration/role_test.go b/clientv3/integration/role_test.go index cc779575a..55f96b8aa 100644 --- a/clientv3/integration/role_test.go +++ b/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) + } } diff --git a/etcdserver/api/v3rpc/rpctypes/error.go b/etcdserver/api/v3rpc/rpctypes/error.go index 3bbc26b8f..e6a281460 100644 --- a/etcdserver/api/v3rpc/rpctypes/error.go +++ b/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) diff --git a/etcdserver/api/v3rpc/util.go b/etcdserver/api/v3rpc/util.go index 748632b5e..281ddc7a0 100644 --- a/etcdserver/api/v3rpc/util.go +++ b/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, diff --git a/etcdserver/etcdserverpb/rpc.pb.go b/etcdserver/etcdserverpb/rpc.pb.go index 73efc3040..199ee6244 100644 --- a/etcdserver/etcdserverpb/rpc.pb.go +++ b/etcdserver/etcdserverpb/rpc.pb.go @@ -4537,7 +4537,7 @@ type AuthClient interface { AuthDisable(ctx context.Context, in *AuthDisableRequest, opts ...grpc.CallOption) (*AuthDisableResponse, error) // Authenticate processes an authenticate request. Authenticate(ctx context.Context, in *AuthenticateRequest, opts ...grpc.CallOption) (*AuthenticateResponse, error) - // UserAdd adds a new user. + // UserAdd adds a new user. User name cannot be empty. UserAdd(ctx context.Context, in *AuthUserAddRequest, opts ...grpc.CallOption) (*AuthUserAddResponse, error) // UserGet gets detailed user information. UserGet(ctx context.Context, in *AuthUserGetRequest, opts ...grpc.CallOption) (*AuthUserGetResponse, error) @@ -4551,7 +4551,7 @@ type AuthClient interface { UserGrantRole(ctx context.Context, in *AuthUserGrantRoleRequest, opts ...grpc.CallOption) (*AuthUserGrantRoleResponse, error) // UserRevokeRole revokes a role of specified user. UserRevokeRole(ctx context.Context, in *AuthUserRevokeRoleRequest, opts ...grpc.CallOption) (*AuthUserRevokeRoleResponse, error) - // RoleAdd adds a new role. + // RoleAdd adds a new role. Role name cannot be empty. RoleAdd(ctx context.Context, in *AuthRoleAddRequest, opts ...grpc.CallOption) (*AuthRoleAddResponse, error) // RoleGet gets detailed role information. RoleGet(ctx context.Context, in *AuthRoleGetRequest, opts ...grpc.CallOption) (*AuthRoleGetResponse, error) @@ -4726,7 +4726,7 @@ type AuthServer interface { AuthDisable(context.Context, *AuthDisableRequest) (*AuthDisableResponse, error) // Authenticate processes an authenticate request. Authenticate(context.Context, *AuthenticateRequest) (*AuthenticateResponse, error) - // UserAdd adds a new user. + // UserAdd adds a new user. User name cannot be empty. UserAdd(context.Context, *AuthUserAddRequest) (*AuthUserAddResponse, error) // UserGet gets detailed user information. UserGet(context.Context, *AuthUserGetRequest) (*AuthUserGetResponse, error) @@ -4740,7 +4740,7 @@ type AuthServer interface { UserGrantRole(context.Context, *AuthUserGrantRoleRequest) (*AuthUserGrantRoleResponse, error) // UserRevokeRole revokes a role of specified user. UserRevokeRole(context.Context, *AuthUserRevokeRoleRequest) (*AuthUserRevokeRoleResponse, error) - // RoleAdd adds a new role. + // RoleAdd adds a new role. Role name cannot be empty. RoleAdd(context.Context, *AuthRoleAddRequest) (*AuthRoleAddResponse, error) // RoleGet gets detailed role information. RoleGet(context.Context, *AuthRoleGetRequest) (*AuthRoleGetResponse, error) diff --git a/etcdserver/etcdserverpb/rpc.proto b/etcdserver/etcdserverpb/rpc.proto index 565f8fae5..423eabada 100644 --- a/etcdserver/etcdserverpb/rpc.proto +++ b/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"