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

Trim spaces in header values for singature calculation #875

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

krishnasrinivas
Copy link
Contributor

No description provided.

@kannappanr
Copy link
Contributor

@krishnasrinivas Probably functional test needs to be changed

Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

@kannappanr kannappanr merged commit 83aed6d into minio:master Nov 25, 2020
@ebozduman
Copy link
Contributor

ebozduman commented Nov 25, 2020

@kannappan, @krishnasrinivas

I am not sure trimming spaces for SigV4 is the right way to go as the fix is supposed to url encode and replace the space with "%20" in the header.
Besides, this fix only addresses the space character. Other special characters should also be taken care of by the correct Encoder package.
Otherwise, all other characters needs to be taken care of separately one by one the same way like in this PR.

I am testing right now to see how we behave for other special characters.

@harshavardhana
Copy link
Member

I am not sure trimming spaces for SigV4 is the right way to go as the fix is supposed to url encode and replace the space with "%20" in the header.

HTTP headers are not URL encoded @ebozduman look at the minio-go implementation on how to correctly handle this. Only query params and resource path get URL encoded.

HTTP headers support UTF-8 and do not need any special encoding over the wire as they are sent as-is.

@ebozduman
Copy link
Contributor

ebozduman commented Nov 25, 2020

Thank you @harshavardhana
I also realized the trimming is done on excessive spaces, that is more than one space is replaced by a single space.

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.

4 participants