Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Vault and IAM integration #208

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Vault and IAM integration #208

wants to merge 9 commits into from

Conversation

psibi
Copy link
Member

@psibi psibi commented Jul 12, 2019


name: Pull request template
about: Make a PR to terraform-aws-foundation

Please include the following in your PR:

Please also note that these are not hard requirements, but merely serve to define
what maintainers are looking for in PR's. Including these will more likely lead
to your PR being reviewed and accepted.

  • Update the changelog
  • Make sure that modules and files are documented. This can be done inside the module and files.
  • Make sure that new modules directories contain a basic README.md file.
  • Make sure that the module is added to tests/main.tf
  • Make sure that the linting passes on CI.
  • Make sure that there is an up to date example for your code:
    - For new modules this would entail example code for how to use the module or some explanation in the module readme.
    - For new examples please provide a README explaining how to run the example. It's also ideal to provide a basic makefile to use the example as well.
  • Make sure that there is a manual CI trigger that can test the deployment.

@psibi psibi closed this Jul 12, 2019
@psibi psibi deleted the vault-work branch July 12, 2019 09:06
modules/vault-iam/Makefile Outdated Show resolved Hide resolved
modules/vault-iam/main.tf Outdated Show resolved Hide resolved
modules/vault-iam/main.tf Outdated Show resolved Hide resolved
@psibi psibi reopened this Jul 18, 2019
@psibi psibi changed the title WIP: Vault and IAM integration Vault and IAM integration Jul 21, 2019
@ketzacoatl
Copy link
Contributor

ketzacoatl commented Jul 22, 2019

I'd like to make the following changes before merging:

  • make vault-iam a module
  • use a data source for the policy JSON
  • parametize the vtest in arn:aws:iam::${data.aws_caller_identity.current.account_id}:user/vtest-* with a variable that defaults to vault
  • update the vault-s3-private example to use the vault-iam module
  • drop the aws provider from the vault-aws-backend module
  • consider dropping the vault_aws_secret_backend resource and use a make target (this resource would include admin secrets in Terraform's state file)
  • fixup commits

@psibi
Copy link
Member Author

psibi commented Jul 25, 2019

@ketzacoatl On further thought, I think we should remove the entire vault-aws-backend module itself from terraform (because we don't want admin secret in Terraform's state file and just having vault_aws_secret_backend_role resource isn't useful).

Also, I don't think it is useful for us to make vault-s3-private use vault-iam module, because they are not dependent. Only the running vault server's secret engine needs the access keys from the vault-iam module. No other output from vault-iam is actually needed for it in vault-s3-private. I think the right way for us to proceed would be:

  • Have make target step for the vault-iam module. This step will create access keys which has to exposed via environment variables by us manully for the next step.
  • Have another make target step (like what you have suggested) to create AWS secret engine path and create an appropriate role for this. (This step will completely replace the vault-aws-backend module). This step will require proper environment variable for access keys to be present for it to work.

What do you think ?

@ketzacoatl
Copy link
Contributor

Yes, that sounds good.

I would comment though, that if there are no secrets, and we can do it in Terraform, we ought to. Some of the vault configuration can be done in Terraform and doesn't include secrets. Those should stay in Terraform. IDK if that is relevant to the vault-aws-backend module, I haven't reviewed the code in a few days.

@ketzacoatl
Copy link
Contributor

@psibi are these updates WIP or is it ok to have one of our engineers make those updates?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants