Make a menu to add onion and auth-cookie to TB
Currently it's very difficult to add an onion address and auth cookie to Tor Browser.
It would be nice to have an option in torbutton menu where you can set HidServAuth and optionally MapAddress, instead of having to edit your TB torrc file.
Activity
added TorBrowserTeam202002R actualpoints::7.5 component::applications/tor browser owner::brade parent::30000 points::8 priority::medium resolution::fixed reviewer::acat reviewer::emmapeel reviewer::richard reviewer::sysrqb severity::normal sponsor::27-must status::closed tbb-9.5a6 tbb-usability tor-hs type::defect ux-team labels
Trac:
Description: Currently it's very difficult to add an onion address and auth cookie to Tor Browser.It would be nice to have an option in torbutton menu where you can set
HidServAuth1 and optionallyMapAddress`, instead of having to edit your TB torrc file.to
Currently it's very difficult to add an onion address and auth cookie to Tor Browser.
It would be nice to have an option in torbutton menu where you can set
HidServAuth1and optionallyMapAddress, instead of having to edit your TB torrc file.Trac:
Description: Currently it's very difficult to add an onion address and auth cookie to Tor Browser.It would be nice to have an option in torbutton menu where you can set
HidServAuth1and optionallyMapAddress, instead of having to edit your TB torrc file.to
Currently it's very difficult to add an onion address and auth cookie to Tor Browser.
It would be nice to have an option in torbutton menu where you can set
HidServAuthand optionallyMapAddress, instead of having to edit your TB torrc file.Replying to teor:
I was wrong, using x.y.onion is not documented or implemented anywhere. (I wonder why I thought that?)
I wanted to add to this old discussion that
x.y.onionexists as a way to do subdomains for an.onion, as of #6344 (moved). (That may be where you were thinking of this from, teor.)So it couldn't reasonably be overloaded for this sort of client-auth feature.
There isn't any existing part of the URL that can be overloaded for this kind of feature, because an onion site can use any URL feature: https://en.m.wikipedia.org/wiki/URL#Syntax
Also, overloading the URL is poor UI. And it won't work when sites embed resources from the authenticated site.
The menu item is a much better idea,
where is it documented not yet (#25277 (moved), #27680 (moved)), but a user gave me this link on irc: comment:1:ticket:27680
Trac:
Parent: N/A to #30000 (moved)
Kathy and I have made enough progress now that we can share some "work in progress" screenshots (the data shown is fake). Antonela and everyone — what do you think?
**a) Onion Services Authentication group added to the Privacy & Security prefs panel: ** 
b) Prefs dialog/overlay that opens when you click
Saved Keys…: Trac:
Cc: dmr, tbb-team to dmr, tbb-team, antonela
Thanks for this work team! Looks great.
Could we have a discussion about how we are calling the secret/password/key/token and be consistent with that wording across the UI?
I agree that the string doesn't look like a password or passphrase or another familiar word for users. What about using
secret key? Is that secret? Personally, I just likeKeybut i wonder what others think.-
Our patches are ready for review. These build upon the #30237 (moved) changes, so you will need those patches too (the
bug19757-01branches in the various brade repos are branched from ourbug30237-04branches and therefore include all of the v3 client auth changes).Torbutton strings: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-01&id=8409822132a2dd092e181ac32ef128861d303107
Torbutton control port module enhancements: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-01&id=d265163919b310ce90e2672712867de4e87085f6
Tor Launcher string: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-01&id=044166fa62424e351d6a6a22a3aa1c44e5b7fee8
Tor Launcher enhancement to create an
onion-authdirectory for storage of the keys: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-01&id=c3f062c5888074cd6e2c3801353106d04f22e95aTor Browser changes ("Remember this key" checkbox plus about:preferences changes): https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19757-01&id=fb12d169bfe97b5a71a9135ad1efe25d39a1c097
Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam202001 deleted, TorBrowserTeam202001R added -
Test builds that include all of our v3 client authentication changes (i.e., the changes from this ticket and from #30237 (moved)) are available here: https://people.torproject.org/~mcs/volatile/v3-auth/
The bundled
torwas built from tor master as of two days ago.Standard disclaimer: Don't use these builds to run your nuclear power plant, etc.
https://gitweb.torproject.org/user/brade/tor-browser.git/tree/browser/components/onionservices/content/savedKeysDialog.js?h=bug19757-01&id=fb12d169bfe97b5a71a9135ad1efe25d39a1c097#n56 {{{ // Remove in reverse index order to avoid issues caused by index changes. for (let i = indexesToDelete.length - 1; i >= 0; --i) { await this._deleteOneKey(torController, indexesToDelete[i]); } }}}
This feels a bit fishy to me. We're saving off a list of indices and then individually removing them from both
this._keyInfoListandthis._tree. This happens in an async context, so wouldn't it be possible for the indices to become invalidated if keys were to be added in the middle of a large batch of deletions?I think a better way to do this would be to first create a list of the hsAdresses we want to remove, do our synchronous work updating
this._keyInfoListandthis._tree, then callonionAuthRemovewith all of our hsAddresses (assuming they can't all be batched together into one call to tor).Alternatively, we could call
loadSavedKeys()after a multi-delete to ensure the UI matches the tor backing store.Apart from that, these changes look good to me.
-
Replying to pospeselr:
This feels a bit fishy to me. We're saving off a list of indices and then individually removing them from both
this._keyInfoListandthis._tree. This happens in an async context, so wouldn't it be possible for the indices to become invalidated if keys were to be added in the middle of a large batch of deletions?Thanks for your review! The only other code that modifies or sets
this._keyInfoListis_loadSavedKeys, which is only called one time (from_init()). That means that the code we wrote should not be fragile in practice.I think a better way to do this would be to first create a list of the hsAdresses we want to remove, do our synchronous work updating
this._keyInfoListandthis._tree, then callonionAuthRemovewith all of our hsAddresses (assuming they can't all be batched together into one call to tor).We can only remove one key at a time from using the Tor control protocol, so the problem I see with your suggested approach is that we would be removing rows from the tree before we know whether tor successfully removed them. If you still think what we have now is too fragile, how about this instead: we can change the code inside
_deleteOneKey()to not assume that the index stays the same during theaTorController.onionAuthRemove()call. If that async call returns without throwing we know that tor removed the key, and then we would use the.hsAddressfield to re-find the row that needs to be removed fromthis._keyInfoList. -
Hmm OK, if we're sure that the indices cannot be invalidated then the code is fine as-is. Iterating through our list and looking for an index that can't have changed seems a bit overkill.
If we could get the tor daemon to just accept a list of onions to remove rather than having to make multiple requests would be helpful here though.
In
savedKeysDialog.js, line 64,controllerFailureMsgis not defined in that scope. I guess it should be declared outside thetry, as you do inauthPrompt.js[eslint caught this one :) ]Speaking of eslint, I'm not sure whether we made a decision about enforcing it for new code or not but at least with the latest patches of pospeselr or mine we tried to do so. I leave it up to you, as I think it's not so important and can always be done later.
./mach lint -l eslint --fix browser/components/onionservicesshould fix all the auto-fixable issues.I tested a bit and saw a couple of cases with a behaviour I'm not sure is the intended one:
-
Added private key, checked "Don't remember" -> after "New Identity" accessing to the .onion still works (I would expect we clear that on New Identity).
-
Added private key, checked "Remember", removed from
about:preferences-> accessing to the .onion still works (but not after "New Identity", which is surprising given the behaviour above).
Should we fix these or is this working as intended?
Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam202001R deleted, TorBrowserTeam202001 added-
-
Replying to acat:
In
savedKeysDialog.js, line 64,controllerFailureMsgis not defined in that scope. I guess it should be declared outside thetry, as you do inauthPrompt.js[eslint caught this one :) ]Good catch! We will fix it.
Speaking of eslint, I'm not sure whether we made a decision about enforcing it for new code or not but at least with the latest patches of pospeselr or mine we tried to do so. I leave it up to you, as I think it's not so important and can always be done later.
./mach lint -l eslint --fix browser/components/onionservicesshould fix all the auto-fixable issues.We will try using eslint soon. Thanks!
I tested a bit and saw a couple of cases with a behaviour I'm not sure is the intended one:
- Added private key, checked "Don't remember" -> after "New Identity" accessing to the .onion still works (I would expect we clear that on New Identity).
This is an interesting test case. Kathy and I agree that "in memory" keys should be cleared upon New Identity. But we should ask the Network Team (asn and dgoulet) how they think it should work.
- Added private key, checked "Remember", removed from
about:preferences-> accessing to the .onion still works (but not after "New Identity", which is surprising given the behaviour above).
I think this second test case points to a bug in tor. Kathy and I are able to reproduce it. It looks like a new circuit is used to load the .onion the second time, so it is not just a matter of circuit reuse.
The New Identity basically sends a
NEWNYMsignal to Tor I believe and that indeed does NOT clear the client authorization cache. Maybe we should considering that we can have non persistent authorization? But the one from disk should not get cleared out?For the second case, that is very weird... If anyone of you could give us a info log (extra points for debug logs) of that, would be grand.
Agreed with dgoulet here.
If you want "new identity" to cleanup the ephemeral keys we can do that. Perhaps open a ticket about it and post on this ticket and we will handle it?
I'm not sure about the last bug you mentioned. What is "removed from about:preferences" supposed to and why do you expect it to remove the keys from the tor side? And yes the part about New Identity is weird.
-
Replying to asn:
Agreed with dgoulet here.
If you want "new identity" to cleanup the ephemeral keys we can do that. Perhaps open a ticket about it and post on this ticket and we will handle it?
Done; see #33139 (moved).
I'm not sure about the last bug you mentioned. What is "removed from about:preferences" supposed to and why do you expect it to remove the keys from the tor side? And yes the part about New Identity is weird.
We will capture a log and email it to you and dgoulet (Kathy and I don't want to leak private .onion addresses publicly in this ticket). The "remove from about:preferences" part means that Tor Browser's new key management UI was used to remove a permanent key. From tor's perspective, this means that an
ONION_CLIENT_AUTH_REMOVEcommand was received on the control port. -
Replying to mcs:
We will capture a log and email it to you and dgoulet (Kathy and I don't want to leak private .onion addresses publicly in this ticket). The "remove from about:preferences" part means that Tor Browser's new key management UI was used to remove a permanent key. From tor's perspective, this means that an
ONION_CLIENT_AUTH_REMOVEcommand was received on the control port.This issue is being handled in #33148 (moved).
-
We rebased our patches to sit on top of the #30237 (moved) commits that sysrqb merged, fixed the
controllerFailureMsgissue that acat found (comment:31), and used eslint to find and fix various style issues in the new JS files (we should separately decide as a team if we want to run all of our existing JS files through eslint, e.g., the other files underbrowser/components/onionservices).Kathy and I think this is ready to merge now. One final thing to consider is whether it would be easier to maintain the #30237 (moved) tor-browser patch and the patch for this ticket if they were merged together; that is, should we handle the tor-browser patch for this ticket as a fixup commit to the #30237 (moved) one?
Here are the new patches (all are on
bug19757-02branches within brade's various repos):Torbutton strings: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-02&id=fddbe371f2eb43d7d6007561db38d1ad8a6ff8e4
Torbutton control port module enhancements: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-02&id=dc1871601fad289645696bb8138e8b14c46dd60a
Tor Launcher string: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-02&id=f53497c7255b040b6c97738c3c64e98b69cc96e4
Tor Launcher enhancement to create an onion-auth directory for storage of the keys: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-02&id=6ceda2e5565702f13933b83653c1951789fc0252
Tor Browser changes ("Remember this key" checkbox plus about:preferences changes): https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19757-02&id=06f115cc028e27124b48b40aaa83e988c28a9d98
Trac:
Keywords: TorBrowserTeam202001 deleted, TorBrowserTeam202002R added
Status: needs_revision to needs_review -
This looks good to me, but a few nits that don't really matter:
165 let checkboxElem = this._getCheckboxElement(); 166 let isPermanent = (checkboxElem && checkboxElem.checked);
I'd move these down within the try block before the
onionAuthAddcall as it doesn't seem likeisPermanentis used anywhere else in the higher scope.19 string: { 20 groupBoxID: "torOnionServiceKeys", 21 headerSelector: "#torOnionServiceKeys-header", 22 overviewSelector: "#torOnionServiceKeys-overview", 23 learnMoreSelector: "#torOnionServiceKeys-learnMore", 24 savedKeysButtonSelector: "#torOnionServiceKeys-savedKeys", 25 },
I'd call
stringsomething like 'selectors' or something like that (here and also in authUtil.jsm, savedKeysDialog.js, etc)Apart from those nits it looks good!
-
Replying to pospeselr:
This looks good to me, but a few nits that don't really matter:
165 let checkboxElem = this._getCheckboxElement(); 166 let isPermanent = (checkboxElem && checkboxElem.checked);
I'd move these down within the try block before the
onionAuthAddcall as it doesn't seem likeisPermanentis used anywhere else in the higher scope.Fixed.
19 string: { 20 groupBoxID: "torOnionServiceKeys", 21 headerSelector: "#torOnionServiceKeys-header", 22 overviewSelector: "#torOnionServiceKeys-overview", 23 learnMoreSelector: "#torOnionServiceKeys-learnMore", 24 savedKeysButtonSelector: "#torOnionServiceKeys-savedKeys", 25 },
I'd call
stringsomething like 'selectors' or something like that (here and also in authUtil.jsm, savedKeysDialog.js, etc)OK. We split things up into various buckets:
domid,selector,message,topic. Kathy and I think this will work well since for #19251 (moved) we have to add more topic values, etc.We also made this a squash! commit so eventually it will be combined with the #30237 (moved) patch.
Finally, we slipped in a minor prompt fix: the revised code accepts lowercase letters within base32-encoded keys (see the comment we added inside
browser/components/onionservices/content/authPrompt.js).We only revised the browser patch; the new one is here: https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19757-03&id=2d0f4658894642a4bf92e3117ef8c36e5528b7e7
See comment:38 for pointers to the Torbutton and Tor Launcher commits.
All the patches look good. It seems like we may want a follow up ticket (probably for after s27) where Tor Browser generates the keys, too.
This idea is from [[https://spec.torproject.org/rend-spec-v3#n625 | rend-spec-v3 [CLIENT-AUTH] ]]:
The right way to exchange these keys is to have the client generate keys and send the corresponding public keys to the hidden service out-of-band. An easier but less secure way of doing this exchange would be to have the hidden service generate the keypairs and pass the corresponding private keys to its clients. See section [CLIENT-AUTH-MGMT] for more details on how these keys should be managed.Emma, we're proposing adding the following new strings. The context for this is giving the user an option for saving a "key" that gives them access to an onion service. They can view the list of saved keys in about:preferences where they can delete individual keys or all keys. You can see some older mockup screenshots in comment:18. The below Tor Launcher string is needed because the "saved keys" are saved within a specific directory in the Tor Browser installation, and the user is warned when that directory is missing.
+<!ENTITY torbutton.onionServices.authPrompt.persistCheckboxLabel "Remember this key"> +onionServices.learnMore=Learn more +onionServices.authPreferences.header=Onion Services Authentication +onionServices.authPreferences.overview=Some onion services require that you identify yourself with a key (a kind of password) before you can access them. +onionServices.authPreferences.savedKeys=Saved Keys… +onionServices.authPreferences.dialogTitle=Onion Service Keys +onionServices.authPreferences.dialogIntro=Keys for the following onionsites are stored on your computer +onionServices.authPreferences.onionSite=Onionsite +onionServices.authPreferences.onionKey=Key +onionServices.authPreferences.remove=Remove +onionServices.authPreferences.removeAll=Remove All +onionServices.authPreferences.failedToGetKeys=Unable to retrieve keys from tor +onionServices.authPreferences.failedToRemoveKey=Unable to remove key+torlauncher.onionauthdir_missing=The Tor onion authentication directory does not exist and could not be created.Trac:
Reviewer: pospeselr, acat, sysrqb to pospeselr, acat, sysrqb, emmapeelReplying to mcs:
Here are the new patches (all are on
bug19757-02branches within brade's various repos):Torbutton strings: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-02&id=fddbe371f2eb43d7d6007561db38d1ad8a6ff8e4
Torbutton control port module enhancements: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-02&id=dc1871601fad289645696bb8138e8b14c46dd60a
These are merged with commit
078ae18e5f1cbf888e7c86c8aecaa383a727bf0don master.Tor Launcher string: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-02&id=f53497c7255b040b6c97738c3c64e98b69cc96e4
Tor Launcher enhancement to create an onion-auth directory for storage of the keys: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-02&id=6ceda2e5565702f13933b83653c1951789fc0252
These are merged with commit
bb1bff811c12fd8638ebede1514be512a9a4fbd7.Replying to mcs:
We only revised the browser patch; the new one is here: https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19757-03&id=2d0f4658894642a4bf92e3117ef8c36e5528b7e7
cherry-picked as commit
aed69dc95387429e18b18ad578fb78d4a83d91f2ontor-browser-68.5.0esr-9.5-1.Replying to sysrqb:
Emma, we're proposing adding the following new strings. Emma said these look okay. We'll see how well they work after we have localizations. We won't have any in this update, but we should have some for the next update.
Thanks everyone!
Trac:
Resolution: N/A to fixed
Status: needs_review to closedmentioned in issue #24310 (moved)
mentioned in issue #27680 (moved)
mentioned in issue #33514 (moved)
mentioned in issue #33533 (moved)
mentioned in issue #33533 (moved)
mentioned in issue #30000 (moved)
mentioned in issue tpo/core/tor#27680 (closed)
mentioned in issue tpo/applications/tor-browser#33514 (closed)
