feat(transport): Integrate with enterprise certificate proxy #1570
Conversation
merge from upstream
|
@codyoss Could you please help review this change? Thanks! |
| defaultCert.source, defaultCert.err = NewEnterpriseCertificateProxySource("") | ||
| if errors.Is(defaultCert.err, errSourceUnavailable) { | ||
| defaultCert.source, defaultCert.err = NewSecureConnectSource("") | ||
| if errors.Is(defaultCert.err, errSourceUnavailable) { |
There was a problem hiding this comment.
I don' think this case3 is needed. In the case that err is non-nil and a source can't be detected we should just return the error.
There was a problem hiding this comment.
So the reason why we return "nil, nil" when DefaultSource is unavailable is because the eventual caller of DefaultSource will take a different action when we don't have a certificate (i.e. it will not use mTLS path) - we don't want to error out in this scenario (in other words, nil Source is an acceptable return value). The other possibility to support this semantics is to export "errSourceUnavailable", so the caller can condition on that, but not sure if exporting custom errors is good practice, and it would also technically be non-backwards-compatible. WDYT?
There was a problem hiding this comment.
You are right it would be a behavior change, but until this commit I don't think we documented that a nil error could be returned. It is definitely a grey area as far as breaking changes are concerned. But after thinking more I agree with you that although the current semantics are not ideal, I think it is best to keep them the same to avoid potential breakages.
Enterprise Certificate Proxy (https://github.com/googleapis/enterprise-certificate-proxy) is the new preferred solution for certificate based access on the client-side, since it integrates directly with the native OS keystores and does not expose the underlying private key. This new solution leverages the ECP client to replace the existing SecureConnect cert provider code-path for devices with the new ECP configuration file and helper binaries installed. (The ECP configuration file and helper binaries will eventually be distributed and managed through gCloud SDK).
High-level change-list summary:
Note that this change is safe and backwards compatible, since the ECP configuration file will not be present on devices without ECP binaries installed, in which case the DefaultSource will fall back to using SecureConnect. Furthermore, both the new ECP code-path and the old SecureConnect code-path are gated behind the env-var GOOGLE_API_USE_CLIENT_CERTIFICATE today.