Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#14845 closed enhancement (fixed)

Controller: retrieve an HS descriptor from the client's cache

Reported by: dgoulet Owned by: dgoulet
Priority: Very Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor-hs, controller, 027-triaged-1-in, SponsorS, nickm-review
Cc: markxr@… Actual Points:
Parent ID: #3521 Points: small
Reviewer: Sponsor: SponsorR

Description

Split #3521 addressing one of the three original goals from the ticket first mentionned by rransom.

The approach here is to add the key hs/desc/id/<addr.onion> to GETINFO command. If found, the descriptor is printed formatted as described in rend-spec.txt section 1.3.

Child Tickets

Change History (21)

comment:1 Changed 5 years ago by nickm

To be clear, this is only if te descriptor is *in the cache*. We shouldn't fetch anything based on a getinfo.

Also, we should probably add separate keys for the client-side cache or the service-side cache. They are different, and it's important to keep them separate.

comment:2 in reply to:  1 Changed 5 years ago by dgoulet

Replying to nickm:

To be clear, this is only if te descriptor is *in the cache*. We shouldn't fetch anything based on a getinfo.

Absolutely correct. This feature is another upcoming ticket for a new command. Other one of #3521.

Also, we should probably add separate keys for the client-side cache or the service-side cache. They are different, and it's important to keep them separate.

The HS service descriptor, that's also an other part of #3521 that I didn't yet have time to open :).

comment:3 Changed 5 years ago by dgoulet

Status: newneeds_review

See branch ticket14845_01 for the control-spec.txt change (https://git.torproject.org/user/dgoulet/torspec.git)

If that makes sense, I'll submit for review the little-t tor code.

comment:4 Changed 5 years ago by dgoulet

Status: needs_reviewneeds_revision

Hrm wait, this change is wrong, it uses the same command for the service and client which is not right because an HS can be a clienté. It's important to differentiate both of them in the controller command.

comment:5 Changed 5 years ago by dgoulet

Status: needs_revisionneeds_review

Ok, try 2! :)

I've settled for hs/client/desc/id/<ADDR>, a bit long but clearly indicates what we are looking for.

See branch ticket14845_02 for the control-spec.txt change (​https://git.torproject.org/user/dgoulet/torspec.git)

comment:6 Changed 5 years ago by nickm_mobile

To be clear, i don't think there should be a hsdir server equivalent.

comment:7 Changed 5 years ago by arma

And if the descriptor isn't found, it replies with an empty string, that is,

250 hs/client/desc/id/<ADDR>=

?

comment:8 in reply to:  7 Changed 5 years ago by dgoulet

Replying to arma:

And if the descriptor isn't found, it replies with an empty string, that is,

250 hs/client/desc/id/<ADDR>=

?

Oh true, forgot that, for instance GETINFO ns/id/<OR identity> returns "552 Unrecognized key" if not found so going with that makes sense. But then GETINFO ns/purpose/timeout returns "551 Internal error"... Much prefer 552 though. Unless we want to move towards a scheme of "250 NOT FOUND" ?...

comment:9 Changed 5 years ago by dgoulet

Ok so seems 551 is more suited after looking at the control code and how getinfo error msg are handled.

Here is the spec + tor code patch (original patch from asn, cleaned up a bit)

Control-spec: see branch ticket14845_03.
tor patch: see branch bug14845_026_01

comment:10 Changed 4 years ago by rl1987

I _think_ the added error messages should be more explicit. The first one should say that address is considered invalid specifically due to wrong length (if, for example, user added $ needlessly in front). The other should probably say "not found in client cache".

Otherwise, this looks good to me.

comment:11 Changed 4 years ago by yawning

The patch LGTM, with a separate patch at a later date adding a generic onion address validator routine to rendcommon.h/c that can return descriptive reasons for why we dislike a particular onion address (and to ease the amount of cleanup we need to do when Ed25519 happens).

Maybe do: "const rend_cache_entry_t *e = NULL;" to catch dumb in the future, though it's fine either way.

comment:12 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:13 Changed 4 years ago by isabela

Keywords: SponsorS added
Points: small
Priority: normaltrivial
Version: Tor: 0.2.7

comment:14 Changed 4 years ago by nickm

Merged, and added f5fa6ac534d9262701382cab3152826cc56a9e5d to fix a memory leak and d48df89d19a71752b1c849f7410877c852702b5a for changes file.

comment:15 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged the control-spec change too.

comment:16 Changed 4 years ago by markxr@…

Not working for me as of commit 411049d0d44963b8d9ec6f96c8dc62a106d6cc30

Transcript:

authenticate ""
250 OK
setevents hs_desc
250 OK
hsfetch facebookcorewwwi
250 OK
650 HS_DESC REQUESTED facebookcorewwwi NO_AUTH $B53F5DE5FD55C836BED6BDDBFC1DC906B4543821~curly wu5uc7ph33qdsgnvet5yesdvvw2bhn6r
650 HS_DESC RECEIVED facebookcorewwwi NO_AUTH $B53F5DE5FD55C836BED6BDDBFC1DC906B4543821~curly
getinfo hs/client/desc/id/facebookcorewwwi
551 Not found in cache

I've found found any way of making hs/client/desc/id/anything return anything except "551 Not found in cache" - either by hsfetch command, or by connecting to the HS via socks.

comment:17 Changed 4 years ago by markxr@…

Cc: markxr@… added
Resolution: fixed
Status: closedreopened

comment:18 Changed 4 years ago by yawning

Keywords: nickm-review added
Status: reopenedneeds_review

comment:19 Changed 4 years ago by dgoulet

Goot catch, this broke when we merged the refactoring of rend cache lookup :S.

lgtm! Thanks!

comment:20 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

comment:21 Changed 4 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.