-
Notifications
You must be signed in to change notification settings - Fork 593
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
[CORE-8436]: Add support for SASL/PLAIN #24525
base: dev
Are you sure you want to change the base?
Changes from all commits
8885778
8ace98e
6cab505
08d63c9
4beb86a
ca1b2ec
588ec77
ef9dcc5
176570e
d888e7e
daf5ea2
98d6056
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,7 +115,7 @@ std::optional<ss::sstring> validate_client_groups_byte_rate_quota( | |
std::optional<ss::sstring> | ||
validate_sasl_mechanisms(const std::vector<ss::sstring>& mechanisms) { | ||
constexpr auto supported = std::to_array<std::string_view>( | ||
{"GSSAPI", "SCRAM", "OAUTHBEARER"}); | ||
{"GSSAPI", "SCRAM", "OAUTHBEARER", "PLAIN"}); | ||
|
||
// Validate results | ||
for (const auto& m : mechanisms) { | ||
|
@@ -124,6 +124,12 @@ validate_sasl_mechanisms(const std::vector<ss::sstring>& mechanisms) { | |
return ssx::sformat("'{}' is not a supported SASL mechanism", m); | ||
} | ||
} | ||
|
||
if (mechanisms.size() == 1 && mechanisms[0] == "PLAIN") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the condition supposed to be "at least one other" or "at least SCRAM as well"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From SUCCESS-02: "We will not support a cluster configured with only a PLAIN mechanism", which I understand to mean that at least one other secure mechanism is required, which includes all of the others; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The preceding sentence of SUCCESS-02 is "Users only need to create a single account to support either SCRAM or PLAIN authentication." which makes me think that it is SCRAM specifically that's also required. I think it's worth asking @deniscoady to see what he meant here. (I'd comment on the PRD, but I don't have access.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
SCRAM Users are the single source of truth for users which are managed internally. Historically I've pushed for them to be named "SCRAM Uses" over "SASL Users" (c.f., SASL/OAUTHBEARER, SASL/GSSAPI, which are managed externally), but although they are created as SCRAM users, they are supported via HTTP Basic Auth, and now, via SASL/PLAIN. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I've got that. I guess what I'm trying to say is that it's not clear to me is whether the goal is: If (a), then this implementation makes sense. But I think (b) would also be reasonable, given that I would expect most customers to use PLAIN while migrating over to Redpanda and away from PLAIN over to SCRAM. In that case, I think they would start by enabling PLAIN+SCRAM, then migrate, and finally disable PLAIN. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed with @deniscoady and I think we may modify this requirement slightly to make it so if you enable |
||
return "When PLAIN is enabled, at least one other mechanism must be " | ||
"enabled"; | ||
} | ||
|
||
return std::nullopt; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ | |
#include "security/gssapi_authenticator.h" | ||
#include "security/mtls.h" | ||
#include "security/oidc_authenticator.h" | ||
#include "security/plain_authenticator.h" | ||
#include "security/scram_algorithm.h" | ||
#include "security/scram_authenticator.h" | ||
#include "ssx/future-util.h" | ||
|
@@ -720,6 +721,16 @@ ss::future<response_ptr> sasl_handshake_handler::handle( | |
} | ||
} | ||
|
||
if (supports("PLAIN")) { | ||
supported_sasl_mechanisms.emplace_back( | ||
security::plain_authenticator::name); | ||
if (request.data.mechanism == security::plain_authenticator::name) { | ||
ctx.sasl()->set_mechanism( | ||
std::make_unique<security::plain_authenticator>( | ||
ctx.credentials())); | ||
} | ||
} | ||
Comment on lines
+724
to
+732
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussion: Getting clarification from REQ-01. Not sure if the wording was wrong or if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it "{PLAIN} implies {PLAIN, SCRAM}" or "{PLAIN} without SCRAM is invalid" or "{PLAIN} implies only {PLAIN} on the kafka procotol, but you can still manage SCRAM users through the admin API"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PLAIN means enable PLAIN. One other secure mechanism must be explicitly enabled. |
||
|
||
if (supports("GSSAPI")) { | ||
supported_sasl_mechanisms.emplace_back( | ||
security::gssapi_authenticator::name); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
/* | ||
* Copyright 2024 Redpanda Data, Inc. | ||
* | ||
* Use of this software is governed by the Business Source License | ||
* included in the file licenses/BSL.md | ||
* | ||
* As of the Change Date specified in that file, in accordance with | ||
* the Business Source License, use of this software will be governed | ||
* by the Apache License, Version 2.0 | ||
*/ | ||
#include "security/plain_authenticator.h" | ||
|
||
#include "base/vlog.h" | ||
#include "security/acl.h" | ||
#include "security/credential_store.h" | ||
#include "security/errc.h" | ||
#include "security/logger.h" | ||
#include "security/scram_authenticator.h" | ||
#include "security/types.h" | ||
#include "strings/utf8.h" | ||
|
||
#include <seastar/util/defer.hh> | ||
|
||
namespace security { | ||
|
||
ss::future<result<bytes>> plain_authenticator::authenticate(bytes auth_bytes) { | ||
constexpr size_t max_length{255}; | ||
constexpr std::string_view sep{"\0", 1}; | ||
|
||
auto make_failed = ss::defer([this] { _state = state::failed; }); | ||
|
||
if (_state != state::init) { | ||
vlog( | ||
seclog.warn, | ||
"invalid plain state: {}", | ||
_state == state::failed ? "failed" : "complete"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
auto auth_str = std::string_view( | ||
reinterpret_cast<char*>(auth_bytes.data()), auth_bytes.size()); | ||
|
||
if (!is_valid_utf8(auth_str)) { | ||
vlog(seclog.warn, "invalid utf8"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
// [authorization identity] not supported | ||
if (!auth_str.starts_with(sep)) { | ||
vlog(seclog.warn, "[authorization identity] not supported"); | ||
co_return errc::invalid_credentials; | ||
} | ||
auth_str = auth_str.substr(sep.length()); | ||
auto it = auth_str.find(sep); | ||
if (std::string_view::npos == it) { | ||
vlog(seclog.warn, "seperator not found"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
credential_user username{auth_str.substr(0, it)}; | ||
credential_password password{auth_str.substr(it + sep.length())}; | ||
|
||
if (username().empty()) { | ||
vlog(seclog.warn, "username not found"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
if (username().length() > max_length) { | ||
vlog(seclog.warn, "username too long"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
if (password().empty()) { | ||
vlog(seclog.warn, "password not found"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
if (password().length() > max_length) { | ||
vlog(seclog.warn, "password too long"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
_audit_user.name = username; | ||
auto cred = _credentials.get<scram_credential>(username); | ||
if (!cred.has_value()) { | ||
vlog(seclog.warn, "credential not found"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
if (!validate_scram_credential(*cred, password).has_value()) { | ||
vlog(seclog.warn, "scram authentication failed"); | ||
co_return errc::invalid_credentials; | ||
} | ||
|
||
vlog(seclog.trace, "Authenticated user {}", username); | ||
|
||
make_failed.cancel(); | ||
|
||
_principal = cred->principal().value_or( | ||
acl_principal{principal_type::user, username()}); | ||
_audit_user.name = _principal.name(); | ||
_audit_user.type_id = audit::user::type::user; | ||
|
||
_state = state::complete; | ||
co_return bytes{}; | ||
} | ||
|
||
} // namespace security |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* | ||
* Copyright 2024 Redpanda Data, Inc. | ||
* | ||
* Use of this software is governed by the Business Source License | ||
* included in the file licenses/BSL.md | ||
* | ||
* As of the Change Date specified in that file, in accordance with | ||
* the Business Source License, use of this software will be governed | ||
* by the Apache License, Version 2.0 | ||
*/ | ||
#pragma once | ||
#include "security/acl.h" | ||
#include "security/fwd.h" | ||
#include "security/sasl_authentication.h" | ||
|
||
namespace security { | ||
|
||
/** | ||
* @class plain_authenticator | ||
* @brief A class that implements SASL/PLAIN authentication mechanism. | ||
* | ||
* This class is responsible for handling the SASL/PLAIN authentication process. | ||
* It authenticates the username and password provided by the client against | ||
* SCRAM users in the credential store. | ||
*/ | ||
class plain_authenticator final : public sasl_mechanism { | ||
public: | ||
static constexpr const char* name = "PLAIN"; | ||
|
||
explicit plain_authenticator(credential_store& credentials) | ||
: _credentials(credentials) {} | ||
|
||
ss::future<result<bytes>> authenticate(bytes auth_bytes) override; | ||
|
||
bool complete() const override { return _state == state::complete; } | ||
bool failed() const override { return _state == state::failed; } | ||
|
||
const acl_principal& principal() const override { | ||
vassert( | ||
_state == state::complete, | ||
"Authentication id is not valid until auth process complete"); | ||
return _principal; | ||
} | ||
|
||
const audit::user& audit_user() const override { return _audit_user; } | ||
|
||
const char* mechanism_name() const override { return "SASL-PLAIN"; } | ||
|
||
private: | ||
enum class state { | ||
init, | ||
complete, | ||
failed, | ||
}; | ||
|
||
state _state{state::init}; | ||
credential_store& _credentials; | ||
acl_principal _principal; | ||
security::audit::user _audit_user; | ||
}; | ||
|
||
} // namespace security |
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's probably worth extracting out the duplicate
_license_nag_is_set
functions.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.
Oh I missed changing those as well