[SSO] Fix OIDC group sync privilege escalation via case folding#36509
Open
mtabebe wants to merge 1 commit into
Open
[SSO] Fix OIDC group sync privilege escalation via case folding#36509mtabebe wants to merge 1 commit into
mtabebe wants to merge 1 commit into
Conversation
mtabebe
force-pushed
the
ma/sso/fix-lowercase
branch
4 times, most recently
from
ea0ef62 to
b334d8c
Compare
SangJunBak
reviewed
May 11, 2026
Comment on lines
+169
to
+170
| // Check case-insensitively so "MZ_SYSTEM" is also blocked. | ||
| if catalog::is_reserved_role_name(&group.to_lowercase()) { |
Contributor
There was a problem hiding this comment.
I wonder if we need to check this case? Thinking the "No matching Materialize Role" warning is fine
Contributor
Author
There was a problem hiding this comment.
I think we should still warn as early as possible? Yes there is a false positive situation, but I feel like it is better to warn in these cases
SangJunBak
reviewed
May 11, 2026
| let groups: Vec<String> = raw_groups | ||
| .into_iter() | ||
| .map(|g| g.trim().to_lowercase()) | ||
| .map(|g| g.trim().to_string()) |
Contributor
There was a problem hiding this comment.
Should we still trim the groups? Given we're not normalizing anymore. E.g. there could exist a role " wee wwoo "
Problem: JWT group names were lowercased before catalog lookup. When two roles differ only by case (e.g. `admin` and `"Admin"`), the lowercase map built by `roles_by_lowercase_name` silently picked the wrong role due to BTreeMap last-writer-wins semantics. Solution: - remove `to_lowercase()` from group extraction, replace the lowercase-map lookup in `sync_jwt_groups` with exact `try_get_role_by_name`, - delete the now-unused `roles_by_lowercase_name` and `try_get_role_by_name_case_insensitive` methods Testing: - updated three oidc.rs unit tests - added mzcompose workflow `oidc_group_sync_case_sensitive` with an OIDC mock server to verify "Admin" cannot escalate to the `admin` role
mtabebe
force-pushed
the
ma/sso/fix-lowercase
branch
from
b334d8c to
2c22d3c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
JWT group names were lowercased before catalog lookup. When two roles differ only by case (e.g.
adminand"Admin"), the lowercase map built byroles_by_lowercase_namesilently picked the wrong role due to BTreeMap last-writer-wins semantics.Solution:
to_lowercase()from group extraction, replace the lowercase-map lookup insync_jwt_groupswith exacttry_get_role_by_name,roles_by_lowercase_nameandtry_get_role_by_name_case_insensitivemethodsTesting:
oidc_group_sync_case_sensitivewith an OIDC mock server to verify "Admin" cannot escalate to theadminroleRemove these sections if your commit already has a good description!
Fixes: SQL-276