Skip to content

Commit

Permalink
refactor: policy validation on config change (#418)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcosentino11
jcosentino11 authored Feb 6, 2024
1 parent 3027cd4 commit 15e55bb
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 50 deletions.
32 changes: 21 additions & 11 deletions src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

package com.aws.greengrass.clientdevices.auth;

import com.amazon.aws.iot.greengrass.component.common.SerializerFactory;
import com.aws.greengrass.authorization.AuthorizationHandler;
import com.aws.greengrass.authorization.exceptions.AuthorizationException;
import com.aws.greengrass.clientdevices.auth.api.ClientDevicesAuthServiceApi;
import com.aws.greengrass.clientdevices.auth.api.UseCases;
import com.aws.greengrass.clientdevices.auth.certificate.CertificatesConfig;
Expand All @@ -21,6 +23,7 @@
import com.aws.greengrass.clientdevices.auth.configuration.RuntimeConfiguration;
import com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor;
import com.aws.greengrass.clientdevices.auth.connectivity.ConnectivityInfoCache;
import com.aws.greengrass.clientdevices.auth.exception.PolicyException;
import com.aws.greengrass.clientdevices.auth.infra.NetworkStateProvider;
import com.aws.greengrass.clientdevices.auth.metrics.MetricsEmitter;
import com.aws.greengrass.clientdevices.auth.metrics.handlers.AuthorizeClientDeviceActionsMetricHandler;
Expand All @@ -44,7 +47,6 @@
import com.aws.greengrass.ipc.VerifyClientDeviceIdentityOperationHandler;
import com.aws.greengrass.lifecyclemanager.PluginService;
import com.aws.greengrass.util.Coerce;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import software.amazon.awssdk.aws.greengrass.GreengrassCoreIPCService;

Expand All @@ -68,6 +70,8 @@
@ImplementsService(name = ClientDevicesAuthService.CLIENT_DEVICES_AUTH_SERVICE_NAME)
public class ClientDevicesAuthService extends PluginService {
public static final String CLIENT_DEVICES_AUTH_SERVICE_NAME = "aws.greengrass.clientdevices.Auth";
private static final ObjectMapper MAPPER = SerializerFactory.getConfigurationSerializerJson();
private static final String KV_NODE = "node";

// TODO: Move configuration related constants to appropriate configuration class
public static final String DEVICE_GROUPS_TOPICS = "deviceGroups";
Expand Down Expand Up @@ -174,7 +178,7 @@ private void configChangeHandler(WhatHappened whatHappened, Node node) {
if (whatHappened == WhatHappened.timestampUpdated || whatHappened == WhatHappened.interiorAdded) {
return;
}
logger.atDebug().kv("why", whatHappened).kv("node", node).log();
logger.atTrace().kv("why", whatHappened).kv(KV_NODE, node).log();
// NOTE: This should not live here. The service doesn't have to have knowledge about where/how
// keys are stored
Topics deviceGroupTopics = this.config.lookupTopics(CONFIGURATION_CONFIG_KEY, DEVICE_GROUPS_TOPICS);
Expand All @@ -201,7 +205,7 @@ private void configChangeHandler(WhatHappened whatHappened, Node node) {
}

if (whatHappened == WhatHappened.initialized || node == null || node.childOf(DEVICE_GROUPS_TOPICS)) {
updateDeviceGroups(whatHappened, deviceGroupTopics);
updateDeviceGroups(deviceGroupTopics);
}

onConfigurationChanged();
Expand Down Expand Up @@ -229,7 +233,7 @@ public void postInject() {
authorizationHandler.registerComponent(this.getName(), new HashSet<>(
Arrays.asList(SUBSCRIBE_TO_CERTIFICATE_UPDATES, VERIFY_CLIENT_DEVICE_IDENTITY,
GET_CLIENT_DEVICE_AUTH_TOKEN, AUTHORIZE_CLIENT_DEVICE_ACTION)));
} catch (com.aws.greengrass.authorization.exceptions.AuthorizationException e) {
} catch (AuthorizationException e) {
logger.atError("initialize-cda-service-authorization-error", e)
.log("Failed to initialize the client device auth service with the Authorization module.");
}
Expand All @@ -255,18 +259,24 @@ public CertificateManager getCertificateManager() {
return context.get(CertificateManager.class);
}

private void updateDeviceGroups(WhatHappened whatHappened, Topics deviceGroupsTopics) {
final ObjectMapper objectMapper = new ObjectMapper().enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS,
MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES);
private void updateDeviceGroups(Topics deviceGroupsTopics) {
GroupConfiguration groupConfiguration;

try {
context.get(GroupManager.class).setGroupConfiguration(
objectMapper.convertValue(deviceGroupsTopics.toPOJO(), GroupConfiguration.class));
groupConfiguration = MAPPER.convertValue(deviceGroupsTopics.toPOJO(), GroupConfiguration.class);
} catch (IllegalArgumentException e) {
logger.atError().kv("event", whatHappened).kv("node", deviceGroupsTopics.getFullName()).setCause(e)
.log("Unable to parse group configuration");
serviceErrored(e);
return;
}

try {
groupConfiguration.validate();
} catch (PolicyException e) {
serviceErrored(e);
return;
}

context.get(GroupManager.class).setGroupConfiguration(groupConfiguration);
}

void updateCACertificateConfig(List<String> caCerts) {
Expand Down
46 changes: 28 additions & 18 deletions src/main/java/com/aws/greengrass/clientdevices/auth/configuration/GroupConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

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

import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException;
import com.aws.greengrass.clientdevices.auth.exception.PolicyException;
import com.aws.greengrass.logging.api.Logger;
import com.aws.greengrass.logging.impl.LogManager;
import com.aws.greengrass.util.Utils;
Expand All @@ -15,12 +15,12 @@
import lombok.Value;

import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

@Value
@JsonDeserialize(builder = GroupConfiguration.GroupConfigurationBuilder.class)
Expand All @@ -45,31 +45,24 @@ public class GroupConfiguration {

@Builder
GroupConfiguration(ConfigurationFormatVersion formatVersion, Map<String, GroupDefinition> definitions,
Map<String, Map<String, AuthorizationPolicyStatement>> policies) throws AuthorizationException {
Map<String, Map<String, AuthorizationPolicyStatement>> policies) {
this.formatVersion = formatVersion == null ? ConfigurationFormatVersion.MAR_05_2021 : formatVersion;
this.definitions = definitions == null ? Collections.emptyMap() : definitions;
this.policies = policies == null ? Collections.emptyMap() : policies;
this.groupToPermissionsMap = constructGroupToPermissionsMap();
this.groupToPermissionsMap = constructGroupPermissions();
}

@JsonPOJOBuilder(withPrefix = "")
public static class GroupConfigurationBuilder {
}

private Map<String, Set<Permission>> constructGroupToPermissionsMap() throws AuthorizationException {
Map<String, Set<Permission>> groupToPermissionsMap = new HashMap<>();

for (Map.Entry<String, GroupDefinition> groupDefinitionEntry : definitions.entrySet()) {
GroupDefinition groupDefinition = groupDefinitionEntry.getValue();
if (!policies.containsKey(groupDefinition.getPolicyName())) {
throw new AuthorizationException(
String.format("Policies doesn't have policy named %s", groupDefinition.getPolicyName()));
}
groupToPermissionsMap.put(groupDefinitionEntry.getKey(),
constructGroupPermission(groupDefinitionEntry.getKey(),
policies.get(groupDefinition.getPolicyName())));
}
return groupToPermissionsMap;
private Map<String, Set<Permission>> constructGroupPermissions() {
return definitions.entrySet().stream().collect(Collectors.toMap(
Map.Entry::getKey,
entry -> constructGroupPermission(
entry.getKey(),
policies.getOrDefault(entry.getValue().getPolicyName(),
Collections.emptyMap()))));
}

private Set<Permission> constructGroupPermission(String groupName,
Expand Down Expand Up @@ -114,4 +107,21 @@ private Set<String> findPolicyVariables(String resource) {
}
return policyVariables;
}

/**
* Validate the deviceGroups configuration.
*
* @throws PolicyException if an invalid policy is detected
*/
public void validate() throws PolicyException {
String missingPolicy = definitions.values().stream()
.map(GroupDefinition::getPolicyName)
.filter(policyName -> !policies.containsKey(policyName))
.findFirst()
.orElse(null);
if (missingPolicy != null) {
throw new PolicyException(
String.format("Policy definition %s does not have a corresponding policy", missingPolicy));
}
}
}
21 changes: 9 additions & 12 deletions src/test/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
import com.aws.greengrass.clientdevices.auth.configuration.GroupManager;
import com.aws.greengrass.clientdevices.auth.configuration.Permission;
import com.aws.greengrass.clientdevices.auth.configuration.RuntimeConfiguration;
import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException;
import com.aws.greengrass.clientdevices.auth.exception.CloudServiceInteractionException;
import com.aws.greengrass.clientdevices.auth.exception.PolicyException;
import com.aws.greengrass.clientdevices.auth.exception.UseCaseException;
import com.aws.greengrass.componentmanager.KernelConfigResolver;
import com.aws.greengrass.config.Topic;
Expand All @@ -29,7 +29,6 @@
import com.aws.greengrass.mqttclient.spool.SpoolerStoreException;
import com.aws.greengrass.testcommons.testutilities.GGExtension;
import com.aws.greengrass.util.GreengrassServiceClientFactory;
import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
import org.hamcrest.collection.IsIterableContainingInAnyOrder;
import org.hamcrest.collection.IsMapContaining;
import org.hamcrest.collection.IsMapWithSize;
Expand Down Expand Up @@ -166,14 +165,12 @@ void GIVEN_no_group_configuration_WHEN_start_service_change_THEN_empty_configura
}

@Test
void GIVEN_bad_group_configuration_WHEN_start_service_THEN_service_in_error_state(ExtensionContext context)
throws Exception {
ignoreExceptionOfType(context, IllegalArgumentException.class);
ignoreExceptionOfType(context, UnrecognizedPropertyException.class);

startNucleusWithConfig("badGroupConfig.yaml", State.ERRORED);

verify(groupManager, never()).setGroupConfiguration(any());
void GIVEN_empty_group_with_unknown_property_WHEN_start_service_THEN_empty_group_created() throws Exception {
startNucleusWithConfig("groupConfigWithUnknownProperty.yaml", State.RUNNING);
verify(groupManager).setGroupConfiguration(configurationCaptor.capture());
GroupConfiguration groupConfiguration = configurationCaptor.getValue();
assertThat(groupConfiguration.getDefinitions(), IsMapWithSize.anEmptyMap());
assertThat(groupConfiguration.getPolicies(), IsMapWithSize.anEmptyMap());
}

@Test
Expand Down Expand Up @@ -218,7 +215,7 @@ void GIVEN_valid_group_configuration_WHEN_start_service_THEN_instantiated_config
void GIVEN_group_has_no_policy_WHEN_start_service_THEN_no_configuration_update(ExtensionContext context)
throws Exception {
ignoreExceptionOfType(context, IllegalArgumentException.class);
ignoreExceptionOfType(context, AuthorizationException.class);
ignoreExceptionOfType(context, PolicyException.class);

startNucleusWithConfig("noGroupPolicyConfig.yaml", State.ERRORED);

Expand Down Expand Up @@ -429,4 +426,4 @@ private X509Certificate pemToX509Certificate(String certPem) throws IOException,
}
return cert;
}
}
}
13 changes: 4 additions & 9 deletions src/test/java/com/aws/greengrass/clientdevices/auth/configuration/GroupManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import com.aws.greengrass.clientdevices.auth.session.Session;
import com.aws.greengrass.clientdevices.auth.session.SessionImpl;
import com.aws.greengrass.clientdevices.auth.configuration.parser.ParseException;
import com.aws.greengrass.clientdevices.auth.exception.AuthorizationException;
import com.aws.greengrass.clientdevices.auth.iot.Thing;
import com.aws.greengrass.testcommons.testutilities.GGExtension;
import org.junit.jupiter.api.Test;
Expand All @@ -27,8 +26,7 @@
@ExtendWith({MockitoExtension.class, GGExtension.class})
public class GroupManagerTest {
@Test
void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet()
throws AuthorizationException {
void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() throws Exception {
GroupManager groupManager = new GroupManager();
groupManager.setGroupConfiguration(new GroupConfiguration(null, null, null));

Expand All @@ -37,8 +35,7 @@ void GIVEN_emptyGroupConfiguration_WHEN_getApplicablePolicyPermissions_THEN_retu
}

@Test
void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet()
throws AuthorizationException, ParseException {
void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmptySet() throws Exception {
GroupConfiguration groupConfiguration = GroupConfiguration.builder()
.definitions(Collections.singletonMap("group1", getGroupDefinition("differentThingName", "policy1")))
.policies(Collections.singletonMap("policy1",
Expand All @@ -51,8 +48,7 @@ void GIVEN_sessionInNoGroup_WHEN_getApplicablePolicyPermissions_THEN_returnEmpty
}

@Test
void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnGroupPermissions()
throws AuthorizationException, ParseException {
void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnGroupPermissions() throws Exception {
Session session = getSessionFromThing("thingName");
GroupConfiguration groupConfiguration = GroupConfiguration.builder()
.definitions(Collections.singletonMap("group1", getGroupDefinition("thingName", "policy1"))).policies(
Expand All @@ -70,8 +66,7 @@ void GIVEN_sessionInSingleGroup_WHEN_getApplicablePolicyPermissions_THEN_returnG
}

@Test
void GIVEN_sessionInMultipleGroups_WHEN_getApplicablePolicyPermissions_THEN_returnMergedGroupPermissions()
throws AuthorizationException, ParseException {
void GIVEN_sessionInMultipleGroups_WHEN_getApplicablePolicyPermissions_THEN_returnMergedGroupPermissions() throws Exception {
Session session = getSessionFromThing("thingName");
GroupConfiguration groupConfiguration =
GroupConfiguration.builder().definitions(new HashMap<String, GroupDefinition>() {{
Expand Down
0 ...ss/clientdevices/auth/badGroupConfig.yaml → .../auth/groupConfigWithUnknownProperty.yaml
View file
File renamed without changes.

0 comments on commit 15e55bb

Please sign in to comment.