Opened 4 years ago

Last modified 4 days ago

#19757 needs_review defect

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, TorBrowserTeam202001R
Cc: dmr, tbb-team, antonela Actual Points: 6.2
Parent ID: #30000 Points: 8
Reviewer: 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 7 weeks ago.
onion auth prefs - privacy & security panel
onion-key-prefs2.png (392.4 KB) - added by mcs 7 weeks ago.
onion auth prefs - key dialog

Download all attachments as: .zip

Change History (30)

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 21 months ago by dmr

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

comment:7 in reply to:  5 Changed 21 months 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 21 months 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 17 months 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 10 months ago by pili

Sponsor: Sponsor27

comment:11 Changed 10 months ago by gk

Sponsor: Sponsor27Sponsor27-must

Add Sponsor27-must items for Objective 2

comment:12 Changed 10 months ago by pili

Parent ID: #30000

comment:13 Changed 5 months ago by gaba

Keywords: network-team-roadmap-september added

comment:14 Changed 5 months ago by gaba

Keywords: network-team-roadmap-september removed

comment:15 Changed 2 months ago by pili

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

comment:16 Changed 2 months ago by brade

Cc: tbb-team added

comment:17 Changed 8 weeks ago by pili

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201911 removed

Moving tickets to December

Changed 7 weeks ago by mcs

Attachment: onion-key-prefs1.png added

onion auth prefs - privacy & security panel

Changed 7 weeks ago by mcs

Attachment: onion-key-prefs2.png added

onion auth prefs - key dialog

comment:18 Changed 7 weeks 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 7 weeks ago by pili

Points: 4

comment:20 Changed 6 weeks 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 3 weeks ago by sysrqb

Keywords: TorBrowserTeam202001 added; TorBrowserTeam201912 removed

comment:22 Changed 12 days ago by mcs

Actual Points: 5
Points: 48

Updated points estimate and added actual points so far.

Last edited 12 days ago by mcs (previous) (diff)

comment:23 Changed 9 days 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 9 days 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 5 days 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 5 days ago by mcs

Actual Points: 56.2

comment:27 in reply to:  25 Changed 5 days 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 4 days 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.

Note: See TracTickets for help on using tickets.