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.

Sign up for GitHub

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

Jump to bottom

feat: implements AWS signature version 4 for signing requests #476

Merged
merged 12 commits into from
Sep 30, 2020
Merged

feat: implements AWS signature version 4 for signing requests #476

merged 12 commits into from
Sep 30, 2020

Conversation

lsirac
Copy link
Contributor

@lsirac lsirac commented Sep 17, 2020

@lsirac lsirac requested a review from a team as a code owner September 17, 2020 00:33
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 17, 2020
elharo
elharo reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

See https://twitter.com/elharo/status/976612920565125124 about constants

I need to take a closer look at the exceptions thrown here.

oauth2_http/java/com/google/auth/oauth2/AwsRequestSignature.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSignature.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSignature.java Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSignature.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/AwsRequestSignerTest.java Outdated Show resolved Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/AwsRequestSignerTest.java Outdated Show resolved Hide resolved
elharo
elharo suggested changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Don't use runtime exceptions for any probles that comes from outside the program such as a malformed HTTP header

oauth2_http/java/com/google/auth/oauth2/AwsRequestSignature.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
elharo
elharo suggested changes Sep 19, 2020
View reviewed changes
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated
import static com.google.api.client.util.Preconditions.checkNotNull;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.api.client.util.Clock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone know why we use these classes instead of the better maintained versions in Java?

oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated Show resolved Hide resolved
chingor13
chingor13 requested changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

I think @elharo is suggesting keeping a Date instance rather than a collection of pre-formatted string values. Then formatting them on the way out when necessary.

oauth2_http/java/com/google/auth/oauth2/AwsRequestSigner.java Outdated
private AwsDates(String originalDate, String amzDate, String formattedDate) {
this.originalDate = checkNotNull(originalDate);
this.amzDate = checkNotNull(amzDate);
this.formattedDate = checkNotNull(formattedDate);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this value is always computed from the amzDate argument which puts burden on the caller to always do the substring(0,8) to take the YYYMMDD part from amzDate. Intentional for some use-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think having a Date instance would then burden the callers to convert the input (always a String) to a date, only to be converted back anyway.

I cleaned it up a little - you're right, there's no need to hold onto the formattedDate since it is always derived from the amzDate.

chingor13
chingor13 approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

The classes are all package private and we can clean up in the final PR if necessary.

@chingor13 chingor13 dismissed elharo’s stale review September 30, 2020 20:48

Review comments resolved, we can revisit in the final PR.

@chingor13 chingor13 merged commit 4ee1a14 into googleapis:3pi Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo left review comments

@chingor13 chingor13 chingor13 approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lsirac @chingor13 @elharo