-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: add experimental s2a-go integration #1874
Conversation
Hi @xmenxk, Thank you for opening this pull request! Can you please open a feature request issue explaining the motivation for this feature (the benefits for users) and providing some background on the stability of s2a-go and how it will work in the context of this HTTP client? |
thanks @quartzmo for the response. Both touch the transport layer code, as far as I can tell, they are for different use cases. #1886 looks like it's specifically for allowing configuring a http-client to talk to oauth2's mtls endpoint. The s2a integration this PR adds is a way for cloud workloads to talk to all google apis using MTLS, in a transparent way. It only runs when no other MTLS approach is specified, so there should't be any conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting up this PR Kui! Left some general comments. I'm not familiar with the S2A architecture and would love to see some more context around this PR to make sure it is integrated smoothly.
6859717
to
983292e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall - I added some comments mostly around naming and code comments. Thanks! Also, please get one of the Yoshi team members to sign off on the change.
Hi @codyoss, could you take a look at the PR? thanks :) |
I've added @codyoss as a reviewer. I'm on rotation this week but I'd feel better with Cody reviewing this change anyways :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for CBA logic.
Added cody back to reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, couple of small things
internal/cba.go
Outdated
@@ -41,10 +52,12 @@ const ( | |||
mTLSModeAuto = "auto" | |||
) | |||
|
|||
// GetClientCertificateSourceAndEndpoint is a convenience function that invokes | |||
var useS2A = flag.Bool("use_s2a", false, "Experimental: if true, the code will try MTLS with S2A as the default for transport security.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that I think a envvar would be preferable here.
Head branch was pushed to by a user without write access
…port (#3326) Modify the Client Libraries gRPC Channel builder to use mTLS via S2A if the experimental environment variable is set, S2A is available (We check this by using utility added in googleapis/google-auth-library-java#1400), and a few more conditions (see `shouldUseS2A`). Following https://google.aip.dev/auth/4115, Only attempt to use S2A after DirectPath and DCA (https://google.aip.dev/auth/4114) are ruled out as options. If conditions to use S2A are not met (env variable not set, or S2A is not running in environment, etc (`shouldUseS2A` returns false)), fall back to default TLS connection. When we are creating S2A-enabled Grpc Channel Credentials, we first try to secure the connection between the client and the S2A via MTLS, using [MTLS-MDS](https://cloud.google.com/compute/docs/metadata/overview#https-mds) credentials. If MTLS-MDS credentials can't be loaded, then we fallback to a plaintext connection between the client and S2A. The parallel go implementation : googleapis/google-api-go-client#1874 (now lives here: https://github.com/googleapis/google-cloud-go/blob/main/auth/internal/transport/cba.go) S2A Java client: https://github.com/grpc/grpc-java/tree/master/s2a Resolving b/376258193 means that S2A.java is no longer experimental --------- Co-authored-by: blakeli <[email protected]>
s2a integration with grpc and http transport code.
With this change, client library will try establishing mtls with the google api's mtls endpoint, using credentials held by s2a.
The added code only changes the default behavior in a way that's transparent to customers; if other mtls approach is specified, e.g. DCA, this code will not run.