Support SSL_CERT_DIR for specifying non-default CA certificate bundles.

[bukzor]

b...@yelp.com commented:

Which version of PhantomJS are you using? Tip: run 'phantomjs --version'.
1.7.0

What steps will reproduce the problem?

1. Set up an https server with self-signed certificate, saved as /tmp/certs/mycert.pem
2. Run c_rehash /tmp/certs/ to create the symlinks that openssl expects.
3. Run SSL_CERT_DIR=/tmp/certs phantomjs repro.js (see attached)

What is the expected output?
success

What do you see instead?
fail

Which operating system are you using?
Linux 2.6.32-41-server #89-Ubuntu SMP Fri Apr 27 22:33:31 UTC 2012 x86_64 GNU/Linux

Did you use binary PhantomJS or did you compile it from source?
source

Please provide any additional information below.

Currently phantomjs can only open https pages (without error) which have been signed by a system-installed Certificate Authority (CA).

It would be useful to allow phantom to optionally read additional CA's from a non-privileged location (such as $HOME/.ssl/CA/, for example).

Various similar systems (curl, lynx) solve this problem by implementing an SSL_CERT_DIR environment variable, which is a colon-separated list of directories of .pem files (and their c_rehash symlinks) which will be trusted as CA's.

Here's curl doing it right:

$ SSL_CERT_DIR=/tmp/certs strace curl https://example.com/login |& grep certs
stat("/tmp/certs/e72c81.0", {st_mode=S_IFREG|0444, st_size=2264, ...}) = 0
open("/tmp/certs/e72c81.0", O_RDONLY) = 4
stat("/tmp/certs/e72c81.1", 0x7ff4f00) = -1 ENOENT (No such file or directory)

Here's w3m doing it right:

$ SSL_CERT_DIR=/tmp/certs strace w3m https://example.com/login |& grep certs
stat("/etc/ssl/certs/e72c81.0", 0x7fff6844d050) = -1 ENOENT (No such file or directory)
stat("/tmp/certs/e72c81.0", {st_mode=S_IFREG|0444, st_size=2264, ...}) = 0
open("/tmp/certs/e72c81.0", O_RDONLY) = 4
stat("/tmp/certs/e72c81.1", 0x7fff6844d050) = -1 ENOENT (No such file or directory)

Here's wget doing it right:

$ SSL_CERT_DIR=/tmp/certs strace wget https://example.com/login |& grep certs
stat("/tmp/certs/e72c81.0", {st_mode=S_IFREG|0444, st_size=2264, ...}) = 0
open("/tmp/certs/e72c81.0", O_RDONLY) = 4
stat("/tmp/certs/e72c81.1", 0x7fff936b61b0) = -1 ENOENT (No such file or directory)

Here's phantomjs doing it wrong:

$ SSL_CERT_DIR=/tmp/certs strace phantomjs repro.js |& grep certs
open("/etc/ssl/certs/", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 9
statfs("/etc/ssl/certs/", {...}) = 0
stat("/etc/ssl/certs/11f154d6.0", {st_mode=S_IFREG|0644, st_size=1484, ...}) = 0

Disclaimer:
This issue was migrated on 2013-03-15 from the project's former issue tracker on Google Code, Issue #916.
🌟   3 people had starred this issue at the time of migration.

[bukzor]

b...@yelp.com commented:

I've done some code archaeology on this topic, and the issue seems to lie in Qt. Before 2010, Qt exclusively used certificates bundled with Qt itself. In 2010, they added support to use system certificates, but they're all hard-coded paths; there's no support for $SSL_CERT_DIR. It could be rectified with a quite simple patch.

Do you all agree with that assessment / implementation plan?

This is the commit I'm referring to. The current version is essentially the same.
http://qt.gitorious.org/qt/qt/commit/9a982779eabc4fafafe18dc9ad1b44fb2425563c/diffs

We could also fix it with a patch to the vendored Qt, and plan to ship it upstream later.

Please advise.

[ariya]

ariya.hi...@gmail.com commented:

I think it is wise to see what upstream Qt developers think about this. Certificates handling is always tricky and it is better to do it right than to rush it.

 
Metadata Updates

• Label(s) removed:
  • Type-Defect
• Label(s) added:
  • Type-Enhancement
  • Component-Logic
  • Domain-Qt
• Milestone updated: FutureRelease (was: ---)
• Status updated: Accepted

[bukzor]

b...@yelp.com commented:

ariya: To be clear, you want this bug pushed to the upstream Qt ticket tracker?

We now have a patch for this: #370
The change is three lines, and exactly duplicates the behavior of curl, w3m, wget (demoed above) and many other tools.

Do you want to see the patch accepted into Qt before accepting it here?

[ariya]

ariya.hi...@gmail.com commented:

Yes, I think we need to ask upstream Qt developers for their opinions. I don't argue about the behavior correctness, I just want to make sure we don't overlook something on the Qt side.

[ariya]

jwilk...@polyclef.net commented:

https://bugreports.qt-project.org/browse/QTBUG-28651

[ariya]

ariya.hi...@gmail.com commented:

I wonder if using QSslConfiguration (instead of patching Qt network code) would work. I think it's a matter of loading the additional certificates and add them to the default configuration. I'm not an SSL expert so this may not work as intended.

Related API:

http://doc.qt.digia.com/qt/qsslconfiguration.html#defaultConfiguration
http://doc.qt.digia.com/qt/qsslconfiguration.html#caCertificates
http://doc.qt.digia.com/qt/qsslconfiguration.html#setCaCertificates
http://doc.qt.digia.com/qt/qsslcertificate.html#fromPath

[vitallium]

vitaliy....@gmail.com commented:

SSL_CERT_DIR is not a environment variable which is definitely set. It's like an additional path where certificates can be loaded from.

I think the better solution would be adding all certificates by manually. Then loading certificates will be looks like:

1. Qt loads the certificates from system certificate store
2. PhantomJS loads additional (user) certificates found in SSL_CERT_DIR environment variable.

[bukzor]

b...@yelp.com commented:

Oh I see. You believe that the environment handling should be done at the app layer (rather than by Qt).

I disagree, since this is standard openssl behavior, but if that's your official decision as a group, that's what we'll do.

[bukzor]

b...@yelp.com commented:

Let's wait to see what the Qt guys say, then the phantomjs decision should be obvious.

[ariya]

ariya.hi...@gmail.com commented:

I think the upstream suggestion is pretty clear: use QSslConfiguration (hence my proposed idea in comment #8).

If it is possible, I prefer a solution which does not require the network stack modification. If the idea with QSslConfiguration, this can work easily and it is even possible to extend it to a command-line option, page configuration, and so on.

[JamesMGreene]

I believe this can be closed with the workaround provided by PR #411 (which is now merged).

@bukzor Are you good with that, or do you think there is more to address?

[bukzor]

Good enough.
On Mar 20, 2013 10:39 AM, "James M. Greene" notifications@github.com
wrote:

I believe this can be closed with the workaround provided by PR #411#411 is now merged).

@bukzor https://github.com/bukzor Are you good with that, or do you
think there is more to address?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/10916#issuecomment-15191267
.