Various small changes / improvements, support for SSH tunnel

[Kami]

First thank you for this software.

This PR includes various small changes and improvements.

1. Add support for specifying password for password protected ssh keys using ssh.password config option entry (same config key which is used for regular password auth when auth method is password). If people think this is too confusing, I can add new config option (e.g. ssh.key_password or ssh.key_passphrase)
2. Change default SSH connection timeout to 3 seconds - I tested it many times and for me the app very rarely successfully connects in less than 2 seconds so perhaps changing default value may be in order since it will result in a better user experience.
3. Add support for gracefully stopping the app from the console using CTRL+C.
4. Make sure raw password is not logged to the console when logging used config options.
5. Add info on recommendation to use rmview specific known hosts file since paramiko can be very slow when loading and parsing large default system known hosts file (on my system it takes ~10+ seconds which slows down the whole connection process and the known hosts file contains only ~1500 entries).
6. Code which kills any old stray VNC server processes before starting a new one on Framebuffer thread start.
7. Move loading of known host files from the class constructor to the run() method. As mentioned above, loading system known hosts can easily take 10 seconds or more on a system with larger known hosts file. Since the loading was happening inside the main thread and not inside the worker thread pool, this was blocking the main QT render event loop which meant the main window was frozen until the loading completed.
8. Update main window to display connecting screen until we are connected.

TODO

• [x] Update docs / readme

[Kami]

Re PEM SSH key format - I noticed the comment for that in the readme.

In another project, we use this piece of code (https://github.com/apache/libcloud/blob/6bac7102df34305ea28e0ac9fe8bc08002f7ce92/libcloud/compute/ssh.py#L626) which transparently and automatically handles loading of the keys which are already in the PEM format, but contain BEGIN PRIVATE KEY header which paramiko doesn't support.

Probably an overkill though :)

[Kami]

Another thing I noticed is that VNC server doesn't expose any kind of authentication.

You could argue that this is not a big deal since it's usually used inside a LAN or over USB, but I still think it would be better to use authentication (+ encryption, if not already) for the VNC server.

There are a couple of options:

1. Add support for password command line argument to the rM-vnc-server (https://github.com/pl-semiotics/rM-vnc-server), generate random password on rmview start up, pass that to binary when starting the server and use it when connecting to the VNS server. Would also need to confirm that current implementation is indeed also using encryption.
2. Use SSH tunnel. This may actually be the best and easiest thing to do - user already needs to set up SSH info for purposes of SSHing to remarkable so we can start the VNC server. This would solve authentication and also the encryption problem (we would then connect to the VNC server over SSH tunnel). It may still require a small change to rM-vnc-server (ability to only listen on localhost).

I quickly verified it and got it to work with SSH tunnel without a problem:

ssh -L 59000:127.0.0.1:5900 root@192.168.160.144
# And then I connected rmview to 127.0.0.1:59000

I also confirmed that it also doesn't seem to add any perceived delay or lag.

As far as actual implementation goes - since project is already using Twisted we could just use that, or perhaps (if we assume this app will always run on Linux like system) we could even also just shell out to the SSH binary...

If you think I should open a new ticket for this, please let me know.

[Kami]

I pushed a commit which adds support for using a ssh tunnel directly to this branch - ba98c0e28b56a512f350f8bde74315ccffc7127f.

I tested it and verified it works well and doesn't add any perceived overhead or delay.

If you think I should open a separate PR for those changes, please let me know.

[bordaigorl]

Wow thanks these are awesome features! It will take me some time to review but I am sure most of this will make it into the next release! I just pushed a development branch which contains new features I am testing; would it be too much trouble to rebase your changes on devel?

A couple of quick comments:

• I used a prompt for the key passphrase because storing it in plaintext in the config makes it pointless...but I guess offering the option would not harm?
• The password erasing to log the configuration is good but overkill: I would simply remove the logging.
• The SSH Tunnel is a great idea
• Not sure about killing running server instances, I think it would be more useful to avoid starting a new one if one is running, allowing people with custom ways of handling the server tablet-side to use rmview
• Was not aware of the known host file performance issues. Good that you caught it. Will evaluate your solution

[Kami]

Thanks for the response.

Will rebase the branch soon.

I used a prompt for the key passphrase because storing it in plaintext in the config makes it pointless...but I guess offering the option would not harm?

Yeah, a lot of software stores unencrypted credentials in ~/.config directory. Ideally we would just warn user in the readme to make sure file is not readable by others (chmod 600 / similar) in case they store plaint text credentials in that file.

So as long as it's opt-in and it's document, I think it should be fine.

Not sure about killing running server instances, I think it would be more useful to avoid starting a new one if one is running, allowing people with custom ways of handling the server tablet-side to use rmview

Yeah, I can remove that change and just add a change instead which logs a message if an existing instance is found.

I think you have a good point with "allowing people with custom ways of handling the server tablet-side to use rmview" (e.g. if someone just wants to run vnc server all the time or similar).

[Kami]

@bordaigorl I've synced up this branch with latest devel (should I also change target for this PR to devel branch?)

One change I didn't pick from devel branch is the one which always loads system level hosts keys because as per my comment, that can be slow (we only do that if rm specific hosts key file is not specified).

I also added a change to log a warning message if config file is readable by others and also a change to try to re-use an existing VNC server instance if one is already running on reMarkable.

For that last change, I think we need to decide on a couple of more things:

1. If instance is already running and we try to re-use that one instead of starting a new one, we likely should not try to kill it on exit. WDYT?
2. Probably a good idea to add "force_start_new_instance" or similar config option which would make sure that in such scenario (existing instance is detected), we still kill it and start a new one.
3. I also need to clean up the code a bit and move functionality into various smaller methods

[bordaigorl]

should I also change target for this PR to devel branch?

Yes please!

One change I didn't pick from devel branch is the one which always loads system level hosts keys because as per my comment, that can be slow (we only do that if rm specific hosts key file is not specified).

Makes sense

1. If instance is already running and we try to re-use that one instead of starting a new one, we likely should not try to kill it on exit. WDYT?

Agreed

2. Probably a good idea to add "force_start_new_instance" or similar config option which would make sure that in such scenario (existing instance is detected), we still kill it and start a new one.

Good idea.

Have you tested the other new features of the devel branch? What do you think?

[Kami]

@bordaigorl I pushed some changes as per comments above.

And yeah, I tested "pause streaming", etc. which seems to work well :+1:

Going forward, I think it would also be good to hook up at least some basic code lint and style checks using Github actions or similar.

[bordaigorl]

Going forward, I think it would also be good to hook up at least some basic code lint and style checks using Github actions or similar.

That sounds like a good idea. Any suggestion? I have never set up something like this (as you can tell by my very inconsistent code formatting)

[Kami]

@bordaigorl I can hook this up (black, pylint, flake8), but it would be better to finish and merge any open changes and PRs since that auto code-reformatting may otherwise cause pain when trying to resolve merge conflicts.

So please let me know once all the outstanding changes have been merged into devel / vnc branch and I will work on the PR with reformatting + lint checks.

[Kami]

@bordaigorl So please let me know when you get a chance to review and merge this PR + any other outstanding changes so I open a PR with formatting, etc. changes.

[bordaigorl]

@Kami will do. I will try to review the changes this week. Thanks for your contribution!

[bordaigorl]

I made a number of changes addressing my own comments, can you have a look?

[Kami]

@bordaigorl Thanks for having a look and sorry for the delay.

I just had a look and things look good to me (I had a small comment on using argparse module, but that's not a blocker).

Feel free to go ahead and merge that branch and once that's done, I will open a PR which sets up Github Actions CI with some basic checks.