Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15881 closed defect (fixed)

Missing descriptor ID in some HS_DESC control event

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: controller
Cc: Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor: SponsorR

Description (last modified by dgoulet)

The control-spec does have it specified (see section 4.1.25) but Tor doesn't provide it for both RECEIVED and FAILED.

Child Tickets

Attachments (1)

0001-Fix-init-HSDirs-list-in-rend_data_service_create.patch (869 bytes) - added by dgoulet 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by dgoulet

Description: modified (diff)
Summary: Missing descriptor ID in the failed HS_DESC control eventMissing descriptor ID in some HS_DESC control event

comment:2 Changed 5 years ago by dgoulet

Status: newneeds_review

Right now it's pretty difficult to print the descriptor ID every time without some "significant" change to Tor on how HS descriptor query are handled. Ticket #15816 is part of the solution which would help us fix the full issue.

For now, we can print it when a descriptor ID is used for the fetch (HSFETCH command).

See branch bug15881_027_01.

comment:3 Changed 5 years ago by dgoulet

Status: needs_reviewneeds_revision

#15816 is being merged so hold up this one so I can provide a second version with the full complete fix.

comment:4 Changed 5 years ago by dgoulet

Status: needs_revisionneeds_review

Full fix: bug15881_027_02

comment:5 Changed 5 years ago by nickm

  • Can this have tests?
  • In get_desc_id_from_query(), is it possible that the set of directories we knew about when we launched the query will not match the set that we know about when we call get_desc_id_from_query()? This could happen if we get a new networkstatus in between.
  • Does get_desc_id_from_query() return a (binary) digest or a hex string or a base32 string or what? It should be documented.

comment:6 in reply to:  5 Changed 5 years ago by dgoulet

Status: needs_reviewneeds_revision

Replying to nickm:

  • Can this have tests?

I think so yes, we have already HS control test, I'll see what I can do.

  • In get_desc_id_from_query(), is it possible that the set of directories we knew about when we launched the query will not match the set that we know about when we call get_desc_id_from_query()? This could happen if we get a new networkstatus in between.

Hrm... indeed, new consensus, churn occurs and thus we have an erroneous HSDir list. Fix to that seems to keep our responsible HSDir in rend_data but that's a bit more complicated because that means one different rend_data per HSDir connection. So would keeping the full list of HSDir fingerprint a reasonable idea?

  • Does get_desc_id_from_query() return a (binary) digest or a hex string or a base32 string or what? It should be documented.

Ack!

comment:7 Changed 5 years ago by yawning

This came up for the UPLOADED event as well. It would be really nice if the descriptor ID was provided, so that the event can be used to determine if a new HS should be reachable or not.

comment:8 Changed 5 years ago by dgoulet

Points: smallmedium
Status: needs_revisionneeds_review

All the fixes now in branch bug15881_027_03.

Extra commit added on top to fix the testing.

comment:9 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merging. Thanks!

comment:10 Changed 5 years ago by dgoulet

Resolution: fixed
Status: closedreopened

Initialization that I missed in rend_data_service_create which causes the rend_data free and dup to fail on a NULL pointer.

Maybe it would be better to check if hsdirs_fp is NULL in those functions, I don't have a preference here.

comment:11 Changed 5 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Merged that. Thanks!

comment:12 Changed 5 years ago by atagar

Yawning mentioned the UPLOADED action. I just checked and that still reports as 'UNKNOWN'. Should that be a separate ticket?

comment:13 in reply to:  12 Changed 5 years ago by dgoulet

Replying to atagar:

Yawning mentioned the UPLOADED action. I just checked and that still reports as 'UNKNOWN'. Should that be a separate ticket?

Yeah let's make another ticket about it because this one is already merged and I've looked to adding it to the UPLOADED event and it's a bit more work.

comment:14 Changed 5 years ago by atagar

Sounds good. Filed #16023 for that.

comment:15 Changed 4 years ago by dgoulet

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