prop224: Update prop224 HSDir code to understand latest descriptor format

changed milestone to %Tor: 0.3.0.x-final

added TorCoreTeam201612 component::core tor/tor milestone::Tor: 0.3.0.x-final owner::asn points::3 priority::medium prop224 resolution::fixed review-group-14 reviewer::dgoulet severity::normal sponsor::R-can status::closed tor-hs type::defect labels

Trac:
Owner: N/A to asn
Status: new to assigned

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.

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.

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.

Trac:
Status: assigned to needs_review

Trac:
Keywords: N/A deleted, review-group-14 added

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.

Trac:
Status: needs_review to needs_revision
Reviewer: N/A to dgoulet

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!

Trac:
Status: needs_revision to needs_review

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

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!

Trac:
Status: needs_review to merge_ready

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.

Looks okay! Merged.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

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

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

closed

changed time estimate to 24h

mentioned in issue #21334 (moved)

prop224: Update prop224 HSDir code to understand latest descriptor format

Child items 0

Activity

a) Fix the HSDir code to conform to prop224

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