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: add policy variable support #415

Merged
merged 27 commits into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f8f4a57
feat: utility method for policy variable support (#339)
cyyan88 Jun 26, 2023
dc5ff68
feat: utility method for updating permissions for device (#344)
cyyan88 Jun 27, 2023
bc3042b
fix: add support for replacing multiple policy variables (#346)
cyyan88 Jun 27, 2023
2fdf274
feat: allow device actions to be evaluated based on policy variable p…
cyyan88 Jun 29, 2023
a73113f
feat: add integration test for policy variable authorization (#355)
cyyan88 Jul 7, 2023
efa45c5
feat: make PermissionEvaluationUtils non static
sameerzuberi Jan 5, 2024
1193d04
fix: check style
sameerzuberi Jan 5, 2024
c44eef6
fix: pmd
sameerzuberi Jan 5, 2024
87e85b3
fix: refactor PermissionEvaluationUtils
sameerzuberi Jan 9, 2024
7698ed9
chore: check style
sameerzuberi Jan 9, 2024
6f755b8
fix: remove device auth client mocks
sameerzuberi Jan 9, 2024
b7bb2cb
fix: remove permission caching
sameerzuberi Jan 9, 2024
6135f3e
fix: case insensitive matching
sameerzuberi Jan 16, 2024
f62ea96
fix: pmd
sameerzuberi Jan 16, 2024
f6960dc
Merge branch 'main' into policy-variable-support
sameerzuberi Jan 16, 2024
75de8ab
fix: update benchmark
sameerzuberi Jan 16, 2024
9d6b8d9
fix: update benchmark
sameerzuberi Jan 16, 2024
cdd12cb
fix: typo
sameerzuberi Jan 16, 2024
37a7841
fix: exception handling
sameerzuberi Jan 18, 2024
a8752af
fix: remove ipc from integration tests and add benchmark for policy vars
robcmann Jan 22, 2024
b85f3a7
chore: parameterize integration tests
robcmann Jan 22, 2024
fa815c2
feat: create foundation to support more policy variables
robcmann Jan 26, 2024
990cf66
feat: identify policy variables during config time
robcmann Jan 30, 2024
6994fc0
feat: performance improvements
robcmann Jan 31, 2024
e107f25
chore: renamed attribute provider exception to policy exception
robcmann Feb 1, 2024
bbb49c9
chore: use set instead of list to store user policy variables
robcmann Feb 1, 2024
cdfc054
chore: fix policy exception logging and use coerce to string to avoid…
robcmann Feb 1, 2024
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 @@ -7,6 +7,7 @@

import com.aws.greengrass.clientdevices.auth.configuration.GroupManager;
import com.aws.greengrass.clientdevices.auth.configuration.Permission;
import com.aws.greengrass.clientdevices.auth.exception.AttributeProviderException;
import com.aws.greengrass.clientdevices.auth.session.Session;
import com.aws.greengrass.logging.api.Logger;
import com.aws.greengrass.logging.impl.LogManager;
Expand Down Expand Up @@ -91,7 +92,7 @@ public boolean isAuthorized(AuthorizationRequest request, Session session) {
}
try {
return compareResource(rsc, e.getResource(session));
} catch (IllegalArgumentException er) {
} catch (AttributeProviderException er) {
logger.atError().setCause(er).log(er.getMessage());
robcmann marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
Expand All @@ -115,7 +116,7 @@ private boolean comparePrincipal(String requestPrincipal, String policyPrincipal
}

private boolean compareOperation(Operation requestOperation, String policyOperation) {
if (requestOperation.toString().equals(policyOperation)) {
if (requestOperation.getOperationStr().equals(policyOperation)) {
return true;
}
if (String.format(SERVICE_OPERATION_FORMAT, requestOperation.getService(), ANY_REGEX).equals(policyOperation)) {
Expand All @@ -125,7 +126,7 @@ private boolean compareOperation(Operation requestOperation, String policyOperat
}

private boolean compareResource(Resource requestResource, String policyResource) {
if (requestResource.toString().equals(policyResource)) {
if (requestResource.getResourceStr().equals(policyResource)) {
return true;
}

Expand All @@ -144,7 +145,8 @@ private Operation parseOperation(String operationStr) {

Matcher matcher = SERVICE_OPERATION_PATTERN.matcher(operationStr);
if (matcher.matches()) {
return Operation.builder().service(matcher.group(1)).action(matcher.group(2)).build();
return Operation.builder().operationStr(operationStr).service(matcher.group(1)).action(matcher.group(2))
.build();
}
throw new IllegalArgumentException(String.format("Operation %s is not in the form of %s", operationStr,
SERVICE_OPERATION_PATTERN.pattern()));
Expand All @@ -157,7 +159,8 @@ private Resource parseResource(String resourceStr) {

Matcher matcher = SERVICE_RESOURCE_PATTERN.matcher(resourceStr);
if (matcher.matches()) {
return Resource.builder().service(matcher.group(1))
return Resource.builder().resourceStr(resourceStr)
.service(matcher.group(1))
.resourceType(matcher.group(2))
.resourceName(matcher.group(3))
.build();
Expand All @@ -170,25 +173,17 @@ private Resource parseResource(String resourceStr) {
@Value
@Builder
private static class Operation {
String operationStr;
String service;
String action;

@Override
public String toString() {
return String.format(SERVICE_OPERATION_FORMAT, service, action);
}
}

@Value
@Builder
private static class Resource {
String resourceStr;
String service;
String resourceType;
String resourceName;

@Override
public String toString() {
return String.format(SERVICE_RESOURCE_FORMAT, service, resourceType, resourceName);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private Set<Permission> convertPolicyStatementToPermission(String groupName,
}
permissions.add(
Permission.builder().principal(groupName).operation(operation).resource(resource)
.policyVariables(findPolicyVariables(resource)).build());
.resourcePolicyVariables(findPolicyVariables(resource)).build());
}
}
return permissions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@

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

import com.aws.greengrass.clientdevices.auth.exception.AttributeProviderException;
import com.aws.greengrass.clientdevices.auth.session.Session;
import lombok.Builder;
import lombok.NonNull;
import lombok.Value;

import java.util.Collections;
import java.util.List;

@Value
Expand All @@ -21,9 +23,10 @@ public class Permission {

@NonNull String resource;
Copy link
Member

Choose a reason for hiding this comment

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

nit: i think renaming to resourceFormat or similar would make it clearer that this shouldn't be used directly anymore


List<String> policyVariables;
@Builder.Default
List<String> resourcePolicyVariables = Collections.emptyList();

public String getResource(Session session) {
return PolicyVariableResolver.resolvePolicyVariables(policyVariables, resource, session);
public String getResource(Session session) throws AttributeProviderException {
return PolicyVariableResolver.resolvePolicyVariables(resourcePolicyVariables, resource, session);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,57 +5,53 @@

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

import com.aws.greengrass.clientdevices.auth.exception.AttributeProviderException;
import com.aws.greengrass.clientdevices.auth.session.Session;
import com.aws.greengrass.util.Coerce;
import com.aws.greengrass.util.Pair;
import org.apache.commons.lang3.StringUtils;
import software.amazon.awssdk.utils.ImmutableMap;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public final class PolicyVariableResolver {
private static final Map<String, Pair<String,String>> policyVariableMap;
private static final String THING_NAMESPACE = "Thing";
private static final String THING_NAME_ATTRIBUTE = "ThingName";

static {
Map<String, Pair<String,String>> map = new HashMap<>();

map.put("${iot:Connection.Thing.ThingName}",
new Pair<>(THING_NAMESPACE, THING_NAME_ATTRIBUTE));

policyVariableMap = Collections.unmodifiableMap(map);
}
private static final Map<String, Pair<String,String>> policyVariableToAttributeProvider = ImmutableMap.of(
"${iot:Connection.Thing.ThingName}", new Pair<>(THING_NAMESPACE, THING_NAME_ATTRIBUTE)
);

private PolicyVariableResolver() {
}

/**
* Utility method to replace policy variables in the resource with device attributes.
*
* @param policyVariables list of policy variables in resource
* @param resource resource to resolve
* @param policyVariables list of policy variables in permission format
* @param format permission format to resolve
* @param session current device session
* @return updated resource
* @throws AttributeProviderException when unable to find a policy variable value
*/
public static String resolvePolicyVariables(List<String> policyVariables, String resource, Session session) {
if (policyVariables == null || policyVariables.isEmpty()) {
return resource;
public static String resolvePolicyVariables(List<String> policyVariables, String format, Session session)
robcmann marked this conversation as resolved.
Show resolved Hide resolved
throws AttributeProviderException {
if (policyVariables.isEmpty()) {
return format;
}
String substitutedResource = resource;
String substitutedFormat = format;
for (String policyVariable : policyVariables) {
String attributeNamespace = policyVariableMap.get(policyVariable).getLeft();
String attributeName = policyVariableMap.get(policyVariable).getRight();
String policyVariableValue = Coerce.toString(session.getSessionAttribute(attributeNamespace,
attributeName));
String attributeNamespace = policyVariableToAttributeProvider.get(policyVariable).getLeft();
jcosentino11 marked this conversation as resolved.
Show resolved Hide resolved
String attributeName = policyVariableToAttributeProvider.get(policyVariable).getRight();
String policyVariableValue = session.getSessionAttribute(attributeNamespace, attributeName).toString();
robcmann marked this conversation as resolved.
Show resolved Hide resolved
if (policyVariableValue == null) {
throw new IllegalArgumentException("No attribute found for current session");
throw new AttributeProviderException(
String.format("No attribute found for policy variable %s in current session", policyVariable));
} else {
substitutedResource = StringUtils.replace(substitutedResource, policyVariable, policyVariableValue);
// StringUtils.replace() is faster than String.replace() since it does not use regex
substitutedFormat = StringUtils.replace(substitutedFormat, policyVariable, policyVariableValue);
}
}
return substitutedResource;
return substitutedFormat;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

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

public class AttributeProviderException extends Exception {
Copy link
Member

Choose a reason for hiding this comment

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

nit: how about PolicyException?

private static final long serialVersionUID = -1L;

public AttributeProviderException(String message) {
super(message);
}

public AttributeProviderException(Throwable e) {
super(e);
}

public AttributeProviderException(String message, Throwable e) {
super(message, e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,8 @@ public String getNamespace() {

@Override
public DeviceAttribute getDeviceAttribute(String attributeName) {
if ("CertificateId".equals(attributeName)) {
return new StringLiteralAttribute(getCertificateId());
}
// TODO: Return other possible DeviceAttributes
return null;
// TODO: Support other DeviceAttributes
return new StringLiteralAttribute(getCertificateId());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,8 @@ public String getNamespace() {

@Override
public DeviceAttribute getDeviceAttribute(String attributeName) {
if ("ThingName".equals(attributeName)) {
return new WildcardSuffixAttribute(thingName);
}
// TODO: Return other possible DeviceAttributes
return null;
// TODO: Support other DeviceAttributes
return new WildcardSuffixAttribute(thingName);
Copy link
Member

Choose a reason for hiding this comment

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

from convo: if we want to still have attributeName passed in we'll need to use it, otherwise would need to be reverted back to just getDeviceAttributes() that returns a map

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,19 +198,19 @@ void GIVEN_valid_group_configuration_WHEN_start_service_THEN_instantiated_config

Permission[] tempSensorPermissions =
{Permission.builder().principal("myTemperatureSensors").operation("mqtt" + ":connect")
.resource("mqtt:clientId:foo").policyVariables(Collections.emptyList()).build(),
.resource("mqtt:clientId:foo").build(),
Permission.builder().principal("myTemperatureSensors").operation("mqtt:publish")
.resource("mqtt:topic:temperature").policyVariables(Collections.emptyList()).build(),
.resource("mqtt:topic:temperature").build(),
Permission.builder().principal("myTemperatureSensors").operation("mqtt:publish")
.resource("mqtt:topic:humidity").policyVariables(Collections.emptyList()).build()};
.resource("mqtt:topic:humidity").build()};
assertThat(permissionMap.get("myTemperatureSensors"), containsInAnyOrder(tempSensorPermissions));
Permission[] humidSensorPermissions =
{Permission.builder().principal("myHumiditySensors").operation("mqtt:connect")
.resource("mqtt:clientId:foo").policyVariables(Collections.emptyList()).build(),
.resource("mqtt:clientId:foo").build(),
Permission.builder().principal("myHumiditySensors").operation("mqtt:publish")
.resource("mqtt:topic:temperature").policyVariables(Collections.emptyList()).build(),
.resource("mqtt:topic:temperature").build(),
Permission.builder().principal("myHumiditySensors").operation("mqtt:publish")
.resource("mqtt:topic:humidity").policyVariables(Collections.emptyList()).build()};
.resource("mqtt:topic:humidity").build()};
assertThat(permissionMap.get("myHumiditySensors"), containsInAnyOrder(humidSensorPermissions));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ void GIVEN_sessionHasPolicyVariablesPermission_WHEN_canDevicePerform_THEN_author
when(groupManager.getApplicablePolicyPermissions(session)).thenReturn(Collections.singletonMap("group1",
Collections.singleton(Permission.builder().operation("mqtt:publish")
.resource("mqtt:topic:${iot:Connection.Thing.ThingName}").principal("group1")
.policyVariables(THING_NAME_POLICY_VARIABLE).build())));
.resourcePolicyVariables(THING_NAME_POLICY_VARIABLE).build())));

boolean authorized = authClient.canDevicePerform(constructPolicyVariableAuthorizationRequest(thingName));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import com.aws.greengrass.clientdevices.auth.configuration.GroupManager;
import com.aws.greengrass.clientdevices.auth.configuration.Permission;
import com.aws.greengrass.clientdevices.auth.exception.AttributeProviderException;
import com.aws.greengrass.clientdevices.auth.iot.Certificate;
import com.aws.greengrass.clientdevices.auth.iot.CertificateFake;
import com.aws.greengrass.clientdevices.auth.iot.InvalidCertificateException;
Expand Down Expand Up @@ -56,18 +57,20 @@ void beforeEach() throws InvalidCertificateException {
}

@Test
void GIVEN_single_permission_with_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource() {
void GIVEN_single_permission_with_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource()
throws AttributeProviderException {
Permission policyVariablePermission =
Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/${iot:Connection.Thing.ThingName}").policyVariables(THING_NAME_POLICY_VARIABLE).build();
.resource("/msg/${iot:Connection.Thing.ThingName}").resourcePolicyVariables(THING_NAME_POLICY_VARIABLE).build();

Permission permission = Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/b").build();
assertThat(policyVariablePermission.getResource(session).equals(permission.getResource()), is(true));
}

@Test
void GIVEN_single_permission_with_invalid_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource() {
void GIVEN_single_permission_with_invalid_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource()
throws AttributeProviderException {
Permission policyVariablePermission =
Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/${iot:Connection.Thing.ThingName/}").build();
Expand All @@ -80,7 +83,8 @@ void GIVEN_single_permission_with_invalid_policy_variable_WHEN_get_permission_re
}

@Test
void GIVEN_single_permission_with_nonexistent_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource() {
void GIVEN_single_permission_with_nonexistent_policy_variable_WHEN_get_permission_resource_THEN_return_updated_permission_resource()
throws AttributeProviderException {
Permission policyVariablePermission =
Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/${iot:Connection.Thing.RealThing}").build();
Expand All @@ -93,11 +97,12 @@ void GIVEN_single_permission_with_nonexistent_policy_variable_WHEN_get_permissio
}

@Test
void GIVEN_single_permission_with_multiple_policy_variables_WHEN_get_permission_resource_THEN_return_updated_permission_resource() {
void GIVEN_single_permission_with_multiple_policy_variables_WHEN_get_permission_resource_THEN_return_updated_permission_resource()
throws AttributeProviderException {
Permission policyVariablePermission =
Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/${iot:Connection.Thing.ThingName}/${iot:Connection.Thing.ThingName}/src")
.policyVariables(THING_NAME_POLICY_VARIABLE).build();
.resourcePolicyVariables(THING_NAME_POLICY_VARIABLE).build();

Permission permission = Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/b/b/src").build();
Expand All @@ -107,11 +112,12 @@ void GIVEN_single_permission_with_multiple_policy_variables_WHEN_get_permission_
}

@Test
void GIVEN_single_permission_with_multiple_invalid_policy_variables_WHEN_get_permission_resource_THEN_return_updated_permission_resource() {
void GIVEN_single_permission_with_multiple_invalid_policy_variables_WHEN_get_permission_resource_THEN_return_updated_permission_resource()
throws AttributeProviderException {
Permission policyVariablePermission =
Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/${iot:Connection.Thing.ThingName}/${iot:Connection}.Thing.RealThing}/src")
.policyVariables(THING_NAME_POLICY_VARIABLE).build();
.resourcePolicyVariables(THING_NAME_POLICY_VARIABLE).build();

Permission permission = Permission.builder().principal("some-principal").operation("some-operation")
.resource("/msg/b/b/src").build();
Expand Down Expand Up @@ -229,7 +235,7 @@ private Map<String, Set<Permission>> prepareGroupVariablePermissionsData() {
Permission[] sensorPermission =
{Permission.builder().principal("sensor").operation("mqtt:publish").resource("mqtt:topic:a").build(),
Permission.builder().principal("sensor").operation("mqtt:*").resource("mqtt:topic:${iot:Connection.Thing.ThingName}")
.policyVariables(THING_NAME_POLICY_VARIABLE).build(),
.resourcePolicyVariables(THING_NAME_POLICY_VARIABLE).build(),
Permission.builder().principal("sensor").operation("mqtt:subscribe")
.resource("mqtt:topic:device:${iot:Connection.FakeThing.ThingName}").build(),
Permission.builder().principal("sensor").operation("mqtt:connect").resource("*").build(),};
Expand Down
Loading
Loading