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

Add excluded headers list in the setting option. for verification … #1381

Merged
merged 5 commits into from
May 16, 2022

Conversation

alonlud
Copy link
Contributor

@alonlud alonlud commented May 10, 2022

…signed request

Motivation and Context

I would like to verify an HTTP request which already been signed with the sigv4, Authorization header.
The header contains:
"SignedHeaders=amz-sdk-invocation-id;amz-sdk-request;amz-sdk-retry;content-length;content-type;host;user-agent;x-amz-date;x-amz-target, ..."

To correctly sign the request and verify the signature, I will also need to sign the User-Agent header.

Description

Add bool flag in the setting option allow_signing_user_agent_header.
The flag is by default false. But can be set to the true outside of the sigv4 crate.
This allows verifying a signed request correctly as the sigv4 is able now to sign also the User-Agent header.

@alonlud alonlud requested a review from a team as a code owner May 10, 2022 12:33
@rcoh
Copy link
Collaborator

rcoh commented May 10, 2022

Can you tell me more about your use case? Usually the user agent header is unsigned because it can be altered by proxies

edit: I see now—you are on the verification side

@rcoh
Copy link
Collaborator

rcoh commented May 10, 2022

I think this is a useful thing to support but I think we may need a different approach—I suspect we'll want to either accept an explicit list of headers to sign or a function. I think the User-Agent will not be the only relevant header. I'm open to design suggestions but otherwise, someone from the Rust SDK team can dig into options

@alonlud
Copy link
Contributor Author

alonlud commented May 10, 2022

I agree with you, high chance that it would be more useful to change it to some kind of vector of HeaderNames which should not be part of the signing. something like this:

pub headers_not_to_sign: Option<Vec<HeaderName>>

The default can be: Some([User-Agent])

I can modify the PR to implement this.
Please let me know if this is a valid suggestion

@jdisanti
Copy link
Collaborator

That sounds reasonable to me.

@alonlud alonlud changed the title Add allow_signing_user_agent_header setting option. for verification … Add excluded headers list in the setting option. for verification … May 12, 2022
@alonlud
Copy link
Contributor Author

alonlud commented May 15, 2022

I have updated the PR.
Kindly, let me know what you think.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@jdisanti jdisanti merged commit 5620922 into smithy-lang:main May 16, 2022
ysaito1001 pushed a commit that referenced this pull request Oct 21, 2022
This commit adds a test for the functionality added in #1381. Technically,
there was an existing test `presigning_header_exclusion` that exercised
the said functionality but it only covered the behavior partially, i.e.
only the case for a default constructed `SigningSettings` where its field
`excluded_headers` just contained `user-agent`.

The new test will randomly add headers to `excluded_headers` and verify
the functionality works as expected.
ysaito1001 pushed a commit that referenced this pull request Oct 21, 2022
This commit adds a test for the functionality added in #1381. Technically,
there was an existing test `presigning_header_exclusion` that exercised
the said functionality but it only covered the behavior partially, i.e.
only a case where the `excluded_headers` field in `SigningSettings`
contained just `user-agent`.

The new test will randomly add headers to `excluded_headers` and verify
the functionality works as expected.
ysaito1001 added a commit that referenced this pull request Oct 21, 2022
* Add test for excluded headers list

This commit adds a test for the functionality added in #1381. Technically,
there was an existing test `presigning_header_exclusion` that exercised
the said functionality but it only covered the behavior partially, i.e.
only a case where the `excluded_headers` field in `SigningSettings`
contained just `user-agent`.

The new test will randomly add headers to `excluded_headers` and verify
the functionality works as expected.

* Update CHANGELOG.next.toml

* Update CHANGELOG.next.toml

Co-authored-by: John DiSanti <[email protected]>

Co-authored-by: Saito <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
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