prop224: Implement standard client authorization

With upcoming work of proposal 224, we'll have a new better improved client authentication scheme that needs to be implemented.

changed milestone to %Tor: 0.3.5.x-final in legacy/trac

added 035-roadmap-master in Legacy / Trac 035-triaged-in-20180711 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.5.x-final in Legacy / Trac owner::haxxpop in Legacy / Trac parent::25955 in Legacy / Trac points::3 in Legacy / Trac priority::very high in Legacy / Trac prop224 in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-hs in Legacy / Trac type::enhancement in Legacy / Trac labels

Latest in https://gitweb.torproject.org/user/asn/torspec.git/commit/?h=prop224_client_auth_3

Some backlogs: https://lists.torproject.org/pipermail/tor-dev/2016-November/011632.html

Trac:
Parent: N/A to legacy/trac#12424 (moved)

Trac:
Sponsor: SponsorR-must to SponsorR-can
Summary: prop224: Implement client authentication to prop224: Implement client authorization

Trac:
Owner: N/A to asn
Status: new to assigned

Triaged out on December 2016 from 030 to 031.

Trac:
Milestone: Tor: 0.3.0.x-final to Tor: 0.3.1.x-final
Keywords: N/A deleted, triage-out-030-201612 added

Prioritize prop224 tickets for 031 milestone. They are all "Enhancement".

Trac:
Priority: Medium to Very High

prop224 tickets going in 032 for early merge. Decided after Amsterdam meeting.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Trac:
Keywords: triage-out-030-201612 deleted, N/A added

Not going in the initial release of prop224.

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.3.x-final
Parent: legacy/trac#12424 (moved) to N/A

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final

Trac:
Owner: asn to haxxpop

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

Trac:
Username: pege
Cc: N/A to peter@arbitrary.ch

We can take this in 0.3.4 if the code is ready, and I hear haxxpop is working on it. :)

Trac:
Keywords: 034-removed-20180328 deleted, N/A added

Trac:
Cc: peter@arbitrary.ch to peter@arbitrary.ch, dmr

See haxxpop's PR here: https://github.com/torproject/tor/pull/36

YEEEEAHHHH I really need HidServAuth for v3 onion!!!

Replying to cypherpunks:

YEEEEAHHHH I really need HidServAuth for v3 onion!!!

Please give haxxpop's code (comment:16) a try and let us know how it works for you.

You can test it by adding HiddenServiceAuthorizeClient basic <client_name> on the service torrc and HidServAuth <onion address> <base64-encoded x25519 private key> on the client torrc. You can get the private key from client_authorized_privkeys/<client_name>.privkey on the service file directory

Someone should also test that it works when the client generates the private key, and only gives the public key to the onion service.

I just tested it. It works.

You can just put the public key in client_authorized_pubkeys before running the HS. The HS will not generate a new key and it will use the public key in that file.

Trac:
Parent: N/A to legacy/trac#25955 (moved)

Uh... I need 'stealth', not loud 'basic'...

Trac:
Summary: prop224: Implement client authorization to prop224: Implement basic client authorization

"basic" and "stealth" don't make sense for v3 onion services, because authentication works differently in v3. We have "descriptor", "intro", and "standard" (both).

Depending on your use case, you might be looking for "descriptor" auth.

Trac:
Summary: prop224: Implement basic client authorization to prop224: Implement standard client authorization

meejah would like feature flags for v3 onion service client authentication. https://trac.torproject.org/projects/tor/ticket/24617#comment:5

I suggest we have separate flags for descriptor and intro.

For new options, we could use GETINFO config/names: https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n729

But we are re-using existing options, so I suggest we create: config/onions/versions = 2 3 config/onions/auth/descriptor = 1 config/onions/auth/intro = 1

We might also want a flag when v3 single onions support IPv6-only, but that's a separate ticket.

Trac:
Status: assigned to needs_revision

Did an initial review of haxxpop's branch here. Next steps include: More careful review, valgrind, more testing on chutney and more testing on real net.

We are hoping to land this in 035.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

I've commented on about 1/4 of the whole branch. I hope to continue tomorrow!

Trac:
Reviewer: N/A to dgoulet

Ok I've got the rest of my review in. There are couple show stopper comments that we need to address/discuss. Thanks!

Trac:
Sponsor: SponsorR-can to N/A
Keywords: 034-triage-20180328 deleted, 035-roadmap-master added

Trac:
Keywords: N/A deleted, 035-triaged-in-20180711 added

Latest spec branch that asn/dgoulet/haxxpop agree on: asn/ticket20700_01

Trac:
Cc: peter@arbitrary.ch, dmr to peter@arbitrary.ch, dmr, nickm

Notes:

• Apparently we have two things called Appendix E in rend-spec-v3.txt now. I don't think this is new with this patch, but let's fix that.
• When you list "./authorized_clients/alice", etc in appendix F, do you intend to specify that this list has to be inside the service directory? I ask because if you're doing this, I don't think it makes sense to list them all separately in the torrc, and list the directory separately as well.
• Do we want to allow multiple keys per file?
• I'd suggest renaming all the client options so that they don't start with "HiddenService": It makes things much easier if only our service-side options start with "HiddenService". How about ClientOnionAuth or something?
• We should say what happens if Tor encounters an unrecognized auth-type, user name, or onion service name in one of these files. I say it should ignore that key.

Replying to nickm:

Notes:

• Apparently we have two things called Appendix E in rend-spec-v3.txt now. I don't think this is new with this patch, but let's fix that.
• When you list "./authorized_clients/alice", etc in appendix F, do you intend to specify that this list has to be inside the service directory? I ask because if you're doing this, I don't think it makes sense to list them all separately in the torrc, and list the directory separately as well.
• Do we want to allow multiple keys per file?
• I'd suggest renaming all the client options so that they don't start with "HiddenService": It makes things much easier if only our service-side options start with "HiddenService". How about ClientOnionAuth or something?
• We should say what happens if Tor encounters an unrecognized auth-type, user name, or onion service name in one of these files. I say it should ignore that key.

Thanks for the review. Pushed relevant fixes on my github repo, same branch name.

Trac:
Status: needs_revision to needs_review

I've commented on the new PR. Very very good stuff :). Minor easy-to-fix things I pointed it out.

There is an issue that I've been discussing with haxxpop on IRC so I'll summarize it here. If the client gets the descriptor but can't decrypt it, there are two cases:

1. Client has no client authorization configured for the .onion.

What happens in this case is that we end up with many scaring warnings:

Aug 22 14:07:03.704 [warn] Encrypted service descriptor MAC check failed
Aug 22 14:07:03.704 [warn] Decrypting encrypted desc failed.
Aug 22 14:07:03.704 [warn] Service descriptor decryption failed.
Aug 22 14:07:03.704 [warn] Could not parse received descriptor as client.

... and then the client will refetch the descriptor on all HSDir leading to 8 times these failures. So the question is what to do if decryption fails? My opinion is:

• Downgrade the logs to info(). Then, iff the client can't decrypt the descriptor, then log info that it probably doesn't have a valid authorization for the service and stop the refetch. Creating a thundering herd on all HSDir because the first can't be decrypted is not good imo.

2. Client has client authorization configured for the .onion but not working.

In this case, same will happen as above but we do know in this case that the client has an authorization for the .onion but it simply didn't worked. So same as above, I would not make the client refetch but this time log notice that their configured authorization is not working.

Trac:
Status: needs_review to needs_revision

Replying to dgoulet:

I've commented on the new PR. Very very good stuff :). Minor easy-to-fix things I pointed it out.

There is an issue that I've been discussing with haxxpop on IRC so I'll summarize it here. If the client gets the descriptor but can't decrypt it, there are two cases:

1. Client has no client authorization configured for the .onion.

What happens in this case is that we end up with many scaring warnings: {{{ Aug 22 14:07:03.704 [warn] Encrypted service descriptor MAC check failed Aug 22 14:07:03.704 [warn] Decrypting encrypted desc failed. Aug 22 14:07:03.704 [warn] Service descriptor decryption failed. Aug 22 14:07:03.704 [warn] Could not parse received descriptor as client. }}}

... and then the client will refetch the descriptor on all HSDir leading to 8 times these failures. So the question is what to do if decryption fails? My opinion is:

• Downgrade the logs to info(). Then, iff the client can't decrypt the descriptor, then log info that it probably doesn't have a valid authorization for the service and stop the refetch. Creating a thundering herd on all HSDir because the first can't be decrypted is not good imo.

Sounds plausible. Perhaps we should still log a single line in notice that the client tried to access an HS with client authorization? So that the user "learns" why the connection failed, otherwise it's completely hidden.

2. Client has client authorization configured for the .onion but not working.

In this case, same will happen as above but we do know in this case that the client has an authorization for the .onion but it simply didn't worked. So same as above, I would not make the client refetch but this time log notice that their configured authorization is not working.

Sounds plausible.

The underlying assumption here is that all HSDirs will serve the same descriptor, so if one can't satisfy us, the others can't satisfy us either, so no point in trying. I think that's a reasonable assumption for now.

After many many reviews and revisions from me/asn/haxxpop, I present the branch for upstream merge:

Branch: ticket20700_035_02 PR: https://github.com/torproject/tor/pull/301

Props goes to haxxpop for this big piece of work and his patience!

Trac:
Status: needs_revision to needs_review
Reviewer: dgoulet to nickm

Looks pretty solid -- I left a bunch of comments on the branch.

Trac:
Status: needs_review to needs_revision

I've done most of the revision, see the fixup commits in the PR.

I've proposed that we address 3 things post-merge, see in the PR.

Last but not least this needs to be dealt with but I wanted us to take the chance to look at it before: https://github.com/torproject/tor/pull/301#discussion_r215264304

Replying to dgoulet:

I've done most of the revision, see the fixup commits in the PR.

I've proposed that we address 3 things post-merge, see in the PR.

Last but not least this needs to be dealt with but I wanted us to take the chance to look at it before: https://github.com/torproject/tor/pull/301#discussion_r215264304

I implemented the changes suggested in that link and updated the discussion.

BTW, the current branch (without my changes) has 13 failing unittests both in my box and in travis.

I also reviewed all of dgoulet's fixups. Most of them are right but there is a bug that I think is causing all the test failures: https://github.com/torproject/tor/pull/301#commitcomment-30416961

I also asked this question: https://github.com/torproject/tor/pull/301#pullrequestreview-152944342

All the rest is fine from what I see.

Revisions have been pushed in the PR. For some reasons, the OSX clang Travis build failed during "setup" so I don't know how to restart that but everything else is green.

Trac:
Status: needs_revision to needs_review

If you supply a corrupted privkey (e.g. wrong length) on the client-side you get the following assert:

Sep 07 15:04:28.397 [err] tor_assertion_failed_(): Bug: src/lib/encoding/binascii.c:100: base32_decode: Assertion (nbits/8) <= destlen failed; aborting. (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug: Assertion (nbits/8) <= destlen failed in base32_decode at src/lib/encoding/binascii.c:100. Stack trace: (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug:     ./src/app/tor(log_backtrace_impl+0x45) [0x555d5b50da95] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug:     ./src/app/tor(tor_assertion_failed_+0x94) [0x555d5b5091d4] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug:     ./src/app/tor(base32_decode+0x2cf) [0x555d5b4f0a9f] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug:     ./src/app/tor(hs_config_client_authorization+0x1d2) [0x555d5b40cf02] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.397 [err] Bug:     ./src/app/tor(hs_config_client_auth_all+0x2e) [0x555d5b4abf9e] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(+0x16693e) [0x555d5b48c93e] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(options_init_from_string+0x419) [0x555d5b490bb9] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(options_init_from_torrc+0x450) [0x555d5b4911a0] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(tor_init+0x303) [0x555d5b3802e3] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(tor_run_main+0x6d) [0x555d5b380d8d] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(tor_main+0x3a) [0x555d5b37a0ba] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(main+0x19) [0x555d5b379e09] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) [0x7ff681179a87] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)
Sep 07 15:04:28.398 [err] Bug:     ./src/app/tor(_start+0x2a) [0x555d5b379e5a] (on Tor 0.3.5.0-alpha-dev 0b60cc10f5536b17)

We can fix either now or after the thing is merged.

There is also the question of what to do when a recognized line fails to be parsed on the client or service side. Right now on the service side we just warn that we failed to parse the client pubkey and move on. Is this the right thing? Shouldn't we just abort since something is obviously wrong? Otherwise the operator might not even notice. Same for the client-case above.

Ok fixed by asn. Haxxpop pushed a new fixup also to try to optimize the HUP process when comparing client auth.

See PR.

Branch ticket20700_035_03 is the squashed version of _02. One tiny cosmetic difference happened during the rebase, shouldn't matter.

Merged to master. Awesome!

Let's close this once we decide what to do with the child.

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

HSv3: Actually do base32 in the client auth pubkey example: [https://github.com/torproject/torspec/pull/40]

Trac:
Username: jchevali
Resolution: fixed to N/A
Status: closed to reopened

I've opened legacy/trac#28264 (moved) for this. Ideally, we don't reopen old tickets for missing stuff :). Thanks!

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

closed

changed time estimate to 24h

mentioned in issue legacy/trac#20742 (moved)

mentioned in issue legacy/trac#24617 (moved)

mentioned in issue legacy/trac#25955 (moved)

mentioned in issue legacy/trac#27544 (moved)

mentioned in issue legacy/trac#27550 (moved)

mentioned in issue legacy/trac#27680 (moved)

moved from legacy/trac#20700 (moved)

added Feature label and removed 1 deleted label

added 1 deleted label and removed 1 deleted label