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

Quick attempt at resolving AWS signing problems #1596

Draft
wants to merge 1 commit into
base: series/0.18
Choose a base branch
from

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Sep 27, 2024

This is more of a random attempt rather than a methodically applied implementation, but early testing has proven it to be successful.

Potentially closes #1568, #1532 (works on my machine but needs investigation into whether this is actually a more correct implementation). It would be amazing if there are protocol tests for this...

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@kubukoz kubukoz changed the title Quick attempt at resolving AWS signing problems (#1568, #1532) Quick attempt at resolving AWS signing problems Sep 27, 2024
val endpointPrefix = awsService.endpointPrefix.getOrElse(endpoint.id.name)
val endpointPrefix =
awsService.endpointPrefix
.orElse(awsService.arnNamespace.map(_.value))
Copy link
Member Author

Choose a reason for hiding this comment

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

question: should this be sigv4 rather than arnNamespace?

should we even be looking at endpointPrefix? sigv4 is supposed to match arnNamespace but smithy says it's a SHOULD (see documentation trait of the sigv4 trait members).

For what it's worth, both Sagemaker and Location work if sigv4 is used instead, in both locations of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

we should eventually just go through all services on https://docs.aws.amazon.com/general/latest/gr/aws-service-information.html one-by-one and validate that we compute the right urls...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a bit misleading because of the endpointPrefix value name, but really we should be using the sigv4 value here.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok I'll change to sigv4 then, and maybe run through the services to see if it's present on all of them (and what the fallback should be).

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.

Sagemaker: invalid signature (Credential should be scoped to correct service)
2 participants