diff --git a/src/adapter/src/catalog.rs b/src/adapter/src/catalog.rs index c161785e44a03..838c51f9d43b5 100644 --- a/src/adapter/src/catalog.rs +++ b/src/adapter/src/catalog.rs @@ -928,14 +928,6 @@ impl Catalog { self.state.try_get_role_by_name(role_name) } - pub fn try_get_role_by_name_case_insensitive(&self, role_name: &str) -> Option<&Role> { - self.state.try_get_role_by_name_case_insensitive(role_name) - } - - pub fn roles_by_lowercase_name(&self) -> BTreeMap { - self.state.roles_by_lowercase_name() - } - pub fn try_get_role_auth_by_id(&self, id: &RoleId) -> Option<&RoleAuth> { self.state.try_get_role_auth_by_id(id) } diff --git a/src/adapter/src/catalog/state.rs b/src/adapter/src/catalog/state.rs index 27dd43fc1469f..b3ff4cce8cfc5 100644 --- a/src/adapter/src/catalog/state.rs +++ b/src/adapter/src/catalog/state.rs @@ -914,31 +914,6 @@ impl CatalogState { .map(|id| &self.roles_by_id[id]) } - /// Case-insensitive role lookup for JWT group-to-role sync. - /// - /// Groups from JWTs are lowercased during extraction. SQL role names are - /// stored as-is (unquoted identifiers are lowercased by the parser, but - /// quoted identifiers preserve case). This method iterates all roles to - /// find a case-insensitive match. - pub fn try_get_role_by_name_case_insensitive(&self, role_name: &str) -> Option<&Role> { - let lower = role_name.to_lowercase(); - self.roles_by_name - .iter() - .find(|(name, _)| name.to_lowercase() == lower) - .map(|(_, id)| &self.roles_by_id[id]) - } - - /// Returns a map from lowercased role name to `Role` for all roles. - /// - /// Use this when doing multiple case-insensitive lookups (e.g. iterating - /// JWT group claims) to avoid O(n) per lookup. - pub fn roles_by_lowercase_name(&self) -> BTreeMap { - self.roles_by_name - .iter() - .map(|(name, id)| (name.to_lowercase(), &self.roles_by_id[id])) - .collect() - } - pub(super) fn get_role_auth(&self, id: &RoleId) -> &RoleAuth { self.role_auth_by_id .get(id) diff --git a/src/adapter/src/coord/group_sync.rs b/src/adapter/src/coord/group_sync.rs index 52685143e0e25..a32b5c7c1b723 100644 --- a/src/adapter/src/coord/group_sync.rs +++ b/src/adapter/src/coord/group_sync.rs @@ -44,7 +44,7 @@ pub struct GroupSyncDiff { /// - `current_membership`: The user's current `RoleMembership.map` /// (role_id → grantor_id). /// - `target_role_ids`: Role IDs resolved from the JWT group names via -/// case-insensitive catalog lookup. +/// exact (case-sensitive) catalog lookup. /// /// # Semantics /// - Only roles granted by the JWT sync sentinel (`MZ_JWT_SYNC_ROLE_ID`) @@ -149,8 +149,8 @@ impl Coordinator { /// Syncs the user's role memberships based on JWT group claims. /// - /// Resolves group names to catalog role IDs (case-insensitive), - /// computes the diff against current memberships, and executes + /// Resolves group names to catalog role IDs via exact (case-sensitive) + /// lookup, computes the diff against current memberships, and executes /// grant/revoke operations via `catalog_transact`. /// /// Groups that map to reserved role names (`mz_`/`pg_` prefixes) are @@ -162,13 +162,12 @@ impl Coordinator { groups: &[String], notices: &mut Vec, ) -> Result<(), AdapterError> { - let role_map = self.catalog().roles_by_lowercase_name(); - - // Resolve group names to role IDs (case-insensitive). + // Resolve group names to role IDs (exact, case-sensitive match). let mut target_role_ids = BTreeSet::new(); for group in groups { // Filter out reserved role names (mz_/pg_ prefixes, PUBLIC). - if catalog::is_reserved_role_name(group) { + // Check case-insensitively so "MZ_SYSTEM" is also blocked. + if catalog::is_reserved_role_name(&group.to_lowercase()) { warn!( group = group.as_str(), "OIDC group maps to reserved role name, skipping" @@ -179,7 +178,7 @@ impl Coordinator { continue; } - match role_map.get(&group.to_lowercase()).copied() { + match self.catalog().try_get_role_by_name(group) { Some(role) => { // Skip if the group resolves to the user's own role. This // happens when an IdP echoes the username/email into the diff --git a/src/authenticator/src/oidc.rs b/src/authenticator/src/oidc.rs index 99f5e874b57b1..6cfee658f221e 100644 --- a/src/authenticator/src/oidc.rs +++ b/src/authenticator/src/oidc.rs @@ -251,8 +251,9 @@ impl OidcClaims { /// /// Returns `None` if the claim is absent (skip sync, preserve current state), /// `Some(vec![])` if the claim is present but empty (revoke all sync-granted - /// roles), or `Some(vec![...])` with normalized (lowercased, deduplicated, - /// sorted) group names. + /// roles), or `Some(vec![...])` with deduplicated, sorted group names + /// (exact case preserved — matching against catalog role names is + /// case-sensitive). /// /// Accepts arrays of strings, single strings, or mixed arrays (non-string /// elements are filtered out). Other JSON types are treated as absent. @@ -280,15 +281,14 @@ impl OidcClaims { } }; - let normalized: Vec = raw_groups + let groups: Vec = raw_groups .into_iter() - .map(|g| g.trim().to_lowercase()) .filter(|g| !g.is_empty()) .collect::>() .into_iter() .collect(); - Some(normalized) + Some(groups) } } @@ -752,9 +752,14 @@ mod tests { fn test_groups_mixed_case() { let json = r#"{"sub":"user","iss":"issuer","exp":1234,"aud":"app","groups":["Analytics","PLATFORM_ENG","analytics"]}"#; let claims: OidcClaims = serde_json::from_str(json).unwrap(); + // Case is preserved; "Analytics" and "analytics" are distinct groups. assert_eq!( claims.groups("groups"), - Some(vec!["analytics".to_string(), "platform_eng".to_string()]) + Some(vec![ + "Analytics".to_string(), + "PLATFORM_ENG".to_string(), + "analytics".to_string(), + ]) ); } @@ -849,32 +854,32 @@ mod tests { #[mz_ore::test] fn test_groups_whitespace_only_single_string() { - // Whitespace-only string trims to empty and is filtered out. + // Whitespace-only string is preserved as-is (exact matching, no trim). let json = r#"{"sub":"user","iss":"issuer","exp":1234,"aud":"app","groups":" "}"#; let claims: OidcClaims = serde_json::from_str(json).unwrap(); - assert_eq!(claims.groups("groups"), Some(vec![])); + assert_eq!(claims.groups("groups"), Some(vec![" ".to_string()])); } #[mz_ore::test] fn test_groups_whitespace_names() { - // Leading/trailing whitespace is trimmed from group names. + // Leading/trailing whitespace is preserved (exact matching, no trim). let json = r#"{"sub":"user","iss":"issuer","exp":1234,"aud":"app","groups":[" spaces ","eng"]}"#; let claims: OidcClaims = serde_json::from_str(json).unwrap(); assert_eq!( claims.groups("groups"), - Some(vec!["eng".to_string(), "spaces".to_string()]) + Some(vec![" spaces ".to_string(), "eng".to_string()]) ); } #[mz_ore::test] fn test_groups_unicode_names() { - // Unicode group names should be lowercased correctly + // Unicode group names are preserved as-is (no case folding). let json = r#"{"sub":"user","iss":"issuer","exp":1234,"aud":"app","groups":["Développeurs","INGÉNIEURS"]}"#; let claims: OidcClaims = serde_json::from_str(json).unwrap(); assert_eq!( claims.groups("groups"), - Some(vec!["développeurs".to_string(), "ingénieurs".to_string()]) + Some(vec!["Développeurs".to_string(), "INGÉNIEURS".to_string()]) ); } @@ -895,11 +900,19 @@ mod tests { } #[mz_ore::test] - fn test_groups_case_insensitive_dedup() { - // "Eng" and "eng" and "ENG" should all collapse to one "eng" + fn test_groups_no_case_folding() { + // Case is preserved; "Eng", "eng", "ENG", "eNg" are four distinct groups. let json = r#"{"sub":"user","iss":"issuer","exp":1234,"aud":"app","groups":["Eng","eng","ENG","eNg"]}"#; let claims: OidcClaims = serde_json::from_str(json).unwrap(); - assert_eq!(claims.groups("groups"), Some(vec!["eng".to_string()])); + assert_eq!( + claims.groups("groups"), + Some(vec![ + "ENG".to_string(), + "Eng".to_string(), + "eNg".to_string(), + "eng".to_string(), + ]) + ); } #[mz_ore::test] diff --git a/test/http-auth/listener_config_oidc.json b/test/http-auth/listener_config_oidc.json new file mode 100644 index 0000000000000..de9768ebad588 --- /dev/null +++ b/test/http-auth/listener_config_oidc.json @@ -0,0 +1,44 @@ +{ + "sql": { + "internal": { + "addr": "0.0.0.0:6877", + "authenticator_kind": "None", + "allowed_roles": "Internal", + "enable_tls": false + } + }, + "http": { + "external": { + "addr": "0.0.0.0:6876", + "authenticator_kind": "Oidc", + "allowed_roles": "NormalAndInternal", + "enable_tls": false, + "routes": { + "base": true, + "webhook": false, + "internal": false, + "metrics": false, + "profiling": false, + "mcp_agent": false, + "mcp_developer": false, + "console_config": false + } + }, + "internal": { + "addr": "0.0.0.0:6878", + "authenticator_kind": "None", + "allowed_roles": "NormalAndInternal", + "enable_tls": false, + "routes": { + "base": false, + "webhook": false, + "internal": false, + "metrics": true, + "profiling": false, + "mcp_agent": false, + "mcp_developer": false, + "console_config": false + } + } + } +} diff --git a/test/http-auth/mzcompose.py b/test/http-auth/mzcompose.py index f533adfb6dad9..21ac53c16c828 100644 --- a/test/http-auth/mzcompose.py +++ b/test/http-auth/mzcompose.py @@ -7,13 +7,112 @@ # the Business Source License, use of this software will be governed # by the Apache License, Version 2.0. +import base64 +import json +import time + +import jwt import requests +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa from materialize import MZ_ROOT from materialize.mzcompose.composition import Composition +from materialize.mzcompose.service import Service from materialize.mzcompose.services.materialized import Materialized +OIDC_PORT = 8443 +OIDC_USER = "alice@example.com" +OIDC_KID = "test-key-1" +OIDC_ISSUER = f"http://oidc-mock:{OIDC_PORT}" +OIDC_KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +def _jwks(): + pub = OIDC_KEY.public_key().public_numbers() + n = pub.n.to_bytes((pub.n.bit_length() + 7) // 8, "big") + e = pub.e.to_bytes((pub.e.bit_length() + 7) // 8, "big") + enc = base64.urlsafe_b64encode + return json.dumps( + { + "keys": [ + { + "kty": "RSA", + "kid": OIDC_KID, + "use": "sig", + "alg": "RS256", + "n": enc(n).rstrip(b"=").decode(), + "e": enc(e).rstrip(b"=").decode(), + } + ] + } + ) + + +def _make_jwt(groups): + pem = OIDC_KEY.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + now = int(time.time()) + return jwt.encode( + { + "iss": OIDC_ISSUER, + "sub": OIDC_USER, + "groups": groups, + "exp": now + 3600, + "iat": now, + }, + pem, + algorithm="RS256", + headers={"kid": OIDC_KID, "alg": "RS256"}, + ) + + +def _http_sql(port, token, query): + r = requests.post( + f"http://localhost:{port}/api/sql", + headers={"Authorization": f"Bearer {token}"}, + json={"query": query}, + ) + assert r.status_code == 200, f"HTTP SQL failed: {r.status_code} {r.text}" + return r.json()["results"][0] + + +class OidcMock(Service): + def __init__(self): + super().__init__( + name="oidc-mock", + config={ + "image": "python:3.12-slim", + "command": [ + "python3", + "/mock/oidc_mock_server.py", + OIDC_ISSUER, + _jwks(), + str(OIDC_PORT), + ], + "ports": [OIDC_PORT], + "volumes": [ + f"{MZ_ROOT}/test/http-auth/oidc_mock_server.py:/mock/oidc_mock_server.py:ro" + ], + "healthcheck": { + "test": [ + "CMD", + "python3", + "-c", + f"import urllib.request; urllib.request.urlopen('http://localhost:{OIDC_PORT}/.well-known/openid-configuration')", + ], + "interval": "1s", + "start_period": "10s", + }, + }, + ) + + SERVICES = [ + OidcMock(), Materialized(), ] @@ -98,3 +197,55 @@ def sql_user(url, query, user=None): "permission denied" in r.json()["results"][0]["error"]["message"].lower() ) + + for name in c.workflows: + if name == "default": + continue + with c.test_case(name): + c.workflow(name) + + +def workflow_oidc_group_sync_case_sensitive(c: Composition) -> None: + """Regression test for SQL-276: OIDC group sync must use exact (case-sensitive) + role name matching. A JWT group "Admin" must NOT grant access to a role named + "admin" — only to a role named exactly "Admin".""" + with c.override( + Materialized( + listeners_config_path=f"{MZ_ROOT}/test/http-auth/listener_config_oidc.json", + additional_system_parameter_defaults={ + "oidc_issuer": OIDC_ISSUER, + "oidc_authentication_claim": "sub", + "oidc_group_role_sync_enabled": "true", + "oidc_group_claim": "groups", + }, + depends_on=["oidc-mock"], + sanity_restart=False, + ) + ): + c.up("oidc-mock") + c.up("materialized") + admin = c.sql_cursor(service="materialized", port=6877, user="mz_system") + http_port = c.port("materialized", 6876) + + admin.execute("CREATE ROLE admin") + admin.execute("CREATE TABLE secrets (data TEXT)") + admin.execute("INSERT INTO secrets VALUES ('top-secret')") + admin.execute("GRANT SELECT ON TABLE secrets TO admin") + admin.execute('CREATE ROLE "Admin"') + + with c.test_case("case_collision_no_privilege_escalation"): + # JWT group "Admin" must match only the "Admin" role (no privileges), + # not the lowercase "admin" role (which has SELECT on secrets). + token = _make_jwt(["Admin"]) + result = _http_sql(http_port, token, "SELECT data FROM secrets") + assert ( + "error" in result + ), f"attacker should NOT be able to read secrets via case collision, but got: {result}" + + with c.test_case("exact_match_grants_correct_role"): + # JWT group "admin" (exact match) must grant the lowercase "admin" role. + token = _make_jwt(["admin"]) + result = _http_sql(http_port, token, "SELECT data FROM secrets") + assert "rows" in result and result["rows"] == [ + ["top-secret"] + ], f"user with group 'admin' should be able to read secrets, but got: {result}" diff --git a/test/http-auth/oidc_mock_server.py b/test/http-auth/oidc_mock_server.py new file mode 100644 index 0000000000000..3612cf2395626 --- /dev/null +++ b/test/http-auth/oidc_mock_server.py @@ -0,0 +1,39 @@ +# Copyright Materialize, Inc. and contributors. All rights reserved. +# +# Use of this software is governed by the Business Source License +# included in the LICENSE file at the root of this repository. +# +# 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. + +import json +import sys +from http.server import BaseHTTPRequestHandler, HTTPServer + +ROUTES = {} + + +class Handler(BaseHTTPRequestHandler): + def do_GET(self): + if self.path not in ROUTES: + self.send_error(404) + return + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(ROUTES[self.path].encode()) + + def log_message(self, *_): + pass + + +if __name__ == "__main__": + issuer, jwks_json = sys.argv[1], sys.argv[2] + port = int(sys.argv[3]) if len(sys.argv) > 3 else 8443 + ROUTES["/.well-known/openid-configuration"] = json.dumps( + {"issuer": issuer, "jwks_uri": f"{issuer}/.well-known/jwks.json"} + ) + ROUTES["/.well-known/jwks.json"] = jwks_json + print(f"OIDC mock listening on :{port}", flush=True) + HTTPServer(("0.0.0.0", port), Handler).serve_forever()