Unverified Commit 5dd5f3f8 authored by ysaito1001's avatar ysaito1001 Committed by GitHub
Browse files

Normalize URI paths during signing (except for S3) (#2018) 

* De-dupe multiple leading forward slashes in request

This commit fixes a bug reported in
https://github.com/awslabs/aws-sdk-rust/issues/661.
Note that this de-duping is not part of URI path normalization because it
must happen to a request for any service, including S3.

* Normalize URI paths for canonical requests

This commit enables URI path normalization in aws-sigv4. It follows
https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
to create the canonical URI that is part of the canonical request.

* Avoid using OS specific path separator

This commit updates the implementation of normalize_path_segment so that it
does not use an OS specific path separator, specifically "\\" on Windows and
always uses a forward slash "/". It addresses a test failure on Windows:
https://github.com/awslabs/smithy-rs/actions/runs/3518627877/jobs/5897764889.

* Fix the rustdoc::bare_urls warning from cargo doc

* Fix a typo Normalzie -> Normalize

* Limit de-duping multiple leading forward slashes to S3

This commit addresses https://github.com/awslabs/smithy-rs/pull/2018#issuecomment-1325283254.
There is a case  where an empty string between leading double forward
slashes is meaningful so blindly de-duping them breaks the use case.
Therefore, handling multiple leading forward slashes in this manner
should be applied only to S3.

* Add an integration test per https://github.com/awslabs/aws-sdk-rust/issues/661



This commit adds an integration test verifying the special rule for URI
path normalization applies to S3. That is, leading forward slashes should
be de-deduped and normalizing URI paths as prescribed in RFC 3986 should
be ignored.

* Update CHANGELOG.next.toml

* Revert 87b749c

This commit reverts 87b749c, the reason being that we need to better
understand the requirement for de-duplicating leading forward slashes
in requests, i.e. do we even need to do it or if so where should we
perform the de-dupe operation?

* Revert naming of UriPathNormalizationMode variants

This commit reverts the naming of variants in UriPathNormalizationMode.
They were PerRfc3986 and ForS3, but they should be more generic like
Enabled and Disabled as we had before.

* Update CHANGELOG.next.toml

* Simplify integration test using capture_request

This commit simplifies the integration test normalize-uri-path for s3.
Previously, the test used ReplayingConnection but it was an overkill
to test something as simple as whether the generated request contained
the expected URI and signature.

* Update aws/rust-runtime/aws-sigv4/src/http_request/settings.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* Update aws/rust-runtime/aws-sigv4/src/http_request/settings.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* Update aws/rust-runtime/aws-sigv4/src/http_request/uri_path_normalization.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* Address review feedback on normalize_path_segment

This commit addresses the following code review feedback:
https://github.com/awslabs/smithy-rs/pull/2018#discussion_r1034192229
https://github.com/awslabs/smithy-rs/pull/2018#discussion_r1034194621
https://github.com/awslabs/smithy-rs/pull/2018#discussion_r1034195625

* Update CHANGELOG.next.toml

This commit addresses https://github.com/awslabs/smithy-rs/pull/2018#discussion_r1034198861

* Preallocate the Vec capacity for normalize_path_segment

This commit addresses https://github.com/awslabs/smithy-rs/pull/2018#discussion_r1034195625.
It counts the number of slashes to preallocate the capacity of a Vec used
in normalize_path_segment.

* Address https://github.com/awslabs/smithy-rs/pull/2018\#discussion_r1034188849



* Update aws/rust-runtime/aws-sigv4/src/http_request/settings.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* Update aws/rust-runtime/aws-sigv4/src/http_request/settings.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* Update aws/rust-runtime/aws-sigv4/src/http_request/settings.rs

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

Co-authored-by: default avatarYuki Saito <awsaito@amazon.com>
Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>
parent 17cb98c9

CHANGELOG.next.toml

+6 −0
Original line number Diff line number Diff line
@@ -607,3 +607,9 @@ references = [ 
]

meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }

author = "hlbarber"



[[aws-sdk-rust]]

message = "Normalize URI paths per RFC3986 when constructing canonical requests, except for S3."

references = ["smithy-rs#2018"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "ysaito1001"

aws/rust-runtime/aws-sig-auth/src/signer.rs

+9 −2
Original line number Diff line number Diff line
@@ -6,7 +6,7 @@ 
use crate::middleware::Signature;

use aws_sigv4::http_request::{

    sign, PayloadChecksumKind, PercentEncodingMode, SignableRequest, SignatureLocation,

    SigningParams, SigningSettings,

    SigningParams, SigningSettings, UriPathNormalizationMode,

};

use aws_smithy_http::body::SdkBody;

use aws_types::region::SigningRegion;
@@ -62,6 +62,7 @@ impl OperationSigningConfig { 
            signing_options: SigningOptions {

                double_uri_encode: true,

                content_sha256_header: false,

                normalize_uri_path: true,

            },

            signing_requirements: SigningRequirements::Required,

            expires_in: None,
@@ -88,9 +89,9 @@ pub enum SigningRequirements { 
pub struct SigningOptions {

    pub double_uri_encode: bool,

    pub content_sha256_header: bool,

    pub normalize_uri_path: bool,

    /*

    Currently unsupported:

    pub normalize_uri_path: bool,

    pub omit_session_token: bool,

     */

}
@@ -138,6 +139,12 @@ impl SigV4Signer { 
        } else {

            PayloadChecksumKind::NoHeader

        };

        settings.uri_path_normalization_mode =

            if operation_config.signing_options.normalize_uri_path {

                UriPathNormalizationMode::Enabled

            } else {

                UriPathNormalizationMode::Disabled

            };

        settings.signature_location = match operation_config.signature_type {

            HttpSignatureType::HttpRequestHeaders => SignatureLocation::Headers,

            HttpSignatureType::HttpRequestQueryParams => SignatureLocation::QueryParams,

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

+8 −2
Original line number Diff line number Diff line
@@ -6,7 +6,9 @@ 
use crate::date_time::{format_date, format_date_time};

use crate::http_request::error::CanonicalRequestError;

use crate::http_request::query_writer::QueryWriter;

use crate::http_request::settings::UriPathNormalizationMode;

use crate::http_request::sign::SignableRequest;

use crate::http_request::uri_path_normalization::normalize_uri_path;

use crate::http_request::url_escape::percent_encode_path;

use crate::http_request::PercentEncodingMode;

use crate::http_request::{PayloadChecksumKind, SignableBody, SignatureLocation, SigningParams};
@@ -129,10 +131,14 @@ impl<'a> CanonicalRequest<'a> { 
        // Path encoding: if specified, re-encode % as %25

        // Set method and path into CanonicalRequest

        let path = req.uri().path();

        let path = match params.settings.uri_path_normalization_mode {

            UriPathNormalizationMode::Enabled => normalize_uri_path(path),

            UriPathNormalizationMode::Disabled => Cow::Borrowed(path),

        };

        let path = match params.settings.percent_encoding_mode {

            // The string is already URI encoded, we don't need to encode everything again, just `%`

            PercentEncodingMode::Double => Cow::Owned(percent_encode_path(path)),

            PercentEncodingMode::Single => Cow::Borrowed(path),

            PercentEncodingMode::Double => Cow::Owned(percent_encode_path(&path)),

            PercentEncodingMode::Single => path,

        };

        let payload_hash = Self::payload_hash(req.body());

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

+2 −0
Original line number Diff line number Diff line
@@ -46,6 +46,7 @@ mod error; 
mod query_writer;

mod settings;

mod sign;

mod uri_path_normalization;

mod url_escape;



#[cfg(test)]
@@ -54,5 +55,6 @@ pub(crate) mod test; 
pub use error::SigningError;

pub use settings::{

    PayloadChecksumKind, PercentEncodingMode, SignatureLocation, SigningParams, SigningSettings,

    UriPathNormalizationMode,

};

pub use sign::{sign, SignableBody, SignableRequest};

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

+18 −0
Original line number Diff line number Diff line
@@ -29,6 +29,9 @@ pub struct SigningSettings { 


    /// Headers that should be excluded from the signing process

    pub excluded_headers: Option<Vec<HeaderName>>,



    /// Specifies whether the absolute path component of the URI should be normalized during signing.

    pub uri_path_normalization_mode: UriPathNormalizationMode,

}



/// HTTP payload checksum type
@@ -61,6 +64,20 @@ pub enum PercentEncodingMode { 
    Single,

}



/// Config value to specify whether the canonical request's URI path should be normalized.

/// <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>

///

/// URI path normalization is performed based on <https://www.rfc-editor.org/rfc/rfc3986>.

#[non_exhaustive]

#[derive(Debug, Eq, PartialEq)]

pub enum UriPathNormalizationMode {

    /// Normalize the URI path according to RFC3986

    Enabled,



    /// Don't normalize the URI path (S3, for example, rejects normalized paths in some instances)

    Disabled,

}



impl Default for SigningSettings {

    fn default() -> Self {

        // The user agent header should not be signed because it may be altered by proxies
@@ -72,6 +89,7 @@ impl Default for SigningSettings { 
            signature_location: SignatureLocation::Headers,

            expires_in: None,

            excluded_headers: Some(EXCLUDED_HEADERS.to_vec()),

            uri_path_normalization_mode: UriPathNormalizationMode::Enabled,

        }

    }

}