-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
5a4cbbb
to
1193d04
Compare
0470133
to
c44eef6
Compare
...eengrass/integrationtests/deviceauth/EvaluateClientDeviceActionsWithPolicyVariablesTest.java
Fixed
Show fixed
Hide fixed
return PermissionEvaluationUtils.isAuthorized(request.getOperation(), request.getResource(), | ||
groupManager.getApplicablePolicyPermissions(session)); | ||
return permissionEvaluationUtils.isAuthorized(request.getOperation(), request.getResource(), | ||
permissionEvaluationUtils.transformGroupPermissionsWithVariableValue(session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we're at it, we can clean up by making groupManager a dependency of PermissionEvaluationUtils, then we can do something like
permissionEvaluationUtils.isAuthorized(request, session)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also take performance into account, e.g. when evaluating w/ same permissions we shouldn't do variable substitution again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also take performance into account, e.g. when evaluating w/ same permissions we shouldn't do variable substitution again
Thinking through this more, variables are substituted based on current session, so a caching solution for this would be a bit more involved, assuming it would even be useful, so lets table for now
@@ -60,7 +66,8 @@ public static boolean isAuthorized(String operation, String resource, | |||
return false; | |||
} | |||
|
|||
for (Map.Entry<String, Set<Permission>> entry : groupToPermissionsMap.entrySet()) { | |||
for (Map.Entry<String, Set<Permission>> entry : | |||
groupToPermissionsMap.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need this formatting change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return PermissionEvaluationUtils.isAuthorized(request.getOperation(), request.getResource(), | ||
groupManager.getApplicablePolicyPermissions(session)); | ||
return permissionEvaluationUtils.isAuthorized(request.getOperation(), request.getResource(), | ||
permissionEvaluationUtils.transformGroupPermissionsWithVariableValue(session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also take performance into account, e.g. when evaluating w/ same permissions we shouldn't do variable substitution again
027a98b
to
87e85b3
Compare
src/main/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtils.java
Show resolved
Hide resolved
a68376e
to
7698ed9
Compare
3609884
to
6f755b8
Compare
Code Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against cdfc054 |
} | ||
} else { | ||
logger.atWarn().kv("policyVariable", policyVariable) | ||
.log("Policy variable detected but could not be parsed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policy variable unsupported
, and can also guide user w/ next steps (e.g. fix config)
String attributeName = vars[2]; | ||
|
||
// this supports the ThingName attribute only | ||
if (THING_NAME_VARIABLE.equals(policyVariable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we'd want to be case insensitive, would need to configure the matcher too
Coerce.toString(session.getSessionAttribute(attributeNamespace, attributeName)); | ||
|
||
if (policyVariableValue == null) { | ||
logger.atWarn().kv("attributeName", attributeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we'd want to error here rather than log, it's a failure mode--not doing substitution would mean the policy is acting differently than intended
5665f8b
to
b7bb2cb
Compare
0ea3f4c
to
6135f3e
Compare
0a76038
to
f62ea96
Compare
src/main/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtils.java
Show resolved
Hide resolved
private static final Map<String, Pair<String,String>> policyVariableResolver = | ||
new HashMap<String, Pair<String,String>>() { | ||
{ | ||
put("iot:Connection.Thing.ThingName", new Pair<>("Thing", "ThingName")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make the attr namespace and properties constants
|
||
private static final Map<String, Pair<String,String>> policyVariableResolver = | ||
new HashMap<String, Pair<String,String>>() { | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: avoid {{}} initializers
} | ||
} else { | ||
logger.atWarn().kv("policyVariable", policyVariable) | ||
.log("Policy variable unsupported. Only thing name variables are supported, please fix the " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thing name variables are supported
let's remove, otherwise we'll have to update this every time we add new ones
Putting here for later reference: With regex on the policy eval path, throughput is ~200k ops/s (for variable substitution test case). Before we were hitting 400k. We talked about identifying variables during config time, which should get those perf numbers back up |
src/main/java/com/aws/greengrass/clientdevices/auth/PermissionEvaluationUtils.java
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/clientdevices/auth/iot/Thing.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/clientdevices/auth/iot/Certificate.java
Outdated
Show resolved
Hide resolved
@@ -17,4 +20,10 @@ public class Permission { | |||
@NonNull String operation; | |||
|
|||
@NonNull String resource; |
There was a problem hiding this comment.
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
@@ -17,4 +20,10 @@ public class Permission { | |||
@NonNull String operation; | |||
|
|||
@NonNull String resource; | |||
|
|||
List<String> policyVariables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets name this resourcePolicyVariables
, since these don't apply to operation or principal
* @param session current device session | ||
* @return updated resource | ||
*/ | ||
public static String resolvePolicyVariables(List<String> policyVariables, String resource, Session session) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: in follow up we'll also need to add more validations to the policy (e.g. bad policy variable name, etc), after #418 is merged too
private static final String THING_NAMESPACE = "Thing"; | ||
private static final String THING_NAME_ATTRIBUTE = "ThingName"; | ||
|
||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utils.immutableMap
import java.util.Map; | ||
|
||
public final class PolicyVariableResolver { | ||
private static final Map<String, Pair<String,String>> policyVariableMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use a more descriptive name
String policyVariableValue = Coerce.toString(session.getSessionAttribute(attributeNamespace, | ||
attributeName)); | ||
if (policyVariableValue == null) { | ||
throw new IllegalArgumentException("No attribute found for current session"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets avoid unchecked exceptions, may need to make your own if there's not a suitable one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include attribute name/space in message as well
if (policyVariableValue == null) { | ||
throw new IllegalArgumentException("No attribute found for current session"); | ||
} else { | ||
substitutedResource = StringUtils.replace(substitutedResource, policyVariable, policyVariableValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably worth a quick comment about why this replace impl
@@ -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").build(), | |||
.resource("mqtt:clientId:foo").policyVariables(Collections.emptyList()).build(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also be ok to default policyVariables to empty list within Permission itself
|
||
package com.aws.greengrass.clientdevices.auth.exception; | ||
|
||
public class AttributeProviderException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about PolicyException
?
// TODO: Return other possible DeviceAttributes | ||
return null; | ||
// TODO: Support other DeviceAttributes | ||
return new WildcardSuffixAttribute(thingName); |
There was a problem hiding this comment.
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
Issue #, if available:
Description of changes:
PermissionEvaluationUtils
to use non static methods for future wildcard supportWhy is this change necessary:
How was this change tested:
Unit and integration tests
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.