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

Add publisher delete and queryable reply messages to ACL #1259

Merged
merged 14 commits into from
Jul 24, 2024
Merged

Add publisher delete and queryable reply messages to ACL #1259

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3a78d5d
Expose reply key_expr to interceptors
oteffahi Jul 24, 2024
f45f2dd
Add reply message to ACL logic and config
oteffahi Jul 23, 2024
6a632d9
Update DEFAULT_CONFIG
oteffahi Jul 23, 2024
d2a700b
Update ACL get/queryable tests, add reply tests
oteffahi Jul 23, 2024
b5dcf1b
Improve reply matching
oteffahi Jul 23, 2024
b3c0906
Specify all existing message types in ACL interceptor matching
oteffahi Jul 23, 2024
8e30f55
Add reply to authentication qbl tests configs
oteffahi Jul 24, 2024
99323ad
Add delete message to ACL logic and config
oteffahi Jul 24, 2024
0c2ee09
Add delete message to DEFAULT_CONFIG, format messages
oteffahi Jul 24, 2024
036ac96
Add delete message to ACL pub/sub tests
oteffahi Jul 24, 2024
bf7fb08
Fix clippy errors
oteffahi Jul 24, 2024
8db19ec
Reorder message matching
oteffahi Jul 24, 2024
2ea951e
Revert "Expose reply key_expr to interceptors", use wire_expr for ACL…
oteffahi Jul 24, 2024
26f6ca5
Revert key_expr parsing change to reply ingress
oteffahi Jul 24, 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
6 changes: 4 additions & 2 deletions DEFAULT_CONFIG.json5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@
// /// Id has to be unique within the rule set
// "id": "rule1",
// "messages": [
// "put", "query", "declare_subscriber", "declare_queryable"
// "put", "delete", "declare_subscriber",
// "query", "reply", "declare_queryable",
// ],
// "flows":["egress","ingress"],
// "permission": "allow",
Expand All @@ -210,7 +211,8 @@
// {
// "id": "rule2",
// "messages": [
// "put", "query", "declare_subscriber", "declare_queryable"
// "put", "delete", "declare_subscriber",
// "query", "reply", "declare_queryable",
// ],
// "flows":["ingress"],
// "permission": "allow",
Expand Down
2 changes: 2 additions & 0 deletions commons/zenoh-config/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,11 @@ pub struct PolicyRule {
#[serde(rename_all = "snake_case")]
pub enum AclMessage {
Put,
Delete,
DeclareSubscriber,
Query,
DeclareQueryable,
Reply,
}

#[derive(Clone, Copy, Debug, Serialize, Deserialize, Eq, Hash, PartialEq)]
Expand Down
2 changes: 1 addition & 1 deletion zenoh/src/net/routing/dispatcher/queries.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ pub(crate) fn route_send_response(
ext_tstamp,
ext_respid,
},
"".to_string(), // @TODO provide the proper key expression of the response for interceptors
key_expr.to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

This allocates everytime in the critical path. Why it should return a String and not a keyexpr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because RoutingContext<Msg> expects a String. In other instances where this is called no additional allocation is made, but here it is the case...
For more detail see here: https://github.com/eclipse-zenoh/zenoh/blob/main/zenoh/src/net/routing/mod.rs

Copy link
Member

Choose a reason for hiding this comment

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

Let's then return empty String for the time being until a proper solution is found.
Does returning an empty String impact the 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.

Returning an empty string breaks the keyexpr matching (empty string does not match with any key expression, even **).
Also, rules are made on key expressions, so having an empty string on all replies would impact the behavior of ACL

Copy link
Member

Choose a reason for hiding this comment

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

Ok, understood. Can you then verify the impact on performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to high impact on performance, this change will be reverted. We can use Response{ wire_expr, ... } in ACL code to only get the keyexpr when needed.

));
}
None => tracing::warn!(
Expand Down
98 changes: 95 additions & 3 deletions 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 @@ -26,7 +26,7 @@ use zenoh_config::{
};
use zenoh_protocol::{
core::ZenohIdProto,
network::{Declare, DeclareBody, NetworkBody, NetworkMessage, Push, Request},
network::{Declare, DeclareBody, NetworkBody, NetworkMessage, Push, Request, Response},
zenoh::{PushBody, RequestBody},
};
use zenoh_result::ZResult;
Expand Down Expand Up @@ -243,6 +243,16 @@ impl InterceptorTrait for IngressAclEnforcer {
return None;
}
}
NetworkBody::Push(Push {
payload: PushBody::Del(_),
..
}) => {
if self.action(AclMessage::Delete, "Delete (ingress)", key_expr?)
== Permission::Deny
{
return None;
}
}
NetworkBody::Request(Request {
payload: RequestBody::Query(_),
..
Expand Down Expand Up @@ -278,7 +288,44 @@ impl InterceptorTrait for IngressAclEnforcer {
return None;
}
}
_ => {}
NetworkBody::Response(Response { .. }) => {
if self.action(AclMessage::Reply, "Reply (ingress)", key_expr?) == Permission::Deny
{
return None;
}
}
// Unfiltered Declare messages
NetworkBody::Declare(Declare {
body: DeclareBody::DeclareKeyExpr(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::DeclareFinal(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::DeclareToken(_),
..
}) => {}
// Unfiltered Undeclare messages
NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareKeyExpr(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareToken(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareQueryable(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareSubscriber(_),
..
}) => {}
// Unfiltered remaining message types
NetworkBody::Interest(_) | NetworkBody::OAM(_) | NetworkBody::ResponseFinal(_) => {}
}
Some(ctx)
}
Expand Down Expand Up @@ -313,6 +360,15 @@ impl InterceptorTrait for EgressAclEnforcer {
return None;
}
}
NetworkBody::Push(Push {
payload: PushBody::Del(_),
..
}) => {
if self.action(AclMessage::Delete, "Delete (egress)", key_expr?) == Permission::Deny
{
return None;
}
}
NetworkBody::Request(Request {
payload: RequestBody::Query(_),
..
Expand Down Expand Up @@ -347,7 +403,43 @@ impl InterceptorTrait for EgressAclEnforcer {
return None;
}
}
_ => {}
NetworkBody::Response(Response { .. }) => {
if self.action(AclMessage::Reply, "Reply (egress)", key_expr?) == Permission::Deny {
return None;
}
}
// Unfiltered Declare messages
NetworkBody::Declare(Declare {
body: DeclareBody::DeclareKeyExpr(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::DeclareFinal(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::DeclareToken(_),
..
}) => {}
// Unfiltered Undeclare messages
NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareKeyExpr(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareToken(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareQueryable(_),
..
})
| NetworkBody::Declare(Declare {
body: DeclareBody::UndeclareSubscriber(_),
..
}) => {}
// Unfiltered remaining message types
NetworkBody::Interest(_) | NetworkBody::OAM(_) | NetworkBody::ResponseFinal(_) => {}
}
Some(ctx)
}
Expand Down
6 changes: 6 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 @@ -184,25 +184,31 @@ impl PermissionPolicy {
struct ActionPolicy {
query: PermissionPolicy,
put: PermissionPolicy,
delete: PermissionPolicy,
declare_subscriber: PermissionPolicy,
declare_queryable: PermissionPolicy,
reply: PermissionPolicy,
}

impl ActionPolicy {
fn action(&self, action: AclMessage) -> &PermissionPolicy {
match action {
AclMessage::Query => &self.query,
AclMessage::Put => &self.put,
AclMessage::Delete => &self.delete,
AclMessage::DeclareSubscriber => &self.declare_subscriber,
AclMessage::DeclareQueryable => &self.declare_queryable,
AclMessage::Reply => &self.reply,
oteffahi marked this conversation as resolved.
Show resolved Hide resolved
}
}
fn action_mut(&mut self, action: AclMessage) -> &mut PermissionPolicy {
match action {
AclMessage::Query => &mut self.query,
AclMessage::Put => &mut self.put,
AclMessage::Delete => &mut self.delete,
AclMessage::DeclareSubscriber => &mut self.declare_subscriber,
AclMessage::DeclareQueryable => &mut self.declare_queryable,
AclMessage::Reply => &mut self.reply,
}
}
}
Expand Down
Loading
Loading