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: ensure request parameters are sent with generate_credentials #403

Merged
merged 4 commits into from Mar 18, 2019

Conversation

Projects
None yet
2 participants
@steved
Copy link
Contributor

commented Mar 12, 2019

The JSON body is not read on GET requests, we either need to POST or
pass the arguments as query parameters.

AWS: ensure request parameters are sent with generate_credentials
The JSON body is not read on GET requests, we either need to POST or
pass the arguments as query parameters.

@jeffwecan jeffwecan self-requested a review Mar 12, 2019

@jeffwecan jeffwecan added the aws label Mar 12, 2019

@jeffwecan jeffwecan added this to the 0.7.3 milestone Mar 12, 2019

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

The documentation doesn't explicitly call out POST as an accepted method: https://www.vaultproject.io/api/secret/aws/index.html#generate-credentials.

I haven't dug into much, but given the premise and the current upstream documentation, shouldn't we just change the json keyword argument (that get passed through to the requests module) be changed to params instead of changing the request method to POST (assuming the upstream documentation is up-to-date)?

@steved

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Yeah, unfortunately the docs aren't super consistent as the AWS secret engine documentation uses vault write: https://www.vaultproject.io/docs/secrets/aws/index.html#sts-assumerole.

Either way works for me though, so I'm happy to adhere to their API documentation.

@jeffwecan
Copy link
Collaborator

left a comment

Thanks! I had one comment on the test case update that I'll give you a chance to provide feedback on but I feel like this is a good change to merge in regardless. 👍

@@ -254,11 +254,16 @@ def test_read_role(self, label, configure_first=True, raises=None, exception_mes
d1=json.loads(read_role_response['data']['policy']),
d2=self.TEST_POLICY_DOCUMENT,
)
else:
elif 'credential_types' in read_role_response['data']:

This comment has been minimized.

Copy link
@jeffwecan

jeffwecan Mar 15, 2019

Collaborator

I don't have any issue with working around the credential_type / credential_types change in this secrets engine in this manner, but another option would be to go off the Vault version being tested against. E.g.:

key_name = 'credential_types' if vault_version_lt('1.0.0') else 'credential_type'
self.assertEqual(
    first=read_role_response['data'][key_name],
    second=['iam_user'],
)

I like that route because it makes things a bit more explicit. What do you think?

This comment has been minimized.

Copy link
@steved

steved Mar 18, 2019

Author Contributor

Sure! Changed and added a link to the corresponding vault commit.

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Also for what it is worth, I would be fine with the originally proposed change to a POST method or with the current changes (now that you've done my due diligence in verifying a POST should work just as well 😄).

@steved

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

GET works for me. I sort of understand the semantics of using a POST for this endpoint (creating credentials, ...), but for some reason it still makes more sense as a read.

@jeffwecan jeffwecan merged commit 150ad20 into hvac:develop Mar 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jeffwecan added a commit that referenced this pull request Apr 24, 2019

Merge pull request #430 from JadeHayes/#403-timeout-error
* Generate credentials updated to a post request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.