prop224: Update prop224 HS desriptor generation code to produce the latest descriptor format

This is the older cousin of #20852 (moved). In this ticket we need to go over our prop224 HS descriptor generation code and make it produce descriptors that conform with the latest format (which includes the fake client authorization entries and such).

changed milestone to %Tor: 0.3.1.x-final

added component::core tor/tor milestone::Tor: 0.3.1.x-final owner::asn points::3 priority::very high prop224 resolution::fixed review-group-16 reviewer::nickm severity::normal sponsor::R-must status::closed tor-hs type::enhancement labels

Please see my branch bug21334_v1 for an initial implementation of this feature.

I still need to check the new code coverage and write tests as needed, but I'm putting this branch out in case someone wants to review code.

Trac:
Status: new to needs_review

And here is a gitlab merge request for this branch: https://gitlab.com/asn/tor/merge_requests/10

I also wrote some unittests for the new code that was added.

The final thing that I can think of to make this branch more robust would be to further validate the auth-client elements. There is not much to do there apart from checking the base64 lengths but perhaps this is a good thing to do. Let me know if you'd like me to do that.

(I also force pushed to my bug21334_v1 to fix some check-spaces artifacts)

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

Trac:
Cc: dgoulet to N/A
Reviewer: N/A to dgoulet

Ok I made some comments on the gitlab branch. There are also quite a bit of memory leaks with the unit tests so I suggest to take a look with --enable-fragile-hardening.

Trac:
Priority: Medium to Very High
Status: needs_review to needs_revision

Aight. Thanks for the great review.

All things should be fixed now and I pushed the fixups in my gitlab bug21334_v1.

The unittest memory leaks and problems are also fixed in commits 4b9aa38 and 88a0f3c.

Let me know if there is anything else to be done here, otherwise im gonna squash everything nicely, put it in bug21334_v2 and wait for Nick's eyes.

Trac:
Status: needs_revision to needs_review

Reviewed! Discussed a bit on IRC and it's all ready for v2!

OK. We believe this is ready for the eyes of Nick. Please see my branch bug21334_v2 and my gitlab merge request: https://gitlab.com/asn/tor/merge_requests/12/diffs

The patch basically adds another layer of encryption to the HS descriptor, and also adds the mandatory fake auth client data. On the decoding-side, the patch handles and removes the extra layer of encryption. We still dont do anything with the client auth information since they are all fake and client auth is still not implemented.

Putting this in merge_ready! :)

Trac:
Status: needs_review to merge_ready

okay, calling myself reviewer.

Trac:
Reviewer: dgoulet to nickm

I've been over the gitlab branch, and it looks pretty good -- just a few minor things.

Trac:
Status: merge_ready to needs_revision

One more question: is there a unit test that verifies "round trip" encoding and decoding of a descriptor? That is, start with a struct, encode it, decode it, and make sure you got what you started with?

Thanks for the review. I addressed most of the comments and pushed the fixups in bug21334_v2.

When you are happy with the fixups, I will squash everything into a bug21334_v3 branch to move forward.

Trac:
Status: needs_revision to needs_review

Trac:
Sponsor: N/A to SponsorR-must

It looks like my comments were all addressed; please feel free to squash now. If possible, please base the squash on the same starting point as "bug21334_v2", so that I can run "git diff bug21334_v2 bug21334_v3" and confirm that they are the same? Once that's done and the tickets mentioned in the comments on gitlab are opened, I'm happy to merge this.

Also, in the future, please consider using fixup! or squash! rather than TOSQUASH: "git-rebase" can handle those automatically.

Trac:
Status: needs_review to merge_ready

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

Trac:
Status: assigned to merge_ready

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

Trac:
Type: defect to enhancement

Replying to nickm:

It looks like my comments were all addressed; please feel free to squash now. If possible, please base the squash on the same starting point as "bug21334_v2", so that I can run "git diff bug21334_v2 bug21334_v3" and confirm that they are the same? Once that's done and the tickets mentioned in the comments on gitlab are opened, I'm happy to merge this.

Also, in the future, please consider using fixup! or squash! rather than TOSQUASH: "git-rebase" can handle those automatically.

Hey Nick,

I pushed bug21334_v3 in my repo. The diff should be identical to my v2 branch.

I also opened #21693 (moved) for the double-base64 issue as requested, and I started working on a PoC for it. It's quite messy due to the nature of the change and the char*->uint8_t pointer madness, but I hope to have a draft ready at some point this week.

merged!

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

closed

changed time estimate to 24h

mentioned in issue #21693 (moved)

moved to tpo/core/tor#21334 (closed)

mentioned in issue tpo/core/tor#21693 (closed)