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

AWS IAM Policy for Vault AWS Auth method #71

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

Conversation

boldandbusted
Copy link

Howdy. My second PR for this excellent module. I hope it is useful. Feedback and kind critique is welcome. :)

(I tried to 'git rebase HEAD~3' to squash my commits before the PR, and the commits are no longer listed on the branch in my repo... yet they mysteriously are listed below this PR. Sorry about the noise.)

Cheers!

…ault into Add_reccomended_Vault_IAM_Policy_for_AWS_Auth_method
…switch over to s3-backend later.) Add variable reference.
Copy link
Collaborator

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

A couple questions:

  1. Is it worth updating the run-vault script with a flag to make it easy to enable the AWS auth backend?
  2. Could you paste the output of running the automated tests? We can't run the CI build automatically for external PRs for security reasons.

@@ -139,6 +140,10 @@ data "aws_vpc" "default" {

data "aws_subnet_ids" "default" {
vpc_id = "${data.aws_vpc.default.id}"

tags {
SubnetType = "private"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be configurable by variable and probably disabled by default. I know this is vault cluster private, but the default settings run it in the Default VPC, which for most users will have no private subnets. We could add comments to mention that for prod usage, this should be a custom VPC and private subnets.

@@ -213,3 +214,27 @@ data "aws_iam_policy_document" "vault_s3" {
]
}
}

resource "aws_iam_role_policy" "vault_aws_EC2_IAM_Auth" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use all lower case for resource and variable names.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with latest commit, but I leave it to you to 'resolve', if you're satisfied. :)

@@ -213,3 +214,27 @@ data "aws_iam_policy_document" "vault_s3" {
]
}
}

resource "aws_iam_role_policy" "vault_aws_EC2_IAM_Auth" {
count = "${var.enable_EC2_IAM_Auth ? 1 : 0}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If enable_EC2_IAM_Auth is a boolean, the ? 1 : 0 bit is redundant, as a true is treated as a 1 and a false as a 0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brikis98 Done with latest commit, but I leave it to you to 'resolve', if you're satisfied. :) (TM)

"ec2:DescribeInstances",
"iam:GetInstanceProfile",
"iam:GetUser",
"iam:GetRole",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a link to the docs where the requirements for these policies are defined?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am setting up something similar myself. The docs for this policy are located at: https://www.vaultproject.io/docs/auth/aws.html#recommended-vault-iam-policy

However, it would be nice to have an option for the sts:AssumeRole action in a cross account scenario.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brikis98 Done with latest commit but I leave it to you to 'resolve', if you're satisfied. :) @BMonsalvatge That's exactly where I got them. Thanks for the link. :) Re: cross-account 'sts:AssumeRole': I know the original policies have the sts:AssumeRole stanza, but I'm not sure how to properly enumerate the roles and accounts that's 'generic' for all cases. My present company doesn't really need it - we have sts:AssumeRole added to the IAM Instance Profile at line 175 in this file, and that was enough. However, I added a "TODO", seen in the recent commit. If you have this sussed out, I'd love to see it in the code, too. :)

@@ -185,3 +185,8 @@ variable "force_destroy_s3_bucket" {
description = "If 'configure_s3_backend' is enabled and you set this to true, when you run terraform destroy, this tells Terraform to delete all the objects in the S3 bucket used for backend storage. You should NOT set this to true in production or you risk losing all your data! This property is only here so automated tests of this module can clean up after themselves. Only used if 'enable_s3_backend' is set to true."
default = false
}

variable "enable_EC2_IAM_Auth" {
description = "Configure IAM Instance Profile on Vault cluster members to permit the user to enable AWS Auth backend. Note that this does NOT actually enable the backend, but merely sets policies that will permit it to function as expected."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is a little confusing. Perhaps: "If set to true, create the IAM policies required by the AWS Auth backend. Note that this does NOT..."

@@ -185,3 +185,8 @@ variable "force_destroy_s3_bucket" {
description = "If 'configure_s3_backend' is enabled and you set this to true, when you run terraform destroy, this tells Terraform to delete all the objects in the S3 bucket used for backend storage. You should NOT set this to true in production or you risk losing all your data! This property is only here so automated tests of this module can clean up after themselves. Only used if 'enable_s3_backend' is set to true."
default = false
}

variable "enable_EC2_IAM_Auth" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps create_aws_auth_backend_iam_policies would be a clearer name?

@boldandbusted
Copy link
Author

Hi @brikis98. Time got away from me... An update here. Finally got around to testing this, and see that I'm getting some terratest errors that I'll need to hunt down.

TestVaultClusterPublicWithUbuntuAmi 2018-07-18T18:12:01-04:00 command.go:98: Error: resource 'data.aws_vpc.default' config: unknown variable referenced: 'vpc_id'; define it with a 'variable' block

I'll dig into that. I'm thinking this might be because I had to add code to deal with my present company not having default VPCs defined. But not sure until I look.

@brikis98
Copy link
Collaborator

Ah, yes, all the examples assume the default VPC to keep them simple. Turning on debug logging might make the underlying error easier to track down: https://www.terraform.io/docs/internals/debugging.html.

@brikis98
Copy link
Collaborator

brikis98 commented Oct 1, 2018

@Etiene How does this overlap with the work you're doing?

@boldandbusted
Copy link
Author

I'm having trouble reliably running terratest - it's not the terratest code that is giving me grief, but GOPATH troubles. I was able to run it a few months ago, but now I cd to ./test, 'dep ensure', and I get:

$ dep ensure
root project import: /home/jesse/gits/terraform-aws-vault/test is not within any GOPATH/src

That said, I'm sure I still have to figure out how to properly handle the case of not having a default VPC in the test themselves.

@boldandbusted
Copy link
Author

boldandbusted commented Mar 18, 2019

FYI, I just checked back on the status of this, and hadn't seen a notification regarding a CLA requirement. I hope that was not what was holding this up. :/ I just signed the CLA.

@brikis98
Copy link
Collaborator

Oh, I think at the last comment, you were mentioning issues getting the tests to pass. Did you get that resolved? Also, there's now a merge conflict. Also, @Etiene had been working on something similar, so please check the latest to see what the overlap is.

@boldandbusted
Copy link
Author

Oh, I think at the last comment, you were mentioning issues getting the tests to pass. Did you get that resolved? Also, there's now a merge conflict. Also, @Etiene had been working on something similar, so please check the latest to see what the overlap is.

@brikis98 Ah, sorry, I was wrong. I hadn't confirmed that this code works properly with a default VPC, and what happens with 'null' or empty string variables. But, I've signed the CLA, so that's out of the way.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

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.

4 participants