Opened 13 months ago

Closed 9 months ago

#26812 closed defect (invalid)

hs: Adding client authorization through control port on an existing service fails

Reported by: dgoulet Owned by: asn
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs 035-can
Cc: asn Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

(Comes from reported issue in #26776).

The scenario is this:

  • You have a configured HiddenService that has been configured through the control port (SETCONF) *without* client authorization.
  • From the control port, you then SETCONF that same hidden service directory but with client authorization this time. The service will still be reachable without the client authorization that has just been set up.

The issue I believe comes from our transition from staging to the ready service list. See the crazy function rend_service_prune_list_impl_() for v2, we keep the introduction points alive even if we now have client authorization. Not good...

Child Tickets

Change History (13)

comment:1 Changed 11 months ago by nickm

Priority: MediumHigh

comment:2 Changed 10 months ago by nickm

Owner: set to asn
Status: newassigned

comment:3 Changed 10 months ago by asn

Status: assignedneeds_review

Please see patch for this: https://github.com/torproject/tor/pull/410

FWIW, this does not seem to be an issue of the new rend_service_prune_list_impl_(). I looked back in 0.2.3 and the issue still existed back then, so I think this bug has been around since The Beginning.

This branch includes a simple refactoring tech-debt-reducing commit because originally I was planning to do the changes in that function, but then I changed my mind. Feel free to remove that commit if you don't want it.

Last edited 10 months ago by asn (previous) (diff)

comment:4 Changed 10 months ago by dgoulet

Reviewer: dgoulet

comment:5 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_revision

Some syntactic sugar fixes only. Feel free to put in merge_ready once done.

comment:6 Changed 10 months ago by asn

Status: needs_revisionmerge_ready

Thanks for review. I pushed a new branch bug26812_v2 that addresses your comments, since the old one would not merge cleanly on master.

Marking as merge_ready as requested, but feel free to re-review.

comment:7 in reply to:  6 ; Changed 10 months ago by dgoulet

Cc: asn added
Status: merge_readyneeds_revision

Replying to asn:

Thanks for review. I pushed a new branch bug26812_v2 that addresses your comments, since the old one would not merge cleanly on master.

Marking as merge_ready as requested, but feel free to re-review.

Can you make a branch that is based on 035 so the CI can pass on it as well?

comment:8 in reply to:  7 Changed 10 months ago by asn

Status: needs_revisionmerge_ready

Replying to dgoulet:

Replying to asn:

Thanks for review. I pushed a new branch bug26812_v2 that addresses your comments, since the old one would not merge cleanly on master.

Marking as merge_ready as requested, but feel free to re-review.

Can you make a branch that is based on 035 so the CI can pass on it as well?

035 branch can be found here! https://github.com/torproject/tor/pull/477
FWIW, I did not include the refactoring commit in the 035 branch.

comment:9 Changed 10 months ago by dgoulet

Status: merge_readyneeds_information

Wait I have doubt about this now...

Closing all intro circuits and rebuilding shouldn't impact anything with or without client auth... This kind of ties in with #28275 a little bit.

Last edited 10 months ago by dgoulet (previous) (diff)

comment:10 Changed 10 months ago by dgoulet

Status: needs_informationnew

comment:11 in reply to:  9 Changed 10 months ago by asn

Replying to dgoulet:

Wait I have doubt about this now...

Closing all intro circuits and rebuilding shouldn't impact anything with or without client auth... This kind of ties in with #28275 a little bit.

Hm, if that's not the issue then what's the issue? The rendezvous circuits that are kept up?

Also, is the title of this ticket kinda misleading or what?

comment:12 Changed 9 months ago by nickm

Keywords: 035-can added

comment:13 Changed 9 months ago by dgoulet

Resolution: invalid
Status: newclosed

This is a non issue tbh... and yes the title is very misleading so lets get rid of this for now.

Note: See TracTickets for help on using tickets.