Opened 3 months ago

Last modified 3 weeks ago

#30382 needs_revision defect

Provide control port event for when we are missing v3 client auth for an onion

Reported by: asn Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tbb-usability, hs-auth, network-team-roadmap-2019-Q1Q2, tor-spec
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 (28)

comment:1 Changed 3 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 3 months ago by asn

Points: 6
Reviewer: 6

comment:3 Changed 3 months ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:4 Changed 2 months ago by dgoulet

Sponsor: Sponsor27-must

comment:5 Changed 2 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 2 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 2 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 2 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 2 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 2 months ago by gaba

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

comment:11 Changed 8 weeks 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 8 weeks 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 8 weeks 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 8 weeks 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 8 weeks 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 7 weeks 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 7 weeks ago by dgoulet

Status: needs_reviewaccepted

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

Status: needs_reviewneeds_revision

Waiting for the unittests here.

comment:21 Changed 6 weeks ago by dgoulet

Status: needs_revisionneeds_review

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

comment:22 Changed 5 weeks ago by asn

Status: needs_reviewneeds_revision

Did an initial review on the PR!

comment:23 Changed 5 weeks 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 5 weeks ago by dgoulet

Owner: set to dgoulet
Status: assignedaccepted

comment:25 Changed 5 weeks ago by dgoulet

Status: acceptedneeds_revision

comment:26 Changed 3 weeks 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 3 weeks 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 3 weeks 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.

Note: See TracTickets for help on using tickets.