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: service account is able to use a private token endpoint #784

Merged
merged 13 commits into from Jul 14, 2021
Merged

feat: service account is able to use a private token endpoint #784

merged 13 commits into from Jul 14, 2021

Conversation

liuchaoren
Copy link
Contributor

In Private Service Connect, users can use an endpoint which is private to their VPC network. The request is eventually routed to the oauth2.googleapis.com/token so the "aud" in the assertion still should be oauth2.googleapis.com/token.

After this change, service account can send requests to the private endpoint (if configured) and still use the oauth2.googleapis.com/token in the assertion.

@liuchaoren liuchaoren requested a review from a team as a code owner June 25, 2021 17:40
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2021
@liuchaoren liuchaoren changed the title Service account can use a private token URI feat: service account is able to use a private token endpoint Jun 25, 2021
@busunkim96
Copy link
Contributor

@silvolu @arithmetic1728 PTAL. The corresponding bug is 190841374.

@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 25, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 25, 2021
@parthea parthea added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jun 25, 2021
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 25, 2021
@silvolu
Copy link

silvolu commented Jun 28, 2021

Please wait for me on this.

@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 30, 2021
@tseaver
Copy link
Contributor

tseaver commented Jun 30, 2021

Tagged as do not merge per request of @silvolu.

@silvolu
Copy link

silvolu commented Jul 2, 2021

The fix should be simpler here: when exchanging a JWT for an access token, the value of the 'aud' field must be https://oauth2.googleapis.com/token, so in _make_authorization_grant_assertion the value should be read from a constant or hardcoded string instead of set to the token uri.

@arithmetic1728
Copy link
Contributor

arithmetic1728 commented Jul 2, 2021

The fix should be simpler here: when exchanging a JWT for an access token, the value of the 'aud' field must be https://oauth2.googleapis.com/token, so in _make_authorization_grant_assertion the value should be read from a constant or hardcoded string instead of set to the token uri.

@silvolu The token uri is not hardcoded because

(1) google-auth allows users to provide their own token uri (even though it should be always https://oauth2.googleapis.com/token for googleapis)

def __init__(
self,
signer,
service_account_email,
token_uri,

(2) if the creds is created from service_account.json file, then google-auth reads token uri from the json file

info, signer = _service_account_info.from_filename(
filename, require=["client_email", "token_uri"]
)
return cls._from_signer_and_info(signer, info, **kwargs)

@silvolu
Copy link

silvolu commented Jul 2, 2021

I'm not saying that the token_uri should be hardcoded, I'm saying that the audience field should be hardcoded. This class is not a generic implementation, the description clearly says "This module implements the JWT Profile for OAuth 2.0 Authorization Grants as defined by RFC 7523_ with particular support for how this RFC is implemented in Google's infrastructure."

In Google's infrastructure, when exchanging a JWT for a token, the audience is always https://oauth2.googleapis.com/token. Once that is done, you can actually use different token uris supported by the google infrastructure, e.g. a Private Secure Connect endpoint, because with the audience set to the token_uri, the audience check at the token exchange endpoint fails.

@arithmetic1728
Copy link
Contributor

I'm not saying that the token_uri should be hardcoded, I'm saying that the audience field should be hardcoded. This class is not a generic implementation, the description clearly says "This module implements the JWT Profile for OAuth 2.0 Authorization Grants as defined by RFC 7523_ with particular support for how this RFC is implemented in Google's infrastructure."

In Google's infrastructure, when exchanging a JWT for a token, the audience is always https://oauth2.googleapis.com/token. Once that is done, you can actually use different token uris supported by the google infrastructure, e.g. a Private Secure Connect endpoint, because with the audience set to the token_uri, the audience check at the token exchange endpoint fails.

Sorry I misunderstood your comment. Yea, this way is much simpler.

@liuchaoren
Copy link
Contributor Author

I updated the PR to hard-code the aud which is much simpler. Please take another look.

@arithmetic1728
Copy link
Contributor

looks good to me

@liuchaoren
Copy link
Contributor Author

Hi @tseaver, is it okay to remove the "do not merge" tag given that @silvolu approved it? I confirmed with @silvolu yesterday.

@busunkim96 busunkim96 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 13, 2021
@busunkim96
Copy link
Contributor

@liuchaoren Yes I believe Tres added that to prevent an early merge. I've removed the label.

@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 13, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 13, 2021
@busunkim96 busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 13, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2021
@busunkim96
Copy link
Contributor

@liuchaoren It looks like some unit tests are failing, could you take a look?

To run the unit tests locally, pip install nox and run one of the unit test sessions: nox -s unit-3.6

@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 14, 2021
@busunkim96 busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 14, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 0e26409 into googleapis:master Jul 14, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 14, 2021
arithmetic1728 added a commit that referenced this pull request Jul 20, 2021
busunkim96 pushed a commit that referenced this pull request Jul 20, 2021
…endpoint (#784)" (#808)

revert "feat: service account is able to use a private token endpoint (#784)" until b/194191737 is fixed.

This reverts commit 0e26409.
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. kokoro:run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants