TeamAmaze / AmazeFileManager

Material design file manager for Android
https://teamamaze.xyz
GNU General Public License v3.0
5.1k stars 1.54k forks source link

SFTP client support #863

Closed TranceLove closed 6 years ago

TranceLove commented 6 years ago

This is in response to #498.

Before going further, some disclaimer:

Then, couple of change warnings before this pull request is even opened:

Now back to the pull request.

As said in #498, my implementation was an integration with sshj which I saw it as simpler and a better alternative than jsch.

The connection setup dialog was basically a rip-off from SmbConnectDialog ;) couldn't find an icon handy, so I used the Tux icon to represent SSH servers.

It supports both password and key authentication. I tested it against my Ubuntu Xenial machines (home desktop and VMs) on Lollipop and Nougat emulators.

Upload, download, mkdir, delete, rename should be working properly.

Now let the cruel code review begin... wish me luck.

VishalNehra commented 6 years ago

Really nice and clean implementation. Thanks for all the hard work (: Please also define what you feel to be the areas of improvement or if there are any bugs so that we can also test and help.

VishalNehra commented 6 years ago

Pl. increment the DATABASE_VERSION in UtilsHandler otherwise it'll crash for existing users.

EmmanuelMess commented 6 years ago

That'll clean the database, edit the onUpdate() accordingly, then bump the database version

EmmanuelMess commented 6 years ago

Can you rebase into less commits?

TranceLove commented 6 years ago

@EmmanuelMess will do. Let me get back to you once it's done.

@VishalNehra Known issues:

EmmanuelMess commented 6 years ago

If you enter the wrong IP it seems that the app hangs.

TranceLove commented 6 years ago

@EmmanuelMess requested changes committed in e0a0814 (if I understand correctly)

And app hang... do need to find a way to handle this. Will get back to you once it's solved.

TranceLove commented 6 years ago

Additionally, squashed commit counts down (to 70 currently).

TranceLove commented 6 years ago

To fix app hang on setting up the SSH connection, I moved the procedure of getting SSH host key fingerprint to run non-blocking (no AsyncTask.execute().get()) but with a callback ala JS style.

For SSH authentication, it is happening asynchronously anyway, so I didn't change it.

And added connection timeouts to 30 secs, in align with SMB client.

Finally, added a ProgressDialog on getting the SSH host key fingerprint. It's deprecated API, but seems the other parts of Amaze is still using, so I'll just follow suit.

EmmanuelMess commented 6 years ago

Ok, probably needs a Toast to tell the user that the connection failed. Can't really test the actual connection but I'll give it the visto bueno.

VishalNehra commented 6 years ago

I am failing to stream files, can you confirm if you implemented the support for streaming (on local host as we do in SMB/cloud or from this API itself)?

TranceLove commented 6 years ago

Oops, really didn't thought about it. Will see how I can do with it, or just a Toast to tell user copy to device first.

VishalNehra commented 6 years ago

Alright... Regardless, it won't reach to that, as we've already implemented a standard http streaming API, you can simply implement that if there's no native support with the API you're using.

VishalNehra commented 6 years ago

I'm trying to push commit but getting permission denied error @TranceLove Have you checked the tick for allowing adding changes to this PR?

TranceLove commented 6 years ago

This tick?

screenshot from 2017-12-13 07-29-21

TranceLove commented 6 years ago

CloudStreamer integration added in 5706b5d.

I also added support for PuTTY keys (*.ppk) and OpenSSH v1 keys (which has headers reading BEGIN OPENSSH PRIVATE KEY) in ee988af.

Note however, I found there are keys that can't be parsed by sshj but still can be used from Linux's ssh. I'd suppose if you are using newer key types, like those with smaller PEM file sizes having ecdsa in the header you you'll most likely may run into problems.

I thought sshj should have handled this, maybe just because I haven't read through the source codes, andbut there are already postings around the web suggesting their possible incompatibility.

Will look into it when I have time. Before it gets fixed, use RSA or DSA key types on the safe side.

EDIT: Added 03876cb which passed both my own key having such problem, as well as the key from sshj's test suite.

TranceLove commented 6 years ago

Acknowledged conflicts. Fix is on the way... need test before they appear here.

VishalNehra commented 6 years ago

Can't seem to parse ppk PuTTY keys. Can you please confirm?

EmmanuelMess commented 6 years ago

I would recommend not pulling this until the release 3.3.0 is published.

TranceLove commented 6 years ago

Tested with keys generated with latest puttygen, codebase merged with upstream.

RSA works OK DSA works OK ECDSA parse failed OpenSSHv1 parse failed ED22519 parse failed

However, from sshj's source code and sshj's test case it mentioned only RSA and DSA key types... so I think only PuTTY's RSA and DSA key types are supported. :/

Did you test with non-RSA/DSA keys generated by puttygen?

P.S. If you get authentication failures with DSA keys, it is possible that the SSH server disabled DSA keys as accepted key types.

VishalNehra commented 6 years ago

@TranceLove indeed. I tried with other GUI (WinSCP) they can't seem to parse their own generated PuTTY key :/ I'll give it a shot again, maybe I'm doing something wrong, I'm not used to using SFTP much.

VishalNehra commented 6 years ago

@TranceLove still can't seem to parse neither OpenSSH generated RSA key, nor PuTTY generated RSA key. Please find my private and public keys here Please see their respective descriptions.


                                                                         java.io.IOException: No converter available to parse selected PEM
                                                                             at com.amaze.filemanager.filesystem.ssh.tasks.PemToKeyPairTask.doInBackground(PemToKeyPairTask.java:110)
                                                                             at com.amaze.filemanager.filesystem.ssh.tasks.PemToKeyPairTask.doInBackground(PemToKeyPairTask.java:64)
                                                                             at android.os.AsyncTask$2.call(AsyncTask.java:333)
                                                                             at java.util.concurrent.FutureTask.run(FutureTask.java:266)
                                                                             at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:245)
                                                                             at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
                                                                             at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
                                                                             at java.lang.Thread.run(Thread.java:764)````
TranceLove commented 6 years ago

Hmm... key with passphrase. Seems a bit hairy to tackle this... not to say difficult, but need to add something to the implementation... a dialog box to capture key passphrase, the way to store the passphrase, to begin with.

VishalNehra commented 6 years ago

Ahh alright.. you didn't mention a key with passphrase isn't supported yet. My bit, don't just store it anywhere, asking the passphrase everytime during login would be better.

TranceLove commented 6 years ago

OK, let's see if e938da0 can help.

VishalNehra commented 6 years ago

Most of the things are working correctly. Thanks for all your hard work. Much delay from my side, my apologies for that. We're getting ready for beta versions so next production release should be soon.

FBachofner commented 6 years ago

I just reinstalled Amaze after seeing that SFTP support had been added.

Even though I am registered as a beta tester, the Play Store serves up version 3.2.1 which does not seem to have the SFTP client built in. What's the best way to test this?

TranceLove commented 6 years ago

I shouldn't be the right person to answer this, but anyway...

For end users, you'll have to wait until the latest production version is released on Google Play and/or fdroid. (And in fact, I think they have a more serious problem to solve before release - see #887, #905, and possibly #856) For advanced users who don't mind getting hands dirty and know how to build apks, checkout 120dbc3 and build a playDebug apk. But be advised, current copy implementation has a few problems - see #905 and #887

Maybe they have beta channels for the impatient to play with, but I don't know.

EmmanuelMess commented 6 years ago

@FBachofner Here are the releases, if a new one pops up you'll have the (new) changes in code up to the point of (that) release.

Limero commented 6 years ago

Is there an ETA of when the next release/beta will be released? This is a fantastic thing to have been added.

EmmanuelMess commented 6 years ago

See milestone 3.3.0.