Opened 4 years ago

Closed 6 months ago

Last modified 6 months ago

#19757 closed defect (fixed)

Make a menu to add onion and auth-cookie to TB

Reported by: mrphs Owned by: brade
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ux-team, tbb-usability, tor-hs, TorBrowserTeam202002R, tbb-9.5a6
Cc: dmr, tbb-team, antonela Actual Points: 7.5
Parent ID: #30000 Points: 8
Reviewer: pospeselr, acat, sysrqb, emmapeel Sponsor: Sponsor27-must

Description (last modified by mrphs)

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.

Child Tickets

Attachments (2)

onion-key-prefs1.png (401.6 KB) - added by mcs 8 months ago.
onion auth prefs - privacy & security panel
onion-key-prefs2.png (392.4 KB) - added by mcs 8 months ago.
onion auth prefs - key dialog

Download all attachments as: .zip

Change History (50)

comment:1 Changed 4 years ago by mrphs

Description: modified (diff)

comment:2 Changed 4 years ago by mrphs

Description: modified (diff)

comment:3 Changed 4 years ago by teor

I thought you could pass an auth cookie using the address x.y.onion, where x is the auth cookie, and y is the onion address.

comment:4 Changed 4 years ago by mrphs

I didn't know about this (where is it documented?!), and even if you can it's still hella difficult and confusing.

comment:5 in reply to:  4 ; Changed 4 years ago by teor

Replying to mrphs:

I didn't know about this (where is it documented?!), and even if you can it's still hella difficult and confusing.

I was wrong, using x.y.onion is not documented or implemented anywhere.
(I wonder why I thought that?)

comment:6 Changed 2 years ago by dmr

Cc: dmr added
Keywords: ux-team tor-hs added; UX removed

comment:7 in reply to:  5 Changed 2 years ago by dmr

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.onion exists as a way to do subdomains for an .onion, as of #6344. (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.

comment:8 Changed 2 years ago by teor

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,

comment:9 Changed 2 years ago by traumschule

where is it documented

not yet (#25277, #27680), but a user gave me this link on irc: comment:1:ticket:27680

comment:10 Changed 17 months ago by pili

Sponsor: Sponsor27

comment:11 Changed 17 months ago by gk

Sponsor: Sponsor27Sponsor27-must

Add Sponsor27-must items for Objective 2

comment:12 Changed 17 months ago by pili

Parent ID: #30000

comment:13 Changed 11 months ago by gaba

Keywords: network-team-roadmap-september added

comment:14 Changed 11 months ago by gaba

Keywords: network-team-roadmap-september removed

comment:15 Changed 9 months ago by pili

Keywords: TorBrowserTeam201911 added
Owner: changed from tbb-team to brade
Status: newassigned

comment:16 Changed 9 months ago by brade

Cc: tbb-team added

comment:17 Changed 8 months ago by pili

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201911 removed

Moving tickets to December

Changed 8 months ago by mcs

Attachment: onion-key-prefs1.png added

onion auth prefs - privacy & security panel

Changed 8 months ago by mcs

Attachment: onion-key-prefs2.png added

onion auth prefs - key dialog

comment:18 Changed 8 months ago by mcs

Cc: antonela added

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:

https://trac.torproject.org/projects/tor/raw-attachment/ticket/19757/onion-key-prefs1.png

b) Prefs dialog/overlay that opens when you click Saved Keys…:
https://trac.torproject.org/projects/tor/raw-attachment/ticket/19757/onion-key-prefs2.png

comment:19 Changed 8 months ago by pili

Points: 4

comment:20 Changed 8 months ago by 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 like Key but i wonder what others think.

comment:21 Changed 7 months ago by sysrqb

Keywords: TorBrowserTeam202001 added; TorBrowserTeam201912 removed

comment:22 Changed 7 months ago by mcs

Actual Points: 5
Points: 48

Updated points estimate and added actual points so far.

Last edited 7 months ago by mcs (previous) (diff)

comment:23 Changed 7 months ago by mcs

Keywords: TorBrowserTeam202001R added; TorBrowserTeam202001 removed
Status: assignedneeds_review

Our patches are ready for review. These build upon the #30237 changes, so you will need those patches too (the bug19757-01 branches in the various brade repos are branched from our bug30237-04 branches 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-auth directory for storage of the keys:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-01&id=c3f062c5888074cd6e2c3801353106d04f22e95a

Tor 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

comment:24 Changed 7 months ago by mcs

Test builds that include all of our v3 client authentication changes (i.e., the changes from this ticket and from #30237) are available here:

https://people.torproject.org/~mcs/volatile/v3-auth/

The bundled tor was built from tor master as of two days ago.

Standard disclaimer: Don't use these builds to run your nuclear power plant, etc.

comment:25 Changed 7 months ago by pospeselr

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._keyInfoList and this._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._keyInfoList and this._tree, then call onionAuthRemove with 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.

comment:26 Changed 7 months ago by mcs

Actual Points: 56.2

comment:27 in reply to:  25 Changed 7 months ago by mcs

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._keyInfoList and this._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._keyInfoList is _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._keyInfoList and this._tree, then call onionAuthRemove with 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 the aTorController.onionAuthRemove() call. If that async call returns without throwing we know that tor removed the key, and then we would use the .hsAddress field to re-find the row that needs to be removed from this._keyInfoList.

comment:28 Changed 7 months ago by pospeselr

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.

comment:29 Changed 7 months ago by mcs

Actual Points: 6.26.3

comment:30 Changed 7 months ago by pili

Reviewer: pospeselr, acat

comment:31 Changed 7 months ago by acat

Keywords: TorBrowserTeam202001 added; TorBrowserTeam202001R removed
Status: needs_reviewneeds_revision

In savedKeysDialog.js, line 64, controllerFailureMsg is not defined in that scope. I guess it should be declared outside the try, as you do in authPrompt.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/onionservices should 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?

comment:32 in reply to:  31 Changed 7 months ago by mcs

Replying to acat:

In savedKeysDialog.js, line 64, controllerFailureMsg is not defined in that scope. I guess it should be declared outside the try, as you do in authPrompt.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/onionservices should 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.

comment:33 Changed 7 months ago by dgoulet

The New Identity basically sends a NEWNYM signal 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.

comment:34 Changed 6 months ago by 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?

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.

comment:35 Changed 6 months ago by mcs

Actual Points: 6.36.5

comment:36 in reply to:  34 ; Changed 6 months ago by mcs

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.

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_REMOVE command was received on the control port.

comment:37 in reply to:  36 Changed 6 months ago by mcs

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_REMOVE command was received on the control port.

This issue is being handled in #33148.

comment:38 Changed 6 months ago by mcs

Keywords: TorBrowserTeam202002R added; TorBrowserTeam202001 removed
Status: needs_revisionneeds_review

We rebased our patches to sit on top of the #30237 commits that sysrqb merged, fixed the controllerFailureMsg issue 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 under browser/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 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 one?

Here are the new patches (all are on bug19757-02 branches 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

comment:39 Changed 6 months ago by mcs

Actual Points: 6.57.1

comment:40 Changed 6 months ago by pospeselr

This looks good to me, but a few nits that don't really matter:

https://gitweb.torproject.org/user/brade/tor-browser.git/tree/browser/components/onionservices/content/authPrompt.js?id=06f115cc028e27124b48b40aaa83e988c28a9d98#n165

165 let checkboxElem = this._getCheckboxElement();
166 let isPermanent = (checkboxElem && checkboxElem.checked);

I'd move these down within the try block before the onionAuthAdd call as it doesn't seem like isPermanent is used anywhere else in the higher scope.

https://gitweb.torproject.org/user/brade/tor-browser.git/tree/browser/components/onionservices/content/authPreferences.js?id=06f115cc028e27124b48b40aaa83e988c28a9d98#n19

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 string something like 'selectors' or something like that (here and also in authUtil.jsm, savedKeysDialog.js, etc)

Apart from those nits it looks good!

comment:41 in reply to:  40 Changed 6 months ago by mcs

Replying to pospeselr:

This looks good to me, but a few nits that don't really matter:

https://gitweb.torproject.org/user/brade/tor-browser.git/tree/browser/components/onionservices/content/authPrompt.js?id=06f115cc028e27124b48b40aaa83e988c28a9d98#n165

165 let checkboxElem = this._getCheckboxElement();
166 let isPermanent = (checkboxElem && checkboxElem.checked);

I'd move these down within the try block before the onionAuthAdd call as it doesn't seem like isPermanent is used anywhere else in the higher scope.

Fixed.

https://gitweb.torproject.org/user/brade/tor-browser.git/tree/browser/components/onionservices/content/authPreferences.js?id=06f115cc028e27124b48b40aaa83e988c28a9d98#n19

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 string something 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 we have to add more topic values, etc.

We also made this a squash! commit so eventually it will be combined with the #30237 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.

Last edited 6 months ago by mcs (previous) (diff)

comment:42 Changed 6 months ago by mcs

Actual Points: 7.17.5

comment:43 Changed 6 months ago by sysrqb

Reviewer: pospeselr, acatpospeselr, acat, sysrqb

comment:44 Changed 6 months ago by sysrqb

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 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.

comment:45 Changed 6 months ago by sysrqb

Reviewer: pospeselr, acat, sysrqbpospeselr, acat, sysrqb, emmapeel

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.

Torbutton:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19757-02&id=fddbe371f2eb43d7d6007561db38d1ad8a6ff8e4

+<!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:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19757-02&id=f53497c7255b040b6c97738c3c64e98b69cc96e4

+torlauncher.onionauthdir_missing=The Tor onion authentication directory does not exist and could not be created.

comment:46 in reply to:  38 Changed 6 months ago by sysrqb

Replying to mcs:

Here are the new patches (all are on bug19757-02 branches 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 078ae18e5f1cbf888e7c86c8aecaa383a727bf0d on 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 aed69dc95387429e18b18ad578fb78d4a83d91f2 on tor-browser-68.5.0esr-9.5-1.

comment:47 in reply to:  45 Changed 6 months ago by sysrqb

Resolution: fixed
Status: needs_reviewclosed

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!

comment:48 Changed 6 months ago by sysrqb

Keywords: tbb-9.5a6 added
Note: See TracTickets for help on using tickets.