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 i&a: implement auth resolution workflow in middleware #462

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

lucix-aws
Copy link
Contributor

@lucix-aws lucix-aws commented Oct 16, 2023

main changes:

  • Add authentication middlewares:
    • resolveAuthScheme
    • resolveIdentity
    • signRequestMiddleware
    • NOTE: I haven't reordered the phasing for these at this time, auth resolution currently happens before endpoint resolution (so it's still in serialize unlike how we spec'd to move it to finalize). The operation input isn't available in FinalizeInput, we can add it but I'm leaving it as something to explore externally. For the most part I think what's important is that identity resolution is part of finalize such that retries handle it.
  • Persist resolved auth scheme and identity on operation context.
  • Merge resolved endpoint signer properties into resolved scheme signer properties.
  • Add auth schemes and auth scheme resolver to config
  • Extend AuthSchemeDefinition to expose codegen to plug in IdentityResolverOptions expressions and AuthScheme expressions.

misc:

  • Remove operationName interface on input structs. Operation name is now bound directly on middleware add.
  • Correct typing for property getter keys. They have to be structs instead of vars of anonymous empty structs.
  • Remove HttpBearerAuth integration (moved to SDK), the generated code is not compatible with I&A as it bakes the signer into the client config.
  • Remove "default" auth integrations for now, overriding the integration SDK-side isn't something I want to handle at this time.
  • Refactor endpoint resolution property codegen, returned endpoint properties are now strongly typed and set accordingly.

@lucix-aws lucix-aws requested review from a team as code owners October 16, 2023 21:57
@lucix-aws lucix-aws force-pushed the feat-sraauth-middleware branch from c49b305 to 208fee8 Compare October 16, 2023 21:58
@lucix-aws lucix-aws changed the title Feat sraauth middleware feat: sra i&a: implement auth resolution workflow in middleware Oct 16, 2023
@lucix-aws lucix-aws marked this pull request as draft October 16, 2023 21:59
@lucix-aws lucix-aws force-pushed the feat-sraauth-middleware branch from 208fee8 to 15a7ba2 Compare October 17, 2023 15:15
@lucix-aws lucix-aws force-pushed the feat-sraauth-middleware branch from 15a7ba2 to fcb1d8f Compare October 17, 2023 16:32
@lucix-aws lucix-aws marked this pull request as ready for review October 17, 2023 16:46
@@ -998,6 +999,18 @@ public ChainWritable() {
writables = new ArrayList<>();
}

public static ChainWritable of(GoWriter.Writable... writables) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -294,6 +316,71 @@ private void generateApplicationProtocolTypes() {
}).write("");
}

private void writeProtocolResolvers() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: writeProtocolResolvers and generateProtocolResolvers sound like the same thing. it looks like the former is more writing/generating the invocations and the latter is doing the definitions, so maybe work those intentions into the name of the functions

@@ -2,7 +2,6 @@ package http

import (
"context"
"net/http"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, i wonder if it was just an oversight that we werent using the smithy request object already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this is the new signer type from a previous PR, but yes I felt that it should use the smithy transport type instead of the underlying one.


public static GoWriter.Writable generateAddMiddleware() {
return goTemplate("""
err = stack.Finalize.Add(&$L{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure about this, but I don't think this middleware is needed.

In generateSelectScheme() of the auth scheme middleware, identity resolvers are already invoked to see if it's valid or not for choosing an auth scheme, so I think you can just attach that resolved identity to the resolvedAuthScheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check that the resolver exists there at that time, but we don't actually get the identity until this middleware (hence getIdentityMiddleware).

We could definitely save the resolver on the first call as you've said though it's really only a string lookup.

@lucix-aws lucix-aws merged commit 5a607b0 into feat-sraauth Oct 19, 2023
1 check passed
@lucix-aws lucix-aws deleted the feat-sraauth-middleware branch October 19, 2023 16:02
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.

3 participants