Opened 3 years ago

Closed 3 years ago

#21334 closed enhancement (fixed)

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

Reported by: asn Owned by: asn
Priority: Very High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, review-group-16
Cc: Actual Points:
Parent ID: Points: 3
Reviewer: nickm Sponsor: SponsorR-must

Description

This is the older cousin of #20852. 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).

Child Tickets

Change History (20)

comment:1 Changed 3 years ago by asn

Status: newneeds_review

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.

comment:2 Changed 3 years ago by asn

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

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

Version 0, edited 3 years ago by asn (next)

comment:3 Changed 3 years ago by nickm

Keywords: review-group-16 added

comment:4 Changed 3 years ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet

comment:5 Changed 3 years ago by dgoulet

Priority: MediumVery High
Status: needs_reviewneeds_revision

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.

Last edited 3 years ago by dgoulet (previous) (diff)

comment:6 Changed 3 years ago by asn

Status: needs_revisionneeds_review

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.

comment:7 Changed 3 years ago by dgoulet

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

comment:8 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

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! :)

comment:9 Changed 3 years ago by nickm

Reviewer: dgouletnickm

okay, calling myself reviewer.

comment:10 Changed 3 years ago by nickm

Status: merge_readyneeds_revision

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

comment:11 Changed 3 years ago by nickm

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?

comment:12 Changed 3 years ago by asn

Status: needs_revisionneeds_review

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.

comment:13 Changed 3 years ago by nickm

Sponsor: SponsorR-must

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by nickm

Status: needs_reviewmerge_ready

comment:16 Changed 3 years ago by dgoulet

Owner: set to asn
Status: merge_readyassigned

comment:17 Changed 3 years ago by dgoulet

Status: assignedmerge_ready

comment:18 Changed 3 years ago by dgoulet

Type: defectenhancement

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

comment:19 in reply to:  14 Changed 3 years ago by asn

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 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.

comment:20 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.