Skip to content
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

fix bug in acl config validation #982

Merged
merged 10 commits into from
Apr 30, 2024
Merged

fix bug in acl config validation #982

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
76220b7
fix bug in acl config validation
kauncoder Apr 26, 2024
770d364
Update ACL config error messages
oteffahi Apr 29, 2024
52fa51f
Apply suggestions from code review
Mallets Apr 29, 2024
05feb4c
Update zenoh/src/net/routing/interceptor/authorization.rs
Mallets Apr 29, 2024
8934690
Fix mistake in ACL config error message
oteffahi Apr 29, 2024
a2db2d7
Allow empty interfaces and flows in config
oteffahi Apr 29, 2024
5008e38
Add warning logs for implicit wildcard ACL config
oteffahi Apr 29, 2024
e204f2d
Fix linting
oteffahi Apr 29, 2024
c7bec9a
Add support for undefined interfaces and flows lists
oteffahi Apr 30, 2024
f56829d
Error out when ACL cannot be enabled
oteffahi Apr 30, 2024
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion zenoh/src/net/routing/interceptor/access_control.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub(crate) fn acl_interceptor_factories(
enforcer: Arc::new(policy_enforcer),
}))
}
Err(e) => tracing::error!("Access control inizialization error: {}", e),
Err(e) => tracing::error!("Access control not enabled due to: {}", e),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a security feature that the user wished to enable did not go through correctly, in my opinion it seems dangerous to proceed. Shouldn't we stop execution?

Suggested change
Err(e) => tracing::error!("Access control not enabled due to: {}", e),
Err(e) => panic!("Access control not enabled due to: {}", e),

Copy link
Member

Choose a reason for hiding this comment

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

I think the upper logic should decide what to do with the execution. The interceptor has the freedom to return an error here and it should do so. I wouldn't panic directly at this level.

}
} else {
tracing::debug!("Access control is disabled");
Expand Down
14 changes: 14 additions & 0 deletions zenoh/src/net/routing/interceptor/authorization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,24 @@ impl PolicyEnforcer {
) -> ZResult<PolicyInformation> {
let mut policy_rules: Vec<PolicyRule> = Vec::new();
for config_rule in config_rule_set {
// config validation
if config_rule.interfaces.is_empty()
Copy link
Member

Choose a reason for hiding this comment

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

It seems that with this PR the interface list becomes always required, am I correct?
If that's the case, then I believe this is not an acceptable behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for 0.11 it should be required in the rules field, since it will not make logical sense to have a rule without a subject to implement it for. For 1.0, this will be modified to have any one of : cert_name, username or interface fields present and non-empty.

Copy link
Member

Choose a reason for hiding this comment

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

Why? I could simply want to deny key_expression on any interface/operation a priori.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For that we will require explicit keywords like ALL (this lies in the territory of Groups and Roles). Equating empty list in our explicit rules to ALL is bad access control logic. For example, we already implement similar behavior for key-expressions where ALL is signified specifically using ** and not empty list.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need some special keyword for all, IMHO, if filter filed absent it means that filter not applied for this field.
If filed interfaces absent - apply for all interfaces
If filed flows absent - apply for all flows
And for key-expressions will be better to have the same logic

Copy link
Member

Choose a reason for hiding this comment

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

I agree for interfaces and flows.
I'm still inclined to having key_exprs mandatory though...

|| config_rule.actions.is_empty()
|| config_rule.flows.is_empty()
|| config_rule.key_exprs.is_empty()
{
bail!("error from bad config");
Mallets marked this conversation as resolved.
Show resolved Hide resolved
}
oteffahi marked this conversation as resolved.
Show resolved Hide resolved
for subject in &config_rule.interfaces {
if subject.trim().is_empty() {
bail!("error from bad interface value");
Mallets marked this conversation as resolved.
Show resolved Hide resolved
Mallets marked this conversation as resolved.
Show resolved Hide resolved
}
for flow in &config_rule.flows {
for action in &config_rule.actions {
for key_expr in &config_rule.key_exprs {
if key_expr.trim().is_empty() {
bail!("error from bad key-expression value");
Mallets marked this conversation as resolved.
Show resolved Hide resolved
}
policy_rules.push(PolicyRule {
subject: Subject::Interface(subject.clone()),
key_expr: key_expr.clone(),
Expand Down