Opened 5 weeks ago

Closed 3 weeks ago

#28127 closed defect (fixed)

Hidden service option HiddenServiceAuthorizeClient is incompatible with version 3

Reported by: pege Owned by: neel
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.5.1-alpha
Severity: Normal Keywords: tor-hs, hs-auth
Cc: asn, neel Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

When upgrading from 0.3.4 to 0.3.5 hidden services that have HiddenServiceAuthorizeClient set break. My understanding is that already created onion services should remain v2 and new services are created as v3. However, this doesn't appear to be the case if client authorization is active.

Steps to reproduce:

  1. Create an a hidden service like this in 0.3.4:
HiddenServiceDir /var/lib/tor/hidden_service/
HiddenServiceAuthorizeClient stealth client1,client2
HiddenServicePort 45325 127.0.0.1:22
  1. Upgrade to 0.3.5
  1. You'll see this error:
Oct 19 18:18:35 rasp3-l5 systemd[1]: Starting Anonymizing overlay network for TCP...
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.243 [notice] Tor 0.3.5.3-alpha running on Linux with Libevent 2.0.21-stable, OpenSSL 1.1.0f, Zlib 1.2.8, Liblzma 5.2.2, and Libzstd 1.1.2.
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.243 [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 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.243 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.243 [notice] Read configuration file "/usr/share/tor/tor-service-defaults-torrc".
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.243 [notice] Read configuration file "/etc/tor/torrc".
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.254 [warn] Hidden service option HiddenServiceAuthorizeClient is incompatible with version 3 of service in /var/lib/tor/hidden_service/
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.254 [warn] Failed to parse/validate config: Failed to configure rendezvous options. See logs for details.
Oct 19 18:18:36 rasp3-l5 tor[13098]: Oct 19 18:18:36.254 [err] Reading config failed--see warnings above.

In 0.3.5, setting HiddenServiceVersion 2 and removing it again has the same effect.

Child Tickets

Change History (14)

comment:1 Changed 5 weeks ago by traumschule

Keywords: tor-hs hs-auth added

comment:2 Changed 4 weeks ago by nickm

Cc: asn dgoulet added
Milestone: Tor: 0.3.5.x-final

comment:3 Changed 4 weeks ago by dgoulet

We've identified a couple issue:

1) If HiddenServiceVersion is explicitly set, it should be respected for the entire configuration process that is not even call the "learn version from the keys" mechanism.

2) If (1) is not set, then we should learn the version _early_ and *then* look for invalid options. Right now, the issue above is because tor stops as it found an invalid option while thinking it was a v3 but in reality was v2 because no v3 keys exist on disk.

3) In the code, the code below is wrong. config_learn_service_version() returns -1 if not keys were found to detect the version instead of the already configured version which is what is documented. Trivial fix but should be fixed!

  service->config.version = config_learn_service_version(service);
  switch (service->config.version) {
...

comment:4 Changed 4 weeks ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:5 Changed 4 weeks ago by neel

Status: assignedneeds_review

comment:6 Changed 4 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Thanks neel.

I'm unsure here if we should go with a flag in probably the hs_service_config_t object that we can set if we encounter a HiddenServiceVersion in the generic config process.

This would allow us to avoid re-iterating on all the lines. What do you think is better?

Also, Travis fails because make check-changes fails.

comment:7 in reply to:  6 Changed 4 weeks ago by asn

Replying to dgoulet:

Thanks neel.

I'm unsure here if we should go with a flag in probably the hs_service_config_t object that we can set if we encounter a HiddenServiceVersion in the generic config process.

This would allow us to avoid re-iterating on all the lines. What do you think is better?

Agreed. I think it's preferable to set a flag hs_version_explicitly_set or something instead of reiterating all the lines.

Also, would it be possible to write a unittest about this?

comment:8 Changed 4 weeks ago by neel

I pushed my new changes to the same branch. It uses a flag in hs_service_config_t instead of reading separately, and has a unit test (which passes).

If it passes CI, it's good to go. If not, I will try to fix it before the end of the day.

comment:9 Changed 4 weeks ago by dgoulet

This lgtm;

Lets remember that we need to fix this also (modified version :P):

3) config_learn_service_version() returns -1 if no keys were found in order to detect the version instead of the already configured version which is what is documented. Trivial fix but must be fixed!

comment:10 Changed 4 weeks ago by neel

Is the "already configured version which is what is documented" version 3 onion services, or what's set in the options? If it's the latter, I prevent calls to config_learn_service_version() if we have set HiddenServiceVersion. I assume it's the former.

comment:11 Changed 4 weeks ago by neel

Status: needs_revisionneeds_review

In config_learn_service_version(), a call to hs_service_get_version_from_key() is made (which ends up returning -1 on no key) and the code looks like this:

  version = hs_service_get_version_from_key(service);
  if (version < 0) {
    version = service->config.version;
  }

This means that if hs_service_get_version_from_key() is -1, then the version is automatically used. I did some debugging and a hs_service_get_version_from_key() of -1 with no HiddenServiceVersion returns a config_learn_service_version() of 3.

Setting this as needs_review as I don't think there is a bug and this case is already handled. If there is, would you be willing to clarify?

comment:12 Changed 3 weeks ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet
Status: needs_reviewmerge_ready

Oh yes you are right. My mistake.

I added 3 nitpick syntax stuff in the PR. We can fix that once this is merged as an extra commit so no worries. If you have time to push a fixup before it is merged, great else no biggy.

Thanks!

comment:13 Changed 3 weeks ago by neel

I have made the minor changes and pushed it to the same PR.

comment:14 Changed 3 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

I've pulled in the PR and based it onto 035 (ticket28127_035_01). I've squashed the structural commit as well, no conflict.

Merged upstream! Thanks.

(Note neel: you've become quite the contributor so one thing that would be great is if you can base your branch on the earlier version it will apply on. In this case, it would be 035 so then we can merge it forward. It is OK to forget sometimes, I still do but this practice is how tor is maintained. Big Thanks!)

Note: See TracTickets for help on using tickets.