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

aws secret backend - Validate role's arn #2302

Open
mtougeron opened this issue Jan 25, 2017 · 5 comments
Open

aws secret backend - Validate role's arn #2302

mtougeron opened this issue Jan 25, 2017 · 5 comments

Comments

@mtougeron
Copy link

When you create a new role for the aws secret backend, using the arn parameter, it doesn't validate that it is a valid format.

Example attaching a role instead of a policy

$> vault write aws/roles/testing arn=arn:aws:iam::REDACTED:role/vault-example
Success! Data written to: aws/roles/testing

$> vault read aws/creds/testing                                         
Error reading aws/creds/testing: Error making API request.

URL: GET https://REDACTED:8200/v1/aws/creds/testing
Code: 400. Errors:

* Error attaching user policy: InvalidInput: ARN arn:aws:iam::REDACTED:role/vault-example is not valid.
	status code: 400, request id: 8ca5979f-e32b-11e6-b129-2be51eb74486

Example of a totally invalid arn

$> vault write aws/roles/testing arn=asdf       
Success! Data written to: aws/roles/testing

$> vault read aws/creds/testing                 
Error reading aws/creds/testing: Error making API request.

URL: GET https://REDACTED:8200/v1/aws/creds/testing
Code: 400. Errors:

* Error putting user policy: MalformedPolicyDocument: Syntax errors in policy.
	status code: 400, request id: fd531255-e32a-11e6-a8f3-739bc605fce8

Example attaching a non-existing policy (though I could understand how this example might be a valid use-case so that someone could create the policy later)

$> aws iam get-policy --policy-arn arn:aws:iam::REDACTED:policy/asdf        

An error occurred (NoSuchEntity) when calling the GetPolicy operation: Policy arn:aws:iam::REDACTED:policy/asdf does not exist or is not attachable.

$> vault write aws/roles/testing arn=arn:aws:iam::REDACTED:policy/asdf
Success! Data written to: aws/roles/testing

$> vault read aws/creds/testing                                           
Error reading aws/creds/testing: Error making API request.

URL: GET https:/REDACTED:8200/v1/aws/creds/testing
Code: 400. Errors:

* Error attaching user policy: NoSuchEntity: Policy arn:aws:iam::REDACTED:policy/asdf does not exist or is not attachable.
	status code: 404, request id: bfe9069b-e32a-11e6-a8f3-739bc605fce8

But it should probably at least validate that it is a valid arn for a policy document.

@jefferai
Copy link
Member

If there is a way to do this validation locally via the Go SDK this seems like an easy fix to implement. I'd be more hesitant to add it in if it must round-trip to AWS, partially because then it imposes ordering on setting up roles vs. credentials.

@mtougeron
Copy link
Author

From my quick scan of the Go SDK it doesn't look there is a way but I'm not super familiar with it.

@jefferai
Copy link
Member

@joelthompson thoughts?

@joelthompson
Copy link
Contributor

@jefferai -- sorry, somehow missed this when you first commented.

I think it makes sense to do some basic syntactic checking as part of #4229 -- once the parameters are no longer overloaded, it will make it much easier to do the syntactic checking. Things like, "does it look like a valid ARN?" and "is the policy valid JSON?" I'm sort of split on whether it makes sense to round trip to AWS to validate the ARNs. On the one hand, as you say, it imposes ordering on setting up roles vs. credentials, and it also makes Vault now depend on AWS and so introduces a failure point (e.g., if there were a network issue, or eventual consistency slowness, or AWS API throttling). On the other hand, it gives users earlier feedback that a role won't work before it does. Maybe attempt to validate the ARNs but return a warning if the validation fails for some reason (but still let the role creation through)?

@jefferai
Copy link
Member

That could work.

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

No branches or pull requests

5 participants