Skip to content
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

feat!: SRA Identity & Auth #665

Merged
merged 31 commits into from
Feb 28, 2024
Merged

feat!: SRA Identity & Auth #665

merged 31 commits into from
Feb 28, 2024

Conversation

sichanyoo
Copy link
Contributor

@sichanyoo sichanyoo commented Feb 10, 2024

Issue #

Release milestone ticket: awslabs/aws-sdk-swift#1149
Project ticket: awslabs/aws-sdk-swift#1119

Description of changes

Reworks Swift SDK's Identity & Auth flows to be SRA compliant (aka make it easily extensible).

Changes are in three categories:

  • Runtime swift library used by codegen & runtime.
  • Codegen changes that generate new files (e.g., AuthSchemeResolver).
  • Tests. Both swift tests and kotlin tests are added.

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

sichanyoo and others added 15 commits October 4, 2023 10:31
* Add new identity protocols.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Add signer protocol & refactor HttpContext

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Add signer protocol.

* Add http context changes.
---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Add middlewares - AuthSchemeMiddleware and SignerMiddleware

* Provide hook in auth scheme for signing properties customization.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Add codegen for service specific auth scheme resolver protocol, service specific default auth scheme resolver struct, and service specific auth scheme resolver parameters struct.

* Make ASR throw if passed in ASR params doesn't have region field for SigV4 auth scheme & fix ktlint issues.

* Clean up middlewares.

* Remove auth scheme and signing middlewares from operation stack of protocol tests.

* Update test cases to include new middlewares.

* Codegen more descriptive comment for empty service specific auth scheme resolver protocol.

* Add codegen test for auth scheme resolver generation.

* Move region in middleware context from sdk side to smithy side.

* Remove AWSClientRuntime dependency - signingProperties will be set in auth scheme customization hooks instead.

* Move auth schemes from service specific config to general AWS config.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* chore: Require Swift 5.7, fix deprecation warnings (#600)

* feat: support initial-response in RPC based event streams (#597)

* chore: Updates version to 0.32.0

* chore: Add newline to README.md (#602)

* feat: add limited support in smithy-swift for visionOS (#606)

* feat: add support for requiresLength trait and Transfer-Encoding: Chunked (#604)

* chore: Update to aws-crt-swift 0.15.0 (#607)

* fix: content-length middleware should not error on event streams (#608)

* chore: Updates version to 0.33.0

* chore: Improved downstream task (#568)

* chore: Convert idempotency token middleware from closure to reusable type (#610)

* fix: Update aws-crt-swift dependency to 0.17.0 (#612)

* chore: Updates version to 0.34.0

* fix: Endpoint url should be nil if host or scheme is missing (#614)

* fix: Pool HTTP connections based on scheme, host, and port (#615)

* add default log level to initialize method (#616)

* feat: add utility method for converting SdkHttpRequest to URLRequest. (#613)

* Add extension constructor to URLRequest to convert SDKHttpRequest

* Add preprocessor conditional import functionality to SwiftWriter.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* chore: Updates version to 0.35.0

* Update test cases added from main to reflect sra identity & auth fields in middleware context & sra identity & auth middlewares in operation stack.

---------

Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Co-authored-by: David Yaffe <dayaffe@amazon.com>
Co-authored-by: AWS SDK Swift Automation <github-aws-sdk-swift-automation@amazon.com>
Co-authored-by: Cyprien Ricque <48893621+CyprienRicque@users.noreply.github.com>
Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Add customizations to auth resolve process.

* Add signEvent API to Signer protocol, and rename sign to signRequest.

* Resolve PR comments.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* chore: Require Swift 5.7, fix deprecation warnings (#600)

* feat: support initial-response in RPC based event streams (#597)

* chore: Updates version to 0.32.0

* chore: Add newline to README.md (#602)

* feat: add limited support in smithy-swift for visionOS (#606)

* feat: add support for requiresLength trait and Transfer-Encoding: Chunked (#604)

* chore: Update to aws-crt-swift 0.15.0 (#607)

* fix: content-length middleware should not error on event streams (#608)

* chore: Updates version to 0.33.0

* chore: Improved downstream task (#568)

* chore: Convert idempotency token middleware from closure to reusable type (#610)

* fix: Update aws-crt-swift dependency to 0.17.0 (#612)

* chore: Updates version to 0.34.0

* fix: Endpoint url should be nil if host or scheme is missing (#614)

* fix: Pool HTTP connections based on scheme, host, and port (#615)

* add default log level to initialize method (#616)

* feat: add utility method for converting SdkHttpRequest to URLRequest. (#613)

* Add extension constructor to URLRequest to convert SDKHttpRequest

* Add preprocessor conditional import functionality to SwiftWriter.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* chore: Updates version to 0.35.0

* fix: Add a header to operation doc comments (#621)

* remove unnecessary TODOs (#622)

* fix: Codegen issues re: recursion, Swift keywords in unions (#623)

* feat!: Replace the XML encoder with a custom Smithy implementation (#619)

* feat!: Use closures for processing HTTP response (#624)

* feat: add custom trait PaginationTruncationMember (#625)

* allow isTruncated to be optional bool (#626)

* chore: Updates version to 0.36.0

* chore: Run tvOS old & new in CI (#628)

* fix: Fix Package.swift warning on Mac (#629)

* chore: refactor HttpBody and ByteStream to be a single class ByteStream (#627)

* chore: remove sync read in unused data extension (#630)

* update smithy to 1.42.0 (#631)

* chore: Updates version to 0.37.0

* chore: Update to aws-crt-swift 0.20.0 (#633)

* fix: add back from method with fileHandle (#635)

* fix!: Add no-op behavior for initialize methods of logging system. (#637)

* Add no-op behavior for initialize methods if it isn't the first time being called.

* Make LockingSystem threadsafe.

* Make initialize methods async.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* feat!: URLSession-based HTTP Client (#636)

* Delete missed merge conflict marker.

---------

Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Co-authored-by: David Yaffe <dayaffe@amazon.com>
Co-authored-by: AWS SDK Swift Automation <github-aws-sdk-swift-automation@amazon.com>
Co-authored-by: Cyprien Ricque <48893621+CyprienRicque@users.noreply.github.com>
Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* Rename runtime type and file in Kotlin side to match corresponding type in Swift side, now both called SignerMiddleware.

* Change DefaultIdentityResolverConfiguration's identity resolvers member field to be Attributes so it can store multiple types of identity resolvers and return one with matching identity type. Necessary for supporting multiple types of identities down the line.

* Detect newly added SigV4a trait and handle accordingly.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* chore: Require Swift 5.7, fix deprecation warnings (#600)

* feat: support initial-response in RPC based event streams (#597)

* chore: Updates version to 0.32.0

* chore: Add newline to README.md (#602)

* feat: add limited support in smithy-swift for visionOS (#606)

* feat: add support for requiresLength trait and Transfer-Encoding: Chunked (#604)

* chore: Update to aws-crt-swift 0.15.0 (#607)

* fix: content-length middleware should not error on event streams (#608)

* chore: Updates version to 0.33.0

* chore: Improved downstream task (#568)

* chore: Convert idempotency token middleware from closure to reusable type (#610)

* fix: Update aws-crt-swift dependency to 0.17.0 (#612)

* chore: Updates version to 0.34.0

* fix: Endpoint url should be nil if host or scheme is missing (#614)

* fix: Pool HTTP connections based on scheme, host, and port (#615)

* add default log level to initialize method (#616)

* feat: add utility method for converting SdkHttpRequest to URLRequest. (#613)

* Add extension constructor to URLRequest to convert SDKHttpRequest

* Add preprocessor conditional import functionality to SwiftWriter.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* chore: Updates version to 0.35.0

* fix: Add a header to operation doc comments (#621)

* remove unnecessary TODOs (#622)

* fix: Codegen issues re: recursion, Swift keywords in unions (#623)

* feat!: Replace the XML encoder with a custom Smithy implementation (#619)

* feat!: Use closures for processing HTTP response (#624)

* feat: add custom trait PaginationTruncationMember (#625)

* allow isTruncated to be optional bool (#626)

* chore: Updates version to 0.36.0

* chore: Run tvOS old & new in CI (#628)

* fix: Fix Package.swift warning on Mac (#629)

* chore: refactor HttpBody and ByteStream to be a single class ByteStream (#627)

* chore: remove sync read in unused data extension (#630)

* update smithy to 1.42.0 (#631)

* chore: Updates version to 0.37.0

* chore: Update to aws-crt-swift 0.20.0 (#633)

* fix: add back from method with fileHandle (#635)

* fix!: Add no-op behavior for initialize methods of logging system. (#637)

* Add no-op behavior for initialize methods if it isn't the first time being called.

* Make LockingSystem threadsafe.

* Make initialize methods async.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* feat!: URLSession-based HTTP Client (#636)

* bump up CRT version to 0.22.0 (#639)

* chore: Update version to 0.38.0 (#641)

* chore: Empty commit (#643)

* feat: add wrapper for checksums + unit tests (#642)

* feat: Use the Foundation HTTP client by default on Mac (#646)

* chore: Updates version to 0.39.0

* chore: Change MyURLQueryItem to SDKURLQueryItem (#652)

* feat!: Provide HTTP request components by closure instead of protocol (#654)

* fix: Don't retry modeled errors by default (#653)

* Missed merge conflict marker - deleted.

---------

Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Co-authored-by: David Yaffe <dayaffe@amazon.com>
Co-authored-by: AWS SDK Swift Automation <github-aws-sdk-swift-automation@amazon.com>
Co-authored-by: Cyprien Ricque <48893621+CyprienRicque@users.noreply.github.com>
Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* chore: Require Swift 5.7, fix deprecation warnings (#600)

* feat: support initial-response in RPC based event streams (#597)

* chore: Updates version to 0.32.0

* chore: Add newline to README.md (#602)

* feat: add limited support in smithy-swift for visionOS (#606)

* feat: add support for requiresLength trait and Transfer-Encoding: Chunked (#604)

* chore: Update to aws-crt-swift 0.15.0 (#607)

* fix: content-length middleware should not error on event streams (#608)

* chore: Updates version to 0.33.0

* chore: Improved downstream task (#568)

* chore: Convert idempotency token middleware from closure to reusable type (#610)

* fix: Update aws-crt-swift dependency to 0.17.0 (#612)

* chore: Updates version to 0.34.0

* fix: Endpoint url should be nil if host or scheme is missing (#614)

* fix: Pool HTTP connections based on scheme, host, and port (#615)

* add default log level to initialize method (#616)

* feat: add utility method for converting SdkHttpRequest to URLRequest. (#613)

* Add extension constructor to URLRequest to convert SDKHttpRequest

* Add preprocessor conditional import functionality to SwiftWriter.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* chore: Updates version to 0.35.0

* Add customizations to auth resolve process.

Add internal modeled layer for services (S3, EventBridge) that use rules-based auth scheme resolver.

Rules-based auth scheme resolver work wrap-up.

Wrap-up presign / presign-url refactor.

Wrap-up refactor for fitting in rules-based auth scheme resolver.

Update test cases to include new middlewares.

Move requestSignature getter / setter/ key from aws middleware context extension to here. Also, add saving requestSignature to SignerMiddleware for consumption by event stream signing.

* Add signEvent API to Signer protocol, and rename sign to signRequest.

* Add mock auth scheme resolver, mock auth schemes, mock identity, mock identity resolver, and mock signer to use for middleware unit tests.

* Add unit tests for AuthSchemeMiddleware and SignerMiddleware.

* Update MockSigner to conform to modified Signer API with signEvent.

* Rename directory containing mocks for auth tests from AuthTest to AuthTestUtil.

* fix: Add a header to operation doc comments (#621)

* remove unnecessary TODOs (#622)

* fix: Codegen issues re: recursion, Swift keywords in unions (#623)

* feat!: Replace the XML encoder with a custom Smithy implementation (#619)

* feat!: Use closures for processing HTTP response (#624)

* feat: add custom trait PaginationTruncationMember (#625)

* allow isTruncated to be optional bool (#626)

* chore: Updates version to 0.36.0

* chore: Run tvOS old & new in CI (#628)

* fix: Fix Package.swift warning on Mac (#629)

* chore: refactor HttpBody and ByteStream to be a single class ByteStream (#627)

* chore: remove sync read in unused data extension (#630)

* update smithy to 1.42.0 (#631)

* chore: Updates version to 0.37.0

* chore: Update to aws-crt-swift 0.20.0 (#633)

* fix: add back from method with fileHandle (#635)

* fix!: Add no-op behavior for initialize methods of logging system. (#637)

* Add no-op behavior for initialize methods if it isn't the first time being called.

* Make LockingSystem threadsafe.

* Make initialize methods async.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* feat!: URLSession-based HTTP Client (#636)

* bump up CRT version to 0.22.0 (#639)

* chore: Update version to 0.38.0 (#641)

* chore: Empty commit (#643)

* feat: add wrapper for checksums + unit tests (#642)

* Update to reflect midleware generics change.

* Delete unnessary line from test case.

* Add CloudFront KeyValueStore as one of the services that use rules based auth scheme resolver customization.

* feat: Use the Foundation HTTP client by default on Mac (#646)

* chore: Updates version to 0.39.0

* Fix auth scheme middleware to save the selected auth scheme to middleware context by modifying the original context. Fixes transcribe streaming integration test where streaming signing flow was only accessing the original context and not the newly built one with selected auth scheme that was being passed to next middleware in line.

* chore: Change MyURLQueryItem to SDKURLQueryItem (#652)

* feat!: Provide HTTP request components by closure instead of protocol (#654)

* fix: Don't retry modeled errors by default (#653)

* Address Josh's PR comments.

* Merge updated CRT version from main into feat/test-suite.

---------

Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Co-authored-by: David Yaffe <dayaffe@amazon.com>
Co-authored-by: AWS SDK Swift Automation <github-aws-sdk-swift-automation@amazon.com>
Co-authored-by: Cyprien Ricque <48893621+CyprienRicque@users.noreply.github.com>
Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
* chore: Require Swift 5.7, fix deprecation warnings (#600)

* feat: support initial-response in RPC based event streams (#597)

* chore: Updates version to 0.32.0

* chore: Add newline to README.md (#602)

* feat: add limited support in smithy-swift for visionOS (#606)

* feat: add support for requiresLength trait and Transfer-Encoding: Chunked (#604)

* chore: Update to aws-crt-swift 0.15.0 (#607)

* fix: content-length middleware should not error on event streams (#608)

* chore: Updates version to 0.33.0

* chore: Improved downstream task (#568)

* chore: Convert idempotency token middleware from closure to reusable type (#610)

* fix: Update aws-crt-swift dependency to 0.17.0 (#612)

* chore: Updates version to 0.34.0

* fix: Endpoint url should be nil if host or scheme is missing (#614)

* fix: Pool HTTP connections based on scheme, host, and port (#615)

* add default log level to initialize method (#616)

* feat: add utility method for converting SdkHttpRequest to URLRequest. (#613)

* Add extension constructor to URLRequest to convert SDKHttpRequest

* Add preprocessor conditional import functionality to SwiftWriter.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* chore: Updates version to 0.35.0

* fix: Add a header to operation doc comments (#621)

* remove unnecessary TODOs (#622)

* fix: Codegen issues re: recursion, Swift keywords in unions (#623)

* feat!: Replace the XML encoder with a custom Smithy implementation (#619)

* feat!: Use closures for processing HTTP response (#624)

* feat: add custom trait PaginationTruncationMember (#625)

* allow isTruncated to be optional bool (#626)

* chore: Updates version to 0.36.0

* chore: Run tvOS old & new in CI (#628)

* fix: Fix Package.swift warning on Mac (#629)

* chore: refactor HttpBody and ByteStream to be a single class ByteStream (#627)

* chore: remove sync read in unused data extension (#630)

* update smithy to 1.42.0 (#631)

* chore: Updates version to 0.37.0

* chore: Update to aws-crt-swift 0.20.0 (#633)

* fix: add back from method with fileHandle (#635)

* fix!: Add no-op behavior for initialize methods of logging system. (#637)

* Add no-op behavior for initialize methods if it isn't the first time being called.

* Make LockingSystem threadsafe.

* Make initialize methods async.

---------

Co-authored-by: Sichan Yoo <chanyoo@amazon.com>

* feat!: URLSession-based HTTP Client (#636)

* bump up CRT version to 0.22.0 (#639)

* chore: Update version to 0.38.0 (#641)

* chore: Empty commit (#643)

* feat: add wrapper for checksums + unit tests (#642)

* feat: Use the Foundation HTTP client by default on Mac (#646)

* chore: Updates version to 0.39.0

* chore: Change MyURLQueryItem to SDKURLQueryItem (#652)

* feat!: Provide HTTP request components by closure instead of protocol (#654)

* fix: Don't retry modeled errors by default (#653)

* feat!: Eliminate service client protocols (#655)

* feat: add support for flexible checksums on Normal payloads (#647)

* chore: Update aws-crt-swift to 0.26.0 (#661)

---------

Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Co-authored-by: David Yaffe <dayaffe@amazon.com>
Co-authored-by: AWS SDK Swift Automation <github-aws-sdk-swift-automation@amazon.com>
Co-authored-by: Cyprien Ricque <48893621+CyprienRicque@users.noreply.github.com>
Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
Copy link
Contributor Author

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added informational comments to help reviewers.

@@ -0,0 +1,21 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a container for credentials provider(s) configured on the client. Its main purpose is to avoid passing around the entire client config object when all we need from it is the credentials provider(s). Right now, only AWS credentials are supported, but any other types of credentials can be added to it in future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this support @httpBearerAuth?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@httpBearerAuth is not yet supported in Swift SDK. It will be an additive change after SRA I&A is implemented.

@@ -0,0 +1,22 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the container blueprint for config values required to resolve the identity & sign a request for the given auth scheme. Instances of this struct are constructed in code-generated auth scheme resolvers.

@@ -0,0 +1,21 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the container blueprint for an auth scheme supported by a given service in the SDK. The AuthOptions returned by code-generated auth scheme resolvers based on Smithy models are matched against instances of this struct configured on the service to determine the auth scheme to actually use for client call.

E.g., say operation A supports SigV4 and BearerToken according to its Smithy mode. When client calls A, the auth scheme would resolve to SigV4 in Swift SDK since Swift SDK does not support BearerToken yet.

The method customizeSigningProperties is called in AuthSchemeMiddleware. The confusing signing config resolve logic scattered about between AWSSigningParams and SigV4Configurator in AWSSigningParams.kt in aws-sdk-swift have been consolidated into implementations of this runtime function for better code organization. (See implementation of this method for SigV4AuthScheme.swift in aws-sdk-swift for an example)

@@ -0,0 +1,11 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a protocol all code-generated auth scheme resolvers must conform to.

@@ -0,0 +1,10 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a protocol all code-generated auth scheme resolver parameters must conform to. It's a container for values used by an auth scheme resolver for determining which auth options to return. By default it only has the operation name.

@@ -54,3 +55,13 @@ abstract class ServiceConfig(val writer: SwiftWriter, val clientName: String, va

open fun serviceConfigProperties(): List<ConfigField> = listOf()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values used for codegen.

@@ -0,0 +1,37 @@
package software.amazon.smithy.swift.codegen.integration.middlewares
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code generator that constructs & inserts AuthSchemeMiddleware to operation stack in generated client code.

@@ -0,0 +1,37 @@
package software.amazon.smithy.swift.codegen.integration.middlewares
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code generator that constructs & inserts SignerMiddleware to operation stack in generated client code.

@@ -0,0 +1,115 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here and onward are tests & test resources.

@@ -26,12 +28,18 @@ class MiddlewareExecutionGenerator(
private val model: Model = ctx.model
private val symbolProvider = ctx.symbolProvider

fun render(service: ServiceShape, op: OperationShape, onError: (SwiftWriter, String) -> Unit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add codegen for code that sets flags used by auth schemes in aws-sdk-swift to customize signing properties for presign url & presign request flows.

E.g., SigV4AuthScheme::customizeSigningProperties in aws-sdk-swift sets the type of its signature type (.requestQueryParams v. .requestHeaders) based on the flowType flag set on the middleware context.

@@ -0,0 +1,21 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this support @httpBearerAuth?

Comment on lines 13 to 21
// Identity v. IdentityT v. IdentityKind
// - Identity is the protocol that all identity types must conform to.
// - IdentityT is the associated type / generic type name used by protocols like IdentityResolver and Signer.
// - IdentityKind is the enum that's used by IdentityResolverConfiguration to return correct kind of identity resolver
// for the given auth scheme. E.g., SigV4AuthScheme has idKind field as .aws. And identityResolver method in SigV4AuthScheme
// returns an identity resolver that returns identity of type .aws.
public enum IdentityKind: CaseIterable {
case aws
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying question: I'm still not sure why this enum is needed, is there no way to keep this out of the identity and auth flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IdentityKind enum is used by the auth schemes to tell the identity resolver config instance what identity type they accept (e.g., SigV4AuthScheme accepts .aws type), which then returns an identity resolver that returns that identity type if it finds one from identity resolvers configured on the client.

To convey this information via type information, it would mean the default identity resolver configuration in smithy-swift would have to check the given type value and have a dependency on aws-sdk-swift, which will be a circular dependency. Regarding how this info is handled, SRA writes:

Retrieve an identity resolver for the provided identity type .
For Java , we use runtime type information to resolve the specific identity type , 
but other SDKs may use a string ( uniquely identifying each type of identity resolver ) 
or some other language - idiomatic mechanism for an 
auth scheme to extract the specific type of identity resolver it needs . 

so I figured adding an enum to be used as identity type info container would be good way to resolve this.

Copy link
Contributor Author

@sichanyoo sichanyoo Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had offline discussion, and the enum has now been removed and now scheme ID of auth schemes is used instead for determining what identity resolver to return for a given auth scheme.

Comment on lines 468 to 472
operationMiddleware.appendMiddleware(operation, SignerMiddleware(ctx.model, ctx.symbolProvider))

addProtocolSpecificMiddleware(ctx, operation)

operationMiddleware.appendMiddleware(operation, AuthSchemeMiddleware(ctx.model, ctx.symbolProvider))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rearrange middleware append to match the operation execution order of existing middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remembered the reason why the order was this way after seeing codegen tests failing. The order of middleware here does not reflect the order of execution in generated client code. Reverted the order back the way it was and added clarifying comment on difference between codegen middleware stack vs. generated operation middleware stack.

….kt to match other codegen tests in the repo.
Comment on lines 9 to 21
public protocol Identity {
var expiration: Date? { get }
}

// Identity v. IdentityT v. IdentityKind
// - Identity is the protocol that all identity types must conform to.
// - IdentityT is the associated type / generic type name used by protocols like IdentityResolver and Signer.
// - IdentityKind is the enum that's used by IdentityResolverConfiguration to return correct kind of identity resolver
// for the given auth scheme. E.g., SigV4AuthScheme has idKind field as .aws. And identityResolver method in SigV4AuthScheme
// returns an identity resolver that returns identity of type .aws.
public enum IdentityKind: CaseIterable {
case aws
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying question: Is an expiration all that would be needed to make something an Identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that expiration is the common property in all credentials. Each concrete implementation of this protocol will need to have variable(s) that store the identity data that they use. For example, AWS credentials struct that implements this protocol has variables for access key and secret key. Bearer token struct, when it's implemented, will implement this protocol and add a token variable that stores the token string.

// for the given auth scheme. E.g., SigV4AuthScheme has idKind field as .aws. And identityResolver method in SigV4AuthScheme
// returns an identity resolver that returns identity of type .aws.
public enum IdentityKind: CaseIterable {
case aws
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we expect this list to expand in future?

Copy link
Contributor Author

@sichanyoo sichanyoo Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will expand as we add support for more identity types down the line (e.g., bearer token).

Copy link
Contributor

@dayaffe dayaffe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a few questions

@sichanyoo sichanyoo requested review from dayaffe and syall February 19, 2024 22:33
Sichan Yoo added 9 commits February 21, 2024 14:04
…termine which identity resolver to return for a given auth scheme in default identity resolver config.
…en in smithy-swift side is deleted. This will need to be added back down the line when Andrew refactors smithy-swift client config as well.
…lins sides. Refactor client runtime types for SRA auth for better organization.
… then update codegen and codgen tests accordingly.
@@ -16,5 +16,13 @@ public protocol DefaultHttpClientConfiguration: ClientConfiguration {
/// Configuration for the HTTP client.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add-on to SRA config structure.

…o SignerMiddleware from SigningMiddleware caused it to not be removed properly for generating protocol tests.
Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would like to see the SigV4 special-casing abstracted out into aws-sdk-swift, but LGTM

…ing Steven's feedback on aws-sdk-swift PR.
Copy link
Contributor

@jbelkins jbelkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issues flagged, fix before merging.

switch identityKind {
case .aws:
return self.credentialsProvider.get(key: AttributeKeys.awsIdResolver)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these AWS-specific types be located in smithy-swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue had been resolved with changes made in response to Steven's review earlier. Now schemeID is used as unique ID for retrieving which identity resolver to return.

try await AssertSelectedAuthSchemeMatches(builtContext: context, expectedAuthScheme: "MockAuthSchemeC")
}

func testAuthOderABCNoAuthSelectNoAuth() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check spelling... "testAuthOrder..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for the good catch!

try await AssertSelectedAuthSchemeMatches(builtContext: context, expectedAuthScheme: "smithy.api#noAuth")
}

private func AssertSelectedAuthSchemeMatches(builtContext: HttpContext, expectedAuthScheme: String) async throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When writing a custom test assertion, capture filename & line number then pass them to the XCTest assertion so that the IDE marks up the failed tests appropriately.

Example: https://github.com/smithy-lang/smithy-swift/blob/main/Tests/ClientRuntimeTests/NetworkingTests/ContentLengthMiddlewareTests.swift#L79

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested.

write("let opRegion = context.getRegion()")
write("return ${getSdkId(ctx) + ClientRuntimeTypes.Auth.AuthSchemeResolverParams.name}(operation: opName, region: opRegion)")
} else {
write("return ${getSdkId(ctx) + ClientRuntimeTypes.Auth.AuthSchemeResolverParams.name}(operation: opName)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull the logic out of the string template

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factored out the common logic into a constant.

@sichanyoo sichanyoo merged commit 04c98bf into main Feb 28, 2024
12 checks passed
@sichanyoo sichanyoo deleted the feat/sra-identity-and-auth branch February 28, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants