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

multiple arn options for aws backend over vault client (cli) #2817

Closed
hflamboauto1 opened this issue Jun 6, 2017 · 22 comments
Closed

multiple arn options for aws backend over vault client (cli) #2817

hflamboauto1 opened this issue Jun 6, 2017 · 22 comments

Comments

@hflamboauto1
Copy link

hflamboauto1 commented Jun 6, 2017

This is related to AWS backend and how to apply policies to the aws/role/readonly
From docs example: 1 policy ARN

vault write aws/roles/readonly arn=arn:aws:iam::aws:policy/AmazonEC2ReadOnlyAccess

What if i want to attach another one ? e.g

arn:aws:iam::aws:policy/AmazonS3ReadOnlyAccess

Write multiple comma separated .. client does not complain

vault write aws/roles/readonly arn=arn:aws:iam::aws:policy/AmazonEC2ReadOnlyAccess,arn:aws:iam::aws:policy/AmazonS3ReadOnlyAccess

but when reading: vault read aws/creds/readonly

Error reading aws/creds/readonly: Error making API request.

URL: GET https://vaulthostname/v1/aws/creds/readonly
Code: 400. Errors:

* Error attaching user policy: InvalidInput: ARN arn:aws:iam::aws:policy/AmazonEC2ReadOnlyAccess,arn:aws:iam::aws:policy/AmazonS3ReadOnlyAccess is not valid.
	status code: 400, request id: e3bd4e5c-4ac0-11e7-93e2-a78948e63f86

This is basically a question on how to allow this without having to create a policy.json merging multiple policies in one file

@jefferai
Copy link
Member

jefferai commented Jun 6, 2017

You can't -- if you want to specify anything other than a single ARN you need to submit a policy document.

@jefferai jefferai closed this as completed Jun 6, 2017
@hflamboauto1
Copy link
Author

Thanks @jefferai thats was it 👍

@hflamboauto1
Copy link
Author

hflamboauto1 commented Jun 6, 2017

Hi @jefferai

just reading again docs referring probably to this (a bit confused) https://www.vaultproject.io/intro/getting-started/dynamic-secrets.html

it states:

The AWS backend requires an IAM policy to associate created credentials with. For this example, we'll write just one policy, but you can associate many policies with the backend. Save a file named policy.json with the following contents:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "Stmt1426528957000",
      "Effect": "Allow",
      "Action": [
        "ec2:*"
      ],
      "Resource": [
        "*"
      ]
    }
  ]
}

but you can associate many policies with the backend .. how can i achieve this ? is it referring to have multiple blocks as statements on policy.json ?

@jefferai
Copy link
Member

jefferai commented Jun 6, 2017

Hi @hflamboauto1 ,

You do it via having different roles, and assigning a policy per role.

@hflamboauto1
Copy link
Author

hflamboauto1 commented Jun 6, 2017

I see .. let me explain

My goal is to create Dynamic aws credentials via vault for all devs + some other users with AWS credentials and was thinking to achieve this having different policies applied granting more or less privileges.

As far i understand for that I need to create a role per user so that they can call (vault
read aws/roles/{user}) on their own and obtain the temporary credentials ..

How can one achieve something like "group of roles" to apply different policies per group of users with more and less previledges (merge of policies)

So i guess not possible .. and a unique policy needs to describe the policy per role/user.

Thanks for the hints

@jefferai
Copy link
Member

jefferai commented Jun 6, 2017

That's not possible right now, sorry.

@tcolgate
Copy link

tcolgate commented Sep 9, 2017

The problem with having separate roles is that I need to know what the user is try to do in advance, which is a pain. Multiple ARN support seems like it should be trivial. Would a PR be accepted?

@jefferai
Copy link
Member

jefferai commented Sep 9, 2017

@tcolgate See what @joelthompson thinks, he's the AWS expert around here :-)

@joelthompson
Copy link
Contributor

lol, thanks @jefferai :)

Spent some time over the weekend looking at the code. tl;dr: This specific change should be trivial.

Stepping back, I am a bit worried that this will further muddle the AWS secret backend. There are three different ways to get creds from the AWS secret backend:

  1. Via the creds/ endpoint, which creates IAM users
  2. Via the sts/ endpoint, which does an sts:AssumeRole
  3. Via the sts/ endpoint, which does an sts:GetFederationToken

The role configuration is overloaded, so that the same set of roles is used by all three of these, and the creds/ and sts/ endpoints just look at the role to see if it has the right type of data for each of these. In particular, the arn parameter is overloaded: When used with creds/ it is the ARN of a managed policy to attach to the IAM user being created, while when used with sts/ it is the ARN of the role to assume. It makes sense for the former to allow a (e.g.) comma-separated list of managed policies, but it doesn't make sense for the latter to specify multiple roles to assume.

The specific change @tcolgate is requesting can be done in isolation without having to deal with the overloading here, but I do worry that this overloading can make future changes more difficult. If there's interest from the HashiCorp side, I could probably take a stab at this type of refactoring to clean it up (but also don't want to introduce a refactor just for the sake of a refactor).

@jefferai
Copy link
Member

There's interest on our side ;-)

@joelthompson
Copy link
Contributor

Cool, will try to take a stab at it... after HashConf :)

@jefferai
Copy link
Member

OMG Joel. Waiting until after HashiConf? How lazy are you? :-D

@joelthompson
Copy link
Contributor

At least this lazy, if not more :)

@paddie
Copy link

paddie commented Mar 1, 2018

Hi guys, @joelthompson have you had time to look at this yet? We're currently a bit limited by this as well.

Our specific use-case is that we have a specific auto-generated policy per role that we create, but there's also an additional role that we apply to each role where we allow developers to create their additional role policies that is might be required (such as wanting to have access to a new service in AWS we've not yet added to the per-context policy).

Having the ability to create roles with multiple polices would help immensely!

Could we re-open this issue @jefferai ?

@joelthompson
Copy link
Contributor

Hey @paddie -- I haven't forgotten about this, but my free time unexpectedly took a steep hit last fall (and I'll add my standard disclaimer: I'm not a HashiCorp employee, just a community member who likes Vault, knows a bit about AWS, and wants to give back to the community). Thanks for the nudge :) Will hopefully get something in the next few weeks (e.g., I ran into issues getting the automated tests to work in my personal setup, so I spent some time refactoring those so that I could work on this -- see #4076).

I do have one question about the particular use case. Let's say you can specify multiple policies with a role. Should Vault's behavior be:

  1. Always attach all of the policies to the generated credentials
  2. Allow the user to specify which policies should be attached to the generated creds (ensuring that the requested policies are a subset of the allowed policies on the role)

I realize the second is a strict superset of the first, but I don't know if there's actual demand for that type of behavior and whether it's worth adding the complexity to the API and code. I've got a bunch of ideas (most probably not great) bouncing around inside my head about what to do about this, and I'd like some real-world use cases.

@tcolgate
Copy link

tcolgate commented Mar 6, 2018 via email

@paddie
Copy link

paddie commented Mar 6, 2018

Hi @joelthompson!

Thanks a ton for getting back to me on this one! I concur with @tcolgate that the attach-all approach would be the most intuitive and (probably) would also demand fewer changes to the API.

When creating the role in vault, either the arn could be a list of policy arns, or simply a role arn, where we then iterate through the attached policies of the role when creating a new one. What do you think?

I am rather good within the AWS domain, but very vague in the inner workings on vault just now, so if you need any help with at least the code that is needed to get the attached policies etc. let me know and I can see if I can look at it with you.

@joelthompson
Copy link
Contributor

Hey @paddie -- thanks for the offer, but I'm also quite familiar with AWS as well :)

I have a few concerns with passing in a role ARN and then copying the policies attached to it.

First, as of right now, the different input variables are overloaded between the different ways of getting credentials (which is the first thing I want to tackle). So, passing in a role ARN as you suggest would allow retrieving both IAM user credentials (with policies copied from the role) as well as assumed-role credentials for the role.

Second, is there a reason you want IAM user creds with the same policies as a role but don't want assumed-role credentials? It seems like you can already get pretty much everything you need.

Third, my personal opinion is that security tools like Vault should have as little magic and as much transparency as possible. Copying policies from a role is more magic than I would like. For example, users might confuse IAM user credentials with assumed-role credentials, and those can have subtle differences, like the fact that changes on a role should apply to already-existing credentials while policy changes to a role which Vault uses as a template to copy permissions from won't apply to existing IAM user creds that Vault has handed out, unless Vault is polling AWS to listen for changes to apply to already-provisioned IAM users. That's both more code to write and another way Vault could fail in a way that would have security implications. I think, ultimately, it would be better to not do this.

Anyway, these are just my personal opinions about what this should look like in Vault; as mentioned above, I'm not employed by HashiCorp nor do I represent HashiCorp's viewpoints. I also know that I could definitely be wrong about any/all of my opinions or just be missing something, so I'd love to hear it if you think that's the case! :)

@paddie
Copy link

paddie commented Mar 7, 2018

@joelthompson

I'll address these in a seemingly random way:

  1. Using assumed role credentials can only exist for up to 15 minutes if I am not mistaken. Our use-case requires us to have temporary credentials be a little longer lived - sometimes up to 7 days. But there might be something I am missing?

  2. I absolutely get your point here. At our company, developers have just gotten used to generating a new policy through Vault whenever they're in the early throws of developing a service that they're actively adding/editing policies for. We also have a webhook that listens to new updates to policies and creates a new Vault role automatically, meaning the delay will be almost instant. Additionally, this is the reason why we would like to use the role ARN instead such that we didn't actually have to have a dedicated service listening to these events and create anything in Vault. It would simply generate the new temporary role as a shadow of the existing role every time, including any updates that would've occurred in the meantime to the policies. This might be a bit much though and we are happy to keep our stuff running this would just be a perfect-world thing :)

  3. The problem is unclear the me, is it that one would be able to access the credentials of the underlying role which is a security concern to us? If that is the case, yes, let's not do that and rely on actively passing in a list of policies.

@joelthompson
Copy link
Contributor

Hey @paddie:

  1. It's actually 60 minutes; 15 minutes is the minimum, but 60 minutes is still well short of 7 days. Is there a reason clients can't just call Vault to get updated credentials when they need them (which is really more in line with Vault's philosophy)? The AWS SDKs that I've interacted with all have some notion of a credential provider that could be used to just retrieve credentials from Vault and put in the chain. Here's an example in python I put together: https://gist.github.com/joelthompson/5205b8c66f01134a3703529422c57e7c I just have an allergic reaction anytime I see IAM user credentials, and while Vault mitigates many of my concerns about IAM users, that opinion bleeds through in places like this :) One of the advantages of doing things this way is that your software would get credentials in essentially the same way whether it's running in dev or on an EC2 instance in an IAM instance profile.

  2. Seems like using assumed-role credentials would actually solve this use case for you :) If you throw in a credential provider, your devs might not even notice the difference.

  3. (Assuming you meant 1 and the displayed 4 is a Markdown formatting artifact) Yes, the concern is there's ambiguity around the same Vault role being able to retrieve both assumed-role creds on the role and creds for an IAM user whose policies are copied from that role, assuming that your users have access to both the creds/ and sts/ paths within the mount.

@paddie
Copy link

paddie commented Mar 12, 2018

Hi @joelthompson:

  1. Most of our applications are written in a way that expects IAM to be available in the the hierarchy of the default credentials (instance, environments, profile). Using default AWS services, we basically have no key-rotation, but we use vault to issue 7-day tokens for local development, or for when they need to run long-running scripts agains a database that could potentially last hours. But using local development using your script is definitely an interesting approach. The challenge would be to change all the existing code to use such an approach. We also don't want a runtime dependency on vault, so the change should only apply for development or locally executing code.
  2. Could you point me to an example somewhere regarding the API I would have to use here?
  3. Okay, I get that 👍

joelthompson added a commit to joelthompson/vault that referenced this issue Apr 15, 2018
The AWS secret engine had a lot of confusing overloading with role
paramemters and how they mapped to each of the three credential types
supported. This now adds parameters to remove the overloading while
maintaining backwards compatibility.

With the change, it also becomes easier to add other feature requests.
Attaching multiple managed policies to IAM users and adding a policy
document to STS AssumedRole credentials is now also supported.

Fixes hashicorp#4229
Fixes hashicorp#3751
Fixes hashicorp#2817
@joelthompson
Copy link
Contributor

Hey @paddie and @tcolgate -- I put together a PR in #4360 (based off an RFC in #4229) that I think should solve this use case. Would love your feedback on the approach and if it does what you need!

jefferai pushed a commit that referenced this issue Aug 16, 2018
* Make AWS credential types more explicit

The AWS secret engine had a lot of confusing overloading with role
paramemters and how they mapped to each of the three credential types
supported. This now adds parameters to remove the overloading while
maintaining backwards compatibility.

With the change, it also becomes easier to add other feature requests.
Attaching multiple managed policies to IAM users and adding a policy
document to STS AssumedRole credentials is now also supported.

Fixes #4229
Fixes #3751
Fixes #2817

* Add missing write action to STS endpoint

* Allow unsetting policy_document with empty string

This allows unsetting the policy_document by passing in an empty string.
Previously, it would fail because the empty string isn't a valid JSON
document.

* Respond to some PR feedback

* Refactor and simplify role reading/upgrading

This gets rid of the duplicated role upgrade code between both role
reading and role writing by handling the upgrade all in the role
reading.

* Eliminate duplicated AWS secret test code

The testAccStepReadUser and testAccStepReadSTS were virtually identical,
so they are consolidated into a single method with the path passed in.

* Switch to use AWS ARN parser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants