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

Use Identity instead of Credentials in signing code #2913

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Aug 9, 2023

This PR replaces the access_key, secret_key, and session token fields of signing params with the Orchestrator's Identity type.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@Velfi Velfi requested a review from a team as a code owner August 9, 2023 17:59
@Velfi Velfi added the breaking-change This will require a breaking change label Aug 9, 2023
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@Velfi Velfi requested a review from a team as a code owner August 9, 2023 19:17
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

overall I think this is the right change. Personally, though, I'd keep Credentials on all the private locations and validate very early on that it is indeed credentials inside. We can also have a .credentials method on the signing params builder that accepts credentials—no reason we can't have both credentials and identity.

aws/rust-runtime/aws-sigv4/src/lib.rs Outdated Show resolved Hide resolved
aws/rust-runtime/aws-sigv4/src/lib.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@Velfi Velfi force-pushed the zhessler-refactor-sigv4-identity branch from 9eb3bd8 to f0573aa Compare August 17, 2023 16:49
@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti 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. Just two minor comments.

@@ -52,6 +65,7 @@ enum CanonicalRequestErrorKind {
InvalidHeaderValue { source: InvalidHeaderValue },
InvalidUtf8InHeaderValue { source: Utf8Error },
InvalidUri { source: InvalidUri },
UnsupportedCredentialType,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be called UnsupportedIdentityType

@@ -29,14 +30,11 @@ pub mod event_stream;
pub mod http_request;

/// Parameters to use when signing.
// #[derive(Debug)] is safe because `Identity` handles redaction.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Identity doesn't handle redaction, but the data inside of it is supposed to.

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'll clarify that.

@Velfi Velfi enabled auto-merge August 18, 2023 14:28
@Velfi Velfi added this pull request to the merge queue Aug 18, 2023
@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Merged via the queue into main with commit 39c6476 Aug 18, 2023
@Velfi Velfi deleted the zhessler-refactor-sigv4-identity branch August 18, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants