Optionally save the private key of a running share to settings for reuse

[mig5]

This is an initial stab at #295

When a share has been started, there is an option to 'Save private key'.

When clicked, this writes the private key to the settings json.

All subsequent shares use this key and hence the Onion URL stays the same.

If the user wishes to cancel this behavior, they can uncheck the setting in the Settings Dialog, which unsets the private_key from the json.

Subsequent shares then use a new, self-generated Onion keypair as normal.

UX: The option to uncheck the 'Save private key' only is present in the Settings Dialog if the private key is actually already saved in the json. This prevents anyone wanting to 'check' (enable) the saving of private key directly in the Settings Dialog (the only way to 'save' the private key is when the server is already running, since that's the only time we can obtain the private key from the running hidden service).

I wanted to get your feedback on this before working on the Hidservauth (which might be a bit trickier: you might want to be able to unset saved hidservauth but keep the saved private key? Not sure..)

[mig5]

Interestingly, after stopping the share and then starting it again, there is a Stem error deep under the hood

onionshare.onion.TorErrorProtocolError: ADD_ONION response didn't have an OK status: Onion address collision

https://people.torproject.org/~nickm/tor-auto/doxygen/hs__common_8h.html#a375d3b5c44c65ad158e30b8f05d74b3ba89a47916e39d4fb0d81421043bee2540

I guess although it's ephemeral, it somehow still 'lingers' after stopping the server somehow, maybe until OnionShare itself (the app) is closed and a new Tor connection is established. Not sure where to go from here.

[mig5]

Issue seems to be that Onion.cleanup() is not run until the GUI app is actually closed (after OnionShareGui.closeEvent() fires). It is not run on OnionShareGui.stop_server(). So the ephemeral service is not actually removed til after the app is closed..

Sticking in self.onion.cleanup() in OnionShareGui.stop_server() doesn't much help because then, although it has removed the last ephemeral service, when you try and start a new share, you get a stem.SocketClosed error :/

[mig5]

Right - so the underlying issue is twofold:

1. OnionShareGui.stop_server() doesn't run Onion.cleanup(), so it never actually removes the ephemeral service (almost a security issue, except I think mitigated by the fact that the web server is of course stopped despite this)

2. that Onion.cleanup() is too extreme: it doesn't just remove ephemeral services, it kills the whole Tor process, so if you were to call it, you'd be disconnected from Tor and can't start a new Share.

I have a fix which I am going to roll into this PR but I might open a separate standalone PR for this bug (as I anticipate this one might not get merged for some time, whereas this bug probably should be fixed)

[mig5]

This branch now also saves the HidServAuth string to settings when the private key is saved.

The saved HidServAuth string can be copied to clipboard from the SettingsDialog if need be.

This was challenging - lost time trying to work out why there was no basic_auth string returned in the ADD_ONION response from the Control port. Turns out that's by design if the ADD_ONION request sends a preset password (instead of letting the ADD_ONION generate its own password). This is similar to when sending a preset private key (no PrivateKey comes back in the response), except that the torspec actually points this out with regards to privatekey (but fails to mention it's the same for basic_auth :( ) hooray for reading source code.. https://github.com/torproject/tor/blob/master/src/or/control.c#L4734

Anyway, I think this is 'feature complete' but I expect the UI 'Save Private Key' is maybe not as good as it could be (maybe you want to 'hide' that behind some sort of advanced mode or something?)

I took the liberty of also simplifying the parsing of the Control port responses - no more crazy splitting (especially since the response can vary quite widely, as above), Stem gives most of the attributes back in a cleaner fashion these days.

[mig5]

Couple thoughts:

1. I guess the slug needs to be saved too? Otherwise re-sharing will just lead to 404s for users who have previously visited (or had communicated to them) the share URL.

2. maybe rather than offer the clunky ‘Save Private Key’ button on a started share, it should just be an option in SettingsDialog, and the next share saves the key back to settings if this option was enabled. Better UX perhaps.

[ghostlands]

You're a champ for taking a shot at this, Miguel!

I only have time to comment on the UI, unfortunately, though your logic elsewhere seems good.

maybe rather than offer the clunky ‘Save Private Key’ button on a started share, it should just be an option in SettingsDialog, and the next share saves the key back to settings if this option was enabled. Better UX perhaps.

I think the UI needs to present this option front and center, but then I might be biased for obvious reasons. It does seem like an option new users might not find for awhile without that placement, and in any case my personal use would involve switching back and forth often enough that I would like it more readily available. So I guess front and center in the name of flexibility is my argument.

But that flexibility gets us to another question, which is that this iteration seems to be "persist your key until you don't, then forget forever" due to the fact that there isn't a true export option. I'm not sure what format would be best for an export, whether in a file or simply as a string copied to the clipboard (the latter possibly getting confused with the copy url button's function?). It could just be a generic text file with a .key extension, or a bespoke extension like .onsh.

Or, there could just be a button for copying a string to the clipboard, with clear copy private key labeling leaving it up to the user to paste that key somewhere, hopefully somewhere secure.

I still think greyed out buttons are good UI. They can build kind of a panlingual/pantomime experience for the novice user: "Why can't I do that? Oh look, if I check/unchek this other thing it isn't greyed out anymore." Ergo demonstrating the connections between different functions.

[ghostlands]

Also a settings recommendation:

"Unchecking will remove the private key from settings."

should probably be

"Unchecking will delete the saved private key, and OnionShare will generate a new private key on restart."

Verbose, I know, but if we want it to be explanatory...

[mig5]

I think “Unchecking will delete the saved private key” followed by a ‘More Information’ hyperlink (that takes you to a wiki on this github repo) is probably the most concise way that doesn’t leave the user in the dark.

The key I guess is not ‘exportable’ but is written to the settings config file. I figured since this is ‘advanced’ usage, the user could go and obtain it from there if they want (but would they really need to do so?)

Though I agree maybe it would be better to have an ‘import key?’ checkbox in the main UI, before starting a share, that opens a file dialog where you choose a .key file.

And then an ‘export key’ during a running share that opens a file dialog to save the key.

It would raise new challenges though for the hidservauth string: where to store and load that, if the user has chosen to use a saved key for a share, and is using Stealth mode? (And likewise if we decide we should also save the slug (I’m not sure)). This is why it was convenient to store both values in the settings json file.

Thanks for providing feedback!

[ghostlands]

Thanks for doing the heavy lifting!!

I'd put an expanded explanation as a tooltip appearing upon hovering over the option/checkbox. Having to load a webpage over network to get a basic explanation always strikes me as overkill; it could also break the compartmentalization part of a user's security workflow. As for the hidservauth+slug concerns, in terms of UX they could go into the settings dialog with a button there for import/export of the string(s).

Alternatively, a .json file import/export process could be a catchall for persistence needs. It's a bit cludgy to export all the settings along with the desired keys and strings, but maybe that's mostly ok since there really aren't a lot of settings other than the specific ones we're addressing here. In this workflow, for convenience a user could rename multiple .json files according to the respective use cases of each.

[mig5]

Note that in the next release/master branch, advanced users can already pass an alternative, populated json config file with —config arg (eg perhaps storing the json file on a Tails persistent volume/USB stick)

I’m gonna let @micahflee read through and weigh in on the UX stuff, and we can go from there :)

[micahflee]

I have finally read through this, sorry about sitting on it for a month. This is excellent work. Thanks for catching the gnarly bug you address in #486, and impressive that you had to dig into the Tor source code to learn how it works.

I just read over the changes is source code, Then I ran onionshare-gui, and before testing anything I clicked the Settings button. The settings don't open, and this causes a crash.

user@dev ~/code/onionshare (mig5-reuse_private_key) $ ./dev_scripts/onionshare-gui 
Onionshare 1.1 | https://onionshare.org/
Connecting to the Tor network: 100% - Done
Traceback (most recent call last):
  File "/home/user/code/onionshare/onionshare_gui/onionshare_gui.py", line 224, in open_settings
    d = SettingsDialog(self.onion, self.qtapp, self.config)
  File "/home/user/code/onionshare/onionshare_gui/settings_dialog.py", line 294, in __init__
    save_private_key = self.old_settings.get('private_key')
  File "/home/user/code/onionshare/onionshare/settings.py", line 120, in get
    return self._settings[key]
KeyError: 'private_key'

I think the problem is my ~/.config/onionshare/onionshare.json already existed, didn't include the private_key key. The fix is simple, just add default values for all of the new settings you add into the self.default_settings dict in the Settings class here: https://github.com/micahflee/onionshare/blob/master/onionshare/settings.py#L49

But hmm... I just deleted my ~/.config/onionshare/onionshare.json to make sure I start over, opened this PR branch, started OnionShare, and tried sharing something without actually clicking the settings button, and I get a similar error:

user@dev ~/code/onionshare (mig5-reuse_private_key) $ ./dev_scripts/onionshare-gui 
Onionshare 1.1 | https://onionshare.org/
Connecting to the Tor network: 100% - Done
Configuring onion service on port 17629.
Starting ephemeral Tor onion service and awaiting publication
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/lib64/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "/home/user/code/onionshare/onionshare_gui/onionshare_gui.py", line 251, in start_onion_service
    self.app.start_onion_service()
  File "/home/user/code/onionshare/onionshare/onionshare.py", line 79, in start_onion_service
    self.onion_host = self.onion.start_onion_service(self.port)
  File "/home/user/code/onionshare/onionshare/onion.py", line 403, in start_onion_service
    if self.settings.get('private_key'):
  File "/home/user/code/onionshare/onionshare/settings.py", line 120, in get
    return self._settings[key]
KeyError: 'private_key'

I wonder why you didn't run into it?

I haven't actually tested using this feature yet because of this issue. However, here's my opinion on the UI. @mig5, I think I agree with what you said earlier, and that the "Save private key" button should be somehow more hidden.

I definitely see the value in this feature @ghostlands, but I'm still thinking of it as an advanced feature, and one that most casual users won't see. So I'd prefer to not add new buttons that most people aren't going to click. I'd rather figure out how to add new stuff to the settings dialog, and not clutter the main window.

Maybe @mig5, the "Should the private key be saved for re-use?" checkbox should always been there, not just when the settings contains a secret key. If it's not checked, then the UI remains exactly the same as before this feature. If it's checked, then the next time you start an onion service it saves the key. Every time after that, it uses the saved key. And the main window UI can have a new label at the bottom of the server status area that says:

This share is using a persistent URL

And finally, in the settings dialog, I think it might be more clear to not even mention "private keys" because that probably doesn't mean anything to people who don't understand how onion services work. Instead, the gui_save_private_key_checkbox string can say:

Use a persistent URL (unchecking will delete saved settings)

Does this make sense? Let me know if I'm missing something.

And finally, these UI suggestions don't address @ghostlands's issue relating to exporting and importing private keys. However, I think I'm ok with that because of the --config command line flag. Advanced users can totally figure it out.

Also, we should also remember to document both how to use this feature, and how to use the --config flag, in the wiki.

[mig5]

Thanks for the review - that’s totally bizarre, I definitely didn’t encounter that error, and I definitely remember updating the default_settings too. Maybe somehow something wasn’t committed into my pushed branch. I’ll investigate and update soon.

[mig5]

@micahflee I've fixed that issue with missing default settings (must've forgotten to commit the change!), as well as folded in the other UI improvements.

SettingsDialog checkbox now dictates whether a persistent URL will be used or not. Meanwhile, it's in the backend Onion object to save that private key to the settings json if that option was checked (which means it will work for the CLI too)

Seems to work for me, I also tested that it saves the HidServAuth string too as expected.

What do you think about saving the slug? To me it seems impractical to reuse the private key if the slug will change on the next share, as is currently the case. Should it be 'persistent' too as part of the overall URL?

EDIT: I forgot to add the ‘permanent URL’ label in the status bar widget. I’ll do so soon.

[micahflee]

I'm testing this now, and this is excellent. Here is my feedback:

• I tested the HidServAuth stuff, it all appears to work great.
• I tested with the CLI version, it appears to work fine and keep the same persistent onion.

And things I think need to be added:

• Like you mentioned, you forget to add the persistent URL label.
• I think for this to be useful, the slug needs to be persistent as well. As you mentioned above, and as described in some of the comments of question: is there a way to start onionshare with a consistent sharing url? #295, this PR will let people exchange an OnionShare URL and schedule a time out-of-band. Then at the scheduled time in the future, the recipient can load that OnionShare URL without any further communication. But without the slug being persistent, the sender will have to communicate the new slug before the receiver can download it, and if that's the case why not just also communicate an altogether new URL.

This is great!

[b-meson]

Yes the PR looks LGTM except for the missing slug.

[mig5]

Struggling a bit as the 'persistent URL' widget is immediately overwritten by the copy_url function being called.

Also trying to use a settings-supplied slug in the Web module (instead of generate_slug() is proving tricky, as we seem to pass parameters not from the Settings object in Onionshare module,.

Others are welcome to tackle these if they get to it sooner

[micahflee]

@mig5, I was thinking that the 'persistent URL' message shouldn't be a status message, but an actual new QLabel widget. And yeah, the persistent slug definitely will require some refactoring.

I'm going to continue reviewing and merging open PRs, but when I'm done with that maybe I'll try finishing up this PR, if you don't get to it first.

[mig5]

@micahflee there is now a 'persistent' label and we save the slug \o/

Testing very much welcomed.

The private key, hidservauth string and the slug are saved in the 'backend' (CLI) when the share starts, so the GUI has to do little more than save the settings via the SettingsDialog. I feel that's quite elegant.

There is one exception that might strike you as a peculiarity: we also save the slug in the ServerStatus object once the server has started, when running in GUI mode.

The reason for this is that if OnionShare has already been running with random keys, but then the user sets the persistent mode on, we don't reboot the OnionShare (CLI) object once the new settings have saved. OnionShare object only has an old copy of the Settings object (used for the first time the app is opened, or for CLI mode), so it doesn't know the persistent mode has been set, and therefore won't save the next share's slug.

For the private_key and hidservauth string, this is different/a non-issue, because those are saved in the Onion object, which does already load settings from disk when starting the onion service. But it has no knowledge of the Web object and therefore its slug, so it has to be handled in the __init__.py

Obviously for a CLI user, it all depends on them providing a JSON config file that already has the private key, slug, and optional hidservauth string, or they are using CLI with config file that already had these settings set via the GUI at some point.

Bleh - all that was hard for me to explain properly, but came out of a lot of testing - I am pretty confident that the feature is complete both in CLI and GUI mode :)

[micahflee]

Awesome, just reviewed this and it all appears to work great.

[mig5]

Documentation on persistent URLs added at https://github.com/micahflee/onionshare/wiki/Configuring-OnionShare#use-persistent-url-advanced-usage

[ghostlands]

This feature now works and looks great. Thanks for fulfilling this request! Very appreciated!

[ghostlands]

Argh! Wait. We have some unexpected behavior here. It should have been obvious. To reproduce:

• start onionshare with a consistent URL, add some files to it, leave it running, verify through another connection/computer that it's up

• start another instance of onionshare (it will use the same URL by default), add different files to it, leave it running (now there are two instances withe same URL running)

• refresh the URL at the other connection/computer

• now only the files from the later instance are available

• shut down the second onionshare instance (leave the first instance running)

• refresh the URL at the other connection/computer

• now there is nothing, the URL does not do anything

So it looks like the feature set alluded to in #656 and this feature have created a monster.

O_O

[mig5]

Well - this is a super corner case (running simultaneous instances of OnionShare).

This is mentioned in #551 already:

"But there's also another problem. Previously in Windows and Linux, you can open multiple instances of OnionShare, and each instance just had its own separate tor process and data dir. Now, if you open a second instance, it will crash and die because the first instance is using that data dir already. Maybe this is fine and you should only be allowed to run one instance at a time (or somehow multiple instances should share the same tor?), but if this is the case then we need make OnionShare check for other running instances when opening and handle it cleanly."

We need to deal with this, but we already know we do, so yes, you'll have to bear with us while we either force only 1 instance to be running, or have the instances fully self contained.

[b-meson]

I think a great way to do this would be to prevent multiple PIDs from writing to this directory...

[mig5]

Can we please move any such discussion to #366 instead of this old (closed!) issue. It is not at all specific to persistent URLs and we will lose this discussion in an old merged PR.