Opened 2 years ago

Closed 6 weeks 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 (50)

comment:2 Changed 2 years ago by asn

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

comment:3 Changed 22 months ago by dgoulet

Owner: set to asn
Status: newassigned

comment:4 Changed 22 months 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 20 months ago by dgoulet

Priority: MediumVery High

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

comment:6 Changed 19 months 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 17 months ago by nickm

Keywords: triage-out-030-201612 removed

comment:8 Changed 14 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 9 months ago by dgoulet

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

comment:10 Changed 7 months ago by haxxpop

Owner: changed from asn to haxxpop

comment:11 Changed 7 months ago by nickm

Keywords: 034-triage-20180328 added

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

Cc: peter@… added

comment:14 Changed 6 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 6 months ago by dmr

Cc: dmr added

comment:17 Changed 6 months ago by cypherpunks

YEEEEAHHHH I really need HidServAuth for v3 onion!!!

comment:18 in reply to:  17 Changed 6 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 6 months ago by asn (previous) (diff)

comment:19 Changed 6 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 6 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 6 months ago by cypherpunks

Parent ID: #25955

comment:22 Changed 6 months ago by cypherpunks

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

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

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

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

comment:28 Changed 4 months ago by dgoulet

Reviewer: dgoulet

comment:29 Changed 4 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 4 months ago by nickm

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

comment:31 Changed 3 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:32 Changed 3 months ago by dgoulet

Cc: nickm added

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

comment:33 Changed 3 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 3 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 2 months ago by dgoulet

Status: needs_revisionneeds_review

comment:36 Changed 8 weeks 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 8 weeks 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 7 weeks 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 6 weeks ago by nickm

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

comment:40 Changed 6 weeks ago by nickm

Status: needs_reviewneeds_revision

comment:41 Changed 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 6 weeks ago by nickm

Merged to master. Awesome!

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

comment:50 Changed 6 weeks ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.