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 a trait SignWith for signing HTTP requests #3455

Closed
wants to merge 13 commits into from

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Mar 1, 2024

Motivation and Context

#3388 (comment)

Description

This PR attempts to provide more ergonomic API for request signing. As documented in aws_sigv4, customers need to follow these steps after creating an Identity and a SigningParams:

// Convert the HTTP request into a signable request
let signable_request = SignableRequest::new(
    "GET",
    "https://some-endpoint.some-region.amazonaws.com",
    std::iter::empty(),
    SignableBody::Bytes(&[])
).expect("signable request");

let mut my_req = http::Request::new("...");
// Sign and then apply the signature to the request
let (signing_instructions, _signature) = sign(signable_request, &signing_params)?.into_parts();
signing_instructions.apply_to_request_http1x(&mut my_req);

This PR allows them to call SignWith::sign_with trait method with SigningPackage instead:

let signing_package = SigningPackage::builder()
    .signing_params(&signing_params)
    .build()
    .expect("required fields have been set");
my_req.sign_with(&signing_package);

Testing

Relied on existing tests in CI

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

Copy link

github-actions bot commented Mar 1, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as ready for review March 1, 2024 03:32
@ysaito1001 ysaito1001 requested review from a team as code owners March 1, 2024 03:32
Copy link

github-actions bot commented Mar 1, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

github-actions bot commented Mar 1, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.


// If this is an event stream operation, set up the event stream signer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh removal was by mistake. Will restore it.

fn sign_with(
&mut self,
package: &SigningPackage<'_>,
) -> Result<SigningOutput<SigningInstructions>, BoxError>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs its own error type.

@rcoh
Copy link
Collaborator

rcoh commented Mar 1, 2024

Overall I think that a better API for signing is truly needed and this is directionally good. However, I think we need to work backwards from customer use cases. For example: We should work backwards from a customer who wants to sign an API Gateway request (and write the example!) and handle this that way.

If this code is needed urgently for S3 Express, we could consider putting it in inlineable to iterate on it.

I'm not totally sure about the trait API either. On the surface, it seems like it allows for an implementation on foreign types but I don't think it does in practice due to orphan trait rules. At the very least we should compare with other API designs.

@ysaito1001
Copy link
Contributor Author

As Russell commented, we need more iterations at a design level, and this PR will most likely be irrelevant when that design is settled. So closing this.

For S3 Express, there is a PR created to inline SigV4Signer::sign_http_request into S3ExpressSigner::sign_http_request.

@ysaito1001 ysaito1001 closed this Mar 1, 2024
ysaito1001 added a commit that referenced this pull request Mar 6, 2024
…ugin` (#3457)

## Motivation and Context
In response to
#3455 (comment),
it's clear that we need to iterate more on the potential signing API.

This PR takes a different approach where `S3ExpressSigner` is dropped
and a session token name override, if any, is placed into and retrieved
from `ConfigBag`, which is used within `SigV4Signer::sign_http_request`.

## Testing
Existing tests in CI

A semver failure in CI can be ignored; A public API
SigV4Signer::sign_http_request has been removed that only existed in the
branch.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
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.

5 participants