Port away from QCA
Needs ReviewPublic

Authored by nicolasfella on Dec 25 2018, 2:39 AM.

Details

Reviewers
None
Group Reviewers
KDE Connect
Summary

It is only used for certificate generation. Use OpenSSL directly.
BUG: 402323

Test Plan

Clear config. Pair device, play around

Diff Detail

Repository
R224 KDE Connect
Branch
arcpatch-D17790
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14061
Build 14079: arc lint + arc unit
nicolasfella requested review of this revision.Dec 25 2018, 2:39 AM
nicolasfella created this revision.
pino added a subscriber: pino.Dec 25 2018, 8:39 AM
pino added inline comments.
core/kdeconnectconfig.cpp
198
  • missing handling of the return value of the command (what if fails?)
  • please do not split the arguments from a single string, but directly build a string list with the arguments (in case the paths contain spaces, for example)
201

if QSslCertificate::fromPath fails, the return is an empty list, and this will misbehave

206

hint: output the wrong permissions too, so at least the warning gives more clues

tests/lanlinkprovidertest.cpp
317

this can fail, needs error handling

318

this can fail, needs error handling

320
  • missing handling of the return value of the command (what if fails?)
  • please do not split the arguments from a single string, but directly build a string list with the arguments (in case the paths contain spaces, for example)
322

if QSslCertificate::fromPath fails, the return is an empty list, and this will misbehave

tests/testsslsocketlinereader.cpp
287

this can fail, needs error handling

289

this can fail, needs error handling

291
  • missing handling of the return value of the command (what if fails?)
  • please do not split the arguments from a single string, but directly build a string list with the arguments (in case the paths contain spaces, for example)
293

if QSslCertificate::fromPath fails, the return is an empty list, and this will misbehave

nicolasfella edited the summary of this revision. (Show Details)Dec 25 2018, 12:50 PM

Using the command line tool means this is a runtime dependency only. You could run kdeconnect without having openssl installed and it would fail mysteriously.

We can either check that it exists and output a specific error, or change to a compile-time dependency (libopenssl? Not sure if it exists).

Using the command line tool means this is a runtime dependency only. You could run kdeconnect without having openssl installed and it would fail mysteriously.

We can either check that it exists and output a specific error, or change to a compile-time dependency (libopenssl? Not sure if it exists).

I'm not sure I'm understanding the conversation. The library is called libssl. It's apparently a little complicated to use, but there is a good StackOverflow example of how it is done: https://stackoverflow.com/questions/256405/programmatically-create-x509-certificate-using-openssl

I'm not sure I'm understanding the conversation. The library is called libssl. It's apparently a little complicated to use, but there is a good StackOverflow example of how it is done: https://stackoverflow.com/questions/256405/programmatically-create-x509-certificate-using-openssl

I think using the library is a better solution than running a command line utility and hoping it will be installed/perform as expected.

pino added a comment.EditedJan 7 2019, 11:10 AM

Using the command line tool means this is a runtime dependency only. You could run kdeconnect without having openssl installed and it would fail mysteriously.

We can either check that it exists and output a specific error, or change to a compile-time dependency (libopenssl? Not sure if it exists).

I'm not sure I'm understanding the conversation. The library is called libssl. It's apparently a little complicated to use, but there is a good StackOverflow example of how it is done: https://stackoverflow.com/questions/256405/programmatically-create-x509-certificate-using-openssl

Please do not use libcrypto/libssl directly:

To get to the point of this patch: what is the problem it is trying to solve?
The linked bug (402323) just mentions a generic failure, when loading a certificate via QSslCertificate::fromPath() on macOS. Was any debugging done to know why?

In D17790#387972, @pino wrote:

Please do not use libcrypto/libssl directly:

  • it does not have a stable ABI
  • its API changes often too
  • its license is not exactly nice, imposing extra clauses on what uses it; see https://www.openssl.org/docs/faq.html#LEGAL2 and https://people.gnome.org/~markmc/openssl-and-the-gpl.html (and they will switch soon to Apache 2.0)

    To get to the point of this patch: what is the problem it is trying to solve? The linked bug (402323) just mentions a generic failure, when loading a certificate via QSslCertificate::fromPath() on macOS. Was any debugging done to know why?

Thanks for the info! We wanted to rid of QCA because it's a rather big library and it gave us some problem with the Windows and Mac ports. We only use it to generate a certificate the first time kdeconnect runs, so any library that can do that would work for us.

I added some checks, though I'm not sure what excactly should happen in case the generation fails.

Do we really need the error handling for the tests?

Restricted Application added a project: KDE Connect. · View Herald TranscriptJan 12 2019, 8:41 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
nicolasfella planned changes to this revision.Feb 8 2019, 10:34 PM

GGotta add a runtime check in cmake for packages.

core/kdeconnectconfig.cpp
249

Daemon::reportError

tests/testsslsocketlinereader.cpp
204

????

341–344

break into a couple of lines.

  • Address comments
  • Revert rename
  • Look for openssl in cmake

I changed this code so rebasing this is not trivial now :/

I split the generation from loading, to a separate function and added some error checks if the key can't be loaded, is empty, etc.

apol added a comment.Mar 18 2019, 8:10 PM

Is openssl interface documented as stable somehow? or could it break anytime?

In D17790#433960, @apol wrote:

Is openssl interface documented as stable somehow? or could it break anytime?

I did not find any information on that

apol added a comment.Jun 16 2019, 12:15 AM

Let's discuss at the sprint?
Have you considered what it would look like using libssl.so directly?

Inoki added a subscriber: Inoki.Aug 3 2019, 8:34 AM

Is this good to ship?

Since there is a dylib duplicated link error caused by qca-qt5 on macOS(when we use Craft to pack KDE Connect with qml files), this will be helpful for all qml apps we have up to now :)

Is this good to ship?

Since there is a dylib duplicated link error caused by qca-qt5 on macOS(when we use Craft to pack KDE Connect with qml files), this will be helpful for all qml apps we have up to now :)

I think at the sprint we decided we didn't want to use OpenSSL. It has licensing problems when using the library directly and using the binary seems like it could lead to problems with multi-platform packaging and with the potential for the command-line flags to change in the future. We talked about looking at GnuTLS but nobody has done that yet.

What problem are you having? It seems weird that you would only be having a problem with QML applications now since there are already several, like the SMS app and the settings app and since none of them use QCA

Inoki added a comment.Aug 4 2019, 2:37 PM

Is this good to ship?

Since there is a dylib duplicated link error caused by qca-qt5 on macOS(when we use Craft to pack KDE Connect with qml files), this will be helpful for all qml apps we have up to now :)

I think at the sprint we decided we didn't want to use OpenSSL. It has licensing problems when using the library directly and using the binary seems like it could lead to problems with multi-platform packaging and with the potential for the command-line flags to change in the future. We talked about looking at GnuTLS but nobody has done that yet.

What problem are you having? It seems weird that you would only be having a problem with QML applications now since there are already several, like the SMS app and the settings app and since none of them use QCA

It's described here: https://invent.kde.org/snippets/336

I talked it with Hannah. If we won't port away from qca-qt5, I'd like to fix it in Craft as what I've done in my local env :)

albertvaka added a comment.EditedAug 5 2019, 1:11 AM
In D17790#387972, @pino wrote:

Please do not use libcrypto/libssl directly:

  • it does not have a stable ABI

According to https://github.com/openssl/openssl/issues/9426 the library should have a stable ABI. Maybe what you told us is outdated?