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

internal: Refactor cert logic to support OAuth2 token exchange over mTLS #1886

Merged
merged 15 commits into from Mar 9, 2023

Conversation

andyrzhao
Copy link
Contributor

@andyrzhao andyrzhao commented Mar 3, 2023

With Context Aware Access enabled, users must use the endpoint "https://oauth2.mtls.googleapis.com/token" for token exchange. This PR refactors the cert logic currently used by the transport layer to be reused by the internal credentials layer in order to inject an mTLS-enabled HTTPClient (via the "context" mechanism) for use by the OAuth2 transport (along with the mTLS OAuth2 endpoint if so).

@andyrzhao andyrzhao requested review from a team and yoshi-approver as code owners March 3, 2023 19:45
@quartzmo
Copy link
Member

quartzmo commented Mar 4, 2023

@andyrzhao Is this PR related to #1874 in any way?

@andyrzhao
Copy link
Contributor Author

@andyrzhao Is this PR related to #1874 in any way?

Hi Chris, this PR is not related to #1874 and I'm not familiar with the details of s2a PR - I will have to take a look at that one more closely later to see how it fits into the existing google api transport layer and enterprise certificate proxy stack.

In the meantime, I've shared you an internal doc for the motivation of this PR 1886 - TLDR: we need this patch to unblock a CAA policy rollout which will effectively disable the oauth2 non-mtls token exchange endpoint for users under the policy restriction.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

This PR moves files that contain publicly exposed functions. This is a breaking change.

@andyrzhao
Copy link
Contributor Author

This PR moves files that contain publicly exposed functions. This is a breaking change.

That's correct, it moves transport/cert to internal. Do you have a recommendation on how to proceed? Should we add wrapper files to re-expose the affected functions? Technically, the package comment says "This package is not intended for use by end developers. Use the
// google.golang.org/api/option package to configure API clients." so would it be okay to make a breaking change in this case?

@shinfan
Copy link
Contributor

shinfan commented Mar 6, 2023

@codyoss Can we have some exception for "breaking changes" here? Technically these files are very specific to mTLS use cases and not being used by anyone at this point so the user impact should be negligible.

@codyoss
Copy link
Member

codyoss commented Mar 7, 2023

I will take a closer look at this package today and get back to you

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

After more consideration and looking at our docs I agree, I think this is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants