PHPC-605: Rely on libmongoc for socket handling and TLS #361
Conversation
| } | ||
|
|
||
| if (php_array_existsc(sslOptions, "capath")) { | ||
| ssl_opt.ca_dir = php_array_fetchc_string(sslOptions, "capath", &plen, &free_ca_dir); |
There was a problem hiding this comment.
FYI: When using the Native Darwin or Windows TLS libraries, this will result in an MONGOC_EROR() when used.
That might only be problem if you start falling back on the openssl.capath php.ini settings, but though I'd call it out
There was a problem hiding this comment.
Am I correct in assuming that only different here is customizing the entire CA path? All three will allow a custom CA file, which I suppose most users can stick to if they have a special cert.
Would the default CA path for each generally be sufficient? Since we won't be using PHP's OpenSSL extension at all, I'm on the fence about whether we should even default to openssl.capath.
There was a problem hiding this comment.
That is correct. Most people using their own authority would be providing that cert/bundle as cafile or include it in the OS default chain, and not provide anything.
The only reason we are supporting capath is because why not? -- which then turned into err, we shouldn't have.
| } | ||
|
|
||
| if (php_array_existsc(zoptions, "verify_hostname")) { | ||
| ssl_opt.allow_invalid_hostname = !php_array_fetchc_bool(zoptions, "verify_hostname"); |
There was a problem hiding this comment.
@derickr: HHVM doesn't appear to support this at all.
Also, PHP's own SSL context options don't have anything called verify_hostname, so I propose we just name our option allow_invalid_hostname. We should probably continue to support the undocumented verify_hostname option for BC.
There was a problem hiding this comment.
I can't find any record of verify_hostname being used in the old code or PHP core. It's not a documented SSL context option. Perhaps we should just remove it entirely and stick with allow_invalid_hostname.
There was a problem hiding this comment.
Looks like I was planning on ditching this approach, in favor of custom mortal people readble options instead.
Doesn't look like that materialized, but the accidental NOOP context option remained in the code
|
this lgtm. so much removed code \o/ |
jmikola
changed the title
jmikola
force-pushed
the
phpc-605
branch
2 times, most recently
from
d0d1636 to
f9c9601
Compare
|
@derickr: Ready for a final review. |
jmikola
force-pushed
the
phpc-605
branch
2 times, most recently
from
bf03a0a to
cd501db
Compare
| } | ||
| } | ||
|
|
||
| /* Check canonical option names first and fall back to SSL context options |
There was a problem hiding this comment.
I will need to do the same - are you tests covering all of this?
There was a problem hiding this comment.
I did not bother using both options (canonical and alias) in a single test. All of the tests are just split for using a $driverOptions array with canonical options and a stream context with the BC aliases.
|
Looks mostly good - I had a few comments though. |
This option is not supported by libmongoc's SSL implementation. The test for PHPC-720 can be modified to allow invalid host names.
This test overlaps with the standalone-ssl-no_verify tests, which are already testing SSL connections with no host and cert verification.
Expected output changes now that PHP streams are no longer used for socket IO.
| 'ca_file' => $SSL_DIR . '/ca.pem', | ||
| 'pem_file' => $SSL_DIR . '/client.pem', | ||
| // TODO: this doesn't appear to have any effect. Does the PEM file not have a password? | ||
| 'pem_pwd' => 'qwerty', |
There was a problem hiding this comment.
Forgot to remove this before merging. Will be addressed in #376.
https://jira.mongodb.org/browse/PHPC-605