[CORE-8436]: Add support for SASL/PLAIN #24525

michael-redpanda · 2024-12-11T13:01:30Z

Adds support for SASL/PLAIN authentication mechanism.

Configuration

Adds "PLAIN" as a valid entry in the sasl_mechanisms cluster config list.

Caveats

"PLAIN" cannot be the only entry in the sasl_mechanisms list. Redpanda will log either at warn when "PLAIN" is enabled. Will log at err if "PLAIN" is enabled and TLS is not enabled on any interface.

Fixes: CORE-8436
Fixes: CORE-8458
Fixes: CORE-8459
Fixes: CORE-8460

Todo:

Test for error and warning logs
Validate "PLAIN" only is an error

Backports Required

Release Notes

Features

Redpanda now supports the SASL/PLAIN authentication mechanism. To enable it add "PLAIN" to the sasl_mechanisms cluster configuration list

Pure refactor, no change in behaviour. Signed-off-by: Ben Pope <ben@redpanda.com>

michael-redpanda · 2024-12-11T14:25:13Z

Force push ea25a3f:

Adjusted log logic to also log at error if not Kafka API TLS entries are present
Fixed spelling
Added tests for log messages and config

src/v/security/scram_authenticator.cc

michael-redpanda · 2024-12-11T14:34:00Z

src/v/kafka/server/server.cc

+    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()));
+        }
+    }


discussion: Getting clarification from REQ-01. Not sure if the wording was wrong or if PLAIN implies also enabling SCRAM.

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"?

PLAIN means enable PLAIN. One other secure mechanism must be explicitly enabled.

michael-redpanda · 2024-12-11T15:53:39Z

Force push 7771b04:

Fixed cpplint issue

vbotbuildovich · 2024-12-11T18:38:49Z

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/59603#0193b6b2-1bf2-4981-b99e-25425073658b:

"rptest.tests.e2e_shadow_indexing_test.EndToEndHydrationTimeoutTest.test_hydration_completes_when_consumer_killed"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/59603#0193b6b2-1bf0-4bbb-97af-594a8376f0df:

"rptest.tests.simple_e2e_test.SimpleEndToEndTest.test_relaxed_acks.write_caching=False"

vbotbuildovich · 2024-12-11T19:05:17Z

Retry command for Build#59603

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/simple_e2e_test.py::SimpleEndToEndTest.test_relaxed_acks@{"write_caching":false}
tests/rptest/tests/e2e_shadow_indexing_test.py::EndToEndHydrationTimeoutTest.test_hydration_completes_when_consumer_killed

vbotbuildovich · 2024-12-11T19:09:15Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/59603#0193b6b2-1bf2-4981-b99e-25425073658b
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/59603#0193b6b2-1bf1-4910-8193-47fff943528e

vbotbuildovich · 2024-12-12T15:47:39Z

the below tests from https://buildkite.com/redpanda/redpanda/builds/59656#0193bb20-bbaa-4494-87a5-ce21272fd252 have failed and will be retried

gtest_raft_rpunit

tests/rptest/clients/kafka_cli_tools.py

src/v/security/tests/plain_authenticator_test.cc

BenPope · 2024-12-13T14:23:23Z

src/v/security/tests/plain_authenticator_test.cc

+    EXPECT_EQ(res.assume_error(), security::errc::invalid_credentials);
+    EXPECT_FALSE(authn->complete());
+    EXPECT_TRUE(authn->failed());
+}


Possibly missing a test for invalid utf-8

you're an invalid utf8

Fair, but the character is not rendered or pronounced, so nobody has noticed yet.

pgellert

looks good

src/v/security/scram_authenticator.cc

pgellert · 2024-12-13T15:57:34Z

src/v/config/validators.cc

@@ -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") {


Is the condition supposed to be "at least one other" or "at least SCRAM as well"?

Is the condition supposed to be "at least one other" or "at least SCRAM as well"?

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; SCRAM,OAUTHBEARER, GSSAPI.

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.)

"Users only need to create a single account to support either SCRAM or PLAIN authentication.",

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.

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:
(a) For Redpanda to be opinionated and nudge customers to enable at least one secure sasl mechanism, or
(b) For Redpanda to signal that SCRAM users and SASL users are the same so you probably want to enable SCRAM as well if you enable PLAIN.

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.

I discussed with @deniscoady and I think we may modify this requirement slightly to make it so if you enable PLAIN you must also enable SCRAM. This impacts our internal clients (PP/SR/Auditing) so I think this is reasonable.

pgellert · 2024-12-13T16:06:12Z

src/v/kafka/server/server.cc

+    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()));
+        }
+    }


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"?

src/v/cluster/feature_manager.cc

src/v/security/tests/plain_authenticator_test.cc

pgellert · 2024-12-13T17:59:22Z

src/v/security/tests/plain_authenticator_test.cc

+    EXPECT_EQ(res.assume_error(), security::errc::invalid_credentials);
+    EXPECT_FALSE(authn->complete());
+    EXPECT_TRUE(authn->failed());
+}


Should we add a test that makes some assertions on the audit user as well?

@pgellert what are you looking for irt to those?

Just a simple test that verifies that the audit user's name is properly set on success. If you think it's too granular, I'm fine with that, but it would be a sanity check to ensure that the audit user is updated.

Optimise the password validation when SCRAM-SHA-512 is in use, by avoiding the validation against SCRAM-SHA-256 that will fail. Signed-off-by: Ben Pope <ben@redpanda.com>

SUCCESS-02 Signed-off-by: Ben Pope <ben@redpanda.com>

Signed-off-by: Ben Pope <ben@redpanda.com>

No change in behaviour Signed-off-by: Ben Pope <ben@redpanda.com>

michael-redpanda · 2024-12-16T20:15:50Z

Force push f051233

Updated per PR comments

vbotbuildovich · 2024-12-16T23:07:55Z

Retry command for Build#59821

please wait until all jobs are finished before running the slash command



/ci-repeat 1
tests/rptest/tests/pandaproxy_test.py::PandaProxySASLTest.test_list_topics
tests/rptest/tests/schema_registry_test.py::SchemaRegistryLicenseTest.test_license_nag@{"mode":"redpanda"}
tests/rptest/tests/scram_test.py::SaslPlainConfigTest.test_sasl_plain_log@{"enable_tls":true}
tests/rptest/tests/rbac_test.py::RBACLicenseTest.test_license_nag
tests/rptest/tests/redpanda_oauth_test.py::OIDCLicenseTest.test_license_nag@{"authn_config":{"http_authentication":["OIDC","BASIC"]}}
tests/rptest/tests/redpanda_oauth_test.py::OIDCLicenseTest.test_license_nag@{"authn_config":{"sasl_mechanisms":["OAUTHBEARER","SCRAM"]}}
tests/rptest/tests/redpanda_kerberos_test.py::RedpandaKerberosLicenseTest.test_license_nag
tests/rptest/tests/schema_registry_test.py::SchemaRegistryLicenseTest.test_license_nag@{"mode":"compat"}
tests/rptest/tests/scram_test.py::SaslPlainConfigTest.test_sasl_plain_log@{"enable_tls":false}

vbotbuildovich · 2024-12-17T00:12:07Z

CI test results

test results on build#59821

test_id	test_kind	job_url	test_status	passed
rptest.tests.pandaproxy_test.PandaProxySASLTest.test_list_topics	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-8010-441c-bdca-2cb75d21d08e	FAIL	0/1
rptest.tests.rbac_test.RBACLicenseTest.test_license_nag	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800d-4d5f-88e0-9700ddd19db7	FAIL	0/1
rptest.tests.redpanda_kerberos_test.RedpandaKerberosLicenseTest.test_license_nag	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800f-41cb-af9d-780d73f2edf7	FAIL	0/1
rptest.tests.redpanda_oauth_test.OIDCLicenseTest.test_license_nag.authn_config=.http_authentication.OIDC.BASIC	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800d-4d5f-88e0-9700ddd19db7	FAIL	0/1
rptest.tests.redpanda_oauth_test.OIDCLicenseTest.test_license_nag.authn_config=.sasl_mechanisms.OAUTHBEARER.SCRAM	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800e-40cb-8135-099096669ffb	FAIL	0/1
rptest.tests.schema_registry_test.SchemaRegistryLicenseTest.test_license_nag.mode=SchemaIdValidationMode.COMPAT	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800f-41cb-af9d-780d73f2edf7	FAIL	0/1
rptest.tests.schema_registry_test.SchemaRegistryLicenseTest.test_license_nag.mode=SchemaIdValidationMode.REDPANDA	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-8010-441c-bdca-2cb75d21d08e	FAIL	0/1
rptest.tests.scram_test.SaslPlainConfigTest.test_sasl_plain_log.enable_tls=False	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-800f-41cb-af9d-780d73f2edf7	FAIL	0/1
rptest.tests.scram_test.SaslPlainConfigTest.test_sasl_plain_log.enable_tls=True	ducktape	https://buildkite.com/redpanda/redpanda/builds/59821#0193d177-8010-441c-bdca-2cb75d21d08e	FAIL	0/1

test results on build#59923

test_id	test_kind	job_url	test_status	passed
rptest.tests.cloud_retention_test.CloudRetentionTest.test_cloud_retention.max_consume_rate_mb=None.cloud_storage_type=CloudStorageType.ABS	ducktape	https://buildkite.com/redpanda/redpanda/builds/59923#0193db35-56d5-49e3-97d4-f2a784954cfd	FAIL	0/6
rptest.tests.pandaproxy_test.PandaProxySASLTest.test_list_topics	ducktape	https://buildkite.com/redpanda/redpanda/builds/59923#0193db33-6fee-4267-b448-e0359d266ccd	FAIL	0/1
rptest.tests.pandaproxy_test.PandaProxySASLTest.test_list_topics	ducktape	https://buildkite.com/redpanda/redpanda/builds/59923#0193db35-56d3-4a99-b8bb-ea09805fe11b	FAIL	0/1

pgellert

lgtm (If we do change the requirement discussed in 08d63c9#r1887547776 then that will need another change ~~to this PR~~ but otherwise lgtm.)

michael-redpanda · 2024-12-17T13:09:13Z

lgtm (If we do change the requirement discussed in 08d63c9#r1887547776 then that will need another change to this PR but otherwise lgtm.)

That or we will file another PR. There's discussions on adding metrics and we have plenty of time until release.

BenPope

Looks great

BenPope · 2024-12-18T14:37:47Z

src/v/cluster/feature_manager.cc

              std::chrono::seconds{std::stoi(interval_override)});
            vlog(
              clusterlog.info,
-              "Overriding default license log annoy interval to: {}s",
-              license_check_retry.count());
+              "Overriding default reminder period interval to: {}s",


It's probably worth extracting out the duplicate _license_nag_is_set functions.

Oh I missed changing those as well

michael-redpanda · 2024-12-18T16:43:28Z

Force push 9dec947:

Fixed check for license nag message

REQ-02 Signed-off-by: Ben Pope <ben@redpanda.com>

Renamed `__REDPANDA_LICENSE_CHECK_INTERVAL_SEC` with `__REDPANDA_PERIODIC_REMINDER_INTERVAL_SEC` to better reflect what messages this environmental variable is controlling. Signed-off-by: Michael Boquard <michael@redpanda.com>

This change allows for the selection of SASL/PLAIN as an authentication method using the Python RdKafka library. Signed-off-by: Michael Boquard <michael@redpanda.com>

Signed-off-by: Michael Boquard <michael@redpanda.com>

Added SASL/PLAIN verification tests for: * KCL * RPK * Python RdKafka * Kafka CLI tools Validated that both SCRAM-256 and SCRAM-512 users worked. Fixes: CORE-8458 Fixes: CORE-8459 Fixes: CORE-8460 Tested that a log message appears when SASL/PLAIN is enabled and that PLAIN cannot be the only SASL mechanism in the mechanisms list. Signed-off-by: Michael Boquard <michael@redpanda.com>

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-12-18T17:37:01Z

Force push 98d6056:

Fixed python lint issue

vbotbuildovich · 2024-12-18T20:49:12Z

Retry command for Build#59923

please wait until all jobs are finished before running the slash command



/ci-repeat 1
tests/rptest/tests/pandaproxy_test.py::PandaProxySASLTest.test_list_topics
tests/rptest/tests/cloud_retention_test.py::CloudRetentionTest.test_cloud_retention@{"cloud_storage_type":2,"max_consume_rate_mb":null}

security: Extract validate_scram_credential

8885778

Pure refactor, no change in behaviour. Signed-off-by: Ben Pope <ben@redpanda.com>

michael-redpanda assigned BenPope Dec 11, 2024

michael-redpanda requested a review from a team as a code owner December 11, 2024 13:01

michael-redpanda requested review from a team and pgellert and removed request for a team December 11, 2024 13:01

github-actions bot added area/build area/redpanda labels Dec 11, 2024

michael-redpanda requested a review from BenPope December 11, 2024 13:01

michael-redpanda self-assigned this Dec 11, 2024

michael-redpanda changed the title ~~Sasl plain/core 8407~~ [CORE-8436]: Add support for SASL/PLAIN Dec 11, 2024

michael-redpanda force-pushed the sasl-plain/core-8407 branch from fe74778 to ea25a3f Compare December 11, 2024 14:24

michael-redpanda commented Dec 11, 2024

View reviewed changes

michael-redpanda force-pushed the sasl-plain/core-8407 branch from ea25a3f to 7771b04 Compare December 11, 2024 15:53

BenPope reviewed Dec 13, 2024

View reviewed changes

pgellert reviewed Dec 13, 2024

View reviewed changes

BenPope added 5 commits December 16, 2024 11:06

security: Improve validate_scram_credential

8ace98e

Optimise the password validation when SCRAM-SHA-512 is in use, by avoiding the validation against SCRAM-SHA-256 that will fail. Signed-off-by: Ben Pope <ben@redpanda.com>

security: Introduce plain_authenticator

6cab505

SUCCESS-02 Signed-off-by: Ben Pope <ben@redpanda.com>

config: Add configuration for SASL/PLAIN

08d63c9

Signed-off-by: Ben Pope <ben@redpanda.com>

kafka: Wire up SASL/PLAIN

4beb86a

Signed-off-by: Ben Pope <ben@redpanda.com>

feature_manager: Refactor any_of from absl to std::ranges

ca1b2ec

No change in behaviour Signed-off-by: Ben Pope <ben@redpanda.com>

michael-redpanda force-pushed the sasl-plain/core-8407 branch from 7ac13ca to f051233 Compare December 16, 2024 20:15

michael-redpanda requested a review from BenPope December 16, 2024 20:26

michael-redpanda requested a review from pgellert December 16, 2024 20:26

pgellert previously approved these changes Dec 17, 2024

View reviewed changes

BenPope reviewed Dec 18, 2024

View reviewed changes

michael-redpanda dismissed pgellert’s stale review via 9dec947 December 18, 2024 16:43

michael-redpanda force-pushed the sasl-plain/core-8407 branch from f051233 to 9dec947 Compare December 18, 2024 16:43

BenPope and others added 6 commits December 18, 2024 12:36

feature_manager: Log bad SASL/PLAIN config

588ec77

REQ-02 Signed-off-by: Ben Pope <ben@redpanda.com>

fm/dt: Renamed __REDPANDA_LICENSE_CHECK_INTERVAL_SEC

ef9dcc5

Renamed `__REDPANDA_LICENSE_CHECK_INTERVAL_SEC` with `__REDPANDA_PERIODIC_REMINDER_INTERVAL_SEC` to better reflect what messages this environmental variable is controlling. Signed-off-by: Michael Boquard <michael@redpanda.com>

dt: Added 'PLAIN' to list of simple SASL mechanisms

176570e

This change allows for the selection of SASL/PLAIN as an authentication method using the Python RdKafka library. Signed-off-by: Michael Boquard <michael@redpanda.com>

dt: Permitted selection of SASL algorithm with kafka cli

d888e7e

Signed-off-by: Michael Boquard <michael@redpanda.com>

security/test: Added unit tests for plain authenticator

98d6056

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda force-pushed the sasl-plain/core-8407 branch from 9dec947 to 98d6056 Compare December 18, 2024 17:36

michael-redpanda requested review from pgellert and BenPope December 18, 2024 19:44

pgellert approved these changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-8436]: Add support for SASL/PLAIN #24525

[CORE-8436]: Add support for SASL/PLAIN #24525

michael-redpanda commented Dec 11, 2024 •

edited

Loading

michael-redpanda commented Dec 11, 2024 •

edited

Loading

michael-redpanda Dec 11, 2024

pgellert Dec 13, 2024

BenPope Dec 16, 2024

michael-redpanda commented Dec 11, 2024

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 12, 2024

BenPope Dec 13, 2024

michael-redpanda Dec 13, 2024

BenPope Dec 16, 2024

pgellert left a comment

pgellert Dec 13, 2024

BenPope Dec 16, 2024

pgellert Dec 16, 2024

BenPope Dec 16, 2024

pgellert Dec 16, 2024

michael-redpanda Dec 16, 2024

pgellert Dec 13, 2024

pgellert Dec 13, 2024

michael-redpanda Dec 16, 2024

pgellert Dec 17, 2024

michael-redpanda commented Dec 16, 2024

vbotbuildovich commented Dec 16, 2024 •

edited

Loading

vbotbuildovich commented Dec 17, 2024 •

edited

Loading

pgellert left a comment •

edited

Loading

michael-redpanda commented Dec 17, 2024

BenPope left a comment

BenPope Dec 18, 2024

michael-redpanda Dec 18, 2024

michael-redpanda commented Dec 18, 2024

michael-redpanda commented Dec 18, 2024

vbotbuildovich commented Dec 18, 2024 •

edited

Loading

[CORE-8436]: Add support for SASL/PLAIN #24525

Are you sure you want to change the base?

[CORE-8436]: Add support for SASL/PLAIN #24525

Conversation

michael-redpanda commented Dec 11, 2024 • edited Loading

Backports Required

Release Notes

Features

michael-redpanda commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Dec 11, 2024

vbotbuildovich commented Dec 11, 2024 • edited Loading

vbotbuildovich commented Dec 11, 2024 • edited Loading

Retry command for Build#59603

vbotbuildovich commented Dec 11, 2024 • edited Loading

vbotbuildovich commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgellert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Dec 16, 2024

vbotbuildovich commented Dec 16, 2024 • edited Loading

Retry command for Build#59821

vbotbuildovich commented Dec 17, 2024 • edited Loading

CI test results

pgellert left a comment • edited Loading

Choose a reason for hiding this comment

michael-redpanda commented Dec 17, 2024

BenPope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Dec 18, 2024

michael-redpanda commented Dec 18, 2024

vbotbuildovich commented Dec 18, 2024 • edited Loading

Retry command for Build#59923

michael-redpanda commented Dec 11, 2024 •

edited

Loading

michael-redpanda commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 11, 2024 •

edited

Loading

vbotbuildovich commented Dec 16, 2024 •

edited

Loading

vbotbuildovich commented Dec 17, 2024 •

edited

Loading

pgellert left a comment •

edited

Loading

vbotbuildovich commented Dec 18, 2024 •

edited

Loading