Opened 12 months ago

Last modified 9 hours ago

#20699 needs_review enhancement

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
Cc: Actual Points:
Parent ID: Points: 6
Reviewer: 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 (19)

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

Priority: LowVery High

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

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

Keywords: tor-control added

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

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:12 Changed 11 days 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 10 days ago by dgoulet

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

comment:14 in reply to:  13 ; Changed 5 days 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 5 days 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 5 days 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 5 days 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 13 hours ago by asn

Spec patch at ticket20699_02 LGTM.

comment:19 Changed 9 hours 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).

Note: See TracTickets for help on using tickets.