-
Notifications
You must be signed in to change notification settings - Fork 707
CORE-14878: Migrate "Users" Admin API v1 Security Endpoints to v2 ConnectRPC API #29174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
CORE-14878: Migrate "Users" Admin API v1 Security Endpoints to v2 ConnectRPC API #29174
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements SCRAM credential management in the Admin API v2, providing a protobuf-based interface for creating, retrieving, listing, updating, and deleting SCRAM credentials. The implementation includes validation, idempotency, field mask support for partial updates, and controller leader proxying for write operations.
Key Changes:
- New Admin API v2 endpoints for SCRAM credential CRUD operations with proper validation and error handling
- Helper functions for credential validation, conversion between protobuf and internal representations, and password matching
- Comprehensive unit tests for validation and conversion logic, plus ducktape integration tests verifying behavioral parity between v1 and v2 APIs
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rptest/tests/scram_test.py | Added v2 API test methods and helper functions for SCRAM credential management, parameterized existing tests to run with both v1 and v2 APIs |
| tests/rptest/clients/admin/proto/redpanda/core/admin/v2/security_pb2_connect.py | Auto-generated Connect RPC client code for SCRAM credential endpoints |
| tests/rptest/clients/admin/proto/redpanda/core/admin/v2/security_pb2.pyi | Auto-generated Python type stubs for SCRAM credential protobuf messages |
| tests/rptest/clients/admin/proto/redpanda/core/admin/v2/security_pb2.py | Auto-generated Python protobuf bindings for SCRAM credential messages |
| src/v/security/fwd.h | Added forward declaration for scram_credential class |
| src/v/redpanda/admin/services/tests/security_test.cc | Added comprehensive unit tests for SCRAM credential validation, conversion, and matching functions |
| src/v/redpanda/admin/services/security.h | Added function declarations for SCRAM credential operations and RPC method signatures |
| src/v/redpanda/admin/services/security.cc | Implemented SCRAM credential CRUD operations with validation, conversion, and error handling |
| proto/redpanda/core/admin/v2/security.proto | Added SCRAM credential message definitions and RPC service methods |
| proto/redpanda/core/admin/v2/BUILD | Added field_mask proto dependency for update operations |
|
|
||
| const auto& password = pb_cred.get_password(); | ||
|
|
||
| // TODO: Do we allow empty passwords? |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify whether empty passwords are intentionally allowed or if validation should be added. If empty passwords should be rejected, add validation logic similar to the control character and length checks.
| // TODO: Do we allow empty passwords? | |
| // Empty passwords are not allowed. | |
| if (password.empty()) { | |
| throw serde::pb::rpc::invalid_argument_exception( | |
| "SCRAM credential password must not be empty"); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do at the API but I don't remember if the SCRAM algorithm will reject it
0ea68aa to
e7d2189
Compare
|
Force push to fix issues caught by copilot and python-type-check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
| // TODO: I feel like this isn't right to have. If the state of the | ||
| // credential store isn't caught up, we might incorrectly return success | ||
| // here. Commenting this out for now. | ||
| // auto& cred_store = _controller->get_credential_store().local(); | ||
| // auto user_opt = cred_store.get<security::scram_credential>(name); | ||
| // if (user_opt.has_value() && user_opt.value() == credential) { | ||
| // vlog( | ||
| // securitylog.debug, | ||
| // "User {} already exists with matching credential", | ||
| // name); | ||
| // // Idempotency: if the user already exists with the same credential, | ||
| // // return success. | ||
| // proto::admin::create_scram_credential_response res; | ||
| // res.set_scram_credential(std::move(pb_cred)); | ||
| // co_return res; | ||
| // } | ||
|
|
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented-out idempotency check with TODO should be resolved. Either remove the commented code or create a tracking issue and reference it in the comment.
| // TODO: I feel like this isn't right to have. If the state of the | |
| // credential store isn't caught up, we might incorrectly return success | |
| // here. Commenting this out for now. | |
| // auto& cred_store = _controller->get_credential_store().local(); | |
| // auto user_opt = cred_store.get<security::scram_credential>(name); | |
| // if (user_opt.has_value() && user_opt.value() == credential) { | |
| // vlog( | |
| // securitylog.debug, | |
| // "User {} already exists with matching credential", | |
| // name); | |
| // // Idempotency: if the user already exists with the same credential, | |
| // // return success. | |
| // proto::admin::create_scram_credential_response res; | |
| // res.set_scram_credential(std::move(pb_cred)); | |
| // co_return res; | |
| // } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a TODO, more of a question: is it possible for the controller leader's credential store to be stale and contain a credential that is supposed to be deleted? Is that something that can happen on restart for example? Or is the leader's credential store guaranteed to be up to date and we can exit early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented above but the credential store is a representation in memory of the state of the controller. On the controller leader it is up to date.
| serde::pb::rpc::context, proto::admin::list_scram_credentials_request req) { | ||
| vlog(securitylog.trace, "list_scram_credentials: {}", req); | ||
|
|
||
| // TODO: implement filtering based on request parameters |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment suggests unimplemented functionality. Either implement filtering or document why it's deferred and create a tracking issue.
| // TODO: implement filtering based on request parameters | |
| // Note: request parameters are currently ignored and all non-ephemeral | |
| // SCRAM credentials are returned. If filtering is added to | |
| // list_scram_credentials_request in the future, it should be implemented | |
| // here. |
This commit adds the protobuf messages and RPCs for SCRAM credential management for the Admin API V2, and it extends the security service implementation to include unimplemented stubs for the new SCRAM credential management methods: - CreateScramCredential - GetScramCredential - ListScramCredentials - UpdateScramCredential - DeleteScramCredential
Generate Ducktape protobuf clients to test the new Admin API v2 SCRAM credential security endpoints.
e7d2189 to
a37db5b
Compare
This commit implements the create_scram_credential endpoint in the Admin API v2 to allow users to create SCRAM-SHA-256 and SCRAM-SHA-512 credentials. The implementation redirects to the controller leader to ensure consistent credential state. It also validates credential names and passwords, enforcing FIPS minimum password length requirements and rejecting control characters. Idempotency is handled by checking if an existing user matches the requested credential. If a user already exists with the same credential, the operation succeeds. If a user exists with a different credential, an already_exists exception is thrown. Helper functions are added to validate credentials as well as to convert and match between protobuf and internal security credential types. On success, the response of create_scram_credential will exclude the password and return only the credential name and mechanism type.
This commit implements the get_scram_credential in the Admin API v2 to allow users to retrieve SCRAM credentials by name. The implementation validates the credential name, retrieves the credential from the controller's credential store, and returns a not_found exception if the credential does not exist. If the credential is found, it is converted to the protobuf representation and returned to the caller.
This commit implements the list_scram_credentials endpoint in the Admin API v2 to allow users to retrieve all SCRAM credentials. The implementation iterates through the credential store, filtering for non-ephemeral credentials only. The response excludes passwords, returning only credential names and mechanism types. Filtering based on request parameters is not yet implemented and is left for a future enhancement.
This commit implements the update_scram_credential endpoint in the Admin API v2 to allow users to update existing SCRAM credentials. The implementation redirects to the controller leader, uses protobuf field masks to support partial updates, and validates that the credential exists before updating. The merged credential undergoes the same validation as credential creation. On success, the response excludes the password and returns only the credential name and mechanism type.
This commit implements the delete_scram_credential endpoint in the Admin API v2 to allow users to delete existing SCRAM credentials. The endpoint redirects to the controller leader and validates the credential name before deletion. Idempotency is handled by treating deletion of non-existent credentials as successful, ensuring repeated delete operations do not fail. On success, an empty response is returned.
Add unit tests for the match_scram_credential function in security_test.cc, including: - Valid password matching for both SCRAM-SHA-256 and SCRAM-SHA-512 - Invalid password rejection for both mechanisms - Unknown mechanism error handling - Mismatched mechanism validation
Adds unit tests for the validate_scram_credential_name function in the Admin API v2 security service, covering: - Valid SCRAM credential names (alphanumeric, hyphens, underscores) - Invalid names with control characters (newline, tab, carriage return) - Invalid names with disallowed characters (comma, equals, null) - Empty credential names
Adds unit tests for the validate_pb_scram_credential function in the Admin API v2 security service, covering: - Valid credentials for both SCRAM-SHA-256 and SCRAM-SHA-512 - Invalid credential names (tested via validate_scram_credential_name) - Invalid passwords containing control characters (newline) - Unspecified/invalid SCRAM mechanisms
Adds unit tests for the convert_to_security_scram_credential function in the Admin API v2 security service, covering: - Conversion of protobuf credentials to security credentials for SCRAM-SHA-256 - Conversion of protobuf credentials to security credentials for SCRAM-SHA-512 - Verification that converted credentials match the original password - Error handling for unspecified/unknown SCRAM mechanisms Tests verify that the converted credentials have the correct iterations, non-empty salt/stored_key/server_key, and that the password can be successfully validated against the generated credential.
Adds unit tests for convert_to_pb_scram_credential, which converts security::scram_credential objects to protobuf format. The tests verify: - SCRAM mechanism detection (SHA-256/SHA-512) based on stored key size - Proper name field population - Password field is empty (cannot be recovered from hashed credentials) - Error handling for credentials with unknown/invalid key sizes
a37db5b to
3b7da22
Compare
|
Force pushes to fix error with Buf lint and run clang format. |
3b7da22 to
4a39546
Compare
|
Force push to fix copy & paste errors, thanks copilot 🤖 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
| username = "test" | ||
|
|
||
| if use_v2_api: | ||
| # TODO: Is this actually meaningful to test with v2 API? |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO comment should be resolved or removed before merging. If testing with v2 API is not meaningful for this case, the test should either skip this path or the comment should explain why it's kept.
| # TODO: Is this actually meaningful to test with v2 API? | |
| # Even though the v2 API takes an enum mechanism, this path verifies that an | |
| # invalid algorithm string (containing control characters) mapped via | |
| # scram_mechanism_from_string still results in an INVALID_ARGUMENT error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not?
Retry command for Build#78672please wait until all jobs are finished before running the slash command |
The existing SCRAM ducktape tests previously validated the v1 Admin API endpoints for user management (/v1/security/users). With the introduction of the v2 Admin API for SCRAM credential management, these tests need to verify both implementations work correctly with identical behavior. This commit extends the test suite by adding a `use_v2_api` matrix parameter to existing tests, allowing each test to run against both API versions. The implementation adds v2-specific helper methods (`create_scram_credential_v2`, `update_scram_credential_v2`, `delete_scram_credential_v2`, `list_scram_credentials_v2`) that mirror the v1 helpers, and conditionally invokes the appropriate API based on the test parameter. The following test classes are updated to support both APIs: - `ScramTest`: Core SCRAM functionality tests - `SaslPlainTest`: SASL/PLAIN authentication tests - `ScramBootstrapUserTest`: Bootstrap user validation - `InvalidNewUserStrings`: Input validation tests - `EscapedNewUserStrings`: URL encoding tests A new `test_bootstrap_user_v2` validates bootstrap user operations specifically through the v2 API, complementing the existing v1 test. Helper functions like `scram_mechanism_from_string` convert between string algorithm names (SCRAM-SHA-256) and protobuf enum values required by the v2 API. This approach ensures behavioral parity between v1 and v2 APIs while maintaining test coverage as the v2 API evolves.
4a39546 to
12b732b
Compare
|
Force push to fix broken dt test due to earlier protobuf adjustments to meet buf linter rules. |
michael-redpanda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, one change request (password set at date)
| // The SCRAM mechanism. | ||
| enum ScramMechanism { | ||
| SCRAM_MECHANISM_UNSPECIFIED = 0; | ||
| SCRAM_MECHANISM_SCRAM_SHA_256 = 1; | ||
| SCRAM_MECHANISM_SCRAM_SHA_512 = 2; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be useful to extract this to a common area of security. Maybe a security_types.proto
| // The password for the SCRAM credential. | ||
| string password = 3 | ||
| [debug_redact = true, (google.api.field_behavior) = INPUT_ONLY]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also add a password_set_at field (a, la
redpanda/proto/redpanda/core/admin/v2/shadow_link.proto
Lines 466 to 469 in c3e36a2
| // Timestamp of when the password was last set - only valid if password_set | |
| // is true | |
| google.protobuf.Timestamp password_set_at = 4 | |
| [(google.api.field_behavior) = OUTPUT_ONLY]; |
I do think this may result in having to make controller updates to store the timestamp.
|
|
||
| const auto& password = pb_cred.get_password(); | ||
|
|
||
| // TODO: Do we allow empty passwords? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do at the API but I don't remember if the SCRAM algorithm will reject it
| if ( | ||
| mechanism | ||
| != proto::admin::scram_credential_scram_mechanism:: | ||
| scram_mechanism_scram_sha_256 | ||
| && mechanism | ||
| != proto::admin::scram_credential_scram_mechanism:: | ||
| scram_mechanism_scram_sha_512) { | ||
| throw serde::pb::rpc::invalid_argument_exception( | ||
| ssx::sformat("Unknown SCRAM mechanism: {}", mechanism)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think a switch statement would be clearner/nicer here
| // TODO: I feel like this isn't right to have. If the state of the | ||
| // credential store isn't caught up, we might incorrectly return success | ||
| // here. Commenting this out for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's safe here because we've redirect the request to the controller leader so the credential store state should be entirely up to date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that, below, we also do an idempotency check, I think it may be fine to not include this logic.
| // TODO: I feel like this isn't right to have. If the state of the | ||
| // credential store isn't caught up, we might incorrectly return success | ||
| // here. Commenting this out for now. | ||
| // auto& cred_store = _controller->get_credential_store().local(); | ||
| // auto user_opt = cred_store.get<security::scram_credential>(name); | ||
| // if (user_opt.has_value() && user_opt.value() == credential) { | ||
| // vlog( | ||
| // securitylog.debug, | ||
| // "User {} already exists with matching credential", | ||
| // name); | ||
| // // Idempotency: if the user already exists with the same credential, | ||
| // // return success. | ||
| // proto::admin::create_scram_credential_response res; | ||
| // res.set_scram_credential(std::move(pb_cred)); | ||
| // co_return res; | ||
| // } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented above but the credential store is a representation in memory of the state of the controller. On the controller leader it is up to date.
| def list_scram_credentials_v2(self) -> list[str]: | ||
| res = self.admin.security().list_scram_credentials( | ||
| security_pb2.ListScramCredentialsRequest() | ||
| ) | ||
| return [cred.name for cred in res.scram_credentials] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this just returning the list of names? Why not the list of SCramCredentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking because if we extend the content of ScramCredentials then this and its callers would need updating or a new method would need to be written
| ConnectError, | ||
| lambda e: e.code == expected_error, | ||
| ): | ||
| _ = self.admin.security().create_scram_credential(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these return back the created credentials for validation?
| username = "test" | ||
|
|
||
| if use_v2_api: | ||
| # TODO: Is this actually meaningful to test with v2 API? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not?
This PR implements SCRAM credential management in the Admin API v2. providing a protobuf-based interface for creating, retrieving, listing, updating, and deleting SCRAM credentials.
The main changes include:
CORE-14878
Backports Required
Release Notes
Features