Review Request 124140: Ported over to Netty and added SSL support

Mon Jun 22 16:35:59 UTC 2015

> On June 22, 2015, 4:24 a.m., Albert Vaca Cintora wrote:
> > I've tested it in two phones and it worked at the beginning, but it breaks easily. One of the things I've done that caused problems is: turn the phone WiFi off, unpair, and turn the WiFi on again (so the other device doesn't know that we have unpaired). After that, they started misbehaving: it was not possible to pair them again, they disappeared and reappeared intermittently from the list of available devices of each other, etc. I've seen some crashes in the logcat, this is a stack trace from one of them: http://pastebin.com/4KfpnsYm
> > 
> > You should do some testing (manual if it can't be automated) to make sure that all this edge cases work correctly and the devices are always doing what they should do. Debugging this will definitely take you more time than the time you have before the mid-term evaluation, but I don't think it makes sense to start working on the Qt side until this is not perfect or we will just carry over the same bugs. 
> > 
> > Also, the code needs a lot of clean up and you will also need to polish a bit the user interface (see my comments below).

It is an old bug, I tested on master branch too. If device is unpaired while wifi is off, it can't be paired again until it is unpaired from other device. But yeah disappearance and reappearance is an issue I will fix that.

> On June 22, 2015, 4:24 a.m., Albert Vaca Cintora wrote:
> > build.gradle, lines 56-57
> > <https://git.reviewboard.kde.org/r/124140/diff/1/?file=380819#file380819line56>
> >
> >     Nice that you decided to go with Netty. Is the other package that you changed a dependency of Netty? It was there originally for the SFTP plugin, have you tested that the new version is still compatible and the SFTP plugin still works?

SFTP plugin depends on MINA sshd, so MINA core is removed. Also Bouncycastle bcpkix version is used for generation of certificates, I cant find any use of bcprov version in the project.

> On June 22, 2015, 4:24 a.m., Albert Vaca Cintora wrote:
> > res/xml/general_preferences.xml, line 14
> > <https://git.reviewboard.kde.org/r/124140/diff/1/?file=380822#file380822line14>
> >
> >     This shouldn't be an option, we can decide for the user. SSL is better and there should no reason to want to disable it.

Will remove that.

> On June 22, 2015, 4:24 a.m., Albert Vaca Cintora wrote:
> > res/layout/activity_pair.xml, lines 28-59
> > <https://git.reviewboard.kde.org/r/124140/diff/1/?file=380820#file380820line28>
> >
> >     This info is not user friendly at all:
> >     - When not using SSL it doesn't make any sense to display it.
> >     - Most users don't care about this and/or don't know what to do with that information. For that reason it should not be in the main interface.
> >     
> >     My proposal for improvement: Put it in an options menu (three dots) named "Encryption info" or something like this. If there is no SSL it should say so: "The other device doesn't use a recent version of KDE Connect, using the legacy encryption method.". If there is SSL available, show the key hashes.

So you want the encryption info to be in device activity or in pair activity? If in device activity, you want that it should assume that certificate are correct during pairing?

> On June 22, 2015, 4:24 a.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Device.java, lines 314-315
> > <https://git.reviewboard.kde.org/r/124140/diff/1/?file=380829#file380829line314>
> >
> >     I liked your previous approach with the PairingHandler much better: each type of link could store the information it needed in the Handler without having to put it inside device like this. 
> >     
> >     Also with the handlers every device could use different pairing mechanisms, which can be very useful:
> >     - We could implement tests with a fake pairing handler.  
> >     - A Bluetooth link could rely on the pairing made by Bluetooth, and not ask the user to re-pair in KDE Connect.
> >     
> >     I think you should have finished the "refactor" task before starting to implement this, because without refactoring the pairing now the code is much dirtier here... before we can merge this, I would like to see the pairing-related things refactored out of this class.

Actually earlier we had in mind that each link should manage its own encryption, but since a normal link can be easily converted to a ssl link by adding a ssl handler, make two link will just cause a lot of code duplication and a over head on device to maintain two link where only one will be used most of the time.
Also I am a little bit unsure that if device are paired due to pairing by bluetooh and now they want to connect using LAN, what should be the behaviour? If either of one is paired, pair all and create a meta pairing package and send it or create pairing packets by each pairing handler and send them which will captured by respective pairing handler on other side.
Also since Netty supports handlers in channel pipeline, I was thinking to add SslHandler if link is on SSL or add Encryption and Decryption Handler if not in pipeline. What do you say ?

- Vineet

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

On June 21, 2015, 12:41 p.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124140/
> -----------------------------------------------------------
> 
> (Updated June 21, 2015, 12:41 p.m.)
> 
> 
> Review request for kdeconnect and Albert Vaca Cintora.
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> * Since MINA was tying hands in adding SSL, ported over Netty which has a good, clean and easy interface and a larger user and developer base.
> * Added support to setup links on SSL
> * Links automatically removed if wrong certificate is sent
> * Shows keys based on hash of certificate to check right certificates are received
> * Added a preference to use SSL, as it is experienced to cause high CPU usage on devices.
> * Corrected unit tests, LanLinkProvider is removed as it is nearly impossible to write it with current design. Will find a way
> 
> 
> Diffs
> -----
> 
>   build.gradle 6520c1c 
>   res/layout/activity_pair.xml 6b73edc 
>   res/values/strings.xml c128342 
>   res/xml/general_preferences.xml 82ab0ca 
>   src/org/kde/kdeconnect/Backends/BaseLink.java 579a7af 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLink.java 5994142 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java ae26958 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLink.java add92f8 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLinkProvider.java bd9c41b 
>   src/org/kde/kdeconnect/BackgroundService.java 5e3d8c2 
>   src/org/kde/kdeconnect/Device.java 3262aef 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/RsaHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/SslHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/NetworkPackage.java f62b5d0 
>   src/org/kde/kdeconnect/UserInterface/MainSettingsActivity.java 1b0e230 
>   src/org/kde/kdeconnect/UserInterface/PairActivity.java 7a45751 
>   tests/org/kde/kdeconnect/DeviceTest.java 5d3383d 
>   tests/org/kde/kdeconnect/LanLinkProviderTest.java fe7863a 
>   tests/org/kde/kdeconnect/LanLinkTest.java d3d94c9 
>   tests/org/kde/kdeconnect/NetworkPackageTest.java a21114e 
> 
> Diff: https://git.reviewboard.kde.org/r/124140/diff/
> 
> 
> Testing
> -------
> 
> Tesed on some device with where both supports SSL, also with PC where ssl is not supported, working fine. Need a little bit more testing with more devices.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

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