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

Make SigningInstructions public #2730

Merged
merged 6 commits into from
May 26, 2023
Merged

Conversation

cholcombe973
Copy link
Contributor

@cholcombe973 cholcombe973 commented May 25, 2023

Motivation and Context

I'm building an s3 server in rust and I need signature verification to work from the server side when receiving requests. To do that the server needs to generate a signature and then compare it to the one that the client has already sent along. I believe that the sign module in aws-sigv4 http_request contains the functions required to do that.

Description

Exported the SigningInstructions struct in the sign module of http_request for aws-sigv4.

Testing

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@cholcombe973 cholcombe973 requested a review from a team as a code owner May 25, 2023 23:21
@jdisanti
Copy link
Collaborator

jdisanti commented May 26, 2023

Why can't you just use the aws_sigv4::http_request::sign method? Example here: https://docs.rs/aws-sigv4/0.55.3/aws_sigv4/http_request/index.html

The SigningOutput gives you access to just the signature string if that's all you need: https://docs.rs/aws-sigv4/0.55.3/aws_sigv4/struct.SigningOutput.html#method.signature

@cholcombe973
Copy link
Contributor Author

cholcombe973 commented May 26, 2023

Well I wanted to return the signing instructions from the function and I can't complete the function signature because SigningInstructions is private. I should probably elaborate a little more. I have a custom Request Body that I don't think will inter opt properly with the aws libraries so I'm trying to do a little hack where I return the signing instructions and then apply those to my Request later. It's likely this wasn't the intended use for these functions haha.

@jdisanti
Copy link
Collaborator

I see. I think the correct fix would be to add SigningInstructions to the export here: https://github.com/awslabs/smithy-rs/blob/b023426d1cfd05e1fd9eef2f92a21cad58b93b86/aws/rust-runtime/aws-sigv4/src/http_request/mod.rs#L59

We probably just missed that during initial implementation since I guess it works if you only need to call a function on that returned value rather than name it.

@cholcombe973
Copy link
Contributor Author

Thanks! I'll do that in the morning

@cholcombe973
Copy link
Contributor Author

@jdisanti I pushed that update.

@jdisanti
Copy link
Collaborator

Looks good. Could you also update the PR description/title and add an entry to CHANGELOG.next.toml for [[aws-sdk-rust]]?

@cholcombe973 cholcombe973 changed the title Make sign module public Make SigningInstructions public May 26, 2023
@cholcombe973 cholcombe973 requested a review from a team as a code owner May 26, 2023 15:58
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.

Looks good. I'll apply a couple minor changes and get this merged. Thank you!

CHANGELOG.next.toml Outdated Show resolved Hide resolved
CHANGELOG.next.toml Outdated Show resolved Hide resolved
@jdisanti jdisanti enabled auto-merge May 26, 2023 16:03
auto-merge was automatically disabled May 26, 2023 16:04

Head branch was pushed to by a user without write access

@jdisanti jdisanti enabled auto-merge May 26, 2023 16:24
@cholcombe973
Copy link
Contributor Author

@jdisanti did you want me to update the docs for the SigningInstructions as well? It looks like that's what most of these test breakages are about.

@jdisanti
Copy link
Collaborator

That would be great!

auto-merge was automatically disabled May 26, 2023 18:12

Head branch was pushed to by a user without write access

@jdisanti jdisanti enabled auto-merge May 26, 2023 20:37
@jdisanti jdisanti added this pull request to the merge queue May 26, 2023
Merged via the queue into smithy-lang:main with commit 79ead48 May 26, 2023
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