feat: add toggle to SigV4AuthScheme to turn off body signing #1822

dayaffe · 2024-11-14T21:43:55Z

Issue #

SWIFT-1849, PR: smithy-lang/smithy-swift#857

Description of changes

Add signing property requestUnsignedBody. Note this wont effect middlewares or anything else on purpose until we go to actually sign
In AWSSigV4Signer before we go to sign, check for requestUnsignedBody and if its true, overrule isUnsignedBody, setting header x-amz-content-sha256 to UNSIGNED-PAYLOAD

Usage

let config = try await S3Client.S3ClientConfiguration(
    authSchemes: [SigV4AuthScheme(requestUnsignedBody: true)]
)
let client = S3Client(config: config)

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sichanyoo

Comments re: test

sichanyoo · 2024-11-15T19:59:28Z

IntegrationTests/Services/AWSS3IntegrationTests/S3ToggleUnsignedTests.swift

+        s3Config.authSchemes = [SigV4AuthScheme(requestUnsignedBody: true)]
+    }
+
+    func testS3PresignedRequest() async throws {


nit: FIx test name

sichanyoo · 2024-11-15T20:05:07Z

IntegrationTests/Services/AWSS3IntegrationTests/S3ToggleUnsignedTests.swift

@@ -0,0 +1,45 @@
+//


Please add:

One more test case that has payload over the chunked threshold

An interceptor that asserts that:

Authorization header isn't present as expected

x-amz-content-sha256 header values match expected values (UNSIGNED-PAYLOAD for single chunk request, and STREAMING-UNSIGNED-PAYLOAD-TRAILER for aws chunked request)

I believe Authorization header is still expected to be there. We are still signing the request just not the body. Will do the rest

That's true, and gotcha

sichanyoo

nit: Change integration test file name and test cases to mean unsigned payload, not unsigned request

jbelkins · 2024-11-15T20:57:57Z

IntegrationTests/Services/AWSS3IntegrationTests/S3ToggleUnsignedTests.swift

+
+    func testS3ToggleUnsignedRequestStreaming() async throws {
+        let key = "test-streaming.txt"
+        let data = Data((0..<(1024 * 1024)).map { _ in UInt8.random(in: UInt8.min...UInt8.max) })


We also create a data of specified length filled with randomness in S3ConcurrentTests, probably elsewhere like the chunked streaming tests as well.

Perhaps we should extract the logic to do that to a helper function so it may be reused?

Sure I can move this out

jbelkins · 2024-11-15T20:59:27Z

Sources/Core/AWSSDKHTTPAuth/Sources/AWSSDKHTTPAuth/SigV4AuthScheme.swift

@@ -69,6 +74,11 @@ public struct SigV4AuthScheme: AuthScheme {
            value: context.isChunkedEligibleStream
        )

+        // Optionally toggle unsigned body
+        if self.requestUnsignedBody == true {


assume the explicit == true is because self.requestUnsignedBody may be nil as well as false?

jbelkins · 2024-11-15T21:21:43Z

Sources/Core/AWSSDKHTTPAuth/Tests/AWSSDKHTTPAuthTests/SigV4AuthSchemeTests.swift

+            .withUnsignedPayloadTrait(value: false)
+            .build()
+        let updatedProperties = try customSigV4AuthScheme.customizeSigningProperties(signingProperties: Attributes(), context: context)
+        XCTAssertTrue(try XCTUnwrap(updatedProperties.get(key: SigningPropertyKeys.requestUnsignedBody)))


Maybe break the XCTUnwrap & XCAssert into two lines for clarity

Was following other tests format -- if we think its more clear to separate we should go through and change all the tests effected though I personally dont have any issue with this style

jbelkins · 2024-11-15T21:27:35Z

Sources/Core/AWSSDKHTTPAuth/Sources/AWSSDKHTTPAuth/SigV4AuthScheme.swift

@@ -15,9 +15,14 @@ import struct Smithy.Attributes
 public struct SigV4AuthScheme: AuthScheme {
    public let schemeID: String = "aws.auth#sigv4"
    public let signer: Signer = AWSSigV4Signer()
+    public var requestUnsignedBody: Bool?


The possible values for Bool? are true, false, and nil.

true means unsigned body is requested.
nil means unsigned body was not specified in the auth scheme resolver.

So what is false? I can certainly create a auth scheme resolver with requestUnsignedBody: false. Would that ever be used? Is it undefined? (If it doesn't, we should consider a different data type than Bool? for this setting.)

jbelkins · 2024-11-15T22:36:33Z

IntegrationTests/Services/AWSS3IntegrationTests/S3ConcurrentTests.swift

@@ -18,38 +18,27 @@ final class S3ConcurrentTests: S3XCTestCase {
    // Payload just below chunked threshold
    // Tests concurrent upload of simple data payloads
    func test_10x_1MB_getObject() async throws {
-        fileData = try generateDummyTextData(count: CHUNKED_THRESHOLD - 1)
+        fileData = S3ConcurrentTests.generateRandomTextData(ofSizeInMB: 1)


There is a folder IntegrationTests/AWSIntegrationTestUtils where I think this method belongs

jbelkins · 2024-11-15T22:40:37Z

IntegrationTests/Services/AWSS3IntegrationTests/S3ConcurrentTests.swift

        try await repeatConcurrentlyWithArgs(count: 10, test: getObject, args: fileData!)
    }

    // Payload 256 bytes with 200 concurrent requests, sends as simple data
    // Tests very high concurrency with small data payloads
    func test_200x_256B_getObject() async throws {
-        fileData = try generateDummyTextData(count: 256)
+        let bytes_256 = Double(256) / (1024 * 1024)


Any Swift literal with a decimal point is a Double. So you can just use 256.0 or 256. instead of the Double initializer.

jbelkins · 2024-11-15T22:42:41Z

IntegrationTests/Services/AWSS3IntegrationTests/S3XCTestCase.swift

@@ -116,4 +116,12 @@ class S3XCTestCase: XCTestCase {
        let input = DeleteBucketInput(bucket: bucketName)
        _ = try await client.deleteBucket(input: input)
    }
+
+    static func generateRandomTextData(ofSizeInMB megabytes: Double) -> Data {


This method should take a precise integer number of bytes.

I'm okay with it if we have a convenience form that takes a Double number of MB then converts that to bytes.

Also, as mentioned above, let's move this to integration test utils.

Modified names, was a nit

dayaffe added 4 commits November 14, 2024 16:38

add toggle to SigV4AuthScheme to turn off body signing

52c041e

add test for toggling unsigned body

aa4b374

add unit test, integration test, and fix swiftlint

3a17012

Merge branch 'main' into day/unsigned-body-toggle

4a57e0d

dayaffe requested review from jbelkins and sichanyoo November 15, 2024 15:53

dayaffe mentioned this pull request Nov 15, 2024

feat: add requestUnsignedBody to list of SigningPropertykeys smithy-lang/smithy-swift#857

Merged

2 tasks

sichanyoo requested changes Nov 15, 2024

View reviewed changes

dayaffe added 2 commits November 15, 2024 15:29

beef up integration tests

24a9c1b

Merge branch 'main' into day/unsigned-body-toggle

c9f8e2d

dayaffe requested a review from sichanyoo November 15, 2024 20:51

sichanyoo previously requested changes Nov 15, 2024

View reviewed changes

jbelkins requested changes Nov 15, 2024

View reviewed changes

beef up integration tests more

aea0435

jbelkins requested changes Nov 15, 2024

View reviewed changes

dayaffe added 2 commits November 15, 2024 18:04

move helper function and modify signature

f4574a3

fix missing import

3137195

dayaffe requested review from sichanyoo and jbelkins November 15, 2024 23:13

jbelkins approved these changes Nov 18, 2024

View reviewed changes

dayaffe merged commit f5e6c7f into main Nov 18, 2024
29 checks passed

dayaffe deleted the day/unsigned-body-toggle branch November 18, 2024 16:24

sichanyoo mentioned this pull request Nov 29, 2024

Provides a switch to turn on/off sigv4 body-signing #656

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add toggle to SigV4AuthScheme to turn off body signing #1822

feat: add toggle to SigV4AuthScheme to turn off body signing #1822

dayaffe commented Nov 14, 2024 •

edited

Loading

sichanyoo left a comment

sichanyoo Nov 15, 2024

sichanyoo Nov 15, 2024

dayaffe Nov 15, 2024

sichanyoo Nov 15, 2024

sichanyoo left a comment

jbelkins Nov 15, 2024

dayaffe Nov 15, 2024

jbelkins Nov 15, 2024

dayaffe Nov 15, 2024

jbelkins Nov 15, 2024

dayaffe Nov 15, 2024

jbelkins Nov 15, 2024

jbelkins Nov 15, 2024

jbelkins Nov 15, 2024

jbelkins Nov 15, 2024

feat: add toggle to SigV4AuthScheme to turn off body signing #1822

feat: add toggle to SigV4AuthScheme to turn off body signing #1822

Conversation

dayaffe commented Nov 14, 2024 • edited Loading

Issue #

Description of changes

Usage

New/existing dependencies impact assessment, if applicable

Conventional Commits

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayaffe commented Nov 14, 2024 •

edited

Loading