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 9764
Build 9782: 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
201

Daemon::reportError

tests/testsslsocketlinereader.cpp
186

????

308–311

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