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

Send token as a param, to avoid token been logged by the audit log #250

Merged
merged 2 commits into from Aug 23, 2018

Conversation

Projects
None yet
3 participants
@rastut
Copy link
Contributor

commented Aug 22, 2018

Token should be sent as a parameter in a POST request to the renew endpoint. This will avoid token getting logged in an audit logs, it will also comply with current vault API policy (a warning is produced when token is sent as a URL parameter).

Using a token in the path is unsafe as the token can be logged in many places. Please use POST or PUT with the token passed in via the "token" parameter.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 22, 2018

Codecov Report

Merging #250 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #250   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files          12       12           
  Lines        1014     1014           
=======================================
  Hits          898      898           
  Misses        116      116
Impacted Files Coverage Δ
hvac/v1/__init__.py 85.56% <100%> (ø) ⬆️
@jeffwecan
Copy link
Collaborator

left a comment

Nice catch @rastut, thanks for the contribution! I have just one small query about the changes in the other comment on this review. 👍

@@ -946,11 +946,11 @@ def renew_token(self, token=None, increment=None, wrap_ttl=None):
"""
params = {
'increment': increment,
'token': token,

This comment has been minimized.

Copy link
@jeffwecan

jeffwecan Aug 22, 2018

Collaborator

Perhaps it makes more sense to move setting this token key in the params dict to under the following conditional? I.e., no need to send it in the params if we're doing the renew-self call:

def renew_token(self, token=None, increment=None, wrap_ttl=None):
    params = {
        'increment': increment,
    }

    if token is not None:
        params['token'] = token
        return self._adapter.post('/v1/auth/token/renew', json=params, wrap_ttl=wrap_ttl).json()
    else:
        return self._adapter.post('/v1/auth/token/renew-self', json=params, wrap_ttl=wrap_ttl).json()

This comment has been minimized.

Copy link
@rastut

rastut Aug 22, 2018

Author Contributor

Yes sure! 👍

@jeffwecan jeffwecan added this to the 0.6.4 milestone Aug 22, 2018

@jeffwecan
Copy link
Collaborator

left a comment

Good stuff @rastut, thanks again!

@jeffwecan jeffwecan merged commit b18faf9 into hvac:master Aug 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.