-
Notifications
You must be signed in to change notification settings - Fork 181
RUST-1529 Use AWS SDK for sigv4 signing #1438
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
base: main
Are you sure you want to change the base?
RUST-1529 Use AWS SDK for sigv4 signing #1438
Conversation
…redential_types::Credential
…ngo-rust-driver into RUST-1529-no-ff
@@ -117,27 +126,34 @@ async fn authenticate_stream_inner( | |||
let creds = get_aws_credentials(credential).await.map_err(|e| { | |||
Error::authentication_error(MECH_NAME, &format!("failed to get creds: {e}")) | |||
})?; | |||
let aws_credential = AwsCredential::from_sdk_creds(creds); |
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.
In a previous PR, we introduced from_sdk_creds(...)
to convert from aws_credential_types::Credentials
to locally defined AwsCredential
type. Now that we've converted the rest of the authentication to use the AWS SDK, we no longer need this conversion
@@ -223,7 +223,7 @@ buildvariants: | |||
|
|||
- name: aws-auth | |||
display_name: "AWS Authentication" | |||
patchable: false | |||
# patchable: false |
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.
will uncomment once PR is approved
src/client/auth/aws.rs
Outdated
request.headers().iter().map(|(k, v)| { | ||
( | ||
k.as_str(), | ||
std::str::from_utf8(v.as_bytes()).expect("Header value should be valid UTF-8"), |
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.
This will panic if the conversion fails; instead, we should propagate an authentication error
let (signing_instructions, _signature) = sign(signable_request, &signing_params) | ||
.map_err(|e| Error::authentication_error(MECH_NAME, &format!("Signing failed: {e}")))? | ||
.into_parts(); |
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.
nit: since we're not using the returned signature, I think this can call output
rather than into_parts
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.
Thanks Isabel! I tried switching to .output()
, but ran into a borrow checker error when calling apply_to_request_http1x
on line 291. I double-checked the method signature and saw that it needs an owned value which we get with .into_parts()
, whereas .output()
returns a reference.
The .into_parts()
also matches what I saw in the two references we looked at (1, 2). Let me know if you had a different approach in mind!
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.
ah I missed the reference vs. owned value distinction, feel free to leave as-is
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.
sounds good, thanks!
let (signing_instructions, _signature) = sign(signable_request, &signing_params) | ||
.map_err(|e| Error::authentication_error(MECH_NAME, &format!("Signing failed: {e}")))? | ||
.into_parts(); |
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.
ah I missed the reference vs. owned value distinction, feel free to leave as-is
src/client/auth/aws.rs
Outdated
})?; | ||
Ok((k.as_str(), value)) | ||
}) | ||
.filter_map(Result::ok), |
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.
This will ignore any failures that occur when converting the strings and proceed with authentication. This would likely cause the signature to be computed incorrectly, and the reported error might be confusing for users to parse. Instead, I suggest doing the following to propagate an error here:
let headers: Result<Vec<_>> = request
.headers()
.iter()
.map(|(k, v)| {
let v = v.to_str().map_err(|_| {
Error::authentication_error(
MECH_NAME,
"Failed to convert header value to valid UTF-8",
)
})?;
Ok((k.as_str(), v))
})
.collect();
This uses a handy iterator trick: if an iterator is mapped into Result
values, you can collect
it into a Result<Vec<_>>
(or a Result
of any collection type). That result will either contain Ok(Vec<_>)
if all of the values were Ok
, or an Err(_)
containing the first error that was encountered.
We then need to return an error if needed using ?
and turn the value back into an iterator, which is the type accepted by SignableRequest::new
:
headers?.into_iter()
This should make what happened more clear if something goes wrong with the headers.
The rustdoc error is unrelated, you can merge with main to pull in a fix for it |
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.
LGTM! @abr-egn this is ready for your review
Replace existing implementation for getting sigv4 signing using the AWS SDK.
Testing
Clone https://github.com/isabelatkinson/drivers-evergreen-tools/tree/local-aws and run the following commands:
Previously
Work to be done for RUST-1529