Opened 2 years ago

Closed 11 months ago

#20699 closed enhancement (fixed)

prop224: Add control port events and commands

Reported by: dgoulet Owned by: dgoulet
Priority: Very High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, needs-proposal, prop224-extra, tor-control, prop284, review-group-26, review-group-27
Cc: Actual Points:
Parent ID: Points: 6
Reviewer: nickm Sponsor: SponsorR-must

Description

This is quite massive work as we'll have to either modify the current events/commands to support proposal 224 or create a brand new set. There are probably new command that we would like to support. For instance, client authorization.

This should be it's own proposal and then merged in control-spec.txt.

Child Tickets

TicketTypeStatusOwnerSummary
#24165defectcloseddgouletprop284: Changes needed on the proposal

Change History (29)

comment:1 Changed 2 years ago by dgoulet

Milestone: Tor: 0.3.???Tor: 0.3.1.x-final

Move the 0.3.??? prop224 tickets to the 031 milestone.

comment:2 Changed 2 years ago by dgoulet

Keywords: prop224-extra added

This keyword indicate that it is a nice extra feature to have for prop224 but not needed for the minimal viable implementation.

comment:3 Changed 21 months ago by dgoulet

Priority: LowVery High

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

comment:4 Changed 21 months ago by dgoulet

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

We think it's not realistic for 031.

comment:5 Changed 20 months ago by asn

Just gonna mention here the request from Riastradh that ideally we would have the same private key format used in ADD_ONION and in the secret_key files in the filesystem. We might want to think about this when we make the control port changes here.

comment:6 Changed 17 months ago by dgoulet

Milestone: Tor: 0.3.2.x-finalTor: unspecified

We can't make those for 032 so for now they go in Unspecified.

comment:7 Changed 17 months ago by nickm

Keywords: tor-control added

comment:8 Changed 15 months ago by dgoulet

Keywords: prop224 removed
Parent ID: #12424

Unparenting so we can maybe close #12424 because the minimal viable implementation has been merged.

comment:9 Changed 14 months ago by meejah

Two points:

  • for ADD_ONION, yes a similar key-format would be good. Since the prop224 keys right now are binary-data I don't believe that will work at all in the control-protocol since AFAIK encoding isn't explicit anywhere (e.g. everything must be ASCII as far as I can tell) so e.g. base32, base64 or hex encoding would be way more likely to work with existing controllers. Also easier for people to copy-pasta
  • it would be Really Great if there was a way for a controller to discover if version3/prop224 services are supported in the connected Tor (e.g. a "getinfo"); checking version-numbers is tedious and IMO prone to failure and weirdness.

comment:10 Changed 14 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:12 Changed 13 months ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

I have the GETINFO and ADD/DEL_ONION command implemented here: ticket20699_033_01. Development continues in that branch (be aware, rebasing is frequent).

comment:13 Changed 13 months ago by dgoulet

The HS_DESC event now implemented. Remains: HS_DESC_CONTENT, HSFETCH and HSPOST.

comment:14 in reply to:  13 ; Changed 12 months ago by dgoulet

Replying to dgoulet:

The HS_DESC event now implemented. Remains: HS_DESC_CONTENT, HSFETCH and HSPOST.

I'm getting skeptical of the need to implement HSFETCH. The two main use of that command that I can recall is (1) our bad HSDir scanner and (2) OnionBalance.

However, I spoke with Donnacha and onion balance won't be using HSFETCH for v3 because it is a bit more complicated since keys are cross certified and signed.

As for (1), v3 doesn't have the problem of malicious HSDir harvesting so all in all I'm not entirely sure implementing HSFETCH worth the engineering effort here if we don't really have a use case for now? Maybe feature parity for v2 is enough of a reason?

comment:15 in reply to:  14 Changed 12 months ago by asn

Replying to dgoulet:

Replying to dgoulet:

The HS_DESC event now implemented. Remains: HS_DESC_CONTENT, HSFETCH and HSPOST.

I'm getting skeptical of the need to implement HSFETCH. The two main use of that command that I can recall is (1) our bad HSDir scanner and (2) OnionBalance.

However, I spoke with Donnacha and onion balance won't be using HSFETCH for v3 because it is a bit more complicated since keys are cross certified and signed.

Makes sense. Let's de-prioritize HSFETCH then. I can think of some diagnostic/debugging reasons for having it, but definitely not that useful now that OnionBalance doesn't need to use it. We can do it in the future when/if the need arrives.

comment:16 Changed 12 months ago by dgoulet

@asn, agree on HSFETCH, I think we should implement it on the "we need it" so we avoid bloating the control port with unused command.

Now, the interesting case of HSPOST which is to upload a descriptor a crucial part of OnionBalance. I originally thought we could use it as is that is "here is a blob, upload to X".

It is a bit more complicated in reality. If we want the command to make any kind of validation on the descriptor, we need to decode it and for this we need both the service identity key (onion address) and the blinded key or the blinding factors to recreate it. That would require us to extend the command to take both an onion address and a blinded key or the factors (num period and period length and a secret when client auth will be a thing).

Another option is to only decode the plaintext part, extract blinded key then decode encrypted portion with it. We would still need the onion address for the identity key to compute the subcredential needed for decryption.

Lets say we don't do that and only do basic validation that is decoding the plaintext part (like what directories do) so we can make sure the descriptor won't get rejected by the HSDir.

Last option is to do NO validation whatsoever on the given descriptor. The v2 command does parse the given descriptor to extract the onion address and returns an error if that fails. Which brings us to the last part.

Because v3 doesn't have the onion address in its descriptor like v2 does, we can't attach the identity key to a directory connection hs_dir_conn_ident_t which would make tor not be able to track the request and fire up an HS_DESC event with the correct information. And tor needs the identity key on the connection identifier to function properly, it can't only use the blinded key without a *major* re-engineering.

All in all, (1) we need to extend HSPOST to accept an onion address or a base64 encoded identity pk (not sure which one is ideal) and (2) I would think that only doing plaintext validation is enough which is what the HSDir will do anyway.

See prop284 changes here: ticket20699_02

Introduces the HSAddress= optional field. I went for the onion address instead of a base64 encoded identity pubkey. Now is a good time to make an argument for the base64 instead of the onion.

comment:17 in reply to:  16 Changed 12 months ago by asn

Replying to dgoulet:

@asn, agree on HSFETCH, I think we should implement it on the "we need it" so we avoid bloating the control port with unused command.

All in all, (1) we need to extend HSPOST to accept an onion address or a base64 encoded identity pk (not sure which one is ideal) and (2) I would think that only doing plaintext validation is enough which is what the HSDir will do anyway.

See prop284 changes here: ticket20699_02

Introduces the HSAddress= optional field. I went for the onion address instead of a base64 encoded identity pubkey. Now is a good time to make an argument for the base64 instead of the onion.

I think I also prefer the onion address over the base64 encoded identity pubkey. It seems more natural to me, and something that more people are exposed to.

Also I think it's fine to only do plaintext validation (aka the thing that HSDirs do).

Onwards!

comment:18 Changed 12 months ago by asn

Spec patch at ticket20699_02 LGTM.

comment:19 Changed 12 months ago by dgoulet

Keywords: prop284 added
Status: acceptedneeds_review

This is pretty massive that is 25 commits but I think well separated and each of them aren't that big.

Branch: ticket20699_033_01
OnionGit: https://oniongit.eu/dgoulet/tor/merge_requests/10

As discussed earlier in this ticket, HSFETCH is not implemented for now. It follows proposal 284 (see spec branch).

comment:20 Changed 12 months ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:21 Changed 12 months ago by nickm

Reviewer: nickm

comment:22 Changed 12 months ago by nickm

Nice! This looks pretty solid and straightforward. I've left some comments on the oniongit ticket.

Additionally:

It needs a changes file.

Do all the stem tests still pass?

comment:23 Changed 12 months ago by nickm

Status: needs_reviewneeds_revision

comment:24 Changed 12 months ago by nickm

Keywords: review-group-27 added

comment:25 in reply to:  22 Changed 12 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to nickm:

Nice! This looks pretty solid and straightforward. I've left some comments on the oniongit ticket.

I've answered them all.

Additionally:

It needs a changes file.

It is commit 86c53d768b63acc30363b470029597a1f5ee6aed.

Do all the stem tests still pass?

They all do except test_dump_config_argument which seems to be not related to this branch but rather some memleak that libasan picks up and thus fails the test. Seems related to SchedulerTypes_ list leaking...

comment:26 Changed 12 months ago by nickm

Status: needs_reviewmerge_ready

Yes, this is looking good.

Merged this to master, and merged the spec branch to torspec.

Now we just need a patch for control-spec, and we can call this finished! (Please either open a separate ticket for that, or make this ticket "assigned" again -- either one is fine with me.)

comment:27 Changed 12 months ago by dgoulet

Status: merge_readyneeds_revision

Back in needs_revision for the control-spec.txt patch.

comment:28 Changed 11 months ago by dgoulet

Status: needs_revisionaccepted

comment:29 Changed 11 months ago by dgoulet

Resolution: fixed
Status: acceptedclosed

Closing in favor of #24847 that I just opened.

Note: See TracTickets for help on using tickets.