fix bug in acl config validation #982

snehilzs · 2024-04-26T09:23:04Z

This PR fixes input validation issue in access control config.

Mallets · 2024-04-26T09:29:16Z

zenoh/src/net/routing/interceptor/authorization.rs

@@ -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()


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.

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.

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

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.

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

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

Mallets · 2024-04-29T08:35:16Z

@oteffahi can you help in reviewing this PR?

oteffahi

@Mallets @snehilzs I suggested some changes, apply them if you agree.
Else it LGTM.

oteffahi · 2024-04-29T12:44:34Z

zenoh/src/net/routing/interceptor/access_control.rs

@@ -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),


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),

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.

zenoh/src/net/routing/interceptor/authorization.rs

oteffahi

@Mallets As you suggested, I put each list in a seperate if statement and return the aggregated error message at the end.

zenoh/src/net/routing/interceptor/authorization.rs

Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com>

zenoh/src/net/routing/interceptor/authorization.rs

Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com>

oteffahi · 2024-04-29T17:29:42Z

@Mallets as requested, I added a way to allow empty flows and interfaces lists in ACL rules, and interpret them as "all interfaces" and "all flows" respectively.

I'd like to mention that I maintain my position regarding this change: implicit wildcards are in my opinion a bad practice for security-related features, and that we should not proceed with this change. I added warning logs to alert the user whenever these implicit wildcards are enabled. In the future it would be better to replace this with explicit wildcards that the user has to set for each list to be populated automatically, preferably in alternatively-named configuration fields for each list (i.e: all-flows: true and all-interfaces: true).

zenoh/src/net/routing/interceptor/authorization.rs

Mallets · 2024-04-29T17:50:19Z

@Mallets as requested, I added a way to allow empty flows and interfaces lists in ACL rules, and interpret them as "all interfaces" and "all flows" respectively.

I'd like to mention that I maintain my position regarding this change: implicit wildcards are in my opinion a bad practice for security-related features, and that we should not proceed with this change. I added warning logs to alert the user whenever these implicit wildcards are enabled. In the future it would be better to replace this with explicit wildcards that the user has to set for each list to be populated automatically, preferably in alternatively-named configuration fields for each list (i.e: all-flows: true and all-interfaces: true).

There is a difference between implicit and not-specified. Here we are tackling the configuraiton-aspect of the problem and not the implementation of it. To me, interfaces are not mandatory as part of the ACL rules, especially looking at the evolution of this configuration. Let's assume we make the evolve this ACL towards a x509 certificate check. It goes without saying that the interface may be not relevant since I'd like to always apply it. If interfaces are something that make sense, then I can restrict the configuration to the specific interface. Same thing goes for the flows (egress vs ingress). Moreover, the more we add as mandatory fields to the config files, the more the config file becomes verbose, hard to debug, and more costly to transmit over the network.

This reverts commits a2db2d7 5008e38 e204f2d.

oteffahi · 2024-04-30T09:55:40Z

@Mallets I changed it so the interfaces and flows lists are optional in the config file, they behave as follows:

If they are not provided they will be considered wildcards (apply to all).
If they are provided as empty: error case.

I also made it so we error out when ACL cannot be enabled (the function returns an Error which as you suggested is handled by upper logic, which was already setup to exit in case of a returned Error from interceptor factories)

* fix bug in acl config validation * Update ACL config error messages * Apply suggestions from code review Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com> * Update zenoh/src/net/routing/interceptor/authorization.rs Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com> * Fix mistake in ACL config error message * Allow empty interfaces and flows in config * Add warning logs for implicit wildcard ACL config * Fix linting * Add support for undefined interfaces and flows lists This reverts commits a2db2d7 5008e38 e204f2d. * Error out when ACL cannot be enabled --------- Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com> Co-authored-by: Luca Cominardi <luca.cominardi@gmail.com> Co-authored-by: Oussama Teffahi <oussama.teffahi@zettascale.tech>

fix bug in acl config validation

76220b7

Mallets reviewed Apr 26, 2024

View reviewed changes

oteffahi suggested changes Apr 29, 2024

View reviewed changes

zenoh/src/net/routing/interceptor/authorization.rs Outdated Show resolved Hide resolved

oteffahi and others added 2 commits April 29, 2024 16:02

Update ACL config error messages

770d364

Apply suggestions from code review

52fa51f

Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com>

oteffahi suggested changes Apr 29, 2024

View reviewed changes

zenoh/src/net/routing/interceptor/authorization.rs Outdated Show resolved Hide resolved

Mallets and others added 5 commits April 29, 2024 16:18

Update zenoh/src/net/routing/interceptor/authorization.rs

05feb4c

Co-authored-by: oteffahi <70609372+oteffahi@users.noreply.github.com>

Fix mistake in ACL config error message

8934690

Allow empty interfaces and flows in config

a2db2d7

Add warning logs for implicit wildcard ACL config

5008e38

Fix linting

e204f2d

Mallets reviewed Apr 29, 2024

View reviewed changes

zenoh/src/net/routing/interceptor/authorization.rs Outdated Show resolved Hide resolved

oteffahi added 2 commits April 30, 2024 11:36

Add support for undefined interfaces and flows lists

c7bec9a

This reverts commits a2db2d7 5008e38 e204f2d.

Error out when ACL cannot be enabled

f56829d

oteffahi force-pushed the fix_bug_acl_config branch from 24f1afc to f56829d Compare April 30, 2024 09:48

Mallets approved these changes Apr 30, 2024

View reviewed changes

Mallets merged commit 371ca6b into eclipse-zenoh:main Apr 30, 2024
11 checks passed

Mallets deleted the fix_bug_acl_config branch April 30, 2024 10:40

oteffahi mentioned this pull request May 15, 2024

Explain optional fields in rules eclipse-zenoh/roadmap#134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug in acl config validation #982

fix bug in acl config validation #982

snehilzs commented Apr 26, 2024

Mallets Apr 26, 2024

snehilzs Apr 26, 2024

Mallets Apr 26, 2024

snehilzs Apr 26, 2024

sashacmc Apr 29, 2024

Mallets Apr 29, 2024

Mallets commented Apr 29, 2024

oteffahi left a comment

oteffahi Apr 29, 2024

Mallets Apr 29, 2024

oteffahi left a comment

oteffahi commented Apr 29, 2024 •

edited

Loading

Mallets commented Apr 29, 2024

oteffahi commented Apr 30, 2024 •

edited

Loading

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

fix bug in acl config validation #982

fix bug in acl config validation #982

Conversation

snehilzs commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mallets commented Apr 29, 2024

oteffahi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oteffahi left a comment

Choose a reason for hiding this comment

oteffahi commented Apr 29, 2024 • edited Loading

Mallets commented Apr 29, 2024

oteffahi commented Apr 30, 2024 • edited Loading

oteffahi commented Apr 29, 2024 •

edited

Loading

oteffahi commented Apr 30, 2024 •

edited

Loading