Review Request 124312: Initial SSL implementation

Tue Jul 21 05:28:13 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124312/#review82739
-----------------------------------------------------------

Your implementation works, but looking at the code I feel that the architecture and design is not that good. If you end up with lines like this "link->provider()->pairingHandler()->acceptPairing(this);", it is probably a sign that is telling you that things are not where they should be. The task of refactor doesn't seem well done. I would ask that, now that you have more knowledge of the code, you have a look at my coments and see if you can come up with a better architecutre, organizing things a bit better. For now I only had a look at the code, next week I will test and give you more feedback, but in the meanwhile I would like you to try to reorganize the code a bit.

cli/kdeconnect-cli.cpp (line 151)
<https://git.reviewboard.kde.org/r/124312/#comment57056>

    Use spaces instead of tabs.

core/backends/lan/downloadjob.cpp (line 29)
<https://git.reviewboard.kde.org/r/124312/#comment57062>

    I thought we were not keeping backwards compatibility with non-ssl kdeconnects on the KDE side. What's the purpose fo all this "useSsl" thingy?

core/backends/lan/lanlinkprovider.h (line 28)
<https://git.reviewboard.kde.org/r/124312/#comment57063>

    Redundant?

core/backends/lan/lanlinkprovider.cpp (line 280)
<https://git.reviewboard.kde.org/r/124312/#comment57064>

    I though all our certificates were self-signed, how is this an error that causes us to unpair?

core/backends/lan/server.h (line 28)
<https://git.reviewboard.kde.org/r/124312/#comment57065>

    Can you add a comment explaining why did you have to extend QTcpServer? ie: why was not enough to just use it directly.

core/device.cpp (lines 212 - 213)
<https://git.reviewboard.kde.org/r/124312/#comment57058>

    This long chain of -> looks bad. Change this to be a Q_FOREACH(PairingHandler handler, pairingHandlers). You should have a way to get all the pairing handlers to do that. Now, you are only going through the pairing handlers of the providers which can currently see the device, instead of all.

core/device.cpp (lines 232 - 234)
<https://git.reviewboard.kde.org/r/124312/#comment57059>

    Same as before.

core/device.cpp (lines 284 - 286)
<https://git.reviewboard.kde.org/r/124312/#comment57057>

    Should be done in the pairing handler.

core/device.cpp (lines 365 - 371)
<https://git.reviewboard.kde.org/r/124312/#comment57060>

    I think the timeout should be part of the pairing handler. Other pairing mechanisms might not have a timeout or have a shorter/longer one (eg: 24hours to accept pairing through a cloud server). Actually, the status "Requested" and "RequestedByPeer" should be known only by the PairingHandler. The Device class should only care about Paired/NotPaired.

core/device.cpp (line 472)
<https://git.reviewboard.kde.org/r/124312/#comment57061>

    Split into storeDevicPublicKey and storeDeviceCertificate. PS: Actually I see that you already started doing that with setDeviceProperty and getDeviceProperty, even though you never used it. That seems a better approach than this.

- Albert Vaca Cintora

On July 19, 2015, 9:33 a.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124312/
> -----------------------------------------------------------
> 
> (Updated July 19, 2015, 9:33 a.m.)
> 
> 
> Review request for kdeconnect.
> 
> 
> Repository: kdeconnect-kde
> 
> 
> Description
> -------
> 
> * Similar to Android version, this SSL implementation supports old version also.
> * Certificate is generated when public key is generated using QCA, but used as QSslCertificate because QSslSocket don't like QCA certificate.
> * A new server is written overriding QTcpServer for setting socket descriptor to QSslSocket.
> * QSslSocket works as plain socket to transfer identity package and then either uses encrypted mode or normal mode according to the received package.
> * If connetion is to be on SSL, new link is only added after successful handshake.
> * Certificate is embedded into identity package on start of encrypted mode and then set in device using it.
> * PairingHandler is added like in Android version so that each LinkProvider manage pairing according to its own rules.
> 
> P.S. : Uploaded the patch with some debug statements, will comment out them.
> 
> 
> Diffs
> -----
> 
>   cli/kdeconnect-cli.cpp 3aa49e5 
>   core/CMakeLists.txt dd8fedb 
>   core/backends/lan/CMakeLists.txt 7c5be38 
>   core/backends/lan/downloadjob.h eeeab07 
>   core/backends/lan/downloadjob.cpp bba2e98 
>   core/backends/lan/landevicelink.h 7d31881 
>   core/backends/lan/landevicelink.cpp 0987057 
>   core/backends/lan/lanlinkprovider.h b379434 
>   core/backends/lan/lanlinkprovider.cpp f5d2b22 
>   core/backends/lan/lanpairinghandler.h PRE-CREATION 
>   core/backends/lan/lanpairinghandler.cpp PRE-CREATION 
>   core/backends/lan/server.h PRE-CREATION 
>   core/backends/lan/server.cpp PRE-CREATION 
>   core/backends/lan/socketlinereader.h b3be55a 
>   core/backends/lan/socketlinereader.cpp a6bd85e 
>   core/backends/lan/uploadjob.h 12b6f7e 
>   core/backends/lan/uploadjob.cpp b0d35fc 
>   core/backends/linkprovider.h f4fc309 
>   core/backends/pairinghandler.h PRE-CREATION 
>   core/backends/pairinghandler.cpp PRE-CREATION 
>   core/daemon.h 674ca01 
>   core/daemon.cpp af4c67b 
>   core/device.h 03f528c 
>   core/device.cpp c2dc609 
>   core/kdeconnectconfig.h af6c6df 
>   core/kdeconnectconfig.cpp 919505c 
>   core/networkpackage.h 6a0bf9c 
>   core/networkpackage.cpp 64cfab7 
>   tests/CMakeLists.txt b00a574 
>   tests/testsocketlinereader.cpp 77a9b4b 
> 
> Diff: https://git.reviewboard.kde.org/r/124312/diff/
> 
> 
> Testing
> -------
> 
> * Working fine, tested using connection to Android devices.
> * Pairing handler needs to be tested using two LinkProvider, working fine with one, but there be some problems with two LinkProviders simultaneouly.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20150721/843be6e1/attachment-0001.html>