Opened 3 years ago

Closed 2 years ago

#21979 closed enhancement (implemented)

prop224: Load and configure service

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, review-group-19, review-group-20
Cc: Actual Points:
Parent ID: #20657 Points: 6
Reviewer: asn, nickm Sponsor: SponsorR-must

Description

This is a much bigger patch implementing a key feature of hidden service. Loading and configuring a service from the torrc file.

A new object is added which is hs_service_t representing a v3 service. The hs_config.[ch] files are introduced which loads the options and create an hs_service_t object out of it.

Like the legacy code, it goes in two steps. First, load the options and validate. Then, load/generate the keys if not in validate mode.

Some refactoring of the legacy code was needed in order to have a central entry point for the configuration of the HS options for both v2 and v3.

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by dgoulet

Status: newaccepted

This depends on #21978 branch so I'll wait for it to be merged upstream before submitting a branch for review.

comment:2 Changed 2 years ago by dgoulet

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

This is actually pretty big and introduces a lot of prop224 service code so lets aim for early 032 instead.

comment:3 Changed 2 years ago by dgoulet

Reviewer: asn
Status: acceptedneeds_review

Ready for review. This is pretty big but should be kind of easy to follow (I hope!). I've extracted 11 commits from #20657. Unit tests coverage is 100% I believe.

Branch: ticket21979_032_01
Gitlab review: https://gitlab.com/dgoulet/tor/merge_requests/30

comment:4 Changed 2 years ago by nickm

Keywords: review-group-19 added

comment:5 Changed 2 years ago by dgoulet

asn has started the damage on the branch ;). Important fix went in this new branch.

Branch: ticket21979_032_02
Oniongit review: https://oniongit.eu/dgoulet/tor/merge_requests/1

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

comment:6 Changed 2 years ago by asn

Hey David, I submitted the first round of review on gitlab. I'm still not done with the branch but that's a start. The branch seems to work pretty well here.

BTW, it would be nice if I had some more service-side code to review/test because just loading HSes does not give me much testing surface. If I had some code that also uses the generated HS that would be more useful for review/testing. Do you think that's a good time to prepare for me the second batch of service-side code for review? Or should I keep reviewing this codebase more?

comment:7 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

comment:8 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Oniongit comment addressed. Back in needs_review. Fixup commit in the original branch (ticket21979_032_02).

comment:9 Changed 2 years ago by dgoulet

Keywords: review-group-20 added

comment:10 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

Thanks asn for this review! We are now in "nickm's eagle eye" mode.

This is pretty big so be advised. It's based on git master 948158df33c4cbfa03f2aa4990097b4e9de664a1.

Branch: ticket21979_032_03
Oniongit: https://oniongit.eu/dgoulet/tor/merge_requests/4

comment:11 Changed 2 years ago by nickm

Reviewed! Please let me know if you've any questions.

comment:12 Changed 2 years ago by nickm

Status: merge_readyneeds_revision

comment:13 Changed 2 years ago by dgoulet

Reviewer: asnasn, nickm
Status: needs_revisionneeds_review

I addressed everything I think!

Branch is still: ticket21979_032_03

comment:14 Changed 2 years ago by nickm

Okay! I answered questions on that yesterday, but didn't go over the new code yet. I'll have a look now.

comment:15 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

See branch ticket21979_032_04 for the squashed fixup.

On extra commit on it to fix clang warnings: 965e3a66

comment:16 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

lgtm. Merging!

Note: See TracTickets for help on using tickets.