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.

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(ACL): Apply queryable and subscriber declaration filters on their respective undeclarations #1573

Merged

Conversation

oteffahi
Copy link
Contributor

Addresses the unfiltered Undeclare messages reported in #1562

@oteffahi oteffahi added the bug Something isn't working label Oct 30, 2024
@eclipse-zenoh eclipse-zenoh deleted a comment from github-actions bot Oct 30, 2024
@oteffahi
Copy link
Contributor Author

oteffahi commented Oct 30, 2024

The current routing and interceptor logic is relying on the ext_wire_expr field to fill the keyexpr field in the RoutingContext. This means that a manually crafted UndeclareSubscriber or UndeclareQueryable message can feed inconsistent keyexpr and subscriber/queryable id to the interceptor, effectively undeclaring the sub/qbl even if the keyexpr of the declaration does not match the ext_wire_expr data ; i.e the ACL filters are bypassed.

EDIT: ext_wire_expr takes over the id in the undeclaration logic, so this shouldn't be possible in most cases... Nevertheless, if possible it would be better to rely on the id to avoid adding unnecessary data to the undeclaration messages.

@oteffahi oteffahi marked this pull request as draft October 30, 2024 13:40
@oteffahi oteffahi marked this pull request as ready for review November 7, 2024 09:50
zenoh/src/net/routing/dispatcher/pubsub.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/pubsub.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/pubsub.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/pubsub.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/queries.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/queries.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/queries.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/queries.rs Outdated Show resolved Hide resolved
zenoh/src/net/routing/dispatcher/pubsub.rs Outdated Show resolved Hide resolved
@OlivierHecart OlivierHecart enabled auto-merge (squash) November 7, 2024 10:46
@OlivierHecart OlivierHecart merged commit 7159acf into eclipse-zenoh:main Nov 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants