Opened 2 years ago

Closed 2 years ago

#21871 closed enhancement (implemented)

prop224: Change descriptor format for legacy encryption key

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224
Cc: Actual Points:
Parent ID: #21888 Points: 3
Reviewer: asn Sponsor: SponsorR-must


It turns out that we might have miscalculated the legacy feature for introduction point.

Currently, proposal 224 looks like this for legacy encryption keys:

        Encryption key is specified as follow:

        [Exactly once enc-key per introduction point]

           "enc-key" SP "ntor" SP key NL

             The key is a base64 encoded curve25519 public key used to encrypt
             the introduction request to service.

           "enc-key" SP "legacy" NL key NL

             Base64 encoded RSA key, wrapped in "----BEGIN RSA PUBLIC
             KEY-----" armor, for use with a legacy introduction point as
             described in [LEGACY_EST_INTRO] and [LEGACY-INTRODUCE1] below.

This doesn't make much sense because this encryption key is used to encrypt the ENCRYPTED section of the INTRODUCE1 cell (section 3.2.1. and 3.2.2.). That section can only be decrypted by the service so the introduction point, being legacy or not, doesn't touch it at all, it just passes along the bytes.

So, the descriptor should always contain a ntor key per intro point because we still want the ntor handshake to happen since both client and service do speak the prop224 protocol.

If the intro point is a legacy one, it should also have a "legacy key" which is an extra RSA public key needed in the INTRODUCE1 legacy cell and used by the intro point to relay the cell on the right circuit (used in the ESTABLISH_INTRO):

   LEGACY_KEY_ID        [20 bytes]
   Here, LEGACY_KEY_ID is the hash of the introduction point legacy
   encryption key that was included in the hidden service descriptor.

In the legacy ESTABLISH_INTRO:

    PK   Bob's public key or service key        [KL octets]

In the current legacy code, the intro point validates that this PK field is an ASN.1 encoded RSA key (rend_mid_establish_intro_legacy()).

Fortunately for us, this code is getting release in 030 *but* only be actually used in 032 (#12424).

Child Tickets

Change History (13)

comment:1 Changed 2 years ago by dgoulet

Status: newneeds_review

Here is my proposed patch to proposal 224: ticket21871_01

If this makes sense, let's put this ticket in accepted state so we can proceed with the code change.

comment:2 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

Worked on this with asn on IRC, some changes happened in that branch. It should now be the up to date one. Moving to accepted so we can do the code patch.

Torspec: ticket21871_01

comment:3 Changed 2 years ago by dgoulet

Parent ID: #20657

This will be part of #20657 work since it has too much implications to re-merge in that branch after an upstream merge on 031.

comment:4 Changed 2 years ago by dgoulet

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final
Parent ID: #20657#21888
Points: 13
Status: acceptedneeds_review

Ok, I made it happen afterall but it is built on top of #21895 because it needed to extract the private key out of the descriptor and doing that in two moves is less messy.

TOP 2 commits are the one for this branch. Last 2 commits are from #21895.

Torspec: ticket21871_01
Tor code: ticket21871_031_01
Gitlab review:

comment:5 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

Thanks for the code here dgoulet!
Did an initial review!

comment:6 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Here is a version 2. I had to squash two commits together because the first one (which was the remove private key from hs_descriptor.h) didn't make sense at all with the next commit which changes the descriptor format of encryption keys. It was actually just more complicated to deal with both in two commits and not adding any useful semantic.

I've fixed one of the comment in the merge request, the two others are in discussion mode I guess.

Torspec: ticket21871_01
Tor code: ticket21871_031_02

comment:7 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

Did a review and added some nitpicks to gitlab. I think we are almost there.

I'm setting this to needs_revision but feel free to set it back to merge_ready if you think none of my comments are worth addressing.

comment:8 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_information

Responded. It pushed two fixup commits but an open question for the last comment on the certificate reject time.

comment:9 Changed 2 years ago by dgoulet

Reviewer: asn
Status: needs_informationmerge_ready

Ok, extra fixup commit pushed to address the last comment about the certificate expiry lifetime check. This is ready for merge.

Branch: ticket21871_031_02
Rebased on master and fixup squashed branch: ticket21871_031_03
Spec: ticket21871_01

comment:10 Changed 2 years ago by nickm

Spec merged.

Tried to merge ticket21871_031_03, but the unit tests fail for me; do they work for you?

comment:11 Changed 2 years ago by nickm

Status: merge_readyneeds_revision

comment:12 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Oh my that's embarrassing... Sorry about that!

Fixup commit b8452e20 in ticket21871_031_03

comment:13 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Tests pass now. Merging. Thanks!

Note: See TracTickets for help on using tickets.