-
Notifications
You must be signed in to change notification settings - Fork 707
Add support for PLAIN authn for Shadow Linking #28708
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
Add support for PLAIN authn for Shadow Linking #28708
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 adds support for SASL/PLAIN authentication to Shadow Linking, which previously only supported SASL/SCRAM. The implementation introduces a new PlainConfig message type in the protobuf schema, extends the authentication handling in the converter layer, and adds test coverage for PLAIN authentication.
- Adds
PlainConfigprotobuf message with username/password fields similar toScramConfig - Updates authentication handling to support PLAIN alongside existing SCRAM mechanisms
- Implements PLAIN authentication flow in the Kafka client broker
Reviewed changes
Copilot reviewed 25 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/redpanda/core/admin/v2/shadow_link.proto | Adds PlainConfig message and extends AuthenticationConfiguration oneof |
| src/v/redpanda/admin/services/shadow_link/converter.cc | Implements conversion logic between PlainConfig proto and internal credentials |
| src/v/kafka/client/broker.cc | Adds PLAIN authentication implementation with proper message formatting |
| src/v/kafka/client/broker.h | Declares do_authenticate_plain method |
| src/v/cluster/cluster_link/frontend.cc | Extends validation to accept "PLAIN" mechanism |
| src/v/redpanda/admin/services/shadow_link/tests/converter_test.cc | Adds test coverage for PLAIN config creation and conversion |
| tests/rptest/tests/cluster_linking_topic_syncing_test.py | Adds test class for PLAIN authentication scenario |
| tests/rptest/clients/admin/proto/* | Generated Python protobuf bindings updates |
rockwotj
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.
you need to regenerate protos using the script in the tools directory of the redpanda repository. The old one in vtools is out of date (I will remove)
CI test resultstest results on build#76858
test results on build#76874
|
thanks! is that new? |
9109458 to
9852249
Compare
|
Force push:
|
| std::string bytes; | ||
| // 2 - number of null characters in the PLAIN auth message | ||
| bytes.reserve(2 + username.size() + password.size()); | ||
| bytes.push_back('\0'); | ||
| bytes.append(username.cbegin(), username.cend()); | ||
| bytes.push_back('\0'); | ||
| bytes.append(password.cbegin(), password.cend()); | ||
| req.data.auth_bytes = bytes::from_string(std::move(bytes)); |
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.
nitpick:
| std::string bytes; | |
| // 2 - number of null characters in the PLAIN auth message | |
| bytes.reserve(2 + username.size() + password.size()); | |
| bytes.push_back('\0'); | |
| bytes.append(username.cbegin(), username.cend()); | |
| bytes.push_back('\0'); | |
| bytes.append(password.cbegin(), password.cend()); | |
| req.data.auth_bytes = bytes::from_string(std::move(bytes)); | |
| auto bytes = fmt::format("\0{}\0{}", username, password); | |
| req.data.auth_bytes = bytes::from_string(bytes); |
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.
So I tried this but I think the std::string_view that gets created via the bytes::from_string reports a length of 0 since the string begins with a null character
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.
You, my bad, you'll need:
auto bytes = fmt::format("\0{}\0{}", username, password);
req.data.auth_bytes = bytes::from_string(std::string_view{bytes.begin(), bytes.end()});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.
That unfortunately won't work either... end() is start() + size() and in this case size() is 0
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.
It should work if the format string is a std::string_view!
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.
It should work if the format string is a std::string_view!
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.
Can you provide an example of what you mean please
|
|
||
| class ClusterLinkingTopicSyncingWithPlain(ClusterLinkingTopicSyncingTestBase): | ||
| """ | ||
| Run the same battery of tests with PLAIN |
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.
The same as what?
Is it necessary to run the whole test suite with each auth mechanism?
Yes it is, see: Also the rpk protogen stuff is required for now, although hopefully we can switch those to work off of the bufbuild protos instead of the checked in ones (cc: @r-vasquez) For now you can fix those via: |
9852249 to
f57056a
Compare
f57056a to
81b8dd8
Compare
|
Force push:
Second force push:
|
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
81b8dd8 to
8acce17
Compare
|
Force push:
|
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 3 comments.
| } else { | ||
| throw std::invalid_argument( | ||
| ssx::sformat( | ||
| "Unknown SCRAM mechanism: {}", scram.mechanism)); |
Copilot
AI
Nov 24, 2025
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.
The error message refers to 'SCRAM mechanism' but this error can now be triggered when using PLAIN authentication (which is not a SCRAM mechanism). Consider changing to 'Unknown authentication mechanism: {}' or adding separate validation to distinguish between SCRAM and PLAIN cases.
| "Unknown SCRAM mechanism: {}", scram.mechanism)); | |
| "Unknown authentication mechanism: {}", scram.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.
If we're gonna do that, it can go in another else on the outer scope. It's unreachable, though.
| } else if (mechanism == security::scram_sha512_authenticator::name) { | ||
| co_await do_authenticate_scram512(username, password); | ||
| } else if (mechanism == security::oidc::sasl_authenticator::name) { | ||
| co_await do_authenticate_oauthbearer(password); | ||
| } else if (mechanism == security::plain_authenticator::name) { | ||
| co_await do_authenticate_plain(username, password); | ||
| } else { | ||
| throw broker_error{ | ||
| _node_id, | ||
| error_code::sasl_authentication_failed, | ||
| fmt_with_ctx(ssx::sformat, "Unknown mechanism: {}", mechanism)}; | ||
| } |
Copilot
AI
Nov 24, 2025
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.
The newly added else-if at line 357-358 makes the earlier check at lines 321-328 redundant. The earlier check validates that mechanism must be one of scram_sha256, scram_sha512, oidc, or plain, but the else block at lines 359-364 now provides the same validation with a better error message. Consider removing the earlier validation block to avoid duplication.
| && c.mechanism != "PLAIN") { | ||
| vlog( | ||
| cluster::clusterlog.warn, | ||
| "Unsupported SCRAM mechanism: {}", |
Copilot
AI
Nov 24, 2025
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.
The log message at line 988 still refers to 'Unsupported SCRAM mechanism' but PLAIN is not a SCRAM mechanism. The message should be updated to 'Unsupported authentication mechanism' to accurately reflect that both SCRAM and PLAIN mechanisms are now supported.
| "Unsupported SCRAM mechanism: {}", | |
| "Unsupported authentication mechanism: {}", |
|
/backport v25.3.x |
|
Failed to create a backport PR to v25.3.x branch. I tried: |
This PR adds support for authenticating using SASL/PLAIN with Shadow Linking.
This adds a new authentication configuration called
PlainConfig:Backports Required
Release Notes
Improvements