Opened 7 months ago

Closed 2 weeks ago

#30382 closed enhancement (fixed)

prop304: Implement SOCKS new HS error code

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

Description

For TB to be able to alert the user that they need to input their client auth credentials we need an appropriate control port event.

In particular:

1) When Tor fails to decrypt the second layer of desc encryption, we issue the CLIENT_AUTH_NEEDED <onion> <reason> event. Tor does not go to fetch more descs from the hsdir for this onion.

2) At the same time, we store the broken descriptor into the hs cache, with a special flag that says "missing client auth" and hence desc is NULL.

3) When TB intercepts the event it presents the user with a dialogue (#30237) and adds any client auth creds with the commands from #30381.

4) As part of the #30381 commands the descriptor is decrypted.

5) TB issues another SOCKS request which uses the right descriptor and goes forward.

Child Tickets

Change History (43)

comment:1 Changed 7 months ago by asn

I was thinking that the CLIENT_AUTH_NEEDED event would also need a reason field so that it can distinguish between client auth is wrong and client auth is missing, so that TB can give better error messages (see #30237).

comment:2 Changed 7 months ago by asn

Points: 6
Reviewer: 6

comment:3 Changed 7 months ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:4 Changed 7 months ago by dgoulet

Sponsor: Sponsor27-must

comment:5 Changed 7 months ago by mcs

This all make sense to Kathy and me. One quick question — what will the timing be for:

  1. tor generating a CLIENT_AUTH_NEEDED event
  2. tor failing the SOCKS CONNECT

?

Specifically, I wonder if it will be safe for Tor Browser to assume the control port event will be sent before the browser receives a failure for the CONNECT, or at least to assume that will occur in most cases.

comment:6 in reply to:  5 ; Changed 7 months ago by dgoulet

Replying to mcs:

Specifically, I wonder if it will be safe for Tor Browser to assume the control port event will be sent before the browser receives a failure for the CONNECT, or at least to assume that will occur in most cases.

Hmmm... Control port events, when they occur, are queued in a list and then an tor main loop event will send the queued events on the socket.

So, if I'm not mistaken, you will get a connect failure _before_ you get the control port event because tor will return the SOCKS failure before that control port "empty queue" event is triggered.

comment:7 in reply to:  6 Changed 7 months ago by mcs

Replying to dgoulet:

So, if I'm not mistaken, you will get a connect failure _before_ you get the control port event because tor will return the SOCKS failure before that control port "empty queue" event is triggered.

That makes sense and is good to know. I asked because the technique used by Arthur's old v2 auth Torbutton patch from ticket:14389#comment:14 relies on receiving the control port event before the browser receives a connection failure indication via SOCKS. Unfortunately, it seems like we are back to adding a new error code to SOCKS5 or figuring out how to switch the browser to use HTTP CONNECT (which is a large and risky task due to the complexity of Firefox's networking stack and how HTTP CONNECT was implemented there).

comment:8 Changed 7 months ago by asn

ACK. Makes sense.

While it seems like we are moving towards the SOCKS direction, I should mention that comment:52:ticket:14389 also asks for some enhancements to our SOCKS handshake. We should probably oen a new ticket for this. Also, wrt this ticket, we should consider how to use prop#229 for any additional SOCKS error codes.

comment:9 Changed 7 months ago by dgoulet

Lets go with SOCKS5 error extension code! We'll re-use the prop229 and open a new one for only extending error codes!

comment:10 Changed 7 months ago by gaba

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

comment:11 Changed 7 months ago by dgoulet

Keywords: tor-spec added
Status: acceptedneeds_review

In my torspec repo: https://git.torproject.org/user/dgoulet/torspec.git

Branch: ticket30382_01

I think there are too many codes there for what we need here but I wanted to at least get the basic errors implemented as well. The last two are the one TB needs for this. Going in needs_review, once we are happy with the spec, lets put it back in Assigned so we can do the code and post the proposal on tor-dev@.

comment:12 in reply to:  11 ; Changed 7 months ago by mcs

Replying to dgoulet:

In my torspec repo: https://git.torproject.org/user/dgoulet/torspec.git

Branch: ticket30382_01

I think there are too many codes there for what we need here but I wanted to at least get the basic errors implemented as well. The last two are the one TB needs for this.

Kathy and I think the proposal looks good. Just a couple of comments:

  • I had trouble understanding the note near the beginning. Maybe reword to: "When Tor Browser supports HTTPCONNECT, we plan to stop using these SOCKS5 extensions."
  • Regarding compatibility, it seems like it would be safer for tor to not emit these new error codes unless enabled via a config option (maybe a SocksPort flag). Otherwise, non Tor Browser clients that use the SocksPort may be unhappy. Or maybe enable by default but provide an "escape hatch" that allows them to be disabled somehow.

comment:13 in reply to:  12 ; Changed 7 months ago by dgoulet

Replying to mcs:

Replying to dgoulet:

In my torspec repo: https://git.torproject.org/user/dgoulet/torspec.git

Branch: ticket30382_01

I think there are too many codes there for what we need here but I wanted to at least get the basic errors implemented as well. The last two are the one TB needs for this.

Kathy and I think the proposal looks good. Just a couple of comments:

  • I had trouble understanding the note near the beginning. Maybe reword to: "When Tor Browser supports HTTPCONNECT, we plan to stop using these SOCKS5 extensions."

Was mostly a "future warning" for us to only extend SOCKS5 error code because it is a "bandaid" and ultimately HTTPCONNECT is the way forward. I'll rephrase.

  • Regarding compatibility, it seems like it would be safer for tor to not emit these new error codes unless enabled via a config option (maybe a SocksPort flag). Otherwise, non Tor Browser clients that use the SocksPort may be unhappy. Or maybe enable by default but provide an "escape hatch" that allows them to be disabled somehow.

Hmmm SocksPort flag could be an option. The other way is to create a new authentication method like prop229 does and thus the new error code are only returned if TB authenticated with this method. Former is simple, later is more involving but probably more portable for future compat?

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

Replying to dgoulet:

Hmmm SocksPort flag could be an option. The other way is to create a new authentication method like prop229 does and thus the new error code are only returned if TB authenticated with this method. Former is simple, later is more involving but probably more portable for future compat?

I don't know what the right answer is, but on the browser side we could accommodate either solution. A new SOCKS5 auth method would be more work for us, but should be do-able. But maybe other SOCKS5 clients won't care if they receive new error codes? I don't know.

comment:15 Changed 7 months ago by mcs

To follow up on the conversation we had on IRC, the Tor Browser code (Firefox 60esr) translates unknown SOCKS5 error codes to PR_CONNECT_REFUSED_ERROR, aka connection refused. Examples: nsSOCKSSocketInfo::ReadV5UsernameResponse() and nsSOCKSSocketInfo::ReadV5ConnectResponseTop().

comment:16 Changed 6 months ago by dgoulet

Ok I just posted a new version after discussing with asn. It is also now on tor-dev@:

https://lists.torproject.org/pipermail/tor-dev/2019-May/013835.html

I'll get onto the coding part soon.

comment:17 Changed 6 months ago by dgoulet

Status: needs_reviewaccepted

comment:18 Changed 6 months ago by dgoulet

Reviewer: asn
Status: acceptedneeds_review

Code in ticket30382_042_01. I need asn to look at it to see if the approach seems sensible.

Lots of pieces have been touched but in theory code behavior changed very little.

comment:19 in reply to:  18 Changed 6 months ago by asn

Replying to dgoulet:

Code in ticket30382_042_01. I need asn to look at it to see if the approach seems sensible.

Lots of pieces have been touched but in theory code behavior changed very little.

Approach does seem sensible if it actually works as intended. Bonus points if it's testable.

comment:20 Changed 6 months ago by asn

Status: needs_reviewneeds_revision

Waiting for the unittests here.

comment:21 Changed 6 months ago by dgoulet

Status: needs_revisionneeds_review

PR: https://github.com/torproject/tor/pull/1087
Branch: ticket30382_042_01

comment:22 Changed 6 months ago by asn

Status: needs_reviewneeds_revision

Did an initial review on the PR!

comment:23 Changed 6 months ago by gaba

Owner: dgoulet deleted
Status: needs_revisionassigned

dgoulet will assign himself to the ones he is working on right now.

comment:24 Changed 6 months ago by dgoulet

Owner: set to dgoulet
Status: assignedaccepted

comment:25 Changed 6 months ago by dgoulet

Status: acceptedneeds_revision

comment:26 Changed 5 months ago by dgoulet

Apparently (feedback from mcs), the extended errors is sent out properly but only after SocksTimeout. Most likely, the connection is not properly closed when the extended error is set.

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

Replying to dgoulet:

Apparently (feedback from mcs), the extended errors is sent out properly but only after SocksTimeout. Most likely, the connection is not properly closed when the extended error is set.

A little more info about this: when we use SocksTimeout 15, the correct SOCKS error is sent after 30 seconds the first time we try to load the v3 .onion, but it is sent after 15 seconds the second time we try to load it.

comment:28 Changed 5 months ago by mcs

Anyone who is not watching #30237 but who is interested in the browser UI aspects of this effort should read ticket:30237#comment:19.

comment:29 Changed 5 months ago by gaba

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

comment:30 Changed 3 months ago by nickm

Type: defectenhancement

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

comment:31 Changed 3 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:32 Changed 7 weeks ago by dgoulet

Summary: Provide control port event for when we are missing v3 client auth for an onionprop304: Implement SOCKS new HS error code

Renaming ticket to reflect the work actually done in this ticket now. It is not about control event anymore. This might come later.

tor-dev@ thread that explains the basics of what will be done with prop304: https://lists.torproject.org/pipermail/tor-dev/2019-October/014053.html

I will also create a new branch after I address everything from asn and fix things to follow the proposal on the tor-dev@ thread above . And will rebase to latest 043 in the process. Expect new things! :)

comment:33 Changed 7 weeks ago by dgoulet

New branch is here: ticket30382_043_01.
PR: https://github.com/torproject/tor/pull/1423

So far is implements there extended error codes:

X'F0' Onion Service Descriptor Can Not be Found
X'F1' Onion Service Descriptor Is Invalid
X'F4' Onion Service Missing Client Authorization
X'F5' Onion Service Wrong Client Authorization

The two missing ones are the intro failure and RP failure which are _very_ _very_ tricky to be honest.

For now, the SOCKS reply is not send back before the rendezvous has opened the stream so basically like if there is no optimistic data.

Last edited 7 weeks ago by dgoulet (previous) (diff)

comment:34 Changed 6 weeks ago by asn

Should we put this in needs_review?

comment:35 in reply to:  34 ; Changed 6 weeks ago by dgoulet

Replying to asn:

Should we put this in needs_review?

Not yet, still missing 2 error codes that I need to figure out how to send back properly... not easy.

However, it is definitely ready for TB to use for testing.

comment:36 in reply to:  35 Changed 5 weeks ago by asn

Replying to dgoulet:

Replying to asn:

Should we put this in needs_review?

Not yet, still missing 2 error codes that I need to figure out how to send back properly... not easy.

However, it is definitely ready for TB to use for testing.

ACK. In this case, I have made a branch for TB to use for testing which also includes the control port commands from #30381: https://github.com/torproject/tor/pull/1483

TB team please use the PR in this comment! :)

comment:37 Changed 3 weeks ago by dgoulet

Status: needs_revisionneeds_review

comment:38 Changed 3 weeks ago by asn

Status: needs_reviewneeds_revision

Very good stuff! I have reviewed and requested various changes but mainly for tech debt and documentation things.

Also it seems like we basically have #30022 implemented as part of this branch ;)

comment:39 Changed 3 weeks ago by dgoulet

Status: needs_revisionneeds_review

comment:40 Changed 3 weeks ago by asn

Status: needs_reviewneeds_revision

comment:41 Changed 3 weeks ago by dgoulet

Status: needs_revisionneeds_review

comment:42 Changed 3 weeks ago by asn

Status: needs_reviewmerge_ready

OK great! Merged! Now action moves to #30381.
We also need to make a new ticket for the other two error codes.

Last edited 3 weeks ago by asn (previous) (diff)

comment:43 Changed 2 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Ok we now have #30381 and this merged. We are done. Missing codes are in #32542

Note: See TracTickets for help on using tickets.