[auth] Convert PKCS8 to PKCS1 private keys (for macOS) #5401
Conversation
See description of QgsAuthCertUtils::pkcs8PrivateKey. This fix may be needed on other platforms (untested at this point), because Qt5 QSslkey class *still* does not directly support creation using non-PKCS1 PEM- or DER-encoded data, though QCA, whose qca-ossl plugin is linked to OpenSSL, does support PKCS1 and PKCS8.
dakcarto
changed the title
There was a problem hiding this comment.
To be honest I didn't check all the key manipulation code, I'd need to study for quite some time in order to understand what it does with those different PKCS formats.
BTW if you really need an in-depth review of the key manipulation code, please let me know and I'll schedule the task.
| } | ||
| file.close(); | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| QList<QSslCertificate> QgsAuthCertUtils::certsFromFile( const QString &certspath ) | ||
| { | ||
| QList<QSslCertificate> certs; | ||
| bool pem = certspath.endsWith( QLatin1String( ".pem" ), Qt::CaseInsensitive ); |
There was a problem hiding this comment.
Not in scope with this PR, but here is one of the places were we should be more robust: if the file extension is not a reliable sign of the encoding, we should be trying both encoding before we give up, ignoring the file extension completely.
There was a problem hiding this comment.
Yes, it needs addressed, but it is a larger refactoring, across many auth classes. Note this work already has an internal Boundless JIRA ticket for exactly that auth API change/refactoring.
| @@ -262,37 +273,249 @@ QStringList QgsAuthCertUtils::certKeyBundleToPem( const QString &certpath, | |||
| return QStringList() << certpem << keypem << algtype; | |||
| } | |||
|
|
|||
| bool QgsAuthCertUtils::pemIsPkcs8( const QString &keyPemTxt ) | |||
There was a problem hiding this comment.
See my previous comment: can this function be used to detect PEM vs DER instead of the file extension ?
There was a problem hiding this comment.
It doesn't do encoding determination. It specifically has to work with PEM in order to simply text search to determine PKCS format (much more involved if using DER). I suppose the signature could be more descriptive, e.g. privateKeyPemIsPkcs8 (...).
|
@elpaso thanks for the code review. However, I realize it is not so easy to functionally test, as it requires macOS and the |
src/core/auth/qgsauthcertutils.cpp
Outdated
| pkcs1 = QByteArray( oct_data, oct_len ); | ||
|
|
||
| // !!! SENSITIVE DATA !!! | ||
| QgsDebugMsgLevel( QStringLiteral( "privateKey octet data as PEM: %1" ).arg( QString( pkcs1.toBase64() ) ), 9 ); |
There was a problem hiding this comment.
@elpaso, @m-kuhn, @nyalldawson, (and anyone else) thoughts on whether it is OK to leave this debug output active, but set the QgsDebugMsgLevel high, e.g. 9 here. Previously, I've left these kind of lines in the code, but commented out (with a warning not to leave uncommented). I can envision a circumstance where the env var QGIS_DEBUG might be manipulated somehow, thus exposing the credentials.
There was a problem hiding this comment.
I'd hide it - " there's no reward in the risk" of exposing this to the log.
(+1million if anyone gets that reference)
|
Merged as the latest indention error makes no sense. Previous commit CI passed. Most recent commit would cause no issues. |
Description
This addresses an issue I found during dev meeting in Essen (unreported until now).
Extracts the PrivateKey ASN.1 element of a DER-encoded PKCS#8 private key:
On some platforms, e.g. macOS, where the default SSL backend is not OpenSSL, a QSslKey can not be created using PKCS#8-formatted data. However, PKCS#8 private key ASN.1 structures contain the key data inside a wrapper describing the algorithm used, e.g. RSA, DSA, ECC etc. Extracted PrivateKey ASN.1 data can be used to create a compatible QSslKey, e.g. 'traditional' SSLeay RSA-specific PKCS#1.
By default OpenSSL 1.0.0+ returns private keys as PKCS#8, previously it was PKCS#1.
NOTE: This function requires 'libtasn1' development files and library, which is used to parse and extract the PrivateKey element from an ASN.1 PKCS#8 structure.
Also, adds more PEM- and DER-formatted client keys for testing.
Checklist
fixes #11111in the commit message next to the description[FEATURE]in the commit message[needs-docs]in the commit message and containt sufficient information in the commit message to be documentedscripts/prepare-commit.shscript before each commit