prop304: Implement SOCKS new HS error code

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 (moved)) and adds any client auth creds with the commands from #30381 (moved).

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

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

changed milestone to %Tor: 0.4.3.x-final

Trac:
Parent Ticket: #14389 (moved)

added 042-deferred-20190918 component::core tor/tor hs-auth milestone::Tor: 0.4.3.x-final network-team-roadmap-september owner::dgoulet parent::14389 points::6 priority::medium resolution::fixed reviewer::asn severity::normal sponsor::27-must status::closed tbb-usability tor-hs tor-spec type::enhancement labels

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 (moved)).

Trac:
Points: N/A to 6
Reviewer: 6 to N/A

Trac:
Status: new to accepted
Owner: N/A to dgoulet

Trac:
Sponsor: N/A to Sponsor27-must

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.

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.

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

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.

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

Trac:
Keywords: N/A deleted, network-team-roadmap-2019-Q1Q2 added

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

Trac:
Keywords: N/A deleted, tor-spec added
Status: accepted to needs_review

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.

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?

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.

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

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.

Trac:
Status: needs_review to accepted

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.

Trac:
Reviewer: N/A to asn
Status: accepted to needs_review

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.

Waiting for the unittests here.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

Did an initial review on the PR!

Trac:
Status: needs_review to needs_revision

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

Trac:
Owner: dgoulet to N/A
Status: needs_revision to assigned

Trac:
Status: assigned to accepted
Owner: N/A to dgoulet

Trac:
Status: accepted to needs_revision

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.

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.

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

Trac:
Keywords: network-team-roadmap-2019-Q1Q2 deleted, network-team-roadmap-september added

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

Trac:
Type: defect to enhancement

Defer numerous 0.4.2 tickets to 0.4.3.

Trac:
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.3.x-final
Keywords: N/A deleted, 042-deferred-20190918 added

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! :)

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

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.

Should we put this in needs_review?

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.

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 (moved): https://github.com/torproject/tor/pull/1483

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

Trac:
Status: needs_revision to needs_review

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 (moved) implemented as part of this branch ;)

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

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

Trac:
Status: needs_review to merge_ready

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

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 48h

mentioned in issue #32543 (moved)

mentioned in issue #32546 (moved)