-
Notifications
You must be signed in to change notification settings - Fork 209
PHPC-2613: Test x509 authentication on Atlas #1858
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
Conversation
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.
Pull Request Overview
This PR adds support for testing X.509 certificate authentication on Atlas by introducing new test environments and functionality to handle certificate-based connections. The changes extract base64-encoded certificates from environment variables, write them to temporary files, and modify connection URIs to use the certificate files for authentication.
- Adds X.509 authentication testing with two new Atlas environments (
ATLAS_X509
andATLAS_X509_DEV
) - Refactors connection testing logic into reusable functions
- Implements certificate extraction and temporary file handling for X.509 authentication
918c125
to
0e55a07
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.
Some questions but you can decide what is worth addressing.
tests/atlas.phpt
Outdated
|
||
foreach ($envs as $env) { | ||
echo $env, ': '; | ||
$uri = getenv($env); | ||
$uri = extractUri($env); |
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.
It looks like the only purpose of extractUri()
is to convert false
return value from getenv()
into null
. That seems unnecessary given the conditional below that fails a non-string value.
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.
Good point, I was a little eager with my refactoring. Removed it in favour of getenv
.
chmod($certPath, 0600); | ||
|
||
return [ | ||
'uri' => $uri . '&tlsCertificateKeyFile=' . $certPath, |
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.
Should $certPath
get URL-encoded with rawurlencode()
? I expect tempnam()
will only produce alphanumeric strings, so that may be superfluous.
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.
Yeah, I think it should be safe (famous last words)
tests/atlas.phpt
Outdated
} | ||
|
||
file_put_contents($certPath, $certContents); | ||
chmod($certPath, 0600); |
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.
Is this necessary? tempnam()
should already create the file with these perms.
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.
TIL:
Creates a file with a unique filename, with access permission set to 0600
Removed the chmod
call.
PHPC-2613
This adds two new Atlas environments to the connectivity tests. These test X509 authentication and rely on extracting a certificate string to a file and modifying the connection string accordingly.