-
Notifications
You must be signed in to change notification settings - Fork 193
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 loading and executing of default client cert source #353
Conversation
src/CredentialsLoader.php
Outdated
if (!$clientCertSourceJson = self::loadDefaultClientCertSourceFile()) { | ||
return null; | ||
} | ||
if (!isset($clientCertSourceJson['cert_provider_command'])) { |
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.
WDYT about putting this validation in the body of loadDefaultClientCertSourceFile
?
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.
done
{ | ||
$clientCertSource = CredentialsLoader::getDefaultClientCertSource(); | ||
if (is_null($clientCertSource)) { | ||
$this->markTestSkipped('No client cert source found'); |
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.
What are the cases where this wouldn't be found? Should we stub it out?
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.
The client cert source is only found when the corresponding file exists (~/.secureConnect/context_aware_metadata.json
). This is tested in testNonExistantDefaultClientCertSource
!
/** | ||
* @runInSeparateProcess | ||
*/ | ||
public function testDefaultClientCertSourceInvalidCmdThrowsException() |
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.
Not sure if at some point the original intent was to throw an exception, but I do like that approach. If we keep as is, we should update the method here.
public function testDefaultClientCertSourceInvalidCmdThrowsException() | |
public function testDefaultClientCertSourceInvalidCmdReturnsNull() |
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 throwing an exception is better. I can't remember if this was to ensure we don't break customers, or to be consistent with other languages, but I will update it to throw an exception as that's a clearer UX.
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.
Updated to throw an exception if the command errors
Co-authored-by: David Supplee <[email protected]>
Co-authored-by: David Supplee <[email protected]>
Co-authored-by: David Supplee <[email protected]>
- Adds client option `clientCertSource` - Adds support for client certs to all three transports - Verified calls to mTLS endpoint works locally **Usage** ```php use Google\Cloud\PubSub\V1\PublisherClient; $projectId = 'YOUR_PROJECT_ID'; $pubsub = new PublisherClient([ 'apiEndpoint' => 'pubsub.mtls.googleapis.com', 'clientCertSource' => function () { return file_get_contents(__DIR__ . '/cert.pem'); }, ]); $topics = $pubsub->listTopics('projects/' . $projectId); ``` **TODO** - [x] Add support for checking the existence and loading the default client cert (from `~/.secureConnect/context_aware_metadata.json`) (done in googleapis/google-auth-library-php#353) - [x] Add support for `GOOGLE_API_USE_MTLS_ENDPOINT` and `GOOGLE_API_USE_CLIENT_CERTIFICATE` - [x] Add support to automatically use mTLS endpoint for `apiEndpoint` - [x] Tests
Adds logic for detecting and loading the default client certificates for mTLS
Implementation doc: https://google.aip.dev/auth/4114
Related: googleapis/gax-php#325