-
Notifications
You must be signed in to change notification settings - Fork 709
[CORE-15019] Consider group membership for authz checks #29157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CORE-15019] Consider group membership for authz checks #29157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements OIDC group-based authorization for Redpanda, enabling administrators to grant topic access permissions based on OIDC group membership claims. The implementation adds support for extracting group information from OIDC tokens, configuring how nested groups are handled, and authorizing requests based on Group:* ACL principals. The changes span both core security logic and test infrastructure.
Key Changes
- Extended OIDC authentication to extract and propagate group membership claims throughout the authorization pipeline
- Modified the authorizer to evaluate
Group:*ACL principals alongside user and role principals, with proper precedence handling (deny ACLs take priority) - Added comprehensive C++ unit tests and ducktape integration tests to validate group-based authorization behavior
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/rptest/tests/redpanda_oauth_test.py |
Added two ducktape tests validating group claim mapping and dynamic group membership changes |
tests/rptest/services/keycloak.py |
Added helper methods for creating group mappers and managing service user group membership |
tests/rptest/clients/admin/proto/redpanda/core/admin/v2/security_pb2.pyi |
Updated protobuf type stubs to include groups field in OIDC identity response |
tests/rptest/clients/admin/proto/redpanda/core/admin/v2/security_pb2.py |
Updated generated protobuf code for groups field |
tests/rptest/clients/admin/proto/redpanda/core/admin/v2/__init__.pyi |
Added internal module import |
src/v/security/tests/role_store_bench.cc |
Updated benchmark tests to pass empty groups parameter |
src/v/security/tests/authorizer_test.cc |
Added 15 new test cases for group authorization and updated existing tests to pass groups parameter |
src/v/security/sasl_authentication.h |
Added virtual groups() method to sasl_mechanism base class |
src/v/security/request_auth.h |
Added groups field to request_auth_result class |
src/v/security/request_auth.cc |
Updated authentication to extract and propagate groups from OIDC tokens |
src/v/security/oidc_authenticator.h |
Implemented groups() override in OIDC SASL authenticator |
src/v/security/authorizer.h |
Added group field to auth_result and groups parameter to authorization methods |
src/v/security/authorizer.cc |
Refactored authorization logic to evaluate groups alongside users and roles |
src/v/security/audit/schemas/tests/ocsf_schemas_test.cc |
Updated audit schema tests to pass empty groups |
src/v/redpanda/admin/services/security.cc |
Added groups to OIDC identity resolution admin API response |
src/v/pandaproxy/schema_registry/authorization.cc |
Updated schema registry authorization to pass groups |
src/v/kafka/server/connection_context.h |
Added groups parameter to authorization methods |
src/v/kafka/server/connection_context.cc |
Implemented get_groups() and passed groups to authorizer |
proto/redpanda/core/admin/v2/security.proto |
Added groups field to ResolveOidcIdentityResponse proto definition |
Comments suppressed due to low confidence (1)
tests/rptest/tests/redpanda_oauth_test.py:1
- The
next()call will raiseStopIterationif no group matches the name, but the code then checksif group is None. This logic is unreachable - if no group is found, the exception occurs before the None check. Usenext(g for g in groups if g["name"] == group_name, None)with a default value to properly handle the case when no group is found.
# Copyright 2023 Redpanda Data, Inc.
df0f13b to
f07ac25
Compare
|
Force push:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
|
|
||
| def _get_group_id(self, group_name: str) -> str | None: | ||
| groups = self.kc_admin.get_groups() | ||
| group = next(g for g in groups if g["name"] == group_name) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next() call on line 203 will raise StopIteration if no group with the given name is found, but line 204 checks if group is None, which is unreachable. Use next((...), None) with a default value to properly handle the case when no group is found.
| group = next(g for g in groups if g["name"] == group_name) | |
| group = next((g for g in groups if g["name"] == group_name), None) |
f07ac25 to
91a8bba
Compare
|
Force push:
|
BenPope
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look heavily at the tests (but coverage is great), or keycloak stuff. LGTM.
| name, | ||
| quiet, | ||
| superuser_required, | ||
| get_groups()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of the ordering here, I feel like it should be principal, groups, ... , but this overload set is tricky.
91a8bba to
b4ec01d
Compare
|
Force push:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
| self.logger.debug(f"client_id: {id}") | ||
| return id | ||
|
|
||
| def create_group_mapper(self, client_id: str, use_full_path: bool = True): |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring explaining the purpose of this method, including what the use_full_path parameter controls and how it affects the group claims in tokens.
| def create_group_mapper(self, client_id: str, use_full_path: bool = True): | |
| def create_group_mapper(self, client_id: str, use_full_path: bool = True): | |
| """ | |
| Ensure that the client has an OIDC group membership protocol mapper. | |
| The mapper adds the client's group memberships to the ``groups`` claim | |
| in ID, access, and userinfo tokens. When ``use_full_path`` is ``True``, | |
| groups are emitted as their full path (for example ``/parent/child``); | |
| when ``False``, only the simple group name is used. This flag is | |
| propagated to the Keycloak mapper's ``full.path`` configuration. | |
| """ |
| parent: str | None = None, | ||
| skip_exists: bool = True, | ||
| **kwargs, | ||
| ) -> str: |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring explaining what this method does, especially the meaning of skip_exists and parent parameters, and what the return value represents.
| ) -> str: | |
| ) -> str: | |
| """ | |
| Create a Keycloak group and return its identifier. | |
| This method creates a group with the given ``group_name`` and any additional | |
| attributes provided via ``kwargs``. | |
| :param group_name: Name of the group to create. | |
| :param parent: Optional identifier of the parent group under which the new | |
| group should be created. If ``None``, the group is created at the root | |
| level of the realm. | |
| :param skip_exists: If ``True``, treat an already existing group with the | |
| same name as success and return its ID. If ``False``, a failure to | |
| create the group (for example, because it already exists) will cause a | |
| ``RuntimeError`` to be raised. | |
| :param kwargs: Additional fields to include in the group representation | |
| passed to Keycloak. | |
| :return: The Keycloak identifier of the created group, or of the existing | |
| group if ``skip_exists`` is ``True`` and the group already exists. | |
| :raises RuntimeError: If the group cannot be created and ``skip_exists`` is | |
| ``False``. | |
| """ |
To be used by OIDC authenticator to return the list of groups present in the OIDC token. Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Consolidate the authorization logic by building an effective_principals list that includes the user and their roles. Simplify the check_access function to work with principal views directly, eliminating the separate check_role_access function. This reduces duplication and clarifies the ACL matching flow across both allow and deny permissions. Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
This mapper is used to insert group claims into OIDC tokens Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
Signed-off-by: Michael Boquard <[email protected]>
b4ec01d to
53d94c1
Compare
|
Force push:
|
nguyen-andrew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests and refactor of authorizer::do_authorized!
| // Create many groups (simulate realistic scenario) | ||
| chunked_vector<acl_principal> many_groups; | ||
| std::vector<acl_binding> bindings; | ||
|
|
||
| for (int i = 0; i < 50; ++i) { | ||
| acl_principal group(principal_type::group, fmt::format("group{}", i)); | ||
| many_groups.push_back(group); | ||
|
|
||
| // Only group42 has permissions | ||
| if (i == 42) { | ||
| acl_entry allow_read( | ||
| group, | ||
| acl_host::wildcard_host(), | ||
| acl_operation::read, | ||
| acl_permission::allow); | ||
|
|
||
| resource_pattern resource( | ||
| resource_type::topic, default_topic(), pattern_type::literal); | ||
| bindings.emplace_back(resource, allow_read); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit tweak
| // Create many groups (simulate realistic scenario) | |
| chunked_vector<acl_principal> many_groups; | |
| std::vector<acl_binding> bindings; | |
| for (int i = 0; i < 50; ++i) { | |
| acl_principal group(principal_type::group, fmt::format("group{}", i)); | |
| many_groups.push_back(group); | |
| // Only group42 has permissions | |
| if (i == 42) { | |
| acl_entry allow_read( | |
| group, | |
| acl_host::wildcard_host(), | |
| acl_operation::read, | |
| acl_permission::allow); | |
| resource_pattern resource( | |
| resource_type::topic, default_topic(), pattern_type::literal); | |
| bindings.emplace_back(resource, allow_read); | |
| } | |
| } | |
| constexpr auto num_groups = 50; | |
| constexpr auto selected_group_idx = 42; | |
| static_assert(selected_group_idx < num_groups, "selected_group_idx must be less than num_groups"); | |
| // Create many groups (simulate realistic scenario) | |
| chunked_vector<acl_principal> many_groups; | |
| std::vector<acl_binding> bindings; | |
| for (auto i : std::views::iota(0, num_groups)) { | |
| many_groups.emplace_back( | |
| principal_type::group, | |
| ssx::sformat("group{}", i)); | |
| } | |
| // Only selected group has permissions | |
| acl_entry allow_read( | |
| many_groups[selected_group_idx], | |
| acl_host::wildcard_host(), | |
| acl_operation::read, | |
| acl_permission::allow); | |
| resource_pattern resource( | |
| resource_type::topic, default_topic(), pattern_type::literal); | |
| bindings.emplace_back(resource, allow_read); |
BenPope
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM iff:
- Call append_roles for groups is intended for a future PR
- The perf regression is acceptable
|
|
||
| std::ranges::for_each(groups, [&effective_principals](const auto& g) { | ||
| effective_principals.emplace_back(g); | ||
| // TODO(gbac) Call append_roles for groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended for another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct - we still don't support groups in roles but that's coming very soon
|
|
||
| // Only users can be a member of roles, not ephemeral_users | ||
| if (principal.type() == principal_type::user) { | ||
| append_roles(principal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: There is a potential performance regression here: The collection of roles is now performed eagerly, even if the match would have been on the principal rather than a role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm that's a fair point... maybe the principal needs to be checked first, then groups, and then build the roles list... wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and filed CORE-15238 to come back to this - trying to get this into the QE teams hands asap for system testing and this PR goes a long way to getting them started.
Summary
This PR implements OIDC group-based authorization for Redpanda, allowing administrators to grant topic access permissions based on group membership claims from identity providers like Keycloak. Users in specific IdP groups can now be authorized to access topics via
Group:<group-name>ACL principals, without needing to configure per-user ACLs.Changes
Core Security Changes
groupsclaim from OIDC/JWT tokens, configurable viaoidc_group_claim_pathnested_group_behaviorconfig option (noneorsuffix) to control how hierarchical group paths are processedGroup:*ACL principals against a user's group membershipgroupsfield toResolveOidcIdentityResponseto expose resolved group memberships via the admin APIKafka Connection Flow
sasl_mechanismwith agroups()method to retrieve group membershipsconnection_contextandrequest_authto flow group info through authorization checksauthorized_usermethod to accept groups parameter for ACL evaluationDucktape Test Infrastructure
Added new helper methods to the Keycloak test service:
create_group_mapper()- Creates an OIDC protocol mapper to include groups in tokenscreate_group()- Creates groups in Keycloakadd_service_user_to_group()- Adds a service account to a groupremove_service_user_from_group()- Removes a service account from a groupNew Tests
test_group_claim: Matrix test validating group claim mapping with differentfull_groupandnested_group_modecombinationstest_group_membership_change: End-to-end test verifying that dynamic group membership changes correctly affect topic visibility:group1→ can seetopic1onlygroup2→ can seetopic2onlygroup3(no permissions) → cannot see any topicsBackports Required
Release Notes