Unverified Commit 771f7173 authored by Landon James's avatar Landon James Committed by GitHub
Browse files

Fix Sigv4 signing bug for endpoints with default ports (#4006) 

## 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 https://github.com/awslabs/aws-sdk-rust/issues/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._
parent ec429200

.changelog/default-port-signing-fix.md

0 → 100644
+10 −0
Original line number Diff line number Diff line 
---

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.

aws/rust-runtime/Cargo.lock

+1 −1
Original line number Diff line number Diff line
@@ -228,7 +228,7 @@ version = "0.60.3" 


[[package]]

name = "aws-sigv4"

version = "1.2.8"

version = "1.2.9"

dependencies = [

 "aws-credential-types",

 "aws-smithy-eventstream",

aws/rust-runtime/aws-sigv4/Cargo.toml

+1 −1
Original line number Diff line number Diff line 
[package]

name = "aws-sigv4"

version = "1.2.8"

version = "1.2.9"

authors = ["AWS Rust SDK Team <aws-sdk-rust@amazon.com>", "David Barsky <me@davidbarsky.com>"]

description = "SigV4 signer for HTTP requests and Event Stream messages."

edition = "2021"

aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs

+64 −2
Original line number Diff line number Diff line
@@ -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;
@@ -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
@@ -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>,
@@ -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");