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: allow scopes for self signed jwt #776

Merged
merged 4 commits into from
Jun 16, 2021
Merged

feat: allow scopes for self signed jwt #776

merged 4 commits into from
Jun 16, 2021

Conversation

arithmetic1728
Copy link
Contributor

@arithmetic1728 arithmetic1728 commented Jun 14, 2021 •
edited
Loading

This doc implements https://google.aip.dev/auth/4111. Internal doc: go/yoshi-self-signed-jwt-phase-2.

The main feature here is now we can use scope claim in self signed JWT.

This PR does the following 2 things:
(1) Add always_use_jwt_access property to service account credentials to allow opt-in for the feature.
(2) If always_use_jwt_access is True, then apply the following logic. If alwaysUseJwtAccess is False, the logic is the same as before; if True, then we can always use self signed jwt with scopes or audience.

if (alwaysUseJwtAccess):
    if (scope):
        // create a self signed JWT with "scope" set to the scope
    else if (audience):
        // create a self signed JWT with "aud" set to the audience
    else if (defaultScope):
        // create a self signed JWT with "scope" set to the defaultScope
else:
    if (scope):
        // call OAuth token endpoint
    else if (audience):
        // create a self signed JWT with audience
    else if (defaultScope):
        // call OAuth token endpoint

This PR has been tested with python-kms, googleapis/python-kms#122

The follow up PR in python microgenerator is: googleapis/gapic-generator-python#920

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 14, 2021
@arithmetic1728 arithmetic1728 mentioned this pull request Jun 15, 2021
Merged
@arithmetic1728 arithmetic1728 marked this pull request as ready for review June 15, 2021 03:48
@arithmetic1728 arithmetic1728 requested a review from a team as a code owner June 15, 2021 03:48
tseaver
tseaver reviewed Jun 15, 2021
View reviewed changes
google/auth/jwt.py Outdated Show resolved Hide resolved
busunkim96
busunkim96 reviewed Jun 15, 2021
View reviewed changes
google/oauth2/service_account.py
Comment on lines +425 to +428
elif audience:
self._jwt_credentials = jwt.Credentials.from_signing_credentials(
self, audience
)
Copy link
Contributor

@busunkim96 busunkim96 Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the way google-api-core is written it will always pass through an audience. https://github.com/googleapis/python-api-core/blob/155da5e18cc2fdcfa57de6f956b7d078e79cd4b7/google/api_core/grpc_helpers.py#L249-L251 and the elif self._default_scopes case will never be reached.

Is it necessary to distinguish between the "default" audience and a user defined audience? It looks like Cody asked a question about this in the doc as well.

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 think it is fine that self._default_scopes is never reached. We can just leave it here for logic completeness.

We don't need to distinguish a default audience and a user defined audience. If the user doesn't provide the scope but provides the audience, then they are responsible that the audience is correct.

arithmetic1728
arithmetic1728 commented Jun 16, 2021
View reviewed changes
google/auth/jwt.py Outdated Show resolved Hide resolved
@arithmetic1728 arithmetic1728 merged commit 2cfe655 into master Jun 16, 2021
@arithmetic1728 arithmetic1728 deleted the sijun_jwt branch June 16, 2021 22:30
@busunkim96 busunkim96 mentioned this pull request Jun 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver left review comments

@busunkim96 busunkim96 busunkim96 approved these changes

@bshaffer bshaffer Awaiting requested review from bshaffer

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
@arithmetic1728 @tseaver @busunkim96