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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,40 @@ public static Stream<Arguments> authzRequests() {
.operation("mqtt:publish")
.resource("mqtt:topic:myThing/world")
.expectedResult(false)
.build(),
// mqtt wildcards eval not supported by default
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/test/test/*")
.expectedResult(false)
.build(),
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/#/test/*")
.expectedResult(true)
.build()
)),

Arguments.of("mqtt-wildcards-in-resource.yaml", Arrays.asList(
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:publish")
.resource("mqtt:topic:*/myThing/*")
.expectedResult(true)
.build(),
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:publish")
.resource("mqtt:topic:hello/myThing/world")
.expectedResult(true)
.build(),
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/test/test/test/test")
.expectedResult(true)
.build()
)),

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
services:
aws.greengrass.Nucleus:
configuration:
runWithDefault:
posixUser: nobody
windowsUser: integ-tester
logging:
level: "DEBUG"
aws.greengrass.clientdevices.Auth:
configuration:
enableMqttWildcardEvaluation: true
deviceGroups:
formatVersion: "2021-03-05"
definitions:
myThing:
selectionRule: "thingName: myThing"
policyName: "thingAccessPolicy"
policies:
thingAccessPolicy:
publish:
statementDescription: "mqtt publish"
operations:
- "mqtt:publish"
resources:
- "mqtt:topic:*/myThing/*"
subscribe:
statementDescription: "mqtt subscribe"
operations:
- "mqtt:subscribe"
resources:
- "mqtt:topic:${iot:Connection.Thing.ThingName}/+/test/#"
main:
dependencies:
- aws.greengrass.clientdevices.Auth
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ services:
policyName: "thingAccessPolicy"
policies:
thingAccessPolicy:
policyStatement:
publish:
statementDescription: "mqtt publish"
operations:
- "mqtt:publish"
resources:
- "mqtt:topic:*/myThing/*"
subscribe:
statementDescription: "mqtt subscribe"
operations:
- "mqtt:subscribe"
resources:
- "mqtt:topic:myThing/#/test/*"
main:
dependencies:
- aws.greengrass.clientdevices.Auth
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ private void subscribeToConfigChanges() {
private void onConfigurationChanged() {
try {
cdaConfiguration = CDAConfiguration.from(cdaConfiguration, getConfig());
// TODO decouple
context.get(PermissionEvaluationUtils.class).setCdaConfiguration(cdaConfiguration);
} catch (URISyntaxException e) {
serviceErrored(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package com.aws.greengrass.clientdevices.auth;

import com.aws.greengrass.authorization.WildcardTrie;
import com.aws.greengrass.clientdevices.auth.configuration.CDAConfiguration;
import com.aws.greengrass.clientdevices.auth.configuration.GroupManager;
import com.aws.greengrass.clientdevices.auth.configuration.Permission;
import com.aws.greengrass.clientdevices.auth.exception.PolicyException;
Expand All @@ -14,6 +15,7 @@
import com.aws.greengrass.logging.impl.LogManager;
import com.aws.greengrass.util.Utils;
import lombok.Builder;
import lombok.Setter;
import lombok.Value;
import org.apache.commons.lang3.StringUtils;

Expand All @@ -36,6 +38,8 @@ public final class PermissionEvaluationUtils {
"Resource is malformed, must be of the form: "
+ "([a-zA-Z]+):([a-zA-Z]+):" + RESOURCE_NAME_PATTERN.pattern();
private final GroupManager groupManager;
@Setter
private volatile CDAConfiguration cdaConfiguration;

/**
* Constructor for PermissionEvaluationUtils.
Expand Down Expand Up @@ -134,9 +138,27 @@ private boolean compareResource(Resource requestResource, String policyResource)
return true;
}

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

String name = extractResourceName(policyResource);
WildcardTrie trie = new WildcardTrie();
trie.add(name);
return trie.matchesMQTT(requestResource.getResourceName());
} else {
WildcardTrie trie = new WildcardTrie();
trie.add(policyResource);
return trie.matchesStandard(requestResource.getResourceStr());
}
}

private String extractResourceName(String resource) {
// resource is considered valid at this point, so don't need to duplicate validation from parseResource
String typeAndName = resource.substring(resource.indexOf(':') + 1);
return typeAndName.substring(typeAndName.indexOf(':') + 1);
}

private boolean matchMqttWildcards() {
CDAConfiguration config = cdaConfiguration;
return config != null && config.isEnableMqttWildcardEvaluation();
}

private Operation parseOperation(String operationStr) throws PolicyException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import com.aws.greengrass.clientdevices.auth.configuration.events.MetricsConfigurationChanged;
import com.aws.greengrass.clientdevices.auth.configuration.events.SecurityConfigurationChanged;
import com.aws.greengrass.config.Topics;
import com.aws.greengrass.util.Coerce;
import lombok.Builder;
import lombok.Getter;

import java.net.URISyntaxException;
Expand Down Expand Up @@ -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.
public final class CDAConfiguration {

public static final String ENABLE_MQTT_WILDCARD_EVALUATION = "enableMqttWildcardEvaluation";

private final DomainEvents domainEvents;
private final RuntimeConfiguration runtime;
private final SecurityConfiguration security;
@Getter
private final CAConfiguration certificateAuthorityConfiguration;
private final SecurityConfiguration security;
private final MetricsConfiguration metricsConfiguration;
private final DomainEvents domainEvents;

private CDAConfiguration(DomainEvents domainEvents, RuntimeConfiguration runtime, CAConfiguration ca,
SecurityConfiguration security, MetricsConfiguration metricsConfiguration) {
this.domainEvents = domainEvents;
this.runtime = runtime;
this.security = security;
this.certificateAuthorityConfiguration = ca;
this.metricsConfiguration = metricsConfiguration;
}
@Getter
private final boolean enableMqttWildcardEvaluation;

/**
* Creates the CDA (Client Device Auth) Service configuration. And allows it to be available in the context with the
Expand All @@ -80,9 +78,15 @@

DomainEvents domainEvents = topics.getContext().get(DomainEvents.class);

CDAConfiguration newConfig = new CDAConfiguration(domainEvents, RuntimeConfiguration.from(runtimeTopics),
CAConfiguration.from(serviceConfiguration), SecurityConfiguration.from(serviceConfiguration),
MetricsConfiguration.from(serviceConfiguration));
CDAConfiguration newConfig = CDAConfiguration.builder()
.domainEvents(domainEvents)
.runtime(RuntimeConfiguration.from(runtimeTopics))
.certificateAuthorityConfiguration(CAConfiguration.from(serviceConfiguration))
.security(SecurityConfiguration.from(serviceConfiguration))
.metricsConfiguration(MetricsConfiguration.from(serviceConfiguration))
.enableMqttWildcardEvaluation(
Coerce.toBoolean(serviceConfiguration.find(ENABLE_MQTT_WILDCARD_EVALUATION)))
.build();

newConfig.triggerChanges(newConfig, existingConfig);

Expand Down
Loading