Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20852 closed defect (fixed)

prop224: Update prop224 HSDir code to understand latest descriptor format

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, TorCoreTeam201612, review-group-14
Cc: Actual Points:
Parent ID: Points: 3
Reviewer: dgoulet Sponsor: SponsorR-can

Description

The revamp of prop224 client authorization brought many changes to the HS descriptor format.

We need to update the code we wrote in #17238 so that it uses the new superencrypted descriptor format as specified by:
https://gitweb.torproject.org/torspec.git/commit/?id=890779ffec60f067048c4e4a9895ccdd49d183a5

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by asn

Owner: set to asn
Status: newassigned

comment:2 Changed 3 years ago by asn

OK, I did some preliminary investigation for this ticket. IMO, there are two steps here:

a) Fix the HSDir code to conform to prop224

Only fixing the HSDir part here is the minimum viable product for 0.3.0, so that the network is prepared. It will basically make HSDirs understand the newest prop224 format (with superencrypted etc.). Since this is HSDir-only, we don't need to implement the whole client-auth logic; we just need to be able to parse the plaintext part of the descriptor.

b) Fix the rest of the descriptor code to conform to prop224

This part would involve actually implementing the client-auth parts of the descriptor, and making clients and hidden services actually be able to parse those. IMO, this is a much bigger project than the above, and it's not strictly required for 0.3.0 since it belongs to the client/HS code.


My plan is to finish (a) ASAP, and then only do (b) if there is enough time.

comment:3 Changed 3 years ago by dgoulet

Agree. (a) is the priority. I think (b) has its own ticket already for 031 so if you can do it, great! But I would say not a priority for 030. Thanks!

Side note, this change will bring quite a much bigger descriptor that we have now so we should just make sure the current code doesn't actually have "max length" check too low.

comment:4 Changed 3 years ago by asn

Please find an initial version of (a) in my branch bug20852:
https://gitweb.torproject.org/user/asn/tor.git/log/?h=bug20852

It basically does two things:

  • Changes encrypted to superencrypted in the outter HS desc layer.
  • Changes the max descriptor size to 50kb and introduces a consensus param HSV3MaxDescriptorSize that controls it.

I believe these two things address everything that was pending in prop224 and has to do with HSDir parsing of HS descriptors. Please check out the branch. I'm gonna do another pass of prop224 to make sure I didn't miss something.

comment:5 Changed 3 years ago by asn

Status: assignedneeds_review

comment:6 Changed 3 years ago by nickm

Keywords: review-group-14 added

comment:7 in reply to:  4 ; Changed 3 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

Replying to asn:

Please find an initial version of (a) in my branch bug20852:
https://gitweb.torproject.org/user/asn/tor.git/log/?h=bug20852

It basically does two things:

  • Changes encrypted to superencrypted in the outter HS desc layer.
  • Changes the max descriptor size to 50kb and introduces a consensus param HSV3MaxDescriptorSize that controls it.

I like that this can be controlled by a consensus parameters which will be very helpful because we also plan to have a param for the number of introduction points which will ultimately affect the max size.

One thing I would change is hs_cache_get_max_hs_descriptor_size(). The second "hs" in there is redundant as we are already in the HS subsystem here so `hs_cache_get_max_descriptor_size()" is enough imo.

Also, networkstatus_get_param() returns a int32_t so we should NOT make it unsigned int for the max size but rather make sure we are in range and convert it before return. Even though it's probably not possible to have something not in range of what we ask, MORE safety net is good.

I believe these two things address everything that was pending in prop224 and has to do with HSDir parsing of HS descriptors. Please check out the branch. I'm gonna do another pass of prop224 to make sure I didn't miss something.

comment:8 in reply to:  7 Changed 3 years ago by asn

Replying to dgoulet:

Replying to asn:

Please find an initial version of (a) in my branch bug20852:
https://gitweb.torproject.org/user/asn/tor.git/log/?h=bug20852

It basically does two things:

  • Changes encrypted to superencrypted in the outter HS desc layer.
  • Changes the max descriptor size to 50kb and introduces a consensus param HSV3MaxDescriptorSize that controls it.

I like that this can be controlled by a consensus parameters which will be very helpful because we also plan to have a param for the number of introduction points which will ultimately affect the max size.

One thing I would change is hs_cache_get_max_hs_descriptor_size(). The second "hs" in there is redundant as we are already in the HS subsystem here so `hs_cache_get_max_descriptor_size()" is enough imo.

Also, networkstatus_get_param() returns a int32_t so we should NOT make it unsigned int for the max size but rather make sure we are in range and convert it before return. Even though it's probably not possible to have something not in range of what we ask, MORE safety net is good.

Thanks for the review, David. It was helpful!

I pushed a branch called bug20852_david which addresses both comments above. Please review it, and if you like it I will squash the fixups into a more compact branch for Nick's review.

WRT the networkstatus_get_param() issue I spent quite some time thinking of the right casting logic, and I ended up with restricting the max size to INT32_MAX and then casting to unsinged directly. Let me know if you think that's not the right logic.

Thanks!

comment:9 Changed 3 years ago by asn

Status: needs_revisionneeds_review

comment:10 Changed 3 years ago by asn

Here is how the outer layer of a descriptor looks like with this branch:

hs-descriptor 3
descriptor-lifetime 180
descriptor-signing-key-cert
-----BEGIN ED25519 CERT-----
AQgABko2AeTAAhW4h+yIOrjbBWVL25aJJ9vW4WDE12adqqEtNAarAQAgBAB9HiLr
1Z3xzVDL/01+zT4aPoB2v3QDuR0S6RFQ89+4p9SRolCFM8E3ibI9sXWRggwi1xiR
ctFFt0bzOJdPy+Vh0pOzMVGMVkZMK2XWnkpOi2cuZthpOakmxhPOqD6xxwQ=
-----END ED25519 CERT-----
revision-counter 42
superencrypted
-----BEGIN MESSAGE-----
/Tw4+12EEedrh5Fke7YZRXX5+appjqQr/FtbVE7TPhaaohMOVPVwaN2Vzkz/N64/
35owFBY+lSDg35YDsBJ8AzSkcBCQiqa8ocVqtQzG/0Io4GXJ+8N6imQry4r4CqLM
ZVWmjIllxL9FzinYZvhA32vwHFvX/eDF1zu4aaLlMa/ujJOvwKnne4QhuyxFHjOi
BArNSkmvtSD7Jpu9fcEXoEDpdxxEnhCoIN5X3NgKT//qicTCW8QJuaN5rbgDCl5g
rQcT+xYgM7RT7x2E9FOx/lt9tU7xn32blfnpr8UkIFJ477zST5TbgszpdQmyugjt
MRSLRVshMDnpmjscNunghz5Q1NSodNkig9JoSrSVkeahnFCwbMH4kAD74Lk/Pkrt
mAcPYqQ7y+TlPNJHhW+X8DnzUolP1cO+RKQsn1/GV20eMtJMZBxxZkNHPkdwUR/Y
5V5Qx1RbU9vxxIp/qYHeewMCt5xWTcc1XcNalSDBj55uZkrU98AVs2lFf16a/VX8
WzWaPwxksxPa5mAqynvt21/60mGOjN6mK6QAUbiZdfb6m+LscY3t8ClH6tb35Knr
Gqlqy54oHB/DrroDbnxvSU1ovJK19NbW3iEDpfhzjLpGMh7bna5okOuuJ53z1gBx
a7on+D0fN89OEqGga3IkEHZQD1AQGDwU3w7AedEK0XRV1Z7otfnD3iZqcWph4vIE
kVgwwDZqj7VHZ10DxNaMS4eKgj8D+dzFEzgvpmrV6nEn09kpJ0ZjfAys60jiZYEl
dqboZaqBzOvI42fh8nH/QHWqZX1boNm9cUT2n+YTm2z6iPIOXcFJzI+sBZKGrHfk
DA/8htAQ3viOTm1Bvnk74Lqac+pqqw/PF/EKZmcG7SNMeKtYGrqZdnEmjw26gf8w
s/4xILOG52SLTZlMaIs0YiyoUI674pSx7HrTl4YhtmOmjtdrkMcjmxboyuSF6dco
SG9UyXP7iUI+k/c+PVq84a3xGn4+sF/eqfhcbEces6s/Sfu01AoA9SrfGekmWsSp
M4CR+2rjVGpdoDNZzUZDpCyeEcx+8tF0vq+NVUnHWA67OAw3gPXXgMuwUcP3X6Ux
tluJDOCnqIbkdz4C0xQbyayNoeNENWjDUnQPsClWppnJNAsDQMsVT744Tpw4P17B
X+AxISz6VwHJeNZJnSbRRfNdn3gmq4vFoTvz/Y1jBPF4xUKH4HnauKU92vyqawk5
fGo4HS97uMZ9BI9YOn97dt+2O0vumXLtMwHtGe3H4PpOFUcrpLPbEThIDjrBA74A
SmDMJ/ny1ihmcHXg3eaNPltNz1CoVx91rRWAWT7GO/ICV98aD8E7P/QHIOUh1UgO
rargP6Ogf7Lqoldd4F6TxcIY163PudgfIaKb9ftRvFLOy87RrgznHW0CyFq++c8Y
jeiOuo5oLIcnZpm0/QBEu/xnUuSXH4wCMxBhSL2dNSnCWTSDsDjOrzEIj3PAgN4E
gb4nHMWCQ4oTbScKaHcDUrplvFVt/NEmDEocAYppiQ64oy9NEt0iPP/FLpS381At
YspM0Cc6CFaYjzTfuNMv0VTY8913iCG7P8S1vE2g2mmxG+d6BOm53VqgG3gAtnDz
N/lKtIH3GBUR6LfrDLPnp01Ppq954Mi9lr2SlgZJs32kyjG0rFMPR9/N18m/HY0E
6+PrYT/SLO+xCBIO2bTuOuMcl4FcSL2HVq/q+O3Zi5okAaUk0/U4ZwP/0mBi3WMK
TGvA1KoNAJWAJMnnJzxN4ufZuaFezoJlrQKvcaunX9m3woYhamKoH7IcACzHKkGh
TzhLovA7bElyJgU1cysnwP4JBmTfDyAdE5ywDTpkDqwbUOl6TCxqawYKIwe0ZGlV
UnoQvvoCsLPSpXQagWrOdTBP+HIrIztl13STvkNy33DFaCl3IJTPgIpSBBtpfq7l
E2yMe4akan0CB8xgYxl3SogzehxGDYwq57I5DtbjF2q32Jam2gUMrIZ8tSNHJnv8
QL5APrnURu6raCzqxCGdjLi4QvMyDpn4WfDaS5Iyt7o528jBSskHnttDRIfxxuRs
ZI30MLGd5z3nRraSwOAZQ6+dajISPO4w6fcJMmafyrOgnvZlhwPYQESpqyYMfsa3
y8rICVdNmLYByNQ+M0dOhOH40VPDlcVLyEuOqHHSWAztGaTPz2HYPHRve7xVwmNK
tJJl+PtNIcOkESjjlQeqER5dkaCum8dJFMjs17dAlRU3LIjanBNlkg9oQycjRDob
h+ZZMIe7jAwbR7vYnjBoTkP4IR9yDcmwMOGzIhPAElWZY1i6v3LGjr4hFv7pvcy3
8L7Sp4Ng5GllackgEaN19C7ByU4yDLs/FEgfs3BNnqC+sJxf+z+n8l+WuRXgmuPc
KIcK380MlwhCasB7+5p/Up69cnBTpmkWOX3twT8haKNA/MGRYsXv1fkCSKY+wpCz
V1eYdy5i+x3Y3aeFGRd8edC92YQAiIG0gr8fB9QQ1cnQSJX07PmX8HqDF3aHmCcm
G8tI8saIcfi6SkImbMCt1ApFyv9w55D2FYKM2SqdVaRcSJ9xuHC7kH56GK8PPqa8
Ypi5FS+TjPmA/4mV87az1F17fUCN6uqKMnxZYZyXAY3vM0BsNAPz2y4xfxu2SvQy
9Z3Bl5mDBgO/0UGotj92gsQUWlf0niKu9FMR5JrcIj7zaEAWC9UdagTZFNrQS+gj
nxSAkcXqXX91RQJPghFyLIgxARl7EaGxwoxWC+yzqj4g0qZlEFPGw/tCHEwq1qFg
MzlpBak+LMH6O9KEg2EoSuIMOp0nbTe1AVTqCmVBpyc9uZvm2ZZZBGJ1c+CDtbbt
Z/zxYEFuiW+v44MpO7qeKDAvof9G50XBKl6PbXLZFZ41vEwu4OySrsGZuthcr3VI
rRrqEUjKkvu8lZ3mZ2nnSfbGkPWaaDjA3ghwT8FPDrMcECsmC7TrGy7qNndzDJqN
T9McQlqh30pqMTSFlnVC010YaLScfDXASX62lp6spjAdbAtv+ScQ//kcY9Kfn8Ek
zPgFUzWaErNxBSUtlSZ+/LKW4dV2rfao2ZOWSa6kWrDvrMzE7yF6OreKUJBRnFFJ
xkxhLHPwqUjpfKfomFS9w+y5hCyuFQL6hVBuCzk1BF+N1HNe2BszdDOwiwKMSukL
arbhbHt+fbVGl67a++CySx8t+yochC9T2X5h3Nh2XPk/YefCwUlVZHUbQ+y2KiH3
VHOtFknsG57wUsQ3N32uSnMhgZri47eeTjMLya9g6CwuhBXvw6hXGZI1lGqsbuCP
waIcnzt9iGOTYvkx4b9n3Q18ZLnykWThLKiWEJjfp45yVn03vdya/cTI0jSd+smX
3fCoivACvw9Ls6I7nw++60weR0+OCppmepZv0+1yJk1Vkw4ZnE5+2m8nRCnbNjq1
3Cpczwoo20gKSrf0cHu5JZoh0eLiAgmVfYvDPWUcC1QbdK0yYABraO7O7u9AaAE5
WILU1ckKzanMxo0II615/j7GV+84cHgT99NvPlVzDC2jy0Ws+Ler1bL9wKKcfT2B
eu4wRdtoqxtDEyU9lvNccK0pV5CMFP5Yw77/kTUTv+PzidM63iRD7zYTc9pq03qT
JOmJ7jJtfAvsXiqRW8dj+TLaVN0pet32zG59DNA/evl1HnYXRkjQFihsa4xTSLYH
+KkhLNq0dK2uJiC1BWwTIA6rubUGew48GsPv9iL3KASeShgtnqXxgF/nyBKkC2Qz
90robxnJvFJC/LsHRa+PTo9qnKQSUVc87EIRYYxLxZP0Fj3Z5GoG4TWZerU/e/l4
PEDHV3wvmgicYTJ9wlk3JGhqOfimFKlj4bojnLw5KvgO0u+BoiZZ8yzhAYvCv2CE
a7Q5RulHJI80L3cAxeUJcqtjhBclD1fUOIZwQnjxypWrlLegh1G23kKBFh+kI0aR
9/WC47nYEbuAdLsyaGk23l8UfVgd0PcGKgaNqFvRRp+4rQhSNvGWA0AwmzekdQQM
d2FKzFmiQeHfT9/6GM7qMJvhAPTRIIYagDJwBHJCPjS+Er9+qh39OFr7S3xJhFBK
gGAhj1R/t2yjFzNf7/8py3ocy9hYqJFjlDSg+VkLagm1l4do8O8uewUYsxMWFoWD
4KQaSj7T47FVevwOSpxQ0mU4O14n3kYahjh1bjVi3nT1CKv2cewwV0n00Pgnlfu+
-----END MESSAGE-----
signature zsSuYOLIPSuyS8szt+z+sZPlUYxCPyWrt28elmyPQ0c6mGENvOz40xdLhErwdaJuHIMrN8bGiaBNVQ5TXRt3BQ

comment:11 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

The INT32_MAX is OK with me as cast to an unsigned int should not overflow. And if your arch considers a int to be 2 bytes, well we'll hit the maximum which is ok.

lgtm!

comment:12 in reply to:  11 Changed 3 years ago by asn

Replying to dgoulet:

The INT32_MAX is OK with me as cast to an unsigned int should not overflow. And if your arch considers a int to be 2 bytes, well we'll hit the maximum which is ok.

lgtm!

Thanks for the review David. I squashed everything into bug20852_v1 which should be ready for Nick's eyes.

comment:13 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Looks okay! Merged.

comment:14 Changed 3 years ago by asn

Summary: prop224: Update HSDir prop224 code based on newest descriptor changesprop224: Update prop224 HSDir code to understand latest descriptor format

comment:15 Changed 3 years ago by asn

Personal note: Part (b) of comment:2 is being done in #21334.

Note: See TracTickets for help on using tickets.