From 5b29186784e272bb2c0cdb669ff7fb0e1d0d8537 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Tue, 14 Sep 2021 14:55:09 -0400 Subject: [PATCH 01/12] Working create iam role for egress --- .../lib/data-egress/data-egress-service.js | 97 +++++++++++++++++++ .../environment-resource-service.js | 1 + .../roles-only/filesystem-role-service.js | 2 + .../environment-config-vars-service.js | 24 +++-- .../lib/plugins/env-provisioning-plugin.js | 1 + .../lib/plugins/roles-only-strategy-plugin.js | 1 + .../pre-environment-provisioning.js | 1 + .../backend/config/infra/cloudformation.yml | 58 +++-------- .../backend/config/infra/functions.yml | 1 + 9 files changed, 135 insertions(+), 51 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index ab215066aa..b12c327e62 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -111,11 +111,14 @@ class DataEgressService extends Service { const folderName = `${environment.id}/`; try { + // Where s3 folder gets created s3Service.createPath(bucketName, folderName); } catch (error) { throw this.boom.badRequest(`Error in creating egress store:${folderName} in bucket: ${bucketName}`, true); } + const roleArn = await this.createMainAccountEgressStoreRole(requestContext, environment.id); + // prepare info for ddb and update egress store info const egressStoreId = environment.id; const creationTime = new Date().toISOString; @@ -134,6 +137,7 @@ class DataEgressService extends Service { ver: 0, isAbleToSubmitEgressRequest: false, egressStoreObjectListLocation: null, + roleArn, }; const lockService = await this.service('lockService'); @@ -175,6 +179,7 @@ class DataEgressService extends Service { arn: `arn:aws:s3:::${bucketName}/${environment.id}/`, }, ], + roleArn, }; const memberAccountId = await this.getMemberAccountId(requestContext, environment.id); @@ -231,6 +236,7 @@ class DataEgressService extends Service { ); } + // TODO: Delete created egress role and policy (swb-main-study- and egress-study-) const lockService = await this.service('lockService'); const egressStoreDdbLockId = `egress-store-ddb-access-${egressStoreInfo.id}`; egressStoreInfo.status = TERMINATED_STATUS_CODE; @@ -537,6 +543,97 @@ class DataEgressService extends Service { return memberAccount.accountId; } + async createMainAccountEgressStoreRole(requestContext, environmentId) { + const egressStoreBucketName = this.settings.get('egressStoreBucketName'); + const aws = await this.service('aws'); + const kmsArn = await this.getKmsKeyIdArn(); + + const memberAccountId = await this.getMemberAccountId(requestContext, environmentId); + + const roleName = `swb-main-study-${environmentId}`; + const iam = new aws.sdk.IAM(); + const createRoleResponse = await iam + .createRole({ + AssumeRolePolicyDocument: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Principal: { + AWS: `arn:aws:iam::${memberAccountId}:root`, // member AccountId + }, + Action: 'sts:AssumeRole', + }, + ], + }), + Path: '/', + RoleName: roleName, + }) + .promise(); + + const permissionPolicy = { + Version: '2012-10-17', + Statement: [ + { + Sid: 'S3StudyReadAccess', + Effect: 'Allow', + Action: [ + 's3:GetObject', + 's3:GetObjectVersion', + 's3:GetObjectTagging', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + 's3:PutObjectTagging', + 's3:DeleteObject', + 's3:DeleteObjectVersion', + ], + Resource: `arn:aws:s3:::${egressStoreBucketName}/${environmentId}`, + }, + { + Sid: 'S3StudyList', + Effect: 'Allow', + Action: ['s3:ListBucket', 's3:ListBucketVersions'], + Resource: `arn:aws:s3:::${egressStoreBucketName}`, + Condition: { + StringLike: { + 's3:prefix': `${environmentId}/*`, + }, + }, + }, + { + Sid: 'studyKMSAccess', + Action: ['kms:Decrypt', 'kms:DescribeKey', 'kms:Encrypt', 'kms:GenerateDataKey', 'kms:ReEncrypt*'], + Effect: 'Allow', + Resource: kmsArn, + }, + ], + }; + + const createPolicyResponse = await iam + .createPolicy({ + PolicyName: `egress-study-${environmentId}`, + PolicyDocument: JSON.stringify(permissionPolicy), + }) + .promise(); + + await iam + .attachRolePolicy({ + RoleName: roleName, + PolicyArn: createPolicyResponse.Policy.Arn, + }) + .promise(); + await iam + .putRolePermissionsBoundary({ + RoleName: roleName, + PermissionsBoundary: createPolicyResponse.Policy.Arn, + }) + .promise(); + + return createRoleResponse.Role.Arn; + } + async lockAndUpdate(lockId, dbKey, dbObject) { const lockService = await this.service('lockService'); await lockService.tryWriteLockAndRun({ id: lockId }, async () => { diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js index 3d0f9d6686..a2c91a5395 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js @@ -46,6 +46,7 @@ class EnvironmentResourceService extends Service { ]); } + // TODO: Look here /** * Allocates all the necessary AWS resources to allow access to the studies. This includes acquiring the necessary * filesystem roles, as needed. diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js index be6841a73b..23fa0b1bb6 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js @@ -117,6 +117,7 @@ class FilesystemRoleService extends Service { return result; } + // TODO: Look here /** * Call this method to allocate a filesystem role entity for the given study. This method is smart * enough to reuse an existing filesystem role entity if there is one already for the same study. @@ -424,6 +425,7 @@ class FilesystemRoleService extends Service { const iamClient = await this.getIamClient(fsRoleEntity.appRoleArn, _.get(fsRoleEntity.studies, '[0].id', '')); const trustPolicyDoc = toTrustPolicyDoc(fsRoleEntity); + // TODO: NOTE: PERMISSION BOUNDARY NEEDED let params = { AssumeRolePolicyDocument: JSON.stringify(trustPolicyDoc), RoleName: name, diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js index 9c6a573069..daf44e5172 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js @@ -38,6 +38,7 @@ class EnvironmentConfigVarsService extends Service { constructor() { super(); this.dependency([ + 'aws', 'userService', 'environmentScService', 'environmentScKeypairService', @@ -189,15 +190,22 @@ class EnvironmentConfigVarsService extends Service { const { xAccEnvMgmtRoleArn, externalId, - accountId, + accountId: memberAccountId, vpcId, subnetId, encryptionKeyArn, } = await awsAccountsService.mustFind(requestContext, { id: awsAccountId }); // Check launch pre-requisites - if (!(xAccEnvMgmtRoleArn && externalId && accountId && vpcId && subnetId && encryptionKeyArn)) { - const cause = this.getConfigError(xAccEnvMgmtRoleArn, externalId, accountId, vpcId, subnetId, encryptionKeyArn); + if (!(xAccEnvMgmtRoleArn && externalId && memberAccountId && vpcId && subnetId && encryptionKeyArn)) { + const cause = this.getConfigError( + xAccEnvMgmtRoleArn, + externalId, + memberAccountId, + vpcId, + subnetId, + encryptionKeyArn, + ); throw this.boom.badRequest(`Index "${indexId}" has not been correctly configured: missing ${cause}.`, true); } @@ -218,7 +226,7 @@ class EnvironmentConfigVarsService extends Service { // Share AMIs with the target account (process in batches of 5 at a time) // if there are more than 5 await processInBatches(amisToShare, 5, async imageId => { - return environmentAmiService.ensurePermissions({ imageId, accountId }); + return environmentAmiService.ensurePermissions({ imageId, accountId: memberAccountId }); }); } @@ -227,13 +235,13 @@ class EnvironmentConfigVarsService extends Service { const iamPolicyDocument = await this.getEnvRolePolicy(requestContext, { environment, studies, - memberAccountId: accountId, + memberAccountId, }); const s3Mounts = await this.getS3Mounts(requestContext, { environment, studies, - memberAccountId: accountId, + memberAccountId, }); let egressStoreIamPolicyDocument = {}; @@ -244,7 +252,7 @@ class EnvironmentConfigVarsService extends Service { egressStoreIamPolicyDocument = await this.getEnvEgressStorePolicy(requestContext, { environment, egressStore: egressStoreMount, - memberAccountId: accountId, + memberAccountId, }); } @@ -281,7 +289,7 @@ class EnvironmentConfigVarsService extends Service { envTypeConfigId, name, description, - accountId, + accountId: memberAccountId, projectId, indexId, studyIds, diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js index bba7fc000c..14deb7429d 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js @@ -76,6 +76,7 @@ async function preProvisioning({ requestContext, container, envId }) { const memberAccount = await environmentScService.getMemberAccount(requestContext, environmentScEntity); const pluginRegistryService = await container.find('pluginRegistryService'); + // TODO: Look here if (_.isEmpty(studies)) { await pluginRegistryService.visitPlugins('study-access-strategy', 'updateKMSPolicyForEgress', { payload: { diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js index 45cb7789de..2ff386ca2d 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js @@ -81,6 +81,7 @@ async function allocateEnvStudyResources(payload) { return payload; } +// TODO: Look here async function updateKMSPolicyForEgress(payload) { const { requestContext, container, environmentScEntity, studies, memberAccountId } = payload; diff --git a/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js b/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js index 4777da1289..b143626728 100644 --- a/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js +++ b/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js @@ -40,6 +40,7 @@ class PreEnvironmentProvisioning extends StepBase { const [pluginRegistryService] = await this.mustFindServices(['pluginRegistryService']); + // TODO: Look here try { await pluginRegistryService.visitPlugins(pluginConstants.extensionPoint, 'onEnvPreProvisioning', { payload: { diff --git a/main/solution/backend/config/infra/cloudformation.yml b/main/solution/backend/config/infra/cloudformation.yml index b98144df61..59484ecb5d 100644 --- a/main/solution/backend/config/infra/cloudformation.yml +++ b/main/solution/backend/config/infra/cloudformation.yml @@ -623,49 +623,8 @@ Resources: - dynamodb:Scan - dynamodb:UpdateItem Resource: - - !GetAtt [PasswordsDb, Arn] - - !GetAtt [UsersDb, Arn] - - !GetAtt [LocksDb, Arn] - - !GetAtt [StepTemplatesDb, Arn] - - !GetAtt [WorkflowTemplatesDb, Arn] - - !GetAtt [WorkflowTemplateDraftsDb, Arn] - - !Join ['', [!GetAtt [WorkflowTemplateDraftsDb, Arn], '/index/*']] - - !GetAtt [WorkflowsDb, Arn] - - !GetAtt [WorkflowDraftsDb, Arn] - - !Join ['', [!GetAtt [WorkflowDraftsDb, Arn], '/index/*']] - - !GetAtt [WorkflowInstancesDb, Arn] - - !Join ['', [!GetAtt [WorkflowInstancesDb, Arn], '/index/*']] - - !GetAtt [WfAssignmentsDb, Arn] - - !Join ['', [!GetAtt [WfAssignmentsDb, Arn], '/index/*']] - - !GetAtt [EnvironmentsDb, Arn] - - !GetAtt [EnvironmentsScDb, Arn] - - !Join ['', [!GetAtt [EnvironmentsScDb, Arn], '/index/*']] - - !GetAtt [EnvironmentsTypesDb, Arn] - - !GetAtt [StudiesDb, Arn] - - !GetAtt [ProjectsDb, Arn] - - !GetAtt [StudyPermissionsDb, Arn] - - !GetAtt [IndexesDb, Arn] - - !GetAtt [AccountsDb, Arn] - - !GetAtt [AwsAccountsDb, Arn] - - !GetAtt [AuthenticationProviderTypesDb, Arn] - - !GetAtt [AuthenticationProviderConfigsDb, Arn] - - !GetAtt [RevokedTokensDb, Arn] - - !Join ['', [!GetAtt [StudiesDb, Arn], '/index/*']] - - !GetAtt [IndexesDb, Arn] - - !GetAtt [CostApiCachesDb, Arn] - - !GetAtt [UserRolesDb, Arn] - - !GetAtt [StorageGatewayDb, Arn] - - !GetAtt [DsAccountsDb, Arn] - - !GetAtt [RoleAllocationsDb, Arn] - - !GetAtt [ResourceUsagesDb, Arn] - - !If - - EnableEgressStore - - !Sub 'arn:aws:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${self:custom.settings.dbEgressStore}' - - !Ref 'AWS::NoValue' - - !If - - EnableEgressStore - - !Sub 'arn:aws:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${self:custom.settings.dbEgressStore}/index/*' - - !Ref 'AWS::NoValue' + - !Sub 'arn:aws:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${self:custom.settings.dbPrefix}-*' + - !Sub 'arn:aws:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${self:custom.settings.dbPrefix}-*/index/*' - Effect: Allow Action: - ssm:GetParameter @@ -767,6 +726,19 @@ Resources: - appstream:UpdateImagePermissions Resource: - !Sub 'arn:aws:appstream:${AWS::Region}:${AWS::AccountId}:image/ServiceWorkbench*' + - Effect: Allow + Action: + - iam:CreateRole + - iam:AttachRolePolicy + - iam:PutRolePermissionsBoundary + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + #TODO: Add condition of a permissionBoundary as a requirement + - Effect: Allow + Action: + - iam:CreatePolicy + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' # IAM Role for the workflowLoopRunner Function RoleWorkflowLoopRunner: diff --git a/main/solution/backend/config/infra/functions.yml b/main/solution/backend/config/infra/functions.yml index 3d1d539720..269f980868 100644 --- a/main/solution/backend/config/infra/functions.yml +++ b/main/solution/backend/config/infra/functions.yml @@ -215,3 +215,4 @@ workflowLoopRunner: APP_EGRESS_STORE_KMS_KEY_ARN: !Sub arn:aws:kms:${AWS::Region}:${AWS::AccountId}:alias/${self:custom.settings.egressStoreKmsKeyAlias} APP_EGRESS_STORE_KMS_POLICY_WORKSPACE_SID: ${self:custom.settings.egressStoreKmsPolicyWorkspaceSid} APP_IS_APP_STREAM_ENABLED: ${self:custom.settings.isAppStreamEnabled} +# APP_AWS_REGION_SHORT_NAME: ${self:custom.settings.awsRegionShortName} From 592df15c53e933125418bb509495e0408d3428b9 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Wed, 15 Sep 2021 10:34:38 -0400 Subject: [PATCH 02/12] Update 2 --- .../lib/data-egress/data-egress-service.js | 100 +++++++++++------- .../backend/config/infra/cloudformation.yml | 21 +++- 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index b12c327e62..1ee417728c 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -117,10 +117,10 @@ class DataEgressService extends Service { throw this.boom.badRequest(`Error in creating egress store:${folderName} in bucket: ${bucketName}`, true); } - const roleArn = await this.createMainAccountEgressStoreRole(requestContext, environment.id); + const egressStoreId = environment.id; + const roleArn = await this.createMainAccountEgressStoreRole(requestContext, egressStoreId); // prepare info for ddb and update egress store info - const egressStoreId = environment.id; const creationTime = new Date().toISOString; const dbObject = { id: egressStoreId, @@ -213,7 +213,6 @@ class DataEgressService extends Service { true, ); } - const s3Service = await this.service('s3Service'); const egressStoreStatus = egressStoreInfo.status; const isEgressStoreNotTouched = @@ -235,8 +234,9 @@ class DataEgressService extends Service { true, ); } - // TODO: Delete created egress role and policy (swb-main-study- and egress-study-) + await this.deleteMainAccountEgressStoreRole(egressStoreInfo.id); + const lockService = await this.service('lockService'); const egressStoreDdbLockId = `egress-store-ddb-access-${egressStoreInfo.id}`; egressStoreInfo.status = TERMINATED_STATUS_CODE; @@ -461,6 +461,11 @@ class DataEgressService extends Service { return new aws.sdk.KMS(); } + async getIAM() { + const aws = await this.getAWS(); + return new aws.sdk.IAM(); + } + async getAWS() { const aws = await this.service('aws'); return aws; @@ -543,33 +548,21 @@ class DataEgressService extends Service { return memberAccount.accountId; } - async createMainAccountEgressStoreRole(requestContext, environmentId) { + getMainAccountEgressStoreRole(egressStoreId) { + return `swb-main-study-${egressStoreId}`; + } + + getMainAccountEgressStoreRolePolicyName(egressStoreId) { + return `egress-study-${egressStoreId}`; + } + + async createMainAccountEgressStoreRole(requestContext, egressStoreId) { const egressStoreBucketName = this.settings.get('egressStoreBucketName'); - const aws = await this.service('aws'); const kmsArn = await this.getKmsKeyIdArn(); - const memberAccountId = await this.getMemberAccountId(requestContext, environmentId); + const memberAccountId = await this.getMemberAccountId(requestContext, egressStoreId); - const roleName = `swb-main-study-${environmentId}`; - const iam = new aws.sdk.IAM(); - const createRoleResponse = await iam - .createRole({ - AssumeRolePolicyDocument: JSON.stringify({ - Version: '2012-10-17', - Statement: [ - { - Effect: 'Allow', - Principal: { - AWS: `arn:aws:iam::${memberAccountId}:root`, // member AccountId - }, - Action: 'sts:AssumeRole', - }, - ], - }), - Path: '/', - RoleName: roleName, - }) - .promise(); + const roleName = this.getMainAccountEgressStoreRole(egressStoreId); const permissionPolicy = { Version: '2012-10-17', @@ -589,7 +582,7 @@ class DataEgressService extends Service { 's3:DeleteObject', 's3:DeleteObjectVersion', ], - Resource: `arn:aws:s3:::${egressStoreBucketName}/${environmentId}`, + Resource: `arn:aws:s3:::${egressStoreBucketName}/${egressStoreId}`, }, { Sid: 'S3StudyList', @@ -598,7 +591,7 @@ class DataEgressService extends Service { Resource: `arn:aws:s3:::${egressStoreBucketName}`, Condition: { StringLike: { - 's3:prefix': `${environmentId}/*`, + 's3:prefix': `${egressStoreId}/*`, }, }, }, @@ -610,30 +603,65 @@ class DataEgressService extends Service { }, ], }; - + const iam = await this.getIAM(); const createPolicyResponse = await iam .createPolicy({ - PolicyName: `egress-study-${environmentId}`, + PolicyName: this.getMainAccountEgressStoreRolePolicyName(egressStoreId), PolicyDocument: JSON.stringify(permissionPolicy), }) .promise(); - await iam - .attachRolePolicy({ + const createRoleResponse = await iam + .createRole({ + AssumeRolePolicyDocument: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Principal: { + AWS: `arn:aws:iam::${memberAccountId}:root`, // member AccountId + }, + Action: 'sts:AssumeRole', + }, + ], + }), + Path: '/', RoleName: roleName, - PolicyArn: createPolicyResponse.Policy.Arn, + PermissionsBoundary: createPolicyResponse.Policy.Arn, }) .promise(); + await iam - .putRolePermissionsBoundary({ + .attachRolePolicy({ RoleName: roleName, - PermissionsBoundary: createPolicyResponse.Policy.Arn, + PolicyArn: createPolicyResponse.Policy.Arn, }) .promise(); + // await iam + // .putRolePermissionsBoundary({ + // RoleName: roleName, + // PermissionsBoundary: createPolicyResponse.Policy.Arn, + // }) + // .promise(); + return createRoleResponse.Role.Arn; } + async deleteMainAccountEgressStoreRole(egressStoreId) { + const iam = await this.getIAM(); + const policyArn = (await iam.getRole({ RoleName: this.getMainAccountEgressStoreRole(egressStoreId) }).promise()) + .Role.PermissionsBoundary.PermissionsBoundaryArn; + await iam + .detachRolePolicy({ + RoleName: this.getMainAccountEgressStoreRole(egressStoreId), + PolicyArn: policyArn, + }) + .promise(); + await iam.deleteRole({ RoleName: this.getMainAccountEgressStoreRole(egressStoreId) }).promise(); + await iam.deletePolicy({ PolicyArn: policyArn }).promise(); + } + async lockAndUpdate(lockId, dbKey, dbObject) { const lockService = await this.service('lockService'); await lockService.tryWriteLockAndRun({ id: lockId }, async () => { diff --git a/main/solution/backend/config/infra/cloudformation.yml b/main/solution/backend/config/infra/cloudformation.yml index 59484ecb5d..18c9564a31 100644 --- a/main/solution/backend/config/infra/cloudformation.yml +++ b/main/solution/backend/config/infra/cloudformation.yml @@ -589,6 +589,18 @@ Resources: - appstream:UpdateImagePermissions Resource: - !Sub 'arn:aws:appstream:${AWS::Region}:${AWS::AccountId}:image/ServiceWorkbench*' + - Effect: Allow + Action: + - iam:GetRole + - iam:DeleteRole + - iam:DetachRolePolicy + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + - Effect: Allow + Action: + - iam:DeletePolicy + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' # IAM Role for the apiHandler Function RoleApiHandler: Type: 'AWS::IAM::Role' @@ -729,11 +741,16 @@ Resources: - Effect: Allow Action: - iam:CreateRole + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + Condition: + StringLike: + iam:PermissionsBoundary: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' + - Effect: Allow + Action: - iam:AttachRolePolicy - - iam:PutRolePermissionsBoundary Resource: - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' - #TODO: Add condition of a permissionBoundary as a requirement - Effect: Allow Action: - iam:CreatePolicy From 599e1a3be93c72b613bb47f0aebe011d9a501cfe Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Wed, 15 Sep 2021 13:09:26 -0400 Subject: [PATCH 03/12] Only enableEgressStore when a file was actually added --- .../lambdas/egress-store-object-handler/handler.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/main/solution/post-deployment/src/lambdas/egress-store-object-handler/handler.js b/main/solution/post-deployment/src/lambdas/egress-store-object-handler/handler.js index 499844a371..8a57d26bcc 100644 --- a/main/solution/post-deployment/src/lambdas/egress-store-object-handler/handler.js +++ b/main/solution/post-deployment/src/lambdas/egress-store-object-handler/handler.js @@ -45,11 +45,23 @@ const handlerWithContainer = async (container, event) => { } const dataEgressService = await container.find('dataEgressService'); const egressStoreInfo = await dataEgressService.getEgressStoreInfo(egressStoreId); - if (!egressStoreInfo.isAbleToSubmitEgressRequest) { + if (!egressStoreInfo.isAbleToSubmitEgressRequest && S3FileWasAdded(event.Records)) { await dataEgressService.enableEgressStoreSubmission(egressStoreInfo); } }; +const S3FileWasAdded = records => { + let fileWasAdded = false; + records.forEach(record => { + // Sometimes an S3 Put event is created for a file path/folder being created, but no actual file was placed in the bucket. + // In that scenario the file object size is 0 + if (record.s3.object.size > 0) { + fileWasAdded = true; + } + }); + return fileWasAdded; +}; + // eslint-disable-next-line import/prefer-default-export module.exports.handler = handler; module.exports.handlerWithContainer = handlerWithContainer; From 94ed178534ea1d004d97464b1a581abe76319f5e Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Wed, 15 Sep 2021 15:30:42 -0400 Subject: [PATCH 04/12] Remove unnecessary comments --- .../lib/data-egress/data-egress-service.js | 28 ++++++++----------- .../environment-resource-service.js | 1 - .../roles-only/filesystem-role-service.js | 2 -- .../environment-config-vars-service.js | 23 ++++++--------- .../lib/plugins/env-provisioning-plugin.js | 1 - .../lib/plugins/roles-only-strategy-plugin.js | 1 - .../pre-environment-provisioning.js | 1 - .../backend/config/infra/functions.yml | 1 - 8 files changed, 20 insertions(+), 38 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index 1ee417728c..fcc26993c7 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -111,7 +111,6 @@ class DataEgressService extends Service { const folderName = `${environment.id}/`; try { - // Where s3 folder gets created s3Service.createPath(bucketName, folderName); } catch (error) { throw this.boom.badRequest(`Error in creating egress store:${folderName} in bucket: ${bucketName}`, true); @@ -234,7 +233,6 @@ class DataEgressService extends Service { true, ); } - // TODO: Delete created egress role and policy (swb-main-study- and egress-study-) await this.deleteMainAccountEgressStoreRole(egressStoreInfo.id); const lockService = await this.service('lockService'); @@ -556,14 +554,18 @@ class DataEgressService extends Service { return `egress-study-${egressStoreId}`; } + /** + * Create the main account IAM Role and Policy to allow a workspace on the member account to access + * the egress store S3 bucket + * @param requestContext + * @param egressStoreId + * @returns role Arn + */ async createMainAccountEgressStoreRole(requestContext, egressStoreId) { const egressStoreBucketName = this.settings.get('egressStoreBucketName'); const kmsArn = await this.getKmsKeyIdArn(); - const memberAccountId = await this.getMemberAccountId(requestContext, egressStoreId); - - const roleName = this.getMainAccountEgressStoreRole(egressStoreId); - + const mainAccountRoleName = this.getMainAccountEgressStoreRole(egressStoreId); const permissionPolicy = { Version: '2012-10-17', Statement: [ @@ -603,6 +605,7 @@ class DataEgressService extends Service { }, ], }; + const iam = await this.getIAM(); const createPolicyResponse = await iam .createPolicy({ @@ -619,32 +622,25 @@ class DataEgressService extends Service { { Effect: 'Allow', Principal: { - AWS: `arn:aws:iam::${memberAccountId}:root`, // member AccountId + AWS: `arn:aws:iam::${memberAccountId}:root`, }, Action: 'sts:AssumeRole', }, ], }), Path: '/', - RoleName: roleName, + RoleName: mainAccountRoleName, PermissionsBoundary: createPolicyResponse.Policy.Arn, }) .promise(); await iam .attachRolePolicy({ - RoleName: roleName, + RoleName: mainAccountRoleName, PolicyArn: createPolicyResponse.Policy.Arn, }) .promise(); - // await iam - // .putRolePermissionsBoundary({ - // RoleName: roleName, - // PermissionsBoundary: createPolicyResponse.Policy.Arn, - // }) - // .promise(); - return createRoleResponse.Role.Arn; } diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js index a2c91a5395..3d0f9d6686 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/environment-resource-service.js @@ -46,7 +46,6 @@ class EnvironmentResourceService extends Service { ]); } - // TODO: Look here /** * Allocates all the necessary AWS resources to allow access to the studies. This includes acquiring the necessary * filesystem roles, as needed. diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js index 23fa0b1bb6..be6841a73b 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-source/access-strategy/roles-only/filesystem-role-service.js @@ -117,7 +117,6 @@ class FilesystemRoleService extends Service { return result; } - // TODO: Look here /** * Call this method to allocate a filesystem role entity for the given study. This method is smart * enough to reuse an existing filesystem role entity if there is one already for the same study. @@ -425,7 +424,6 @@ class FilesystemRoleService extends Service { const iamClient = await this.getIamClient(fsRoleEntity.appRoleArn, _.get(fsRoleEntity.studies, '[0].id', '')); const trustPolicyDoc = toTrustPolicyDoc(fsRoleEntity); - // TODO: NOTE: PERMISSION BOUNDARY NEEDED let params = { AssumeRolePolicyDocument: JSON.stringify(trustPolicyDoc), RoleName: name, diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js index daf44e5172..d163143fec 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/environment/service-catalog/environment-config-vars-service.js @@ -190,22 +190,15 @@ class EnvironmentConfigVarsService extends Service { const { xAccEnvMgmtRoleArn, externalId, - accountId: memberAccountId, + accountId, vpcId, subnetId, encryptionKeyArn, } = await awsAccountsService.mustFind(requestContext, { id: awsAccountId }); // Check launch pre-requisites - if (!(xAccEnvMgmtRoleArn && externalId && memberAccountId && vpcId && subnetId && encryptionKeyArn)) { - const cause = this.getConfigError( - xAccEnvMgmtRoleArn, - externalId, - memberAccountId, - vpcId, - subnetId, - encryptionKeyArn, - ); + if (!(xAccEnvMgmtRoleArn && externalId && accountId && vpcId && subnetId && encryptionKeyArn)) { + const cause = this.getConfigError(xAccEnvMgmtRoleArn, externalId, accountId, vpcId, subnetId, encryptionKeyArn); throw this.boom.badRequest(`Index "${indexId}" has not been correctly configured: missing ${cause}.`, true); } @@ -226,7 +219,7 @@ class EnvironmentConfigVarsService extends Service { // Share AMIs with the target account (process in batches of 5 at a time) // if there are more than 5 await processInBatches(amisToShare, 5, async imageId => { - return environmentAmiService.ensurePermissions({ imageId, accountId: memberAccountId }); + return environmentAmiService.ensurePermissions({ imageId, accountId }); }); } @@ -235,13 +228,13 @@ class EnvironmentConfigVarsService extends Service { const iamPolicyDocument = await this.getEnvRolePolicy(requestContext, { environment, studies, - memberAccountId, + accountId, }); const s3Mounts = await this.getS3Mounts(requestContext, { environment, studies, - memberAccountId, + accountId, }); let egressStoreIamPolicyDocument = {}; @@ -252,7 +245,7 @@ class EnvironmentConfigVarsService extends Service { egressStoreIamPolicyDocument = await this.getEnvEgressStorePolicy(requestContext, { environment, egressStore: egressStoreMount, - memberAccountId, + accountId, }); } @@ -289,7 +282,7 @@ class EnvironmentConfigVarsService extends Service { envTypeConfigId, name, description, - accountId: memberAccountId, + accountId, projectId, indexId, studyIds, diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js index 14deb7429d..bba7fc000c 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/env-provisioning-plugin.js @@ -76,7 +76,6 @@ async function preProvisioning({ requestContext, container, envId }) { const memberAccount = await environmentScService.getMemberAccount(requestContext, environmentScEntity); const pluginRegistryService = await container.find('pluginRegistryService'); - // TODO: Look here if (_.isEmpty(studies)) { await pluginRegistryService.visitPlugins('study-access-strategy', 'updateKMSPolicyForEgress', { payload: { diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js index 2ff386ca2d..45cb7789de 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/plugins/roles-only-strategy-plugin.js @@ -81,7 +81,6 @@ async function allocateEnvStudyResources(payload) { return payload; } -// TODO: Look here async function updateKMSPolicyForEgress(payload) { const { requestContext, container, environmentScEntity, studies, memberAccountId } = payload; diff --git a/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js b/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js index b143626728..4777da1289 100644 --- a/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js +++ b/addons/addon-environment-sc-api/packages/environment-sc-workflow-steps/lib/steps/pre-environment-provisioning/pre-environment-provisioning.js @@ -40,7 +40,6 @@ class PreEnvironmentProvisioning extends StepBase { const [pluginRegistryService] = await this.mustFindServices(['pluginRegistryService']); - // TODO: Look here try { await pluginRegistryService.visitPlugins(pluginConstants.extensionPoint, 'onEnvPreProvisioning', { payload: { diff --git a/main/solution/backend/config/infra/functions.yml b/main/solution/backend/config/infra/functions.yml index 269f980868..3d1d539720 100644 --- a/main/solution/backend/config/infra/functions.yml +++ b/main/solution/backend/config/infra/functions.yml @@ -215,4 +215,3 @@ workflowLoopRunner: APP_EGRESS_STORE_KMS_KEY_ARN: !Sub arn:aws:kms:${AWS::Region}:${AWS::AccountId}:alias/${self:custom.settings.egressStoreKmsKeyAlias} APP_EGRESS_STORE_KMS_POLICY_WORKSPACE_SID: ${self:custom.settings.egressStoreKmsPolicyWorkspaceSid} APP_IS_APP_STREAM_ENABLED: ${self:custom.settings.isAppStreamEnabled} -# APP_AWS_REGION_SHORT_NAME: ${self:custom.settings.awsRegionShortName} From ab4f4a2108c641cbd5c2b1d076f682bfa369471c Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Wed, 15 Sep 2021 16:42:14 -0400 Subject: [PATCH 05/12] Update failing tests --- .../__test__/data-egress-service.test.js | 64 +++++++++++++++++-- .../lib/data-egress/data-egress-service.js | 4 +- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js index e6713d241f..99b5a6fe7d 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js @@ -250,9 +250,11 @@ describe('DataEgressService', () => { return undefined; }, }; + const roleArn = 'test-RoleArn'; + const environmentId = 'test-id'; const requestContext = {}; const rawEnvironment = { - id: 'test-id', + id: environmentId, name: 'test-raw-environment-name', createdBy: 'test-raw-environment-createdby', updatedBy: 'test-updatedBy', @@ -291,6 +293,7 @@ describe('DataEgressService', () => { arn: `arn:aws:s3:::test-egressStoreBucketName/${rawEnvironment.id}/`, }, ], + roleArn, }; AWSMock.mock('KMS', 'describeKey', (params, callback) => { expect(params).toMatchObject({ @@ -312,6 +315,30 @@ describe('DataEgressService', () => { }); }); + const createPolicyArn = 'test-policy-arn'; + AWSMock.mock('IAM', 'createPolicy', (params, callback) => { + expect(params.PolicyName).toEqual(`study-${environmentId}`); + callback(null, { + Policy: { + Arn: createPolicyArn, + }, + }); + }); + AWSMock.mock('IAM', 'createRole', (params, callback) => { + expect(params.RoleName).toEqual(`study-${environmentId}`); + callback(null, { + Role: { + Arn: roleArn, + }, + }); + }); + AWSMock.mock('IAM', 'attachRolePolicy', (params, callback) => { + expect(params).toMatchObject({ + RoleName: `study-${environmentId}`, + PolicyArn: createPolicyArn, + }); + callback(); + }); const putBucketPolicyMock = jest.fn((params, callback) => { expect(params).toMatchObject({ Bucket: 'test-egressStoreBucketName', @@ -460,13 +487,14 @@ describe('DataEgressService', () => { }, }; + const egressStoreId = 'test-egress-store-id'; dbService.table.scan.mockResolvedValueOnce([ { status: 'PROCESSED', workspaceId: 'test-workspace-id', s3BucketName: 'test-s3BucketName', s3BucketPath: 'test-s3BucketPath', - id: 'test-egress-store-id', + id: egressStoreId, }, ]); const requestContext = {}; @@ -495,7 +523,6 @@ describe('DataEgressService', () => { Policy: JSON.stringify(s3Policy), }); }); - const putBucketPolicyMock = jest.fn((params, callback) => { expect(params).toMatchObject({ Bucket: 'test-egressStoreBucketName', @@ -506,6 +533,8 @@ describe('DataEgressService', () => { lockService.tryWriteLockAndRun = jest.fn((_params, callback) => callback()); AWSMock.mock('S3', 'putBucketPolicy', putBucketPolicyMock); + mockDeleteEgressStoreRole(egressStoreId); + await dataEgressService.terminateEgressStore(requestContext, envId); expect(dataEgressService.removeEgressStoreBucketPolicy).toHaveBeenCalledTimes(1); expect(dataEgressService.removeEgressStoreBucketPolicy).toHaveBeenCalledWith( @@ -527,6 +556,31 @@ describe('DataEgressService', () => { ); }); + function mockDeleteEgressStoreRole(egressStoreId) { + const policyArn = 'test-PermissionBoundaryArn'; + AWSMock.mock('IAM', 'getRole', (params, callback) => { + expect(params.RoleName).toEqual(`study-${egressStoreId}`); + callback(null, { + Role: { + PermissionsBoundary: { + PermissionsBoundaryArn: policyArn, + }, + }, + }); + }); + AWSMock.mock('IAM', 'detachRolePolicy', (params, callback) => { + expect(params).toMatchObject({ RoleName: `study-${egressStoreId}`, PolicyArn: policyArn }); + callback(); + }); + AWSMock.mock('IAM', 'deleteRole', (params, callback) => { + expect(params).toMatchObject({ RoleName: `study-${egressStoreId}` }); + callback(); + }); + AWSMock.mock('IAM', 'deletePolicy', (params, callback) => { + expect(params).toMatchObject({ PolicyArn: policyArn }); + callback(); + }); + } it('should successfully delete egress store while egress store is not touched since created', async () => { dataEgressService.removeEgressStoreBucketPolicy = jest.fn(); const s3Policy = testS3PolicyFn(); @@ -545,13 +599,14 @@ describe('DataEgressService', () => { }, }; + const egressStoreId = 'test-egress-store-id'; dbService.table.scan.mockResolvedValueOnce([ { status: 'CREATED', workspaceId: 'test-workspace-id', s3BucketName: 'test-s3BucketName', s3BucketPath: 'test-s3BucketPath', - id: 'test-egress-store-id', + id: egressStoreId, isAbleToSubmitEgressRequest: false, }, ]); @@ -591,6 +646,7 @@ describe('DataEgressService', () => { // Mock locking so that the putBucketPolicy actually gets called lockService.tryWriteLockAndRun = jest.fn((_params, callback) => callback()); AWSMock.mock('S3', 'putBucketPolicy', putBucketPolicyMock); + mockDeleteEgressStoreRole(egressStoreId); await dataEgressService.terminateEgressStore(requestContext, envId); expect(dataEgressService.removeEgressStoreBucketPolicy).toHaveBeenCalledTimes(1); diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index fcc26993c7..610e1d03be 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -547,11 +547,11 @@ class DataEgressService extends Service { } getMainAccountEgressStoreRole(egressStoreId) { - return `swb-main-study-${egressStoreId}`; + return `study-${egressStoreId}`; } getMainAccountEgressStoreRolePolicyName(egressStoreId) { - return `egress-study-${egressStoreId}`; + return `study-${egressStoreId}`; } /** From d611b3e6c3046e1653326f3386329b6af51325a1 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Wed, 15 Sep 2021 17:48:11 -0400 Subject: [PATCH 06/12] New role name --- .../solution/backend/config/infra/cloudformation.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/main/solution/backend/config/infra/cloudformation.yml b/main/solution/backend/config/infra/cloudformation.yml index 18c9564a31..63345ca8ee 100644 --- a/main/solution/backend/config/infra/cloudformation.yml +++ b/main/solution/backend/config/infra/cloudformation.yml @@ -595,12 +595,12 @@ Resources: - iam:DeleteRole - iam:DetachRolePolicy Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' - Effect: Allow Action: - iam:DeletePolicy Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/study-*' # IAM Role for the apiHandler Function RoleApiHandler: Type: 'AWS::IAM::Role' @@ -742,20 +742,20 @@ Resources: Action: - iam:CreateRole Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' Condition: StringLike: - iam:PermissionsBoundary: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' + iam:PermissionsBoundary: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/study-*' - Effect: Allow Action: - iam:AttachRolePolicy Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-main-study-*' + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' - Effect: Allow Action: - iam:CreatePolicy Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/egress-study-*' + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/study-*' # IAM Role for the workflowLoopRunner Function RoleWorkflowLoopRunner: From c24b3df6aed960e7a1c43378f77898a3f02af8b4 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Thu, 16 Sep 2021 15:24:39 -0400 Subject: [PATCH 07/12] PR comment --- .../lib/data-egress/data-egress-service.js | 81 +++++++++---------- .../helpers/__tests__/study-policy.test.js | 65 +++++++++++++++ .../backend/config/infra/cloudformation.yml | 50 +++++++++++- .../backend/config/infra/functions.yml | 1 + .../backend/config/settings/.defaults.yml | 2 + .../__tests__/handler.test.js | 40 +++++++++ 6 files changed, 192 insertions(+), 47 deletions(-) create mode 100644 addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index 610e1d03be..ba497bd478 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -27,6 +27,7 @@ const { getRevisedS3Statements, removeAccountFromStatement, } = require('../helpers/utils'); +const { StudyPolicy } = require('../helpers/iam/study-policy'); const settingKeys = { tableName: 'dbEgressStore', @@ -566,45 +567,21 @@ class DataEgressService extends Service { const kmsArn = await this.getKmsKeyIdArn(); const memberAccountId = await this.getMemberAccountId(requestContext, egressStoreId); const mainAccountRoleName = this.getMainAccountEgressStoreRole(egressStoreId); - const permissionPolicy = { - Version: '2012-10-17', - Statement: [ - { - Sid: 'S3StudyReadAccess', - Effect: 'Allow', - Action: [ - 's3:GetObject', - 's3:GetObjectVersion', - 's3:GetObjectTagging', - 's3:AbortMultipartUpload', - 's3:ListMultipartUploadParts', - 's3:PutObject', - 's3:PutObjectAcl', - 's3:PutObjectTagging', - 's3:DeleteObject', - 's3:DeleteObjectVersion', - ], - Resource: `arn:aws:s3:::${egressStoreBucketName}/${egressStoreId}`, - }, - { - Sid: 'S3StudyList', - Effect: 'Allow', - Action: ['s3:ListBucket', 's3:ListBucketVersions'], - Resource: `arn:aws:s3:::${egressStoreBucketName}`, - Condition: { - StringLike: { - 's3:prefix': `${egressStoreId}/*`, - }, - }, - }, - { - Sid: 'studyKMSAccess', - Action: ['kms:Decrypt', 'kms:DescribeKey', 'kms:Encrypt', 'kms:GenerateDataKey', 'kms:ReEncrypt*'], - Effect: 'Allow', - Resource: kmsArn, - }, - ], + const permissionBoundaryArn = this.settings.get('permissionBoundaryPolicyStudyBucketArn'); + + const egressStudyPolicy = new StudyPolicy(); + + const study = { + bucket: egressStoreBucketName, + folder: [egressStoreId], + permission: { + read: true, + write: true, + }, + kmsArn, }; + egressStudyPolicy.addStudy(study); + const permissionPolicy = egressStudyPolicy.toPolicyDoc(); const iam = await this.getIAM(); const createPolicyResponse = await iam @@ -630,7 +607,7 @@ class DataEgressService extends Service { }), Path: '/', RoleName: mainAccountRoleName, - PermissionsBoundary: createPolicyResponse.Policy.Arn, + PermissionsBoundary: permissionBoundaryArn, }) .promise(); @@ -646,16 +623,30 @@ class DataEgressService extends Service { async deleteMainAccountEgressStoreRole(egressStoreId) { const iam = await this.getIAM(); - const policyArn = (await iam.getRole({ RoleName: this.getMainAccountEgressStoreRole(egressStoreId) }).promise()) - .Role.PermissionsBoundary.PermissionsBoundaryArn; - await iam - .detachRolePolicy({ + const listRolePoliciesResponse = await iam + .listAttachedRolePolicies({ RoleName: this.getMainAccountEgressStoreRole(egressStoreId), - PolicyArn: policyArn, }) .promise(); + const policyArns = listRolePoliciesResponse.AttachedPolicies.map(policy => { + return policy.PolicyArn; + }); + await Promise.all( + policyArns.map(arn => { + return iam + .detachRolePolicy({ + RoleName: this.getMainAccountEgressStoreRole(egressStoreId), + PolicyArn: arn, + }) + .promise(); + }), + ); await iam.deleteRole({ RoleName: this.getMainAccountEgressStoreRole(egressStoreId) }).promise(); - await iam.deletePolicy({ PolicyArn: policyArn }).promise(); + await Promise.all( + policyArns.map(arn => { + return iam.deletePolicy({ PolicyArn: arn }).promise(); + }), + ); } async lockAndUpdate(lockId, dbKey, dbObject) { diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js new file mode 100644 index 0000000000..60a96191b5 --- /dev/null +++ b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js @@ -0,0 +1,65 @@ +const _ = require('lodash'); +const { StudyPolicy } = require('../iam/study-policy'); + +describe('study-policy', () => { + it('generate the correct policy', () => { + const studyPolicy = new StudyPolicy(); + + const item = { + bucket: 'test-S3BucketName', + folder: ['test-folder'], + permission: { + read: true, + write: true, + }, + kmsArn: 'testKmsArn', + }; + + studyPolicy.addStudy(item); + expect(studyPolicy.toPolicyDoc()).toEqual({ + Statement: [ + { + Action: [ + 's3:GetObject', + 's3:GetObjectTagging', + 's3:GetObjectTorrent', + 's3:GetObjectVersion', + 's3:GetObjectVersionTagging', + 's3:GetObjectVersionTorrent', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + 's3:PutObjectTagging', + 's3:PutObjectVersionTagging', + 's3:DeleteObject', + 's3:DeleteObjectTagging', + 's3:DeleteObjectVersion', + 's3:DeleteObjectVersionTagging', + ], + Effect: 'Allow', + Resource: ['arn:aws:s3:::test-S3BucketName/test-folder*'], + Sid: 'S3StudyReadWriteAccess', + }, + { + Action: ['s3:ListBucket', 's3:ListBucketVersions'], + Condition: { + StringLike: { + 's3:prefix': ['test-folder*'], + }, + }, + Effect: 'Allow', + Resource: 'arn:aws:s3:::test-S3BucketName', + Sid: 'studyListS3Access1', + }, + { + Action: ['kms:Decrypt', 'kms:DescribeKey', 'kms:Encrypt', 'kms:GenerateDataKey', 'kms:ReEncrypt*'], + Effect: 'Allow', + Resource: ['testKmsArn'], + Sid: 'studyKMSAccess', + }, + ], + Version: '2012-10-17', + }); + }); +}); diff --git a/main/solution/backend/config/infra/cloudformation.yml b/main/solution/backend/config/infra/cloudformation.yml index 63345ca8ee..e43b9768c7 100644 --- a/main/solution/backend/config/infra/cloudformation.yml +++ b/main/solution/backend/config/infra/cloudformation.yml @@ -594,6 +594,7 @@ Resources: - iam:GetRole - iam:DeleteRole - iam:DetachRolePolicy + - iam:ListAttachedRolePolicies Resource: - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' - Effect: Allow @@ -620,6 +621,7 @@ Resources: PolicyWorkflowLoopRunner: Type: AWS::IAM::ManagedPolicy + DependsOn: PermissionBoundaryPolicyStudyBucket Properties: Description: Allows main account to create resources required for SWB backend functionality PolicyDocument: @@ -744,8 +746,8 @@ Resources: Resource: - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' Condition: - StringLike: - iam:PermissionsBoundary: !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/study-*' + StringEquals: + iam:PermissionsBoundary: !Ref PermissionBoundaryPolicyStudyBucket - Effect: Allow Action: - iam:AttachRolePolicy @@ -757,6 +759,50 @@ Resources: Resource: - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/study-*' + PermissionBoundaryPolicyStudyBucket: + Type: AWS::IAM::ManagedPolicy + Properties: + Description: Permission boundary for study bucket + ManagedPolicyName: '${self:custom.settings.permissionBoundaryPolicyStudyBucket}' + PolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Action: + - s3:GetObject + - s3:GetObjectTagging, + - s3:GetObjectTorrent, + - s3:GetObjectVersion, + - s3:GetObjectVersionTagging, + - s3:GetObjectVersionTorrent, + - s3:AbortMultipartUpload, + - s3:ListMultipartUploadParts, + - s3:PutObject, + - s3:PutObjectAcl, + - s3:PutObjectTagging, + - s3:PutObjectVersionTagging, + - s3:DeleteObject, + - s3:DeleteObjectTagging, + - s3:DeleteObjectVersion, + - s3:DeleteObjectVersionTagging + Resource: + - 'arn:aws:s3:::${self:custom.settings.egressStoreBucketName}/*' + - Effect: Allow + Action: + - s3:ListBucket + - s3:ListBucketVersions + Resource: + - 'arn:aws:s3:::${self:custom.settings.egressStoreBucketName}' + - Effect: Allow + Action: + - kms:Decrypt + - kms:DescribeKey + - kms:Encrypt + - kms:GenerateDataKey + - kms:ReEncrypt* + Resource: + - !Sub 'arn:aws:kms:${AWS::Region}:${AWS::AccountId}:alias/${self:custom.settings.egressStoreKmsKeyAlias}' + # IAM Role for the workflowLoopRunner Function RoleWorkflowLoopRunner: Type: 'AWS::IAM::Role' diff --git a/main/solution/backend/config/infra/functions.yml b/main/solution/backend/config/infra/functions.yml index 3d1d539720..1b0c9b5ef3 100644 --- a/main/solution/backend/config/infra/functions.yml +++ b/main/solution/backend/config/infra/functions.yml @@ -215,3 +215,4 @@ workflowLoopRunner: APP_EGRESS_STORE_KMS_KEY_ARN: !Sub arn:aws:kms:${AWS::Region}:${AWS::AccountId}:alias/${self:custom.settings.egressStoreKmsKeyAlias} APP_EGRESS_STORE_KMS_POLICY_WORKSPACE_SID: ${self:custom.settings.egressStoreKmsPolicyWorkspaceSid} APP_IS_APP_STREAM_ENABLED: ${self:custom.settings.isAppStreamEnabled} + APP_PERMISSION_BOUNDARY_POLICY_STUDY_BUCKET_ARN: !Sub arn:aws:iam::${AWS::AccountId}:policy/${self:custom.settings.permissionBoundaryPolicyStudyBucket} diff --git a/main/solution/backend/config/settings/.defaults.yml b/main/solution/backend/config/settings/.defaults.yml index 3035ec003f..46eaf47da7 100644 --- a/main/solution/backend/config/settings/.defaults.yml +++ b/main/solution/backend/config/settings/.defaults.yml @@ -281,6 +281,8 @@ dbTableStudyPermissions: ${self:custom.settings.dbPrefix}-DbStudyPermissions # DynamoDB table name for KeyPairs dbTableKeyPairs: ${self:custom.settings.dbPrefix}-DbKeyPairs +permissionBoundaryPolicyStudyBucket: ${self:custom.settings.namespace}-PermissionBoundaryPolicyStudyBucket + # ================================ Data Egress Feature Settings =========================================== # NOTE: Following properties are ONLY allowed to change for the initial deployment. It's NOT recommended to change the following properties if you have enabled data egress feature. diff --git a/main/solution/post-deployment/src/lambdas/egress-store-object-handler/__tests__/handler.test.js b/main/solution/post-deployment/src/lambdas/egress-store-object-handler/__tests__/handler.test.js index 8fcad7f071..9909659334 100644 --- a/main/solution/post-deployment/src/lambdas/egress-store-object-handler/__tests__/handler.test.js +++ b/main/solution/post-deployment/src/lambdas/egress-store-object-handler/__tests__/handler.test.js @@ -127,4 +127,44 @@ describe('handler', () => { // CHECK expect(s3Service.putObjectTag).toHaveBeenCalledTimes(0); }); + + it('should not enable store egress submission on empty folder', async () => { + s3Service.putObjectTag = jest.fn(); + dataEgressService.getEgressStoreInfo = jest.fn().mockResolvedValue({ isAbleToSubmitEgressRequest: false }); + dataEgressService.enableEgressStoreSubmission = jest.fn(); + const event = { + Records: [ + { + eventVersion: '2.1', + eventSource: 'aws:s3', + awsRegion: 'us-east-1', + eventName: 'ObjectCreated:Put', + userIdentity: { + principalId: 'AWS:test:test', + }, + s3: { + s3SchemaVersion: '1.0', + configurationId: 'test-configurationId', + bucket: { + name: 'test-bucketName', + ownerIdentity: { + principalId: 'test-principalId', + }, + arn: 'arn:aws:s3:::test-bucketName', + }, + object: { + key: encodeURIComponent('test-objectfolder/'), + size: 0, + }, + }, + }, + ], + }; + + // EXECUTE + await handlerWithContainer(container, event); + + // CHECK + expect(dataEgressService.enableEgressStoreSubmission).toHaveBeenCalledTimes(0); + }); }); From dbd42aef93ed00522e31719499ddae6213a4662f Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Thu, 16 Sep 2021 15:45:42 -0400 Subject: [PATCH 08/12] Remove unused lodash function --- .../lib/helpers/__tests__/study-policy.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js index 60a96191b5..66a4d4f65d 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/__tests__/study-policy.test.js @@ -1,4 +1,3 @@ -const _ = require('lodash'); const { StudyPolicy } = require('../iam/study-policy'); describe('study-policy', () => { From fa5012fbe8dea4f08b9c113e0aaefaae810ea7e7 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Thu, 16 Sep 2021 15:57:23 -0400 Subject: [PATCH 09/12] Update tests --- .../data-egress/__test__/data-egress-service.test.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js index 99b5a6fe7d..62b24fb4ed 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js @@ -558,14 +558,15 @@ describe('DataEgressService', () => { function mockDeleteEgressStoreRole(egressStoreId) { const policyArn = 'test-PermissionBoundaryArn'; - AWSMock.mock('IAM', 'getRole', (params, callback) => { + AWSMock.mock('IAM', 'listAttachedRolePolicies', (params, callback) => { expect(params.RoleName).toEqual(`study-${egressStoreId}`); callback(null, { - Role: { - PermissionsBoundary: { - PermissionsBoundaryArn: policyArn, + AttachedPolicies: [ + { + PolicyName: 'test-PermissionBoundaryName', + PolicyArn: policyArn, }, - }, + ], }); }); AWSMock.mock('IAM', 'detachRolePolicy', (params, callback) => { From 944836115add6e03321c69398c09df02ffb3897c Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Thu, 16 Sep 2021 22:57:34 -0400 Subject: [PATCH 10/12] Create role only for EnableEgressStore --- .../backend/config/infra/cloudformation.yml | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/main/solution/backend/config/infra/cloudformation.yml b/main/solution/backend/config/infra/cloudformation.yml index e43b9768c7..5f09bf0355 100644 --- a/main/solution/backend/config/infra/cloudformation.yml +++ b/main/solution/backend/config/infra/cloudformation.yml @@ -740,14 +740,17 @@ Resources: - appstream:UpdateImagePermissions Resource: - !Sub 'arn:aws:appstream:${AWS::Region}:${AWS::AccountId}:image/ServiceWorkbench*' - - Effect: Allow - Action: - - iam:CreateRole - Resource: - - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' - Condition: - StringEquals: - iam:PermissionsBoundary: !Ref PermissionBoundaryPolicyStudyBucket + - !If + - EnableEgressStore + - Effect: Allow + Action: + - iam:CreateRole + Condition: + StringEquals: + iam:PermissionsBoundary: !Ref PermissionBoundaryPolicyStudyBucket + Resource: + - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/study-*' + - !Ref 'AWS::NoValue' - Effect: Allow Action: - iam:AttachRolePolicy @@ -761,6 +764,7 @@ Resources: PermissionBoundaryPolicyStudyBucket: Type: AWS::IAM::ManagedPolicy + Condition: EnableEgressStore Properties: Description: Permission boundary for study bucket ManagedPolicyName: '${self:custom.settings.permissionBoundaryPolicyStudyBucket}' From bb485a9bae487621f8bd1a08452b2af5f8bcf88c Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Mon, 20 Sep 2021 14:48:10 -0400 Subject: [PATCH 11/12] Allow terminating Egress Store even when store is in Created state --- .../lib/data-egress/data-egress-service.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js index ba497bd478..2e1dc882b1 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/data-egress-service.js @@ -215,17 +215,13 @@ class DataEgressService extends Service { } const s3Service = await this.service('s3Service'); const egressStoreStatus = egressStoreInfo.status; - const isEgressStoreNotTouched = - egressStoreStatus.toUpperCase() === CREATED_STATUS_CODE && egressStoreInfo.isAbleToSubmitEgressRequest === false; if (egressStoreStatus.toUpperCase() === PROCESSING_STATUS_CODE) { throw this.boom.forbidden( - `Egress store: ${egressStoreInfo.id} is still in processing. The egress store is not terminated and the workspce can not be terminated before egress request is processed.`, + `Egress store: ${egressStoreInfo.id} is still in processing. The egress store is not terminated and the workspace can not be terminated before egress request is processed.`, true, ); - } else if (egressStoreStatus.toUpperCase() === PROCESSED_STATUS_CODE || isEgressStoreNotTouched) { - // ONLY terminate the egress store if it has been processed or the egress store is empty - + } else if ([PROCESSED_STATUS_CODE, CREATED_STATUS_CODE].includes(egressStoreStatus.toUpperCase())) { try { await s3Service.clearPath(egressStoreInfo.s3BucketName, egressStoreInfo.s3BucketPath); } catch (error) { From 8b4369894b017972a1d36379e82777c794faf610 Mon Sep 17 00:00:00 2001 From: Tim Nguyen Date: Mon, 20 Sep 2021 16:28:45 -0400 Subject: [PATCH 12/12] Add unit tests --- .../__test__/data-egress-service.test.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js index 62b24fb4ed..17a10b3270 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/data-egress/__test__/data-egress-service.test.js @@ -582,7 +582,16 @@ describe('DataEgressService', () => { callback(); }); } - it('should successfully delete egress store while egress store is not touched since created', async () => { + + it('should successfully delete egress store that is in CREATED state: deleteEgressStoreInCreatedStateTest = true', async () => { + await deleteEgressStoreInCreatedStateTest(true); + }); + + it('should successfully delete egress store that is in CREATED state: deleteEgressStoreInCreatedStateTest = false', async () => { + await deleteEgressStoreInCreatedStateTest(false); + }); + + async function deleteEgressStoreInCreatedStateTest(isAbleToSubmitEgressRequest) { dataEgressService.removeEgressStoreBucketPolicy = jest.fn(); const s3Policy = testS3PolicyFn(); dataEgressService._settings = { @@ -608,7 +617,7 @@ describe('DataEgressService', () => { s3BucketName: 'test-s3BucketName', s3BucketPath: 'test-s3BucketPath', id: egressStoreId, - isAbleToSubmitEgressRequest: false, + isAbleToSubmitEgressRequest, }, ]); const requestContext = {}; @@ -668,7 +677,7 @@ describe('DataEgressService', () => { }, 'test-accountId', ); - }); + } it('should remove bucket policy', async () => { dataEgressService.getS3BucketAndPolicy = jest.fn().mockResolvedValueOnce({