feat(auth): Add ResourcesNameable to BasicAmazonDeployDescription, de…

…fault to no authorization (#4205) * feat(auth): Add ResourcesNameable to BasicAmazonDeployDescription, default to no authorization Additionally, corrected some terminology (authn -> authz) and removed a dangerous `requiresAuthentication` boolean from `DescriptionAuthorizer` that could potentially cause authz to skip on resources * fix(*): Optionally get resource names and add interface doc * fix(*): Descriptive authorization skipped metric name
spinnaker · Apr 20, 2020 · 3c5629c · 3c5629c
1 parent 81b28f9
commit 3c5629c
Show file tree

Hide file tree

Showing 10 changed files with 132 additions and 20 deletions.
diff --git a/...nnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy b/...nnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy
@@ -17,23 +17,46 @@
 package com.netflix.spinnaker.clouddriver.aws.deploy.converters
 
 import com.netflix.spinnaker.clouddriver.aws.AmazonOperation
+import com.netflix.spinnaker.clouddriver.aws.services.RegionScopedProviderFactory
 import com.netflix.spinnaker.clouddriver.deploy.DeployAtomicOperation
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
 import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport
 import com.netflix.spinnaker.clouddriver.aws.deploy.description.BasicAmazonDeployDescription
+import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
 
 @AmazonOperation(AtomicOperations.CREATE_SERVER_GROUP)
 @Component("basicAmazonDeployDescription")
 class BasicAmazonDeployAtomicOperationConverter extends AbstractAtomicOperationsCredentialsSupport {
+
+  @Autowired
+  RegionScopedProviderFactory regionScopedProviderFactory
+
   AtomicOperation convertOperation(Map input) {
     new DeployAtomicOperation(convertDescription(input))
   }
 
   BasicAmazonDeployDescription convertDescription(Map input) {
     def converted = objectMapper.convertValue(input, BasicAmazonDeployDescription)
     converted.credentials = getCredentialsObject(input.credentials as String)
-    converted
+
+    if (converted.securityGroups != null && !converted.securityGroups.isEmpty()) {
+      for (Map.Entry<String, List<String>> entry : converted.availabilityZones) {
+        String region = entry.key
+
+        RegionScopedProviderFactory.RegionScopedProvider regionScopedProvider =
+          regionScopedProviderFactory.forRegion(converted.credentials, region)
+
+        converted.securityGroupNames.addAll(
+          regionScopedProvider
+            .getSecurityGroupService()
+            .getSecurityGroupNamesFromIds(converted.securityGroups)
+            .keySet()
+        )
+      }
+    }
+
+    return converted
   }
 }
diff --git a/.../netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy b/.../netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy
@@ -21,12 +21,14 @@ import com.netflix.spinnaker.clouddriver.aws.model.AmazonBlockDevice
 import com.netflix.spinnaker.clouddriver.deploy.DeployDescription
 import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent
 import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable
+import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable
 import groovy.transform.AutoClone
 import groovy.transform.Canonical
 
 @AutoClone
 @Canonical
-class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription implements DeployDescription, ApplicationNameable {
+class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription implements
+  DeployDescription, ApplicationNameable, ResourcesNameable {
   String application
   String amiName
   String stack
@@ -91,6 +93,7 @@ class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription
   List<String> loadBalancers
   List<String> targetGroups
   List<String> securityGroups
+  List<String> securityGroupNames = []
   List<AmazonAsgLifecycleHook> lifecycleHooks = []
   Map<String, List<String>> availabilityZones = [:]
   Capacity capacity = new Capacity()
@@ -102,6 +105,16 @@ class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription
     return [application]
   }
 
+  @Override
+  Collection<String> getNames() {
+    return loadBalancers + targetGroups + securityGroupNames
+  }
+
+  @Override
+  boolean requiresAuthorization() {
+    return false
+  }
+
   @Canonical
   static class Capacity {
     Integer min

diff --git a/.../com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy b/.../com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy
@@ -30,7 +30,7 @@ class UpsertAmiTagsDescription extends AbstractAmazonCredentialsDescription {
   }
 
   @Override
-  boolean requiresAuthentication(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
+  boolean requiresAuthorization(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     return !opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts.contains(account)
   }
 }
diff --git a/...louddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy b/...louddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy
@@ -16,9 +16,12 @@
 
 package com.netflix.spinnaker.clouddriver.aws.deploy.converters
 
+import com.amazonaws.services.ec2.AmazonEC2
 import com.fasterxml.jackson.databind.ObjectMapper
-import com.netflix.spinnaker.clouddriver.aws.deploy.converters.BasicAmazonDeployAtomicOperationConverter
+import com.netflix.spinnaker.clouddriver.aws.model.SubnetAnalyzer
 import com.netflix.spinnaker.clouddriver.aws.security.NetflixAmazonCredentials
+import com.netflix.spinnaker.clouddriver.aws.services.RegionScopedProviderFactory
+import com.netflix.spinnaker.clouddriver.aws.services.SecurityGroupService
 import com.netflix.spinnaker.clouddriver.deploy.DeployAtomicOperation
 import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider
 import com.netflix.spinnaker.clouddriver.aws.deploy.description.BasicAmazonDeployDescription
@@ -30,14 +33,49 @@ class BasicAmazonDeployAtomicOperationConverterUnitSpec extends Specification {
   @Shared
   ObjectMapper mapper = new ObjectMapper()
 
+  @Shared
+  RegionScopedProviderFactory regionScopedProviderFactory
+
   @Shared
   BasicAmazonDeployAtomicOperationConverter converter
 
+  RegionScopedProviderFactory.RegionScopedProvider regionScopedProvider =
+    Mock(RegionScopedProviderFactory.RegionScopedProvider)
+
+  SecurityGroupService securityGroupService = new SecurityGroupService(Mock(AmazonEC2), Mock(SubnetAnalyzer))
+
   def setupSpec() {
     def accountCredentialsProvider = Stub(AccountCredentialsProvider) {
       getCredentials('test') >> Stub(NetflixAmazonCredentials)
     }
-    this.converter = new BasicAmazonDeployAtomicOperationConverter(objectMapper: mapper, accountCredentialsProvider: accountCredentialsProvider)
+    this.regionScopedProviderFactory = Stub(RegionScopedProviderFactory)
+    this.converter = new BasicAmazonDeployAtomicOperationConverter(objectMapper: mapper,
+      accountCredentialsProvider: accountCredentialsProvider,
+      regionScopedProviderFactory: regionScopedProviderFactory)
+  }
+
+  void "converts securityGroups to securityGroupNames"() {
+    setup:
+    def securityGroups = ["sg-12345678", "sg-87654321"]
+    def input = [application      : "asgard", amiName: "ami-000", stack: "asgard-test", instanceType: "m3.medium",
+                 availabilityZones: ["us-west-1": ["us-west-1a"]], capacity: [min: 1, max: 2, desired: 5],
+                 credentials      : "test", securityGroups: securityGroups]
+
+    regionScopedProviderFactory.forRegion(_ as NetflixAmazonCredentials, _ as String) >> regionScopedProvider
+    regionScopedProvider.getSecurityGroupService() >> securityGroupService
+    securityGroupService.getSecurityGroupNamesFromIds(_ as Collection<String>) >> [(input.application): input.securityGroups[0]]
+
+    when:
+    def description = converter.convertDescription(input)
+
+    then:
+    description instanceof BasicAmazonDeployDescription
+
+    when:
+    def operation = converter.convertOperation(input)
+
+    then:
+    operation instanceof DeployAtomicOperation
   }
 
   void "basicAmazonDeployDescription type returns BasicAmazonDeployDescription and DeployAtomicOperation"() {

diff --git a/...-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java b/...-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java
@@ -27,6 +27,7 @@
 import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable;
 import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
@@ -46,6 +47,7 @@ public class DescriptionAuthorizer<T> {
   private final FiatPermissionEvaluator fiatPermissionEvaluator;
   private final SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps;
 
+  private final Id skipAuthorizationId;
   private final Id missingApplicationId;
   private final Id authorizationId;
 
@@ -59,6 +61,7 @@ public DescriptionAuthorizer(
     this.fiatPermissionEvaluator = fiatPermissionEvaluator.orElse(null);
     this.opsSecurityConfigProps = opsSecurityConfigProps;
 
+    this.skipAuthorizationId = registry.createId("authorization.skipped");
     this.missingApplicationId = registry.createId("authorization.missingApplication");
     this.authorizationId = registry.createId("authorization");
   }
@@ -73,19 +76,25 @@ public void authorize(T description, Errors errors) {
     String account = null;
     List<String> applications = new ArrayList<>();
     boolean requiresApplicationRestriction = true;
-    boolean requiresAuthentication = true;
 
     if (description instanceof AccountNameable) {
       AccountNameable accountNameable = (AccountNameable) description;
-      account = accountNameable.getAccount();
 
       requiresApplicationRestriction = accountNameable.requiresApplicationRestriction();
-      requiresAuthentication = accountNameable.requiresAuthentication(opsSecurityConfigProps);
-      if (!requiresAuthentication) {
+
+      if (!accountNameable.requiresAuthorization(opsSecurityConfigProps)) {
+        registry
+            .counter(
+                skipAuthorizationId.withTag(
+                    "descriptionClass", description.getClass().getSimpleName()))
+            .increment();
+
         log.info(
-            "Skipping authentication for operation `{}` in account `{}`.",
+            "Skipping authorization for operation `{}` in account `{}`.",
             description.getClass().getSimpleName(),
             accountNameable.getAccount());
+      } else {
+        account = accountNameable.getAccount();
       }
     }
 
@@ -100,16 +109,33 @@ public void authorize(T description, Errors errors) {
 
     if (description instanceof ResourcesNameable) {
       ResourcesNameable resourcesNameable = (ResourcesNameable) description;
-      applications.addAll(
-          Optional.ofNullable(resourcesNameable.getResourceApplications())
-              .orElse(Collections.emptyList()).stream()
-              .filter(Objects::nonNull)
-              .collect(Collectors.toList()));
+
+      if (!resourcesNameable.requiresAuthorization()) {
+        registry
+            .counter(
+                skipAuthorizationId.withTag(
+                    "descriptionClass", description.getClass().getSimpleName()))
+            .increment();
+
+        Collection<String> resourceNames =
+            Optional.ofNullable(resourcesNameable.getNames()).orElse(Collections.emptyList());
+
+        log.info(
+            "Skipping authorization for operation={}, resource names={}, resource applications={}",
+            description.getClass().getSimpleName(),
+            resourceNames,
+            resourcesNameable.getResourceApplications().toString());
+      } else {
+        applications.addAll(
+            Optional.ofNullable(resourcesNameable.getResourceApplications())
+                .orElse(Collections.emptyList()).stream()
+                .filter(Objects::nonNull)
+                .collect(Collectors.toList()));
+      }
     }
 
     boolean hasPermission = true;
-    if (requiresAuthentication
-        && account != null
+    if (account != null
         && !fiatPermissionEvaluator.hasPermission(auth, account, "ACCOUNT", "WRITE")) {
       hasPermission = false;
       errors.reject("authorization", format("Access denied to account %s", account));

diff --git a/...src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy b/...src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy
@@ -114,7 +114,7 @@ class DescriptionAuthorizerSpec extends Specification {
     }
 
     @Override
-    boolean requiresAuthentication(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
+    boolean requiresAuthorization(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
       return !opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts.contains(account)
     }
   }

diff --git a/...x/spinnaker/clouddriver/google/deploy/description/UpsertGoogleImageTagsDescription.groovy b/...x/spinnaker/clouddriver/google/deploy/description/UpsertGoogleImageTagsDescription.groovy
@@ -24,7 +24,7 @@ class UpsertGoogleImageTagsDescription extends AbstractGoogleCredentialsDescript
   String accountName
 
   @Override
-  boolean requiresAuthentication(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
+  boolean requiresAuthorization(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     return !opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts.contains(account)
   }
 }
diff --git a/...y/src/main/groovy/com/netflix/spinnaker/clouddriver/security/config/SecurityConfig.groovy b/...y/src/main/groovy/com/netflix/spinnaker/clouddriver/security/config/SecurityConfig.groovy
@@ -41,6 +41,8 @@ class SecurityConfig {
   static class OperationsSecurityConfigurationProperties {
     SecurityAction onMissingSecuredCheck = SecurityAction.WARN
     SecurityAction onMissingValidator = SecurityAction.WARN
+
+    //TODO(jonsie): should be `allowUnauthorizedImageTaggingInAccounts`
     List<String> allowUnauthenticatedImageTaggingInAccounts = []
   }
 

diff --git a/...src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/AccountNameable.java b/...src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/AccountNameable.java
@@ -30,7 +30,7 @@ default boolean requiresApplicationRestriction() {
     return true;
   }
 
-  default boolean requiresAuthentication(
+  default boolean requiresAuthorization(
       SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     return true;
   }

diff --git a/...c/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java b/...c/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java
@@ -36,4 +36,14 @@ default Collection<String> getResourceApplications() {
         .map(name -> Names.parseName(name).getApp())
         .collect(Collectors.toList());
   }
+
+  /**
+   * Determine if the resource should be authz evaluated by the {@link
+   * com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator}.
+   *
+   * @return boolean
+   */
+  default boolean requiresAuthorization() {
+    return true;
+  }
 }