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

Smithy Bearer Auth #3102

Merged
merged 14 commits into from
Sep 10, 2024
Merged

Smithy Bearer Auth #3102

merged 14 commits into from
Sep 10, 2024

Conversation

sbera87
Copy link
Contributor

@sbera87 sbera87 commented Sep 4, 2024

Issue #, if available:

Description of changes:

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sbera87 sbera87 changed the title Bearer Auth [Work in progress] Smithy Bearer Auth Sep 9, 2024

namespace smithy
{
constexpr char BEARER_SIGNER[] = "Bearer";
Copy link
Contributor

Choose a reason for hiding this comment

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

was thinking about this the other day, do we really need this in its own constant or can we just pass the string in the parent ctor i.e.

AuthScheme("Bearer")

we avoid some namespace pollution that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had raised this before (during autha) but we agreed putting this in namespace smithy to comply with what Sergey originally proposed. We do pass it in the base class constructor. I can simply pass the raw string .
Whatever you finalize. I am ok with either

        std::shared_ptr<AwsCredentialIdentityResolverT> identityResolver,
        const Aws::String &serviceName, const Aws::String &region)
        : AuthScheme(BEARER_SIGNER), m_identityResolver{identityResolver},
          m_signer{Aws::MakeShared<smithy::BearerTokenSigner>(
              "BearerTokenAuthScheme", serviceName, region)}

Copy link
Contributor

@sbiscigl sbiscigl Sep 9, 2024

Choose a reason for hiding this comment

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

raw string is better in my opinion thinking about it, one less thing in the namespace that is actually internal to us

};

AuthSchemeOption BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption =
AuthSchemeOption("Bearer");
Copy link
Contributor

Choose a reason for hiding this comment

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

where did you get this string btw? in the smithy docs its smithy.api#HTTPBearerAuth

Copy link
Contributor Author

@sbera87 sbera87 Sep 9, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@sbera87 sbera87 Sep 9, 2024

Choose a reason for hiding this comment

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

Are we moving to a new signer name? Coz, that's not what legacy signer name is . If so, an easy change.

Copy link
Contributor

Choose a reason for hiding this comment

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

why do you think that is the name of the signer? look at what exists for the SigV4AuthSchemeOption
https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-core/include/smithy/identity/auth/built-in/SigV4AuthSchemeOption.h#L15

and SigV4aAuthSchemeOption
https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-core/include/smithy/identity/auth/built-in/SigV4aAuthSchemeOption.h#L15

this is the AuthScheme name, which as in the SRA reference is named as smithy.api#HTTPBearerAuth. You can find the exact same in the scheme id for java for bearer auth
https://github.com/aws/aws-sdk-java-v2/blob/5253ae375004b0f51036657eda6fc539f9cd2035/core/http-auth/src/main/java/software/amazon/awssdk/http/auth/scheme/BearerAuthScheme.java#L36

namespace smithy
{

static const char SSO_DEFAULT_BEARER_TOKEN_PROVIDER_CHAIN_LOG_TAG[] =
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above, maybe we should just inline these to avoid pollution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put it in the class scope to make it better


char BearerTokenAuthSchemeOption::BEARER_SCHEME[] = "smithy.api#HTTPBearerAuth";

AuthSchemeOption BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption =
Copy link
Contributor

Choose a reason for hiding this comment

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

why not inline this like in sigv4 and sigv4a, what makes this auth scheme different where a client needs this symbol and couldnt access it through BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption::schemeId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change that but the main reason for having the variable is usage from other places, so that way we just refer to one constant instead of copies elsewhere. I think we should extend this to the other auths too if you like that

Copy link
Contributor

Choose a reason for hiding this comment

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

we can refer to it other places using BearerTokenAuthSchemeOption::bearerTokenAuthSchemeOption::schemeId without needing the static const char

@sbera87 sbera87 merged commit 5913125 into main Sep 10, 2024
4 checks passed
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.

2 participants