Skip to content
Open
Changes from all commits
Commits
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
15 changes: 13 additions & 2 deletions crates/api-core/src/auth/internal_rbac_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -943,8 +943,8 @@ impl InternalRBACRules {
pub(super) fn allowed(&self, msg: &str, user_principals: &[crate::auth::Principal]) -> bool {
if let Some(perm_info) = self.perms.get(msg) {
if user_principals.is_empty() {
// No proper cert presented, but we will allow stuff that allows just Anonymous
return perm_info.principals.as_slice() == [Principal::Anonymous];
// No proper cert presented, but we allow any rule that lists Anonymous.
return perm_info.principals.contains(&Principal::Anonymous);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a regression test for this case? The existing anonymous test covers a rule containing only Anonymous (DiscoverMachine), while this fix is specifically for rules where Anonymous appears alongside other principals. An assertion that certless requests can access GetJWKS and GetOpenIDConfiguration would prevent this from regressing.

}
user_principals.iter().any(|user_principal| {
perm_info
Expand Down Expand Up @@ -1101,6 +1101,17 @@ mod rbac_rule_tests {
));
}

#[test]
fn anonymous_rules_allow_certless_callers() {
// Certless callers must be allowed when a rule lists Anonymous among other principals.
for method in ["GetJWKS", "GetOpenIDConfiguration"] {
assert!(
InternalRBACRules::allowed_from_static(method, &[]),
"{method}"
);
}
}

#[test]
fn rbac_rule_tests() -> Result<(), eyre::Report> {
assert!(InternalRBACRules::allowed_from_static(
Expand Down
Loading