#30381 closed enhancement (fixed)

Provide control port commands to ADD/REMOVE/VIEW v3 client-auth

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tbb-usability, ux-team, hs-auth, network-team-roadmap-september, 042-deferred-20190918
Cc: antonela, arthuredelstein, brade, mcs, gk, michael, special, erinn, patrick@…, lunar, linda, dmr Actual Points: 5
Parent ID: #14389 Points: 6
Reviewer: dgoulet Sponsor: Sponsor27-must

Description (last modified by asn)

We need control port commands so that TB can add/remove/view client auth credentials.

Furthermore, the 'add' command should be able to decrypt any existing non-decryptable descriptors in the cache (see #30382).

Child Tickets

TicketTypeStatusOwnerSummary
#32667defectclosedCID 1456199: Resource leak in test_hs_control_store_permanent_creds()

Attachments (1)

tor-os-auth-crash.txt (4.7 KB) - added by mcs 16 months ago.
assertion failure log

Download all attachments as: .zip

Change History (37)

comment:1 Changed 18 months ago by asn

Points: 6

comment:3 Changed 18 months ago by asn

Description: modified (diff)

comment:4 Changed 18 months ago by asn

Owner: set to asn
Status: newassigned

comment:6 Changed 18 months ago by pili

Sponsor: Sponsor27-must

comment:7 Changed 17 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:8 Changed 17 months ago by asn

Status: assignedneeds_review

Pushed branch here: https://github.com/torproject/tor/pull/1070

It's based on david's #30382 branch so that I can use his 'decrypt pending descriptors upon add' functionality. That's also the only part that is not unittested.

I also pushed a fixup commit to my spec patch based on the latest implementation: https://github.com/torproject/torspec/pull/81/commits/dafda3944241e4ab6dfe0fee90d2e97979ac8f94

comment:9 Changed 17 months ago by asn

Reviewer: dgoulet

comment:10 Changed 17 months ago by asn

Actual Points: 4.5

comment:11 Changed 17 months ago by dgoulet

Status: needs_reviewneeds_revision

Review on the PR. Looking good. No show stopper, just few minor things.

Changed 16 months ago by mcs

Attachment: tor-os-auth-crash.txt added

assertion failure log

comment:12 Changed 16 months ago by mcs

Kathy and I are trying to integrate our work-in-progress Tor Browser code with a tor we built from the code at https://github.com/asn-d6/tor/tree/bug30381. Unfortunately, when we access a v3 onion service that requires client auth we encounter an assertion failure inside tor. See the attached file tor-os-auth-crash.txt. Please let us know if you need any more info in order to debug the problem.

comment:13 Changed 16 months ago by dgoulet

Oh no... that is currently fixed in #30382... Let me do an updated branch for you to work with:

Try to use this branch: ticket30381_042_01
https://gitweb.torproject.org/user/dgoulet/tor.git/log/?h=ticket30381_042_01

It is not clean or anything but will have the latest code we have for your testing. Hopefully, should help you go forward!

comment:14 in reply to:  13 Changed 16 months ago by mcs

Replying to dgoulet:

Oh no... that is currently fixed in #30382... Let me do an updated branch for you to work with:

Try to use this branch: ticket30381_042_01
https://gitweb.torproject.org/user/dgoulet/tor.git/log/?h=ticket30381_042_01

Thanks; this works much better. We now receive the correct SOCKS error code (after a 120 second delay, which is a problem asn already mentioned in his review of your #30382 pull request). Kathy and I should be able to make more progress using this branch though. Next we will experiment with ONION_CLIENT_AUTH_ADD.

comment:15 Changed 15 months ago by gaba

Keywords: network-team-roadmap-september added; network-team-roadmap-2019-Q1Q2 removed

comment:16 Changed 15 months ago by asn

OK I pushed fixes to David's comments in: https://github.com/torproject/tor/pull/1070

The branch needs to be rebased to latest master, and also incorporated withe fixes of comment:13. I will do that when David fixes up #30382 and we have a final branch from that side.

comment:17 Changed 14 months ago by nickm

Type: defectenhancement

Mark a number of current 0.4.2.x "defects" as "enhancements."

comment:18 Changed 13 months ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final

Defer numerous 0.4.2 tickets to 0.4.3.

comment:19 Changed 12 months ago by asn

OK here is branch based on David's updated #30381: https://github.com/torproject/tor/pull/1483

This is meant to be used by the Tb team for testing since it includes all features.

comment:20 Changed 11 months ago by asn

Status: needs_revisionneeds_review

comment:21 Changed 11 months ago by dgoulet

Status: needs_reviewneeds_revision

We need a version based on the latest master once #30382 gets merged. Moving to needs_revision until then.

comment:22 Changed 11 months ago by asn

Status: needs_revisionneeds_review

OK now that #30382 is merged upstream, here is the #30381 branch rebased to latest master: https://github.com/torproject/tor/pull/1550

comment:23 Changed 11 months ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

Awesome work! lgtm! Merged to master!

comment:24 Changed 11 months ago by mcs

Flags=Permanent does not seem to have any effect. Is there another ticket for that or should someone open one?

comment:25 in reply to:  24 Changed 11 months ago by dgoulet

Replying to mcs:

Flags=Permanent does not seem to have any effect. Is there another ticket for that or should someone open one?

Wow you are right! CLIENT_AUTH_FLAG_IS_PERMANENT is not used in the HS client code. We need to open a ticket for this. I've pinged asn about it. Thanks mcs!

comment:26 Changed 11 months ago by asn

Hm, I opened #32562 for this. Is this something we want to support as part of s27?

comment:27 in reply to:  26 Changed 11 months ago by mcs

Replying to asn:

Hm, I opened #32562 for this. Is this something we want to support as part of s27?

Kathy and I have been working under the assumption that we need this functionality. Without it, someone who users an onion that requires client authentication will (obviously) need to re-enter their key every time they restart their browser... and that would be very inconvenient. But I admit that I do not know how people use onion services in the "real world." Antonela (or anyone) — any thoughts on this?

comment:28 Changed 11 months ago by dgoulet

Good to know. We will make it in 043 for sure soon. So, assume it is there :).

The gist of this feature is that a default ClientOnionAuthDir will be used to write the file in unless it is specified ofc.

So as Tor Browser, you might want to set that torrc value to the place you want those client auth file(s) to be written.

comment:29 Changed 11 months ago by lunar

IMHO, I think this should be treated the same way as HTTP “Basic Auth”.

Ideally, I'd like a checkbox “Save this key” and then I should be able to look it up in Tor Browser “Saved logins” like other passwords.

Maybe this is worth more research, but I don't see why conceptually this should be treated differently than other passwords. :)

comment:30 in reply to:  29 Changed 11 months ago by mcs

Replying to lunar:

IMHO, I think this should be treated the same way as HTTP “Basic Auth”.

Ideally, I'd like a checkbox “Save this key” and then I should be able to look it up in Tor Browser “Saved logins” like other passwords.

Kathy and I envision something very similar, although at least for now we do not plan to integrate the list of saved v3 onion service keys with the browser's "Saved logins" UI (while that might be possible, it would require some complex and difficult-to-maintain patches to Firefox). Our vision is to add a checkbox to the client auth prompt along with a text box where the user can enter an optional nickname, and we will add a management interface to about:preferences that allows people to view, remove, and possibly add additional keys.

But if this is not needed, we can save a lot of development time :)

comment:31 Changed 11 months ago by antonela

IMHO, I think this should be treated the same way as HTTP “Basic Auth”.

+1 to lunar's comment. Ideally, recurrent users can save the passphrase, so they don't need to paste it every time.

Altho, this is not a blocker for this objective. If we don't have capacity to work on it for this delivery, we can aim to reach it for the next iteration.

comment:32 Changed 11 months ago by mcs

Kathy and I experimented with this API today. Very cool stuff! Unfortunately, there is one problem: the ONION_CLIENT_AUTH_VIEW response needs to be changed to include the HS address. Otherwise, we cannot present a management interface and allow users to remove keys, etc. (they won't know what .onion each key is associated with).

comment:33 Changed 11 months ago by asn

Resolution: fixed
Status: closedreopened

That's a good point mcs. Reopening the ticket for that.

comment:34 Changed 11 months ago by asn

Actual Points: 4.55
Status: reopenedneeds_review

Please find torspec branch here: https://github.com/torproject/torspec/pull/97
and little-t-tor branch here: https://github.com/torproject/tor/pull/1582

Since this hasn't been released I guess we don't need a changes file?

comment:35 Changed 11 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm. I'll let you merge it asn since I'm still with a "illness brain" ;).

comment:36 Changed 11 months ago by asn

Merged it! Thanks! Waiting for the child ticket to close before I can close this one.

mcs let us know how you like it!

comment:37 Changed 10 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.