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

feat: mqtt-style resource wildcard matching #436

Closed
wants to merge 3 commits into from

Conversation

jcosentino11
Copy link
Member

@jcosentino11 jcosentino11 commented Sep 11, 2024

Issue #, if available:

Description of changes:

Proposes a new configuration option, enableMqttWildcardEvaluation, to allow policy resources to be evaluated using MQTT wildcard.

e.g.

       policies:
          thingAccessPolicy:
            subscribe:
              statementDescription: "mqtt subscribe"
              operations:
                - "mqtt:subscribe"
              resources:
                - "mqtt:topic:${iot:Connection.Thing.ThingName}/+/test/#"

Allows client devices to subscribe to thingName/path/test/path1/path2, for example

Why is this change necessary:

How was this change tested:

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Member

@MikeDombo MikeDombo left a comment

Choose a reason for hiding this comment

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

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Copy link

github-actions bot commented Sep 11, 2024

Code Coverage Report

File Coverage
All files 74%

Minimum allowed coverage is 50%

Generated by 🐒 cobertura-action against 6b156de

@@ -48,23 +50,19 @@
* |
* </p>
*/
@Builder

Check notice

Code scanning / CodeQL

Use of default toString() Note

Default toString(): MetricsConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): SecurityConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): CAConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): RuntimeConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): DomainEvents inherits toString() from Object, and so is not suitable for printing.
@jcosentino11
Copy link
Member Author

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Ah I see https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html now, looks like ? is used for pseduo-+ matching

@MikeDombo
Copy link
Member

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Ah I see https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html now, looks like ? is used for pseduo-+ matching

Well yeah, but that's for IPC which isn't MQTT :)
So.... reasons, but maybe not good reasons.

Copy link

github-actions bot commented Sep 11, 2024

Benchmark Results

Benchmark Score
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_thing_name_variable_WHEN_auth_request_THEN_successful_auth 1256388.94 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_wildcards_WHEN_auth_request_THEN_successful_auth 221451.01 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_single_group_permission_WHEN_simple_auth_request_THEN_successful_auth 2539896.74 ops/s

@jcosentino11
Copy link
Member Author

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Ah I see https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html now, looks like ? is used for pseduo-+ matching

Well yeah, but that's for IPC which isn't MQTT :) So.... reasons, but maybe not good reasons.

Should be for MQTT/HTTP/Websockets? IoT Core publish/subscribe, unless im missing something here

@MikeDombo
Copy link
Member

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Ah I see https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html now, looks like ? is used for pseduo-+ matching

Well yeah, but that's for IPC which isn't MQTT :) So.... reasons, but maybe not good reasons.

Should be for MQTT/HTTP/Websockets? IoT Core publish/subscribe, unless im missing something here

Not too sure what you mean. I made the IPC policy more lenient to avoid common customer misunderstandings when compared with V1. But we wanted to keep CDA aligned with IoT Core as much as possible.

@jcosentino11
Copy link
Member Author

This was purposefully not included because it will differ from how IoT Core works. If you have permission to subscribe to A/# then you have permission to subscribe to that and exactly that.

Ah I see https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html now, looks like ? is used for pseduo-+ matching

Well yeah, but that's for IPC which isn't MQTT :) So.... reasons, but maybe not good reasons.

Should be for MQTT/HTTP/Websockets? IoT Core publish/subscribe, unless im missing something here

Not too sure what you mean. I made the IPC policy more lenient to avoid common customer misunderstandings when compared with V1. But we wanted to keep CDA aligned with IoT Core as much as possible.

I think we're talking about two different things 😅 Was saying that https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html and ? matching is IoT Core specific

WildcardTrie wildcardTrie = new WildcardTrie();
wildcardTrie.add(policyResource);
return wildcardTrie.matchesStandard(requestResource.getResourceStr());
if (matchMqttWildcards()) {
Copy link
Member

Choose a reason for hiding this comment

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

Switching comment to threads to make it more sane.

My point is that IoT Core for MQTT policies does not let you subscribe to A/B when the policy is A/#. Greengrass PubSub does allow this, but CDA does not in order to match the behavior of IoT Core.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay yeah I understand your point now. So to have parity with IoT Core then, ? single char matching would be needed, so then CDA knows about * and ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah guess so, if iot core just added that in.

Copy link
Member

Choose a reason for hiding this comment

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

But wildcards # and + shouldn't change the way they currently work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep will either close or revise this. Reason it came up was customer ask so was exploring the idea

@jcosentino11
Copy link
Member Author

Closing in favor of ? matching: #437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants