Validation and url_decode for SCRAM usernames

src/v/redpanda/admin/security.cc

@@ -20,6 +20,8 @@
 #include "security/scram_credential.h"
 
 #include <seastar/coroutine/as_future.hh>
+#include <seastar/http/exception.hh>
+#include <seastar/http/url.hh>
 
 namespace {
 
@@ -172,6 +174,11 @@ admin_server::create_user_handler(std::unique_ptr<ss::http::request> 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(
@@ -211,7 +218,12 @@ admin_server::delete_user_handler(std::unique_ptr<ss::http::request> 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);
@@ -238,7 +250,12 @@ admin_server::update_user_handler(std::unique_ptr<ss::http::request> 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());
 

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<ss::sstring> 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

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<hmac_sha512, hash_sha512, 130, 4096>;
 

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]
@@ -88,10 +94,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 +466,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 +479,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):
         """
@@ -500,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'