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: generate a default audience for clients #475

Merged
merged 3 commits into from
Dec 3, 2020
Merged

feat: generate a default audience for clients #475

merged 3 commits into from
Dec 3, 2020

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Nov 3, 2020

This option will be used to generate self-signed JWTs when the
client is authenticated with a service account and the user does
not provide additional scopes.

Also, now marking the generated scopes as default so we can
distinguish between generated and user provided scopes.

Related: googleapis/google-api-go-client#738

This option will be used to generate self-signed JWTs when the
client is authenticated with a service account and the user does
not provide additional scopes.

Also, now marking the generated scopes as default so we can
distinguish between generated and user provided scopes.

Related: googleapis/google-api-go-client#738
@codyoss codyoss requested review from a team as code owners November 3, 2020 21:01
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 3, 2020
@codyoss codyoss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 3, 2020
@codyoss
Copy link
Member Author

codyoss commented Nov 3, 2020

Before this PR is merged:

  1. The associated PR needs to be merged.
  2. A new version of google-api-go-client needs to be released with those changes
  3. google-cloud-go needs to be updated to depend on the new version.

@codyoss
Copy link
Member Author

codyoss commented Nov 3, 2020

cc @bshaffer

Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

No comment on the JWT semantics, but the Go looks fine. Some minor, non-blocking suggestions.

internal/gengapic/client_init.go Show resolved Hide resolved

// generateDefaultAudience transforms a host into a an audience that can be used
// as the `aud` claim in a JWT.
func generateDefaultAudience(host string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed that the host is a proper host (ie doesn't have a path)? I'd be inclined to defensively remove it (and include that in tests).

Copy link
Member Author

Choose a reason for hiding this comment

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

Guaranteed, no. But it does not happen today I can fairly certainly say. On L63 of this file we append :443 to the the end of the host. If there were path fragments this should be an invalid host and the client would not be able to talk to the backend.

internal/gengapic/client_init.go Outdated Show resolved Hide resolved
internal/gengapic/client_init_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@codyoss codyoss removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 3, 2020
@codyoss codyoss merged commit ed195cc into googleapis:master Dec 3, 2020
@codyoss codyoss deleted the default-aud branch December 3, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants