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

Catch invalid token values and error out without value exposure #194

Conversation

NotTheEvilOne
Copy link
Contributor

This PR handles tokens that have been previously for example encoded in base64 for Kubernetes in an erroneous way containing newline characters.

Currently the error returned return the token value. This may be an unexpected value exposure not intended by an operator.

Example:
net/http: invalid header field value "Bearer broken\n" for key Authorization

Applying this change changes the error to not contain any sensitive data anymore:
Authorization token contains invalid characters

@LKaemmerling
Copy link
Member

Hey @NotTheEvilOne,

thank you for this request and sorry for the quite long response. Would it be possible that you add a test case as well? Also we would suggest mirroring the function httpguts.ValidHeaderFieldValue(token) into an internal function, because we would like to not include more dependencies. Would this be possible for you?

@NotTheEvilOne
Copy link
Contributor Author

NotTheEvilOne commented Jun 15, 2022

Hi,

Would it be possible that you add a test case as well?

I'll do that.

Also we would suggest mirroring the function httpguts.ValidHeaderFieldValue(token) into an internal function, because we would like to not include more dependencies. Would this be possible for you?

I'm not sure if that makes a difference. In fact net/http uses the same dependency already. The dependency and code use is already given therefore and would only result in code duplication. Please advice if you would like to integrate the function anyway.

@LKaemmerling
Copy link
Member

Hey @NotTheEvilOne,

thank you, you are right!

@LKaemmerling LKaemmerling merged commit d5b0ace into hetznercloud:main Jun 17, 2022
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.

2 participants