Added support for ssl client certificates #3206

Merged
merged 9 commits into from Jan 5, 2017

Projects

None yet

3 participants

@jasonparallel
Contributor

This adds support for using client certificates for https connections to servers. The private key and public key should be in PEM format. The path to the certificate files or the PEM encoded certificate can be passed as parameters along with the password for the private key. Optionally the client cert public key file can contain the PEM encoded cert public keys for it's certificate chain appended to the end of the file.

This is a fix for issue #3090

@ashkulz
Member
ashkulz commented Nov 23, 2016 edited

Although the changes look good, I'm not in favor of adding so many options for something which is likely to be used by <1% of the users (maybe drop the string options to always use path?). Also, I don't think changes have been made in the right place -- there is no implementation for wkhtmltoimage.

@jasonparallel
Contributor

Thanks for taking a look @ashkulz. I'll remove the string option. I initially had the ssl configuration code in multipageloader.cc but it was being called multiple times when there was multiple http(s) request for an operation. So I moved it to pdfconverter.cc so it would only run once. What would you recommend so the code will be shared between the pdf and the image libraries?

@ashkulz
Member
ashkulz commented Nov 26, 2016

I'd remove all the fprintf calls. The correct place is in MyNetworkAccessManager::createRequest, where you should use QNetworkRequest::setSslConfiguration instead of QSslConfiguration::setDefaultConfiguration, which will not be correct (if using the API, it will change the default configuration, so will apply even for other requests until reverted).

@sid88in
sid88in commented Nov 29, 2016 edited

@jasonparallel @ashkulz I am using wkhtmltopdf for rendering tableau sheets in a pdf. But the tableau server has SSL enabled. The following command generates the pdf but with SSL warnings (and the page shows a dialogue box with missing js files)

screen shot 2016-11-27 at 7 05 17 pm

wkhtmltopdf --javascript-delay 60000 https://server/trusted/ticketnumber/views/WBName/ViewName

Looks like this PR might resolve this issue?

P.S: I don't see this error dialogue box when the SSL is disabled on the server.

@jasonparallel
Contributor

Thanks @ashkulz! I'm on it.

@sid88in Seems possible. Is the server set to require client certificates? Do you see that error when using a browser that does not have the client certificate loaded?

@sid88in
sid88in commented Dec 2, 2016

@jasonparallel thats for getting back! Yes the tableau server has SSL enabled. I get the same error when I hit the link using QTweb (with no certs).

@sid88in
sid88in commented Dec 2, 2016

I think you PR might help me to test wkhtmltopdf by providing server certs by the client like:

wkhtmltopdf --cert X.cert --key Y.crt --javascript-delay 60000 https://server/trusted/ticketnumber/views/WBName/ViewName

What do you think? the problem is I don't know how to compile this code and make it run :) Right now I am using wkhtmltopdf package in nodejs

@jasonparallel
Contributor

@sid88in Seems like it would be worth trying. I'll shoot you a link to my linux test build of the executable once I test the updates. the syntax will be wkhtmltopdf --ssl-crt-path /path/X.cert --ssl-key-path /path/Y.key --ssl-key-password password --javascript-delay 60000 https://server/trusted/ticketnumber/views/WBName/ViewName

Also take note that QT was giving me some issues when I tried to run it on a system with openssl 1.0.1. (QT is curretnly compiled against 1.0.2)

@jasonparallel jasonparallel Removed key and cert string params & moved client ssl code
Removed key and cert string params. Moved client ssl code to
MyNetworkAccessManager::createRequest
b138dfd
@ashkulz

Hello, looks good to me but you need to make a few changes. Keep the same formatting as present in earlier code (tabs/spaces/whatever) so that the diff looks okay.

src/lib/multipageloader.cc
@@ -104,6 +109,35 @@ QNetworkReply * MyNetworkAccessManager::createRequest(Operation op, const QNetwo
foreach (const HT & j, settings.customHeaders)
r3.setRawHeader(j.first.toLatin1(), j.second.toLatin1());
}
+
+ if(settings.clientSslKeyPath != NULL && settings.clientSslKeyPassword != NULL
@ashkulz
ashkulz Dec 2, 2016 Member

Use !settings.clientSslKeyPath.isEmpty().

src/lib/multipageloader.cc
+
+ //key not supplied as string use path
+ QFile keyFile(settings.clientSslKeyPath);
+ success = keyFile.open(QFile::ReadOnly);
@ashkulz
ashkulz Dec 2, 2016 Member

Use if (keyFile.open(QFile::ReadOnly) { so that success is not required, and remove the comment (doesn't make sense) or change it.

src/lib/multipageloader.cc
+ sslConfig.setPrivateKey(key);
+ keyFile.close();
+
+ if(success){
@ashkulz
ashkulz Dec 2, 2016 Member

Remove this

src/lib/multipageloader.cc
+ sslConfig.setLocalCertificate(chainCerts.first());
+ sslConfig.setCaCertificates(cas);
+
+ if(success){
@ashkulz
ashkulz Dec 2, 2016 Member

Remove this too and make it unconditional

include/wkhtmltox/loadsettings.hh
@@ -49,7 +49,7 @@ struct DLL_PUBLIC PostItem {
struct DLL_PUBLIC LoadGlobal {
LoadGlobal();
//! Path of the cookie jar file
- QString cookieJar;
+ QString cookieJar;
@ashkulz
ashkulz Dec 2, 2016 Member

Keep previous formatting.

AUTHORS
@@ -38,3 +38,4 @@ Mehdi Abbad
Lyes Amazouz
Pascal Bach
Mário Silva
+JasonParallel
@ashkulz
ashkulz Dec 2, 2016 Member

Add your full name and email address

src/lib/loadsettings.hh
@@ -52,7 +52,7 @@ struct DLL_PUBLIC PostItem {
struct DLL_PUBLIC LoadGlobal {
LoadGlobal();
//! Path of the cookie jar file
- QString cookieJar;
+ QString cookieJar;
src/lib/pdfconverter.cc
@@ -131,6 +131,8 @@ PdfConverterPrivate::PdfConverterPrivate(PdfGlobal & s, PdfConverter & o) :
int height = viewportSizeList.last().toInt();
viewportSize = QSize(width,height);
}
+
@ashkulz
ashkulz Dec 2, 2016 Member

Remove whitespace changes

jasonparallel added some commits Dec 9, 2016
@jasonparallel jasonparallel Code Review Updates
Code Review Updates
8064b08
@jasonparallel jasonparallel Code Review Updates
Code Review Updates
9152ab2
@jasonparallel jasonparallel Code Review Formatting Correction
Code Review Formatting Correction
b6646f1
@jasonparallel jasonparallel Code Review Formating Update
Code Review Formating Update
cd91ee1
@jasonparallel jasonparallel Updating contact info
Updating contact info
385c5e0
@jasonparallel
Contributor

Thanks for taking a look @ashkulz . Sorry for all that ugly space formatting that was in there. All of your comments should be addressed in the latest.

@ashkulz
Member
ashkulz commented Dec 10, 2016

Thanks, all the changes look good. Just one question: Have you checked what happens when an invalid clientSslCrtPath is provided? I suspect there will be no elements in list and the call to .first may fail/segfault. Also, what happens if there are intermediate certs and the one you want is last, or that is not likely to happen?

@jasonparallel jasonparallel Added check for empty or missing ssl ctr file
Added check for empty or missing ssl ctr file
b6a9878
@jasonparallel
Contributor

@ashkulz Sorry for the delay.
Either an invalid clientSslCrtPath or an empty file would have caused a segfault. Adding a check for an empty list of certs took care of both issues. If the first cert in clientSslCrtPath does not match the key you will get an UnknownNetworkError. The standard for openssl is to have the server/client cert first followed by the chain up to the CA. Postgres also requires this order and will error out if the order is not followed. It does not look like there is a way, in QT, to get the modulus from both and compare.

@ashkulz
Member
ashkulz commented Jan 4, 2017

The build is failing, need to wrap with #ifndef QT_NO_OPENSSL for Qt4. For Qt5, probably QT_NO_SSL or something similar.

@jasonparallel jasonparallel Added check for building without ssl support
e36d7e6
@jasonparallel
Contributor

The QT_NO_OPENSSL and QT_NO_SSL worked. Thanks!

@ashkulz
ashkulz approved these changes Jan 5, 2017 View changes

Looks good to me.

@ashkulz ashkulz merged commit 399e6a0 into wkhtmltopdf:master Jan 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ashkulz ashkulz added the Merged label Jan 5, 2017
@ashkulz
Member
ashkulz commented Jan 5, 2017

Thanks for the contribution!

@ashkulz ashkulz added this to the 0.12.5 milestone Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment