Opened 3 years ago

Closed 3 years ago

#20484 closed defect (fixed)

HiddenServiceDir must already exist when making a Single Onion Service

Reported by: pastly Owned by: teor
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.3-alpha
Severity: Normal Keywords: tor-hs, single-onion, review-group-12
Cc: Actual Points: 0.5
Parent ID: Points: 0.2
Reviewer: Sponsor:

Description

Full 0.2.9.4 testing notes are at this single onion service.

Oct 27 23:47:13.000 [notice] Tor 0.2.9.4-alpha (git-8b0755c9bb296ae2) opening log file.
Oct 27 23:47:13.501 [notice] Tor 0.2.9.4-alpha (git-8b0755c9bb296ae2) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.1t and Zlib 1.2.8.
Oct 27 23:47:13.501 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Oct 27 23:47:13.501 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Oct 27 23:47:13.501 [notice] Read configuration file "/media/b82d/x76slv/.torclient2/torrc".
Oct 27 23:47:13.510 [notice] HiddenServiceSingleHopMode is enabled; disabling UseEntryGuards.
Oct 27 23:47:13.510 [warn] HiddenServiceNonAnonymousMode is set. Every hidden service on this tor instance is NON-ANONYMOUS. If the HiddenServiceNonAnonymousMode option is changed, Tor will refuse to launch hidden services from the same directories, to protect your anonymity against config errors. This setting is for experimental use only.
Oct 27 23:47:13.000 [warn] This copy of Tor was compiled or configured to run in a non-anonymous mode. It will provide NO ANONYMITY.
Oct 27 23:47:13.000 [warn] Could not create single onion poison file /media/b82d/x76slv/.my-apache2-sos/hidden_service/onion_service_non_anonymous
Oct 27 23:47:13.000 [warn] Failed to mark new hidden services as non-anonymous.
Oct 27 23:47:13.000 [err] set_options(): Bug: Acting on config options left us in a broken state. Dying. (on Tor 0.2.9.4-alpha 8b0755c9bb296ae2)

But if I create the ~/.my-apache2-sos/hidden_service directory before starting Tor, it starts up fine.

Child Tickets

TicketStatusOwnerSummaryComponent
#20529closedCheck the directory for each rend service, not just the last oneCore Tor/Tor

Change History (25)

comment:1 Changed 3 years ago by teor

Keywords: tor-hs single-onion added
Points: 0.2
Version: Tor: 0.2.9.3-alpha

HiddenServiceDir says "DIRECTORY must be an existing directory".
So I think we could split this into two bugs:

  • this bug: log a warning message that asks the user to create the hidden service directory, rather than logging the BUG message,
  • breaking change #20486 in 0.3.0: create the directory for the user, and change the man page (we create the DataDirectory for the user, why not the HiddenServiceDirectory?).

comment:2 in reply to:  1 Changed 3 years ago by pastly

Replying to teor:

HiddenServiceDir says "DIRECTORY must be an existing directory".

I now see that the documentation says this, but that is not the case for regular onion services. I don't think I've ever made a HiddenServiceDir before tonight.

comment:3 Changed 3 years ago by teor

Ah, you're right, this works fine for me:

src/or/tor HiddenServiceDir `mktemp -d` HiddenServicePort 80 DataDirectory `mktemp -d`

As does this:

src/or/tor HiddenServiceDir `mktemp -d` HiddenServicePort 80 DataDirectory `mktemp -d` HiddenServiceNonAnonymousMode 1 HiddenServiceSingleHopMode 1 SocksPort 0

And this:

src/or/tor HiddenServiceDir `mktemp -d -u` HiddenServicePort 80 DataDirectory `mktemp -d -u`

But not this:

src/or/tor HiddenServiceDir `mktemp -d -u` HiddenServicePort 80 DataDirectory `mktemp -d -u` HiddenServiceNonAnonymousMode 1 HiddenServiceSingleHopMode 1 SocksPort 0

So we need to do this:

  • Call the code that creates the directory before trying to poison single onion services

And we might as well fix #20486 at the same time (document that the directory does not need to exist).

comment:4 Changed 3 years ago by teor

And while we're doing that, let's document the poison file in the FILES section of the man page.

comment:5 Changed 3 years ago by teor

And add a unit test to make sure we don't regress on this

comment:6 Changed 3 years ago by nickm

Status: newneeds_review

What do you think about my branch bug20484_029 as a fix here?

(I didn't do the documentation fix or the unit test part; please feel free to add them if you would like)

comment:7 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

It still fails:
src/or/tor HiddenServiceDir mktemp -d -u HiddenServicePort 80 DataDirectory mktemp -d -u HiddenServiceNonAnonymousMode 1 HiddenServiceSingleHopMode 1 SocksPort 0
likely because we attempt to poison the directory before the keys are created.

(I want to add the unit test equivalent of this - we could call the existing unit test with a parameter that says whether to create each directory or not.)

But it's part of the way there, I think we just need to call rend_service_check_and_create_private_dir before poisoning as well.

comment:8 Changed 3 years ago by nickm

Okay; you want to revise or should I?

comment:9 in reply to:  7 ; Changed 3 years ago by teor

Replying to teor:

It still fails:
src/or/tor HiddenServiceDir mktemp -d -u HiddenServicePort 80 DataDirectory mktemp -d -u HiddenServiceNonAnonymousMode 1 HiddenServiceSingleHopMode 1 SocksPort 0
likely because we attempt to poison the directory before the keys are created.

(I want to add the unit test equivalent of this - we could call the existing unit test with a parameter that says whether to create each directory or not.)

But it's part of the way there, I think we just need to call rend_service_check_and_create_private_dir before poisoning as well.

Sorry, I was wrong about this, it turns out I had compiled the wrong code.

comment:10 Changed 3 years ago by teor

Actual Points: 0.5

See my branch bug20484_029 on github, which also has a fix for #20529.

comment:11 Changed 3 years ago by teor

Status: needs_revisionneeds_review

comment:12 in reply to:  9 ; Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Question: Why don't we try to create the directory in rend_config_services() (for each service that your patch does) instead of in rend_service_load_all_keys() which is called much later in config.c. Basically, I think if we do ask for creation when configuring the services, we then have no need for it in the load keys function. I did the test and works well.

The rest lgtm so whatever you decide, feel free to merge_ready this.

comment:13 Changed 3 years ago by nickm

This LGTM modulo dgoulet's question above. Once you decide whether to change that, please merge_ready. :)

comment:14 in reply to:  12 Changed 3 years ago by teor

Status: needs_revisionneeds_review

There is some new code and some refactoring in bug20484_029, but it's mainly to do with testing. Still, I'd like it reviewed.

Replying to dgoulet:

Question: Why don't we try to create the directory in rend_config_services() (for each service that your patch does) instead of in rend_service_load_all_keys() which is called much later in config.c. Basically, I think if we do ask for creation when configuring the services, we then have no need for it in the load keys function. I did the test and works well.

Turns out this is possible, but requires a few more changes than you might expect.
2579d1d Create HS directories in rend_config_services, then check before use

I left the directory checks in those functions, but made them check-only (not create). It's a BUG if we get that far without having directories created.

I also found #20559 in the process, which I think we want to fix, particularly if people are going to switch between hidden services and single onion services:
428ee66 Stop ignoring misconfigured hidden services

Then finally, the new directory creation code means updated unit tests:
f37d3ed Update unit tests for 20484, 20529, 20559

And I almost forgot:
f702253 Add onion_service_non_anonymous file to man page

comment:15 Changed 3 years ago by nickm

This serious got pretty big! Right now the branch looks okay to me, but it looks more like 0.3.0 material than 0.2.9. After all, it is much much more than "bugfix only" at this point.

Is there a subset of this that we should take in 0.2.9, or should we defer the whole thing to 0.3.0, or is there a case that every part of this should go into 0.2.9?

comment:16 Changed 3 years ago by teor

I would take everything up to and including 0ee9049 in 0.2.9, and cherry-pick the doc change in f702253.

The directory creation reordering in 2579d1d, refactoring and updated unit tests can go in 0.3.0. There's no need to have them in 0.2.9, and we should probably allow people some time to get used to stricter checks on their hidden services anyway.

comment:17 Changed 3 years ago by teor

(0ee9049 is the point that dgoulet said "The rest lgtm so whatever you decide, feel free to merge_ready this.")

comment:18 Changed 3 years ago by teor

These are now available on my github as bug20484_029_v2 and bug20484_030_v2.

comment:19 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

Merged bug20484_029_v2. Going to call bug20484_030_v2 needs_review for 0.3.0 and have another look

comment:20 Changed 3 years ago by nickm

Keywords: review-group-12 added

comment:21 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_information

I can't find bug20484_030_v2 and seems that nickm/bug20484_029 as been merged already in 030 so what's the action tiem here?

Scratch that, branch is in teor/bug20484_030_v2.

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

comment:22 Changed 3 years ago by dgoulet

Status: needs_informationneeds_review

comment:23 Changed 3 years ago by teor

#20692 is a duplicate of part of this issue, it is fixed in commit ca21552 of teor/bug20484_030_v2.

comment:24 Changed 3 years ago by teor

Owner: set to teor
Status: needs_reviewassigned

comment:25 Changed 3 years ago by teor

Resolution: fixed
Status: assignedclosed

The remaining work for this task is in #20638 and #20559.

Note: See TracTickets for help on using tickets.