Skip to content

[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.

Sign up for GitHub

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

Merged
michael-redpanda merged 15 commits into from
Jan 8, 2026
Merged

[CORE-15019] Consider group membership for authz checks #29157

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
20d1a0b
security: Add groups() method to sasl_mechanism
michael-redpanda Dec 15, 2025
cbc7c84
security/oidc: Implement groups method
michael-redpanda Dec 15, 2025
ee219e8
k/server: Add get_groups method to connection_context
michael-redpanda Dec 15, 2025
82d24e3
k/server: Add groups paramter to authorized_user method
michael-redpanda Dec 15, 2025
cf5bcea
security: Add groups to request_auth result
michael-redpanda Dec 16, 2025
eb27943
security: Add groups to authorized method
michael-redpanda Dec 15, 2025
d03d1c7
security: Created authz result for groups
michael-redpanda Dec 16, 2025
8b2e920
security: Refactor authorization to check all effective principals
michael-redpanda Jan 7, 2026
f73689c
security: Handle groups in authz checks
michael-redpanda Dec 16, 2025
8d5b860
dt: Add method to create group_mapper
michael-redpanda Dec 16, 2025
f3fb23e
dt: Add method to create groups in keycloak
michael-redpanda Dec 16, 2025
372ba79
dt: Add method to add a service account to a group
michael-redpanda Dec 16, 2025
fd3ac57
proto: Add groups to ResolveOidcIdentityResponse
michael-redpanda Dec 19, 2025
728e57d
admin/security: Implement groups
michael-redpanda Dec 19, 2025
53d94c1
dt: Added basic group authz test
michael-redpanda Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/v/kafka/server/connection_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,12 @@ security::auth_result connection_context::authorized(
}

return authorized_user(
get_principal(), operation, name, quiet, superuser_required);
get_principal(),
operation,
name,
quiet,
superuser_required,
get_groups());
Copy link
Member

@BenPope BenPope Jan 7, 2026

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.

}

template security::auth_result connection_context::authorized<model::topic>(
Expand Down Expand Up @@ -306,15 +311,17 @@ security::auth_result connection_context::authorized_user(
security::acl_operation operation,
const T& name,
authz_quiet quiet,
superuser_required superuser_required) {
superuser_required superuser_required,
const chunked_vector<security::acl_principal>& groups) {
auto authorized = _server.authorizer().authorized(
name,
operation,
principal,
security::acl_host(_client_addr),
security::superuser_required{
superuser_required ? security::superuser_required::yes
: security::superuser_required::no});
: security::superuser_required::no},
groups);

if (!authorized) {
if (_sasl) {
Expand Down Expand Up @@ -369,31 +376,35 @@ connection_context::authorized_user<model::topic>(
security::acl_operation operation,
const model::topic& name,
authz_quiet quiet,
superuser_required);
superuser_required,
const chunked_vector<security::acl_principal>& groups);

template security::auth_result
connection_context::authorized_user<kafka::group_id>(
security::acl_principal principal,
security::acl_operation operation,
const kafka::group_id& name,
authz_quiet quiet,
superuser_required);
superuser_required,
const chunked_vector<security::acl_principal>& groups);

template security::auth_result
connection_context::authorized_user<kafka::transactional_id>(
security::acl_principal principal,
security::acl_operation operation,
const kafka::transactional_id& name,
authz_quiet quiet,
superuser_required);
superuser_required,
const chunked_vector<security::acl_principal>& groups);

template security::auth_result
connection_context::authorized_user<security::acl_cluster_name>(
security::acl_principal principal,
security::acl_operation operation,
const security::acl_cluster_name& name,
authz_quiet quiet,
superuser_required);
superuser_required,
const chunked_vector<security::acl_principal>& groups);

ss::future<> connection_context::revoke_credentials(std::string_view name) {
if (
Expand Down Expand Up @@ -606,6 +617,15 @@ ss::future<> connection_context::handle_auth_v0(const size_t size) {
co_await conn->write(std::move(msg));
}

const chunked_vector<security::acl_principal>&
connection_context::get_groups() const {
if (_sasl && _sasl->has_mechanism()) {
return _sasl->mechanism().groups();
}
static const chunked_vector<security::acl_principal> empty;
return empty;
}

bool connection_context::is_finished_parsing() const {
return conn->input().eof() || abort_requested();
}
Expand Down
5 changes: 4 additions & 1 deletion src/v/kafka/server/connection_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,8 @@ class connection_context final
security::acl_operation operation,
const T& name,
authz_quiet quiet,
superuser_required superuser_required);
superuser_required superuser_required,
const chunked_vector<security::acl_principal>& groups);

security::acl_principal get_principal() const {
if (_mtls_state) {
Expand All @@ -317,6 +318,8 @@ class connection_context final
return security::acl_principal{security::principal_type::user, {}};
}

const chunked_vector<security::acl_principal>& get_groups() const;

bool is_finished_parsing() const;

// Reserve units from memory from the memory semaphore in proportion
Expand Down
9 changes: 6 additions & 3 deletions src/v/pandaproxy/schema_registry/authorization.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ void handle_authz(
op,
params.principal,
params.host,
security::superuser_required::no);
security::superuser_required::no,
auth_result.get_groups());
},
[&](const detail::no_auth auto&) {
return security::auth_result::authz_disabled(
Expand Down Expand Up @@ -187,7 +188,8 @@ void handle_get_schemas_ids_id_authz(
op,
params.principal,
params.host,
security::superuser_required::no);
security::superuser_required::no,
auth_result.value().get_groups());

if (res.is_authorized()) {
authorizing_result = std::move(res);
Expand Down Expand Up @@ -237,7 +239,8 @@ void handle_get_subjects_authz(
op,
params.principal,
params.host,
security::superuser_required::no);
security::superuser_required::no,
auth_result.value().get_groups());
if (res.is_authorized()) {
passing_results.emplace_back(subject(), subject_resource_type);
return false; // keep
Expand Down
6 changes: 4 additions & 2 deletions src/v/security/audit/schemas/tests/ocsf_schemas_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ BOOST_AUTO_TEST_CASE(make_api_activity_event_authorized) {
security::credential_user{username},
security::credential_password{"password"},
"sasl",
request_auth_result::superuser::no};
request_auth_result::superuser::no,
{}};

auto api_activity = sa::api_activity::construct(
req, auth_result, "http", true, std::nullopt);
Expand Down Expand Up @@ -681,7 +682,8 @@ BOOST_AUTO_TEST_CASE(make_authentication_event_success) {
security::credential_user{username},
security::credential_password{"password"},
"sasl",
request_auth_result::superuser::no};
request_auth_result::superuser::no,
{}};

auto authn = sa::authentication::construct(sa::authentication_event_options {
.auth_protocol = "sasl",
Expand Down
122 changes: 53 additions & 69 deletions src/v/security/authorizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,10 @@ auth_result authorizer::authorized(
acl_operation operation,
const acl_principal& principal,
const acl_host& host,
superuser_required superuser_required) const {
superuser_required superuser_required,
const chunked_vector<acl_principal>& groups) const {
auth_result r = do_authorized(
resource_name, operation, principal, host, superuser_required);
resource_name, operation, principal, host, superuser_required, groups);
_probe->record_authz_result(
r.is_authorized() ? authz_result::allow
: r.empty_matches ? authz_result::empty
Expand All @@ -182,7 +183,8 @@ auth_result authorizer::do_authorized(
acl_operation operation,
const acl_principal& principal,
const acl_host& host,
superuser_required superuser_required) const {
superuser_required superuser_required,
const chunked_vector<acl_principal>&) const {
auto type = get_resource_type<T>();
auto acls = store().find(type, resource_name());

Expand All @@ -205,30 +207,43 @@ auth_result authorizer::do_authorized(
bool(_allow_empty_matches));
}

chunked_vector<acl_principal_view> effective_principals;

const auto append_roles = [&effective_principals,
this](const acl_principal& p) {
std::ranges::copy(
_role_store->roles_for_member(role_member_view::from_principal(p))
| std::views::transform(
[](const auto& r) { return role::to_principal_view(r); }),
std::back_inserter(effective_principals));
};

effective_principals.emplace_back(principal);

// Only users can be a member of roles, not ephemeral_users
if (principal.type() == principal_type::user) {
append_roles(principal);
Copy link
Member

@BenPope BenPope Jan 8, 2026

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.

Copy link
Contributor Author

@michael-redpanda michael-redpanda Jan 8, 2026

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?

Copy link
Contributor Author

@michael-redpanda michael-redpanda Jan 8, 2026

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.

}

auto check_access =
[this, &acls, &operation, &host, &resource_name](
acl_permission perm,
const security::acl_principal& user,
std::optional<const security::acl_principal_base*> role
= std::nullopt) -> std::optional<auth_result> {
vassert(
!role
|| (*role != nullptr && (*role)->type() == principal_type::role),
"Role principal should be non-null and have 'role' type if "
"present");
const acl_principal_base& to_check = *role.value_or(&user);
const acl_principal& user,
acl_principal_view check_principal) -> std::optional<auth_result> {
bool is_allow = perm == acl_permission::allow;
std::optional<security::acl_match> entry;
if (is_allow) {
entry = acl_any_implied_ops_allowed(
acls, to_check, host, operation);
acls, check_principal, host, operation);
} else {
entry = acls.find(operation, to_check, host, perm);
entry = acls.find(operation, check_principal, host, perm);
}

if (!entry) {
return std::nullopt;
}
switch (to_check.type()) {

switch (check_principal.type()) {
case principal_type::user:
case principal_type::ephemeral_user:
return auth_result::acl_match(
Expand All @@ -238,66 +253,29 @@ auth_result authorizer::do_authorized(
case principal_type::group:
return auth_result::role_acl_match(
user,
security::role_name{to_check.name_view()},
security::role_name{check_principal.name_view()},
host,
operation,
resource_name,
is_allow,
*entry);
}
__builtin_unreachable();
std::unreachable();
};

auto check_role_access =
[this, &principal, &check_access](
acl_permission perm,
const acl_principal& user) -> std::optional<auth_result> {
switch (principal.type()) {
case security::principal_type::user: {
auto result
= _role_store->roles_for_member(
security::role_member_view::from_principal(principal))
| std::views::transform(
[](const auto& e) { return role::to_principal_view(e); })
| std::views::transform(
[&user, &check_access, perm](const auto& e) {
return check_access(perm, user, &e);
})
| std::views::filter([](const std::optional<auth_result>& r) {
return r.has_value();
})
| std::views::take(1);
return (result.empty() ? std::nullopt : result.front());
for (const auto& p : effective_principals) {
if (auto r = check_access(acl_permission::deny, principal, p);
r.has_value()) {
return *r;
}
case security::principal_type::ephemeral_user:
case security::principal_type::role:
// TODO(GBAC) - CORE-14895
case security::principal_type::group:
return std::nullopt;
}
__builtin_unreachable();
};

if (auto result = check_access(acl_permission::deny, principal);
result.has_value()) {
return std::move(result).value();
}

if (auto result = check_role_access(acl_permission::deny, principal);
result.has_value()) {
return std::move(result).value();
}

if (auto result = check_access(acl_permission::allow, principal);
result.has_value()) {
return std::move(result).value();
}

if (auto result = check_role_access(acl_permission::allow, principal);
result.has_value()) {
return std::move(result).value();
for (const auto& p : effective_principals) {
if (auto r = check_access(acl_permission::allow, principal, p);
r.has_value()) {
return *r;
}
}

return auth_result::opt_acl_match(
principal, host, operation, resource_name, std::nullopt);
}
Expand All @@ -307,42 +285,48 @@ template auth_result authorizer::authorized(
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>&) const;

template auth_result authorizer::authorized(
const kafka::group_id&,
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>& groups) const;

template auth_result authorizer::authorized(
const security::acl_cluster_name&,
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>&) const;

template auth_result authorizer::authorized(
const kafka::transactional_id&,
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>&) const;

template auth_result authorizer::authorized(
const pandaproxy::schema_registry::subject&,
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>&) const;

template auth_result authorizer::authorized(
const pandaproxy::schema_registry::registry_resource&,
acl_operation,
const acl_principal&,
const acl_host&,
superuser_required) const;
superuser_required,
const chunked_vector<acl_principal>&) const;

std::optional<security::acl_match> authorizer::acl_any_implied_ops_allowed(
const acl_matches& acls,
Expand Down
Loading