-
Notifications
You must be signed in to change notification settings - Fork 80
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
chore: Add an integration test for SigV4A signing region #1852
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
64 changes: 64 additions & 0 deletions
64
IntegrationTests/Services/AWSS3IntegrationTests/SigV4ASigningRegionTest.swift
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// | ||
// Copyright Amazon.com Inc. or its affiliates. | ||
// All Rights Reserved. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
|
||
import AWSS3 | ||
import AWSSDKHTTPAuth | ||
import ClientRuntime | ||
import Smithy | ||
import SmithyHTTPAPI | ||
import SmithyHTTPAuthAPI | ||
import SmithyTestUtil | ||
import XCTest | ||
|
||
// Checks that S3's rules-based SigV4A operations resolve signing region to "*" using PutObject with MRAP ARN. | ||
class SigV4ASigningRegionTest: XCTestCase { | ||
private var sigv4aClient: S3Client! | ||
private var sigv4aConfig: S3Client.S3ClientConfiguration! | ||
|
||
override func setUp() async throws { | ||
sigv4aConfig = try await S3Client.S3ClientConfiguration(region: "dummy-region") | ||
sigv4aConfig.authSchemes = [SigV4AAuthScheme()] | ||
sigv4aConfig.httpClientEngine = ProtocolTestClient() // Mock HTTP client that doesn't actually send a request | ||
sigv4aConfig.addInterceptorProvider(SigningRegionAssertInterceptorProvider()) | ||
sigv4aClient = S3Client(config: sigv4aConfig) | ||
} | ||
|
||
func testResolvedSigningRegionIsAsterisk() async throws { | ||
do { | ||
// Providing input.bucket value in the format of MRAP ARN causes endpoint to resolve to MRAP endpoint with | ||
// auth schemes property that has SigV4A. | ||
// Then, when signing properties are resolved by the auth scheme, signing region resolved by the service's | ||
// AuthSchemeResolver, saved in AuthOption, supersedes the one provided in config, saved in context. | ||
_ = try await sigv4aClient.putObject(input: PutObjectInput( | ||
body: ByteStream.data(Data()), | ||
bucket: "arn:aws:s3::123456789012:accesspoint/mfzwi23gnjvgw.mrap", // Dummy ARN for MRAP | ||
key: "dummy-key" | ||
)) | ||
} catch is TestCheckError { | ||
// This is the expected error thrown by Mock HTTP client; no-op. | ||
} | ||
} | ||
} | ||
|
||
class SigningRegionAssertInterceptor<InputType, OutputType>: Interceptor { | ||
typealias RequestType = HTTPRequest | ||
typealias ResponseType = HTTPResponse | ||
|
||
func readBeforeSigning(context: some AfterSerialization<InputType, RequestType>) async throws { | ||
let resolvedSigningRegion = context.getAttributes().selectedAuthScheme?.signingProperties?.get( | ||
key: SigningPropertyKeys.signingRegion | ||
) | ||
XCTAssertNotNil(resolvedSigningRegion) | ||
XCTAssertEqual(resolvedSigningRegion!, "*") // Assert that the resolved signing region is "*", as expected. | ||
} | ||
} | ||
|
||
class SigningRegionAssertInterceptorProvider: HttpInterceptorProvider { | ||
func create<InputType, OutputType>() -> any Interceptor<InputType, OutputType, HTTPRequest, HTTPResponse> { | ||
return SigningRegionAssertInterceptor() | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we either (or both) need to 1) re-write this test to use a real endpoint that resolves to 2 different regions and then make requests to both regions (as in the sev2 test code) 2) change this to a unit test since we don't actually need to make a real request to check if the resolvedSigningRegion 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.
*
forsigningRegionSet
value, including SESv2 that caused the Sev2.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.
After discussion offline we concluded that maybe I could just use SESv2's multiregion (global) endpoint to test it in real-service scenario, but that was called off because all (all two of them) SESv2 API using global endpoints (
sendEmail
andsendBulkEmail
) require involved email verification process that either requires manual verification via clicking a button in the actual email sent to the provided email address, or providing a key pair for a domain name.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.
After offline discussion, having the test separated out like this vs. including it into S3 MRAP test is pending tie-breaker from Josh.