aws-greengrass / aws-greengrass-client-device-auth

Apache License 2.0
2 stars 3 forks source link

feat: mqtt-style resource wildcard matching #436

Closed jcosentino11 closed 3 weeks ago

jcosentino11 commented 3 weeks ago

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.

github-actions[bot] commented 3 weeks ago

Code Coverage Report

File Coverage
All files 74% :white_check_mark:

Minimum allowed coverage is 50%

Generated by :monkey: cobertura-action against 6b156de2dfc049b8adcaf37b548bbcfe4dd30e43

jcosentino11 commented 3 weeks ago

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 commented 3 weeks ago

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.

github-actions[bot] commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

jcosentino11 commented 3 weeks ago

Closing in favor of ? matching: https://github.com/aws-greengrass/aws-greengrass-client-device-auth/pull/437