From 75b0cb6bf4ea12e0574e014e89c143e3fd41c105 Mon Sep 17 00:00:00 2001 From: Oren Leiman Date: Thu, 30 Nov 2023 19:55:35 -0800 Subject: [PATCH 1/4] admin/security: Add SCRAM username validation to create_user_handler Includes simple validator against SASLNAME regex in scram_algorithm.cc --- src/v/redpanda/admin/security.cc | 6 ++++++ src/v/security/scram_algorithm.cc | 20 ++++++++++++++++++++ src/v/security/scram_algorithm.h | 2 ++ 3 files changed, 28 insertions(+) diff --git a/src/v/redpanda/admin/security.cc b/src/v/redpanda/admin/security.cc index 13ce19784ce5..8c9e1d0c9e50 100644 --- a/src/v/redpanda/admin/security.cc +++ b/src/v/redpanda/admin/security.cc @@ -20,6 +20,7 @@ #include "security/scram_credential.h" #include +#include namespace { @@ -172,6 +173,11 @@ admin_server::create_user_handler(std::unique_ptr req) { auto username = security::credential_user(doc["username"].GetString()); validate_no_control(username(), string_conversion_exception{username()}); + if (!security::validate_scram_username(username())) { + throw ss::httpd::bad_request_exception( + fmt::format("Invalid SCRAM username {{{}}}", username())); + } + if (is_no_op_user_write( _controller->get_credential_store().local(), username, credential)) { vlog( diff --git a/src/v/security/scram_algorithm.cc b/src/v/security/scram_algorithm.cc index 0d65ec6ead87..d4d86185f41e 100644 --- a/src/v/security/scram_algorithm.cc +++ b/src/v/security/scram_algorithm.cc @@ -42,6 +42,8 @@ // NOLINTNEXTLINE #define SASLNAME "(?:" VALUE_SAFE_CHAR "|=2C|=3D)+" +#define BARE_SASLNAME "(" SASLNAME ")" + // value-char = value-safe-char / "=" // value = 1*value-char // NOLINTNEXTLINE @@ -187,6 +189,19 @@ parse_server_final(std::string_view message) { return server_final_match{.error = ss::sstring(spv(error))}; } +static std::optional parse_saslname(std::string_view message) { + static thread_local const re2::RE2 re(BARE_SASLNAME, re2::RE2::Quiet); + vassert(re.ok(), "saslname regex failure: {}", re.error()); + + re2::StringPiece username; + + if (!re2::RE2::FullMatch(message, re, &username)) { + return std::nullopt; + } + + return ss::sstring(spv(username)); +} + namespace security { client_first_message::client_first_message(bytes_view data) { @@ -343,4 +358,9 @@ server_final_message::server_final_message(bytes_view data) { _signature = std::move(match->signature); } +bool validate_scram_username(std::string_view username) { + auto match = parse_saslname(username); + return match.has_value() && match.value() == username; +} + } // namespace security diff --git a/src/v/security/scram_algorithm.h b/src/v/security/scram_algorithm.h index ccda3f1a7229..b84c3c49da19 100644 --- a/src/v/security/scram_algorithm.h +++ b/src/v/security/scram_algorithm.h @@ -346,6 +346,8 @@ class scram_algorithm { } }; +bool validate_scram_username(std::string_view username); + // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers) using scram_sha512 = scram_algorithm; From 4350ec2cd1259c5bf26d0d08a71c165217e8ab64 Mon Sep 17 00:00:00 2001 From: Oren Leiman Date: Thu, 30 Nov 2023 19:59:51 -0800 Subject: [PATCH 2/4] scram_test: Add test case for excluded username chars --- tests/rptest/tests/scram_test.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/rptest/tests/scram_test.py b/tests/rptest/tests/scram_test.py index 4a541c6cdee1..4065d5d4a11d 100644 --- a/tests/rptest/tests/scram_test.py +++ b/tests/rptest/tests/scram_test.py @@ -88,10 +88,11 @@ def gen(length): self.logger.debug(f"User Creation Arguments: {data}") res = requests.post(url, json=data) - assert res.status_code == expected_status_code + assert res.status_code == expected_status_code, f"Expected {expected_status_code}, got {res.status_code}: {res.content}" if err_msg is not None: - assert res.json()['message'] == err_msg + assert res.json( + )['message'] == err_msg, f"{res.json()['message']} != {err_msg}" return password @@ -459,7 +460,8 @@ def generate_string_with_control_character(length: int): @cluster(num_nodes=3) def test_invalid_user_name(self): """ - Validates that usernames that contain control characters are properly rejected + Validates that usernames that contain control characters and usernames which + do not match the SCRAM regex are properly rejected """ username = self.generate_string_with_control_character(15) @@ -471,6 +473,15 @@ def test_invalid_user_name(self): f'Parameter contained invalid control characters: {username.translate(CONTROL_CHARS_MAP)}' ) + # Two ordinals (corresponding to ',' and '=') are explicitly excluded from SASL usernames + for ordinal in [0x2c, 0x3d]: + username = f"john{chr(ordinal)}doe" + self.create_user( + username=username, + algorithm='SCRAM-SHA-256', + expected_status_code=400, + err_msg=f'Invalid SCRAM username {"{" + username + "}"}') + @cluster(num_nodes=3) def test_invalid_alg(self): """ From 93d575f427f38e2af86d4bd3a6f91b8369e9ebd2 Mon Sep 17 00:00:00 2001 From: Oren Leiman Date: Thu, 30 Nov 2023 20:01:43 -0800 Subject: [PATCH 3/4] admin/security: Unescape characters in username path params - `DELETE /v1/security/users/{user}` - `PUT /v1/security/users/{user}` --- src/v/redpanda/admin/security.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/v/redpanda/admin/security.cc b/src/v/redpanda/admin/security.cc index 8c9e1d0c9e50..8d7a564b6a89 100644 --- a/src/v/redpanda/admin/security.cc +++ b/src/v/redpanda/admin/security.cc @@ -21,6 +21,7 @@ #include #include +#include namespace { @@ -217,7 +218,12 @@ admin_server::delete_user_handler(std::unique_ptr req) { throw co_await redirect_to_leader(*req, model::controller_ntp); } - auto user = security::credential_user(req->param["user"]); + ss::sstring user_v; + if (!ss::http::internal::url_decode(req->param["user"], user_v)) { + throw ss::httpd::bad_param_exception{fmt::format( + "Invalid parameter 'user' got {{{}}}", req->param["user"])}; + } + auto user = security::credential_user(user_v); if (!_controller->get_credential_store().local().contains(user)) { vlog(adminlog.debug, "User '{}' already gone during deletion", user); @@ -244,7 +250,12 @@ admin_server::update_user_handler(std::unique_ptr req) { throw co_await redirect_to_leader(*req, model::controller_ntp); } - auto user = security::credential_user(req->param["user"]); + ss::sstring user_v; + if (!ss::http::internal::url_decode(req->param["user"], user_v)) { + throw ss::httpd::bad_param_exception{fmt::format( + "Invalid parameter 'user' got {{{}}}", req->param["user"])}; + } + auto user = security::credential_user(user_v); auto doc = co_await parse_json_body(req.get()); From 8f40bf0c272d495bd865774ab2d2cd3d4a05aa72 Mon Sep 17 00:00:00 2001 From: Oren Leiman Date: Thu, 30 Nov 2023 20:09:01 -0800 Subject: [PATCH 4/4] scram_test: Add tests for escaped usernames - Create usernames with characters that will require URL escaping. - Verify that these users can be updated/deleted --- tests/rptest/tests/scram_test.py | 94 +++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 3 deletions(-) diff --git a/tests/rptest/tests/scram_test.py b/tests/rptest/tests/scram_test.py index 4065d5d4a11d..77c427afea00 100644 --- a/tests/rptest/tests/scram_test.py +++ b/tests/rptest/tests/scram_test.py @@ -12,10 +12,12 @@ import requests from requests.exceptions import HTTPError import time +import urllib.parse import re from ducktape.mark import parametrize from ducktape.utils.util import wait_until +from ducktape.errors import TimeoutError from rptest.services.cluster import cluster from rptest.tests.redpanda_test import RedpandaTest @@ -33,11 +35,13 @@ class BaseScramTest(RedpandaTest): def __init__(self, test_context, **kwargs): super(BaseScramTest, self).__init__(test_context, **kwargs) - def update_user(self, username): + def update_user(self, username, quote: bool = True): def gen(length): return "".join( random.choice(string.ascii_letters) for _ in range(length)) + if quote: + username = urllib.parse.quote(username, safe='') password = gen(20) controller = self.redpanda.nodes[0] @@ -52,11 +56,13 @@ def gen(length): assert res.status_code == 200 return password - def delete_user(self, username): + def delete_user(self, username, quote: bool = True): + if quote: + username = urllib.parse.quote(username, safe='') controller = self.redpanda.nodes[0] url = f"http://{controller.account.hostname}:9644/v1/security/users/{username}" res = requests.delete(url) - assert res.status_code == 200 + assert res.status_code == 200, f"Status code: {res.status_code} for DELETE user {username}" def list_users(self): controller = self.redpanda.nodes[0] @@ -511,6 +517,88 @@ def test_invalid_password(self): err_msg='Parameter contained invalid control characters: PASSWORD') +class EscapedNewUserStrings(BaseScramTest): + + # All of the non-control characters that need escaping + NEED_ESCAPE = [ + '!', + '"', + '#', + '$', + '%', + '&', + '\'', + '(', + ')', + '+', + # ',', Excluded by SASLNAME regex + '/', + ':', + ';', + '<', + # '=', Excluded by SASLNAME regex + '>', + '?', + '[', + '\\', + ']', + '^', + '`', + '{', + '}', + '~', + ] + + @cluster(num_nodes=3) + def test_update_delete_user(self): + """ + Verifies that users whose names contain characters which require URL escaping can be subsequently deleted. + i.e. that the username included with a delete request is properly unescaped by the admin server. + """ + + su_username = self.redpanda.SUPERUSER_CREDENTIALS.username + + users = [] + + self.logger.debug( + "Create some users with names that will require URL escaping") + + for ch in self.NEED_ESCAPE: + username = f"john{ch}doe" + self.create_user(username=username, + algorithm="SCRAM-SHA-256", + password="passwd", + expected_status_code=200) + users.append(username) + + admin = Admin(self.redpanda) + + def _users_match(expected: list[str]): + live_users = admin.list_users() + live_users.remove(su_username) + return len(expected) == len(live_users) and set(expected) == set( + live_users) + + wait_until(lambda: _users_match(users), timeout_sec=5, backoff_sec=0.5) + + self.logger.debug( + "We should be able to update and delete these users without issue") + for username in users: + self.update_user(username=username) + self.delete_user(username=username) + + try: + wait_until(lambda: _users_match([]), + timeout_sec=5, + backoff_sec=0.5) + except TimeoutError: + live_users = admin.list_users() + live_users.remove(su_username) + assert len( + live_users + ) == 0, f"Expected no users, got {len(live_users)}: {live_users}" + + class SCRAMReauthTest(BaseScramTest): EXAMPLE_TOPIC = 'foo'