Opened 3 years ago

Closed 9 months ago

#20700 closed enhancement (fixed)

prop224: Implement standard client authorization

Reported by: dgoulet Owned by: haxxpop
Priority: Very High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-hs, 035-roadmap-master, 035-triaged-in-20180711
Cc: peter@…, dmr, nickm Actual Points:
Parent ID: #25955 Points: 3
Reviewer: nickm Sponsor:

Description

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

Child Tickets

Change History (52)

comment:2 Changed 3 years ago by asn

Sponsor: SponsorR-mustSponsorR-can
Summary: prop224: Implement client authenticationprop224: Implement client authorization

comment:3 Changed 3 years ago by dgoulet

Owner: set to asn
Status: newassigned

comment:4 Changed 3 years ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Triaged out on December 2016 from 030 to 031.

comment:5 Changed 2 years ago by dgoulet

Priority: MediumVery High

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

comment:6 Changed 2 years ago by dgoulet

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

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

comment:7 Changed 2 years ago by nickm

Keywords: triage-out-030-201612 removed

comment:8 Changed 23 months ago by dgoulet

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Parent ID: #12424

Not going in the initial release of prop224.

comment:9 Changed 18 months ago by dgoulet

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

comment:10 Changed 16 months ago by haxxpop

Owner: changed from asn to haxxpop

comment:11 Changed 16 months ago by nickm

Keywords: 034-triage-20180328 added

comment:12 Changed 16 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:13 Changed 16 months ago by pege

Cc: peter@… added

comment:14 Changed 16 months ago by nickm

Keywords: 034-removed-20180328 removed

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

comment:15 Changed 15 months ago by dmr

Cc: dmr added

comment:17 Changed 15 months ago by cypherpunks

YEEEEAHHHH I really need HidServAuth for v3 onion!!!

comment:18 in reply to:  17 Changed 15 months ago by asn

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

Last edited 15 months ago by asn (previous) (diff)

comment:19 Changed 15 months ago by teor

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

comment:20 Changed 15 months ago by haxxpop

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.

comment:21 Changed 15 months ago by cypherpunks

Parent ID: #25955

comment:22 Changed 15 months ago by cypherpunks

Summary: prop224: Implement client authorizationprop224: Implement basic client authorization

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

comment:23 Changed 15 months ago by teor

Summary: prop224: Implement basic client authorizationprop224: Implement standard 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.

comment:24 Changed 15 months ago by teor

Status: assignedneeds_revision

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.

comment:25 Changed 15 months ago by asn

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.

comment:26 Changed 14 months ago by asn

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

We are hoping to land this in 035.

comment:27 Changed 13 months ago by dgoulet

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

comment:28 Changed 13 months ago by dgoulet

Reviewer: dgoulet

comment:29 Changed 13 months ago by dgoulet

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

comment:30 Changed 13 months ago by nickm

Keywords: 035-roadmap-master added; 034-triage-20180328 removed
Sponsor: SponsorR-can

comment:31 Changed 12 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:32 Changed 12 months ago by dgoulet

Cc: nickm added

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

comment:33 Changed 12 months ago by 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.

comment:34 in reply to:  33 Changed 12 months ago by asn

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.

comment:35 Changed 11 months ago by dgoulet

Status: needs_revisionneeds_review

comment:36 Changed 11 months ago by dgoulet

Status: needs_reviewneeds_revision

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

comment:37 in reply to:  36 Changed 11 months ago by asn

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.

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

comment:38 Changed 11 months ago by dgoulet

Reviewer: dgouletnickm
Status: needs_revisionneeds_review

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!

comment:39 Changed 11 months ago by nickm

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

comment:40 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

comment:41 Changed 11 months ago by 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

comment:42 in reply to:  41 Changed 10 months ago by asn

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.

comment:43 Changed 10 months ago by asn

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.

comment:44 Changed 10 months ago by dgoulet

Status: needs_revisionneeds_review

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.

comment:45 Changed 10 months ago by asn

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.

comment:46 Changed 10 months ago by asn

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.

comment:47 Changed 10 months ago by dgoulet

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

See PR.

comment:48 Changed 10 months ago by dgoulet

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

comment:49 Changed 10 months ago by nickm

Merged to master. Awesome!

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

comment:50 Changed 10 months ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

comment:51 Changed 9 months ago by jchevali

Resolution: fixed
Status: closedreopened

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

comment:52 Changed 9 months ago by dgoulet

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.