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: single character wildcard matching #437

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 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,34 @@ public static Stream<Arguments> authzRequests() {
.operation("mqtt:publish")
.resource("mqtt:topic:myThing/world")
.expectedResult(false)
.build(),
// single character eval not supported by default
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/#/test/abc")
.expectedResult(false)
.build(),
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/#/test/???")
.expectedResult(true)
.build()
)),

Arguments.of("single-character-wildcards-in-resource.yaml", Arrays.asList(
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/abc/test/a/b")
.expectedResult(true)
.build(),
AuthZRequest.builder()
.thingName("myThing")
.operation("mqtt:subscribe")
.resource("mqtt:topic:myThing/abcd/test/a/b")
.expectedResult(false)
.build()
)),

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
services:
aws.greengrass.Nucleus:
configuration:
runWithDefault:
posixUser: nobody
windowsUser: integ-tester
logging:
level: "DEBUG"
aws.greengrass.clientdevices.Auth:
configuration:
enableSingleCharacterWildcardMatching: true
deviceGroups:
formatVersion: "2021-03-05"
definitions:
myThing:
selectionRule: "thingName: myThing"
policyName: "thingAccessPolicy"
policies:
thingAccessPolicy:
subscribe:
statementDescription: "mqtt subscribe"
operations:
- "mqtt:subscribe"
resources:
- "mqtt:topic:${iot:Connection.Thing.ThingName}/???/test/*"
Copy link
Member

Choose a reason for hiding this comment

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

Are the combination of single level ? and multi level * valid in the same level and should we test for it? Ex: mqtt:topic:myThing/???* or mqtt:topic:myThing/*???

Copy link
Member Author

@jcosentino11 jcosentino11 Sep 24, 2024

Choose a reason for hiding this comment

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

Evaluation here is more regex-like than mqtt: * doesn't know anything about topic levels.

There should be test cases for this in WildcardTrieTest already (not all are passing quite yet)

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 @@ -5,15 +5,17 @@

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;
import com.aws.greengrass.clientdevices.auth.session.Session;
import com.aws.greengrass.clientdevices.auth.util.WildcardTrie;
import com.aws.greengrass.logging.api.Logger;
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 @@ -133,10 +137,14 @@ private boolean compareResource(Resource requestResource, String policyResource)
if (Objects.equals(requestResource.getResourceStr(), policyResource)) {
return true;
}
WildcardTrie trie = new WildcardTrie();
trie.add(policyResource);
return trie.matches(requestResource.getResourceStr(), matchSingleCharacterWildcard());
}

WildcardTrie wildcardTrie = new WildcardTrie();
wildcardTrie.add(policyResource);
return wildcardTrie.matchesStandard(requestResource.getResourceStr());
private boolean matchSingleCharacterWildcard() {
CDAConfiguration config = cdaConfiguration;
return config != null && config.isMatchSingleCharacterWildcard();
}

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 = "enableSingleCharacterWildcardMatching";

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 matchSingleCharacterWildcard;

/**
* 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))
.matchSingleCharacterWildcard(
Coerce.toBoolean(serviceConfiguration.find(ENABLE_MQTT_WILDCARD_EVALUATION)))
.build();

newConfig.triggerChanges(newConfig, existingConfig);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package com.aws.greengrass.clientdevices.auth.util;


import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;

public class WildcardTrie {
private static final String GLOB_WILDCARD = "*";
private static final String SINGLE_CHAR_WILDCARD = "?";

private final Map<String, WildcardTrie> children = new DefaultHashMap<>(WildcardTrie::new);

private boolean isTerminal;
private boolean isGlobWildcard;
private boolean isSingleCharWildcard;

public void add(String subject) {
add(subject, true);
}

private WildcardTrie add(String subject, boolean isTerminal) {
if (subject == null || subject.isEmpty()) {
this.isTerminal |= isTerminal;
return this;
}
StringBuilder currPrefix = new StringBuilder(subject.length());
for (int i = 0; i < subject.length(); i++) {
char c = subject.charAt(i);
if (c == GLOB_WILDCARD.charAt(0)) {
return addGlobWildcard(subject, currPrefix.toString(), isTerminal);
}
if (c == SINGLE_CHAR_WILDCARD.charAt(0)) {
return addSingleCharWildcard(subject, currPrefix.toString(), isTerminal);
}
currPrefix.append(c);
}
WildcardTrie node = children.get(currPrefix.toString());
node.isTerminal |= isTerminal;
return node;
}

private WildcardTrie addGlobWildcard(String subject, String currPrefix, boolean isTerminal) {
WildcardTrie node = this;
node = node.add(currPrefix, false);
node = node.children.get(GLOB_WILDCARD);
node.isGlobWildcard = true;
// wildcard at end of subject is terminal
if (subject.length() - currPrefix.length() == 1) {
node.isTerminal = isTerminal;
return node;
}
return node.add(subject.substring(currPrefix.length() + 2), true);
}

private WildcardTrie addSingleCharWildcard(String subject, String currPrefix, boolean isTerminal) {
WildcardTrie node = this;
node = node.add(currPrefix, false);
node = node.children.get(SINGLE_CHAR_WILDCARD);
node.isSingleCharWildcard = true;
// wildcard at end of subject is terminal
if (subject.length() - currPrefix.length() == 1) {
node.isTerminal = isTerminal;
return node;
}
return node.add(subject.substring(currPrefix.length() + 1), true);
}

public boolean matches(String s) {
return matches(s, true);
}

public boolean matches(String s, boolean matchSingleCharWildcard) {
Copy link
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The cyclomatic complexity of this method is 21. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.

We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 123-132 into a separate method.

if (s == null) {
return children.isEmpty();
}

if ((isWildcard() && isTerminal) || (isTerminal && s.isEmpty())) {
return true;
}

boolean childMatchesWildcard = children
.values()
.stream()
.filter(WildcardTrie::isWildcard)
.filter(childNode -> matchSingleCharWildcard || !childNode.isSingleCharWildcard)
.anyMatch(childNode -> childNode.matches(s, matchSingleCharWildcard));
if (childMatchesWildcard) {
return true;
}

if (matchSingleCharWildcard) {
boolean childMatchesSingleCharWildcard = children
.values()
.stream()
.filter(childNode -> childNode.isSingleCharWildcard)
.anyMatch(childNode -> childNode.matches(s, matchSingleCharWildcard));
if (childMatchesSingleCharWildcard) {
return true;
}
}

boolean childMatchesRegularCharacters = children
.keySet()
.stream()
.filter(s::startsWith)
.anyMatch(childToken -> {
WildcardTrie childNode = children.get(childToken);
String rest = s.substring(childToken.length());
return childNode.matches(rest, matchSingleCharWildcard);
});
if (childMatchesRegularCharacters) {
return true;
}

if (isWildcard() && !isTerminal) {
return findMatchingChildSuffixesAfterWildcard(s, matchSingleCharWildcard)
.entrySet()
.stream()
.anyMatch((e) -> {
String suffix = e.getKey();
WildcardTrie childNode = e.getValue();
return childNode.matches(suffix, matchSingleCharWildcard);
});
}
return false;
}

private Map<String, WildcardTrie> findMatchingChildSuffixesAfterWildcard(String s, boolean matchSingleCharWildcard) {
Map<String, WildcardTrie> matchingSuffixes = new HashMap<>();
for (Map.Entry<String, WildcardTrie> e : children.entrySet()) {
String childToken = e.getKey();
WildcardTrie childNode = e.getValue();
int suffixIndex = s.indexOf(childToken);
if (matchSingleCharWildcard && suffixIndex > 1) {
continue;
}
while (suffixIndex >= 0) {
matchingSuffixes.put(s.substring(suffixIndex + childToken.length()), childNode);
suffixIndex = s.indexOf(childToken, suffixIndex + 1);
}
}
return matchingSuffixes;
}

private boolean isWildcard() {
return isGlobWildcard || isSingleCharWildcard;
}

@SuppressFBWarnings("EQ_DOESNT_OVERRIDE_EQUALS")
private static class DefaultHashMap<K, V> extends HashMap<K, V> {

Check failure

Code scanning / CodeQL

No clone method Error

No clone method, yet implements Cloneable.
private final transient Supplier<V> defaultVal;

public DefaultHashMap(Supplier<V> defaultVal) {
super();
this.defaultVal = defaultVal;
}

@Override
@SuppressWarnings("unchecked")
public V get(Object key) {
return super.computeIfAbsent((K) key, (k) -> defaultVal.get());
}

@Override
public boolean containsKey(Object key) {
return super.get(key) != null;
}
}
}
Loading
Loading