feat(auth): Optionally skip authentication for image tagging (#3795)

* feat(auth): Optionally skip authentication for image tagging Adds a new feature flag operations.security.allowUnauthenticatedImageTaggingInAccounts that takes a list of account names, and will skip the authentication check for tagging cloud images in AWS or GCE if the account name matches. The motivation is that if the bakery puts images in a protected account, any tagging stage will fail if the user or service account that triggered the pipeline does not have permissions to write to the account that contains the image, even if the same user just created the image. The feature defaults to disabled to not change the current behaviour. * Move the functionality out of the controller and into the authorizer Also make the feature more granular by taking a list of accounts instead of just a boolean. * Fix wiring of descriptionAuthorizer
spinnaker · Sep 27, 2019 · 7b4caf6 · 7b4caf6
1 parent 757b6f6
commit 7b4caf6
Show file tree

Hide file tree

Showing 7 changed files with 94 additions and 6 deletions.
diff --git a/.../com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy b/.../com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy
@@ -17,6 +17,8 @@
 
 package com.netflix.spinnaker.clouddriver.aws.deploy.description
 
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig
+
 class UpsertAmiTagsDescription extends AbstractAmazonCredentialsDescription {
   String amiName
   Collection<String> regions
@@ -26,4 +28,9 @@ class UpsertAmiTagsDescription extends AbstractAmazonCredentialsDescription {
   boolean requiresApplicationRestriction() {
     return false
   }
+
+  @Override
+  boolean requiresAuthentication(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
+    return !opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts.contains(account)
+  }
 }
diff --git a/...er-core/src/main/groovy/com/netflix/spinnaker/clouddriver/config/CloudDriverConfig.groovy b/...er-core/src/main/groovy/com/netflix/spinnaker/clouddriver/config/CloudDriverConfig.groovy
@@ -90,6 +90,7 @@ import com.netflix.spinnaker.clouddriver.security.AccountCredentialsRepository
 import com.netflix.spinnaker.clouddriver.security.AllowedAccountsValidator
 import com.netflix.spinnaker.clouddriver.security.DefaultAccountCredentialsProvider
 import com.netflix.spinnaker.clouddriver.security.MapBackedAccountCredentialsRepository
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig
 import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator
 import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.kork.jackson.ObjectMapperSubtypeConfigurer
@@ -352,11 +353,13 @@ class CloudDriverConfig {
   @Bean
   DescriptionAuthorizer descriptionAuthorizer(Registry registry,
                                               ObjectMapper objectMapper,
-                                              Optional<FiatPermissionEvaluator> fiatPermissionEvaluator) {
+                                              Optional<FiatPermissionEvaluator> fiatPermissionEvaluator,
+                                              SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     return new DescriptionAuthorizer(
       registry,
       objectMapper,
-      fiatPermissionEvaluator
+      fiatPermissionEvaluator,
+      opsSecurityConfigProps
     )
   }
 

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
@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spectator.api.Id;
 import com.netflix.spectator.api.Registry;
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig;
 import com.netflix.spinnaker.clouddriver.security.resources.AccountNameable;
 import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable;
 import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable;
@@ -44,17 +45,20 @@ public class DescriptionAuthorizer<T> {
   private final Registry registry;
   private final ObjectMapper objectMapper;
   private final FiatPermissionEvaluator fiatPermissionEvaluator;
+  private final SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps;
 
   private final Id missingApplicationId;
   private final Id authorizationId;
 
   public DescriptionAuthorizer(
       Registry registry,
       ObjectMapper objectMapper,
-      Optional<FiatPermissionEvaluator> fiatPermissionEvaluator) {
+      Optional<FiatPermissionEvaluator> fiatPermissionEvaluator,
+      SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     this.registry = registry;
     this.objectMapper = objectMapper;
     this.fiatPermissionEvaluator = fiatPermissionEvaluator.orElse(null);
+    this.opsSecurityConfigProps = opsSecurityConfigProps;
 
     this.missingApplicationId = registry.createId("authorization.missingApplication");
     this.authorizationId = registry.createId("authorization");
@@ -70,12 +74,20 @@ 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) {
+        log.info(
+            "Skipping authentication for operation `{}` in account `{}`.",
+            description.getClass().getSimpleName(),
+            accountNameable.getAccount());
+      }
     }
 
     if (description instanceof ApplicationNameable) {
@@ -97,7 +109,8 @@ public void authorize(T description, Errors errors) {
     }
 
     boolean hasPermission = true;
-    if (account != null
+    if (requiresAuthentication
+        && 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
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.deploy
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.NoopRegistry
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig
 import com.netflix.spinnaker.clouddriver.security.resources.AccountNameable
 import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable
 import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable
@@ -26,13 +27,20 @@ import org.springframework.security.authentication.TestingAuthenticationToken
 import org.springframework.security.core.context.SecurityContextHolder
 import spock.lang.Specification
 import spock.lang.Subject
+import spock.lang.Unroll
 
 class DescriptionAuthorizerSpec extends Specification {
-  def registry = new NoopRegistry();
+  def registry = new NoopRegistry()
   def evaluator = Mock(FiatPermissionEvaluator)
+  def opsSecurityConfigProps
 
   @Subject
-  DescriptionAuthorizer authorizer = new DescriptionAuthorizer(registry, new ObjectMapper(), Optional.of(evaluator))
+  DescriptionAuthorizer authorizer
+
+  def setup() {
+    opsSecurityConfigProps = new SecurityConfig.OperationsSecurityConfigurationProperties()
+    authorizer = new DescriptionAuthorizer(registry, new ObjectMapper(), Optional.of(evaluator), opsSecurityConfigProps)
+  }
 
   def "should authorize passed description"() {
     given:
@@ -57,6 +65,30 @@ class DescriptionAuthorizerSpec extends Specification {
     errors.allErrors.size() == 4
   }
 
+  @Unroll
+  def "should skip authentication for image tagging description if allowUnauthenticatedImageTaggingInAccounts contains the account"() {
+    given:
+    def description = new TestImageTaggingDescription("testAccount")
+
+    def errors = new DescriptionValidationErrors(description)
+
+    opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts = allowUnauthenticatedImageTaggingInAccounts
+
+    when:
+    authorizer.authorize(description, errors)
+
+    then:
+    expectedNumberOfInvocations * evaluator.hasPermission(*_) >> false
+    0 * evaluator.storeWholePermission()
+    errors.allErrors.size() == expectedNumberOfErrors
+
+    where:
+    allowUnauthenticatedImageTaggingInAccounts || expectedNumberOfInvocations | expectedNumberOfErrors
+    ["testAccount"]                            || 0                           | 0
+    ["anotherAccount"]                         || 1                           | 1
+    []                                         || 1                           | 1
+  }
+
   class TestDescription implements AccountNameable, ApplicationNameable, ResourcesNameable {
     String account
     Collection<String> applications
@@ -68,4 +100,22 @@ class DescriptionAuthorizerSpec extends Specification {
       this.names = names
     }
   }
+
+  class TestImageTaggingDescription implements AccountNameable {
+    String account
+
+    TestImageTaggingDescription(String account) {
+      this.account = account
+    }
+
+    @Override
+    boolean requiresApplicationRestriction() {
+      return false
+    }
+
+    @Override
+    boolean requiresAuthentication(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
@@ -16,8 +16,15 @@
 
 package com.netflix.spinnaker.clouddriver.google.deploy.description
 
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig
+
 class UpsertGoogleImageTagsDescription extends AbstractGoogleCredentialsDescription {
   String imageName
   Map<String, String> tags
   String accountName
+
+  @Override
+  boolean requiresAuthentication(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,7 @@ class SecurityConfig {
   static class OperationsSecurityConfigurationProperties {
     SecurityAction onMissingSecuredCheck = SecurityAction.WARN
     SecurityAction onMissingValidator = SecurityAction.WARN
+    List<String> allowUnauthenticatedImageTaggingInAccounts = []
   }
 
   static enum SecurityAction {

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
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.clouddriver.security.resources;
 
+import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig;
+
 /** Denotes an operation description operates on a specific account. */
 public interface AccountNameable {
   String getAccount();
@@ -27,4 +29,9 @@ public interface AccountNameable {
   default boolean requiresApplicationRestriction() {
     return true;
   }
+
+  default boolean requiresAuthentication(
+      SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
+    return true;
+  }
 }