From 3c5629c26e9d2463be7cde6a7fd46deec6140eea Mon Sep 17 00:00:00 2001 From: Chris Smalley Date: Mon, 20 Apr 2020 16:46:41 -0700 Subject: [PATCH] feat(auth): Add ResourcesNameable to BasicAmazonDeployDescription, default 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 --- ...mazonDeployAtomicOperationConverter.groovy | 25 +++++++++- .../BasicAmazonDeployDescription.groovy | 15 +++++- .../UpsertAmiTagsDescription.groovy | 2 +- ...loyAtomicOperationConverterUnitSpec.groovy | 42 +++++++++++++++- .../deploy/DescriptionAuthorizer.java | 50 ++++++++++++++----- .../deploy/DescriptionAuthorizerSpec.groovy | 2 +- .../UpsertGoogleImageTagsDescription.groovy | 2 +- .../security/config/SecurityConfig.groovy | 2 + .../security/resources/AccountNameable.java | 2 +- .../security/resources/ResourcesNameable.java | 10 ++++ 10 files changed, 132 insertions(+), 20 deletions(-) diff --git a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy index 306796ff563..a41c6e049cd 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy +++ b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverter.groovy @@ -17,16 +17,22 @@ 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)) } @@ -34,6 +40,23 @@ class BasicAmazonDeployAtomicOperationConverter extends AbstractAtomicOperations 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> 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/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy index 5ce7e0bc761..ac4dc35f333 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy +++ b/clouddriver-aws/src/main/groovy/com/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 loadBalancers List targetGroups List securityGroups + List securityGroupNames = [] List lifecycleHooks = [] Map> availabilityZones = [:] Capacity capacity = new Capacity() @@ -102,6 +105,16 @@ class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription return [application] } + @Override + Collection getNames() { + return loadBalancers + targetGroups + securityGroupNames + } + + @Override + boolean requiresAuthorization() { + return false + } + @Canonical static class Capacity { Integer min diff --git a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy b/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy index 7ed92c80bfb..7343c3878b1 100644 --- a/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/description/UpsertAmiTagsDescription.groovy +++ b/clouddriver-aws/src/main/groovy/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/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy b/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy index 37ef680c0d3..5be28dc8565 100644 --- a/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/converters/BasicAmazonDeployAtomicOperationConverterUnitSpec.groovy +++ b/clouddriver-aws/src/test/groovy/com/netflix/spinnaker/clouddriver/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) >> [(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/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java index a121292f841..f2e8f5f3550 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java +++ b/clouddriver-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 { 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 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 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/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy b/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy index 5fcace9de00..3fc4f1a4c55 100644 --- a/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy +++ b/clouddriver-core/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/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/description/UpsertGoogleImageTagsDescription.groovy b/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/description/UpsertGoogleImageTagsDescription.groovy index 69a012a7073..8940b685888 100644 --- a/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/description/UpsertGoogleImageTagsDescription.groovy +++ b/clouddriver-google/src/main/groovy/com/netflix/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/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/config/SecurityConfig.groovy b/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/config/SecurityConfig.groovy index 9079ba18faa..f92feac4f8a 100644 --- a/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/config/SecurityConfig.groovy +++ b/clouddriver-security/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 allowUnauthenticatedImageTaggingInAccounts = [] } diff --git a/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/AccountNameable.java b/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/AccountNameable.java index 5b788b3058b..a5ece852a07 100644 --- a/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/AccountNameable.java +++ b/clouddriver-security/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/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java b/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java index e2111e9afb8..07feb5c8db9 100644 --- a/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java +++ b/clouddriver-security/src/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java @@ -36,4 +36,14 @@ default Collection 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; + } }