-
Notifications
You must be signed in to change notification settings - Fork 196
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 builder for signing HTTP request #3441
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SigningApplication is definitely an improvement, but still seems a little non-intuitive since it is a builder pattern that has a side effect.
What do you think about creating a trait, maybe SignWith
, that has a sign_with
method taking the identity/settings args, and then implementing that trait for http::Request
?
That's an interesting approach 💡 Let's discuss further. I'm curious. |
This commit responds to #3441 (review)
A new generated diff is ready to view.
A new doc preview is ready to view. |
should this target main instead? Is this related to S3 express? |
settings: SigningSettings, | ||
identity: &'a Identity, | ||
operation_config: &'a SigV4OperationSigningConfig, | ||
request_timestamp: SystemTime, | ||
) -> Result<v4::SigningParams<'a, SigningSettings>, SigV4SigningError> { | ||
) -> Result<v4::SigningParams<'a, SigningSettings>, BoxError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have caught this in earlier reviews, but I think we should instead make SigV4SigningError
an opaque/informative public error so that we can add methods to it in the future if we need to. See https://github.com/smithy-lang/smithy-rs/blob/main/design/src/rfcs/rfc0022_error_context_and_compatibility.md
It's pretty easy to do this. Just rename the existing error enum to "ErrorKind", and make a public struct with the previous name.
/// request.sign_with(&signing_package) | ||
/// # } | ||
/// ``` | ||
pub trait SignWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your reasoning for putting this in aws-runtime instead of aws-sigv4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that moving forward, we'll be putting items in aws-runtime-api
and aws-runtime
crates, but not in constellation crates, otherwise we can put it in aws-sigv4
instead.
EDIT:
A more practical reason was impl SignWith for HttpRequest
referring to SigV4MessageSigner
that is a private type in aws-runtime
. Putting SignWith
in aws-sigv4
and impl SignWith for HttpRequest
in aws-runtime
would violate the orphan rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl SignWith for HttpRequest
internally uses a module private aws_runtime::auth::apply_signing_instructions
, so we need to move it to aws-sigv4
.
It's just so happens that the S3 express feature branch was a convenient segue for this. But targeting main is actually better, since the S3 express feature branch doesn't need to hold onto these code changes. Will cherry-pick commits and create a different PR that targets main. |
This commit addresses #3441 (comment)
Replaced by #3455 |
This commit addresses #3441 (comment)
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 anIdentity
and aSigningParams
:This PR allows them to call
SignWith::sign_with
trait method withSigningPackage
instead:Testing
Relied on 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 my feature branch.Checklist
CHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.