Skip to content

Commit

Permalink
Fix Sigv4 signing bug for endpoints with default ports (#4006)
Browse files Browse the repository at this point in the history
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->

Bug from awslabs/aws-sdk-rust#1244

## Description
<!--- Describe your changes in detail -->
Fix bug in Sigv4 signing that, when an endpoint contained a default port
(80 for HTTP, 443 for HTTPS), would sign the request with that port in
the `HOST` header even though Hyper excludes default ports from the
`HOST` header.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
Added new unit test to confirm that default ports are appropriately
stripped from HTTP and HTTPS requests.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
landonxjames authored Feb 10, 2025
1 parent ec42920 commit 771f717
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 4 deletions.
10 changes: 10 additions & 0 deletions .changelog/default-port-signing-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
applies_to: ["aws-sdk-rust"]
authors: ["landonxjames"]
references: ["aws-sdk-rust#1244"]
breaking: false
new_feature: false
bug_fix: true
---

Fix bug in Sigv4 signing that, when an endpoint contained a default port (80 for HTTP, 443 for HTTPS), would sign the request with that port in the `HOST` header even though Hyper excludes default ports from the `HOST` header.
2 changes: 1 addition & 1 deletion aws/rust-runtime/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion aws/rust-runtime/aws-sigv4/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "aws-sigv4"
version = "1.2.8"
version = "1.2.9"
authors = ["AWS Rust SDK Team <[email protected]>", "David Barsky <[email protected]>"]
description = "SigV4 signer for HTTP requests and Event Stream messages."
edition = "2021"
Expand Down
66 changes: 64 additions & 2 deletions aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::sign::v4::sha256_hex_string;
use crate::SignatureVersion;
use aws_smithy_http::query_writer::QueryWriter;
use http0::header::{AsHeaderName, HeaderName, HOST};
use http0::uri::{Port, Scheme};
use http0::{HeaderMap, HeaderValue, Uri};
use std::borrow::Cow;
use std::cmp::Ordering;
Expand Down Expand Up @@ -389,10 +390,28 @@ impl<'a> CanonicalRequest<'a> {
match canonical_headers.get(&HOST) {
Some(header) => header.clone(),
None => {
let port = uri.port();
let scheme = uri.scheme();
let authority = uri
.authority()
.expect("request uri authority must be set for signing");
let header = HeaderValue::try_from(authority.as_str())
.expect("request uri authority must be set for signing")
.as_str();
let host = uri
.host()
.expect("request uri host must be set for signing");

// Check if port is default (80 for HTTP, 443 for HTTPS) and if so exclude it from the
// Host header when signing since RFC 2616 indicates that the default port should not be
// sent in the Host header (and Hyper strips default ports if they are present)
// https://datatracker.ietf.org/doc/html/rfc2616#section-14.23
// https://github.com/awslabs/aws-sdk-rust/issues/1244
let header_value = if is_port_scheme_default(scheme, port) {
host
} else {
authority
};

let header = HeaderValue::try_from(header_value)
.expect("endpoint must contain valid header characters");
canonical_headers.insert(HOST, header.clone());
header
Expand Down Expand Up @@ -475,6 +494,15 @@ fn normalize_header_value(header_value: &str) -> Result<HeaderValue, CanonicalRe
HeaderValue::from_str(&trimmed_value).map_err(CanonicalRequestError::from)
}

#[inline]
fn is_port_scheme_default(scheme: Option<&Scheme>, port: Option<Port<&str>>) -> bool {
if let (Some(scheme), Some(port)) = (scheme, port) {
return [("http", "80"), ("https", "443")].contains(&(scheme.as_str(), port.as_str()));
}

false
}

#[derive(Debug, PartialEq, Default)]
pub(crate) struct SignedHeaders {
headers: Vec<CanonicalHeaderName>,
Expand Down Expand Up @@ -692,6 +720,40 @@ mod tests {
);
}

#[test]
fn test_host_header_properly_handles_ports() {
fn host_header_test_setup(endpoint: String) -> String {
let mut req = test::v4::test_request("get-vanilla");
req.uri = endpoint;
let req = SignableRequest::from(&req);
let settings = SigningSettings {
payload_checksum_kind: PayloadChecksumKind::XAmzSha256,
session_token_mode: SessionTokenMode::Exclude,
..Default::default()
};
let identity = Credentials::for_tests().into();
let signing_params = signing_params(&identity, settings);
let creq = CanonicalRequest::from(&req, &signing_params).unwrap();
creq.header_values_for("host")
}

// HTTP request with 80 port should not be signed with that port
let http_80_host_header = host_header_test_setup("http://localhost:80".into());
assert_eq!(http_80_host_header, "localhost",);

// HTTP request with non-80 port should be signed with that port
let http_1234_host_header = host_header_test_setup("http://localhost:1234".into());
assert_eq!(http_1234_host_header, "localhost:1234",);

// HTTPS request with 443 port should not be signed with that port
let https_443_host_header = host_header_test_setup("https://localhost:443".into());
assert_eq!(https_443_host_header, "localhost",);

// HTTPS request with non-443 port should be signed with that port
let https_1234_host_header = host_header_test_setup("https://localhost:1234".into());
assert_eq!(https_1234_host_header, "localhost:1234",);
}

#[test]
fn test_set_xamz_sha_256() {
let req = test::v4::test_request("get-vanilla-query-order-key-case");
Expand Down

0 comments on commit 771f717

Please sign in to comment.