Skip to content
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

Added AWS Backup Restore policy in IAM Role #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dheeraj251
Copy link

Corresponding Issue(s):
  • PRs should have a corresponding issue. If no issue exists, provide details in the Reason for Change(s) section
Summary of change(s):

Added AWS Backup Restore IAM policy in AWS Backup Role

Reason for Change(s):
  • The current module creates with AWS Backup IAM Role with 'AWSBackupServiceRolePolicyForBackup' IAM policy. This helps AWS Backup to take the backup of EBS/RDS/EC2 Instance. However in case customer would require to restore the backup then it's not possible because the AWS Backup IAM role would not have 'AWSBackupServiceRolePolicyForRestores' policy. Hence, in this PR I have made the changes so that this module by default would also attach AWS Backup restore policy in AWS Backup IAM role
Will the change trigger resource destruction or replacement? If yes, please provide justification:
Does this update/change involve issues with other external modules? If so, please describe the scenario.
If input variables or output variables have changed or has been added, have you updated the README?
Do examples need to be updated based on changes?
Note to the PR requester about Closing PR's

Please message the person that opened the issue when auto closing it on slack, as well as any other stake holders of deep interest. Only close the issue if you believe that the issue is fully resolved with this PR.

This PR may auto close the issue associated with it. If you feel the issue is not resolved please reopen the issue.

@johnctitus
Copy link
Contributor

Hey @dheeraj251,

First off, thank you for the contribution. As we discussed offline, it looks like the readme doc is out of sync. I think once it gets regenerated with Terraform-docs 0.11 we should be good.

@twistedgrim
Copy link
Contributor

Hello @dheeraj251 I wanted to add some extra color to this request as well.
So we certainly value security here as a very high priority. With this having a restore role out in the wild for some customers is a big no go. Several Solutions Engineers as well as Cloud Engineers brought up that this was a bad thing and hence the current version doesn't have that role in there at all. It was envisioned that the restore role would be added manually or a restore would be done with a user / fed user with admin power to be able to build the resources from the backups as needed.

We can discuss other options as well so please open a direct message with me and I would be happy to work with you to get whats needed.

I will open a card to add a note to the readme about the above and our security minded focus on the current IAM permissions.

@twistedgrim
Copy link
Contributor

As far as the PR, im going to leave it open until you are able to respond or DM me. Thanks for submitting!

@dheeraj251
Copy link
Author

Sure James let's connect on Teams

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants