Skip to content

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

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

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

rastut
Copy link
Contributor

@rastut rastut 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
Copy link

codecov-io 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%> (ø) ⬆️

Copy link
Member

@jeffwecan jeffwecan left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure! 👍

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

@jeffwecan jeffwecan left a comment

Choose a reason for hiding this comment

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

Good stuff @rastut, thanks again!

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

Successfully merging this pull request may close these issues.

3 participants