-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DT-1395: Sharing staging S3 DAP bucket with Funding Service Find team #467
base: main
Are you sure you want to change the base?
Conversation
] | ||
resources = [ | ||
module.dap_export_bucket.bucket_arn, | ||
"${module.dap_export_bucket.bucket_arn}/latest/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth checking with Hugh if this needs to be restricted further to a specific folder/objects etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yes, I will discuss this with @hugh-emerson
I think here we would want to grant access to the latest folder as this will have info for form-data
and testing-center
dirs which would be required.
@@ -54,3 +54,8 @@ variable "dap_external_role_arns" { | |||
# "DSQSS" is DAP's staging/test server. Added here for MSD-54917, informed they exist in the same environment. | |||
default = ["arn:aws:iam::062321884391:role/DSQL1", "arn:aws:iam::062321884391:role/DSQSS"] | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth checking if this is required in production if the aim is to implement only for staging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to leave these fields empty as if the access is a success, then it would follow we'd need it in production.
I believe I need the empty variable as dap_export_s3.tf
references the variable and is used across each env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think for safety we can limit to staging for now, as this policy grants access from our Dev and Test accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is empty here default = []
and only terraform/staging/variables.tf has the specified user account identifiers.
type = list(string) | ||
description = "Funding service accounts that we wish to have access to staging DAP S3 bucket" | ||
default = ["4a20e1ecba266786127536b068cbbf222b344a2e21024029f1a778f98e8667c0", "5544757b63b565e6774e61121ba15cfa98206f1629455df924f60d942a861d56"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better practice to use 'AWS = "arn:aws:iam::OTHER_ACCOUNT_ID:root"'.
Alternatively to avoid hardcoding completely I believe you can set up a role with the correct permissions etc which can then be assumed by FIND to access the bucket, but that's a whole different approach to the way you've implemented it and I'm not sure how sensitive the stuff inside the bucket is to warrant extra levels of security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YD-MHCLG I'm open to whichever approach you'd prefer from your side. We've used the CanonicalUser approach successfully in the past for our own internal x-account needs.
Allowing Funding service Find team read only access to staging DAP export S3 bucket.