Opened 2 years ago

Closed 14 months ago

#17178 closed enhancement (implemented)

Rendezvous Single Onion Services: One-Hop Intro Point and Rendezvous

Reported by: teor Owned by: teor
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rsos, tor-hs, TorCoreTeam201609, review-group-5, review-group-8
Cc: asn Actual Points: 14
Parent ID: Points: 6.5
Reviewer: nickm Sponsor:

Description

The following patch enables one-hop intro point and rendezvous connections for onion service servers. It is compatible with current tor clients and relays.

Use the following torrc lines to make intro point and rendezvous point connections one-hop on the server:

__OnionSrvIntroRouteLength 1
__OnionSrvRendRouteLength 0

Branch: hs-route-len-v2
Repository: https://github.com/teor2345/tor.git

Child Tickets

TicketTypeStatusOwnerSummary
#17612enhancementclosedUpdate rendezvous single onion services proposal
#17674defectclosedcircuit_handle_first_hop doesn't respect ExtendAllowPrivateAddresses
#18126defectclosedteorUpdate RSOS extend_info_from_node calls to be direct connections
#18196enhancementclosedteorWrite options_validate tests for RSOS
#19663defectclosedOn rend failure, Single Onions should build a 3-hop path
#19677defectclosedCount unix sockets when counting client listeners
#19678defectclosedDon't allow hidden services in Tor2web mode
#20034defectclosedMake Tor2web work with ReachableAddresses
#20073defectclosedUse CircuitBuildTimeout whenever circuit_build_times_disabled is true
#20074defectclosedStop changing UseEntryGuards in authority and Tor2web configs
#20094enhancementclosedModify control spec for ADD_ONION OnionServiceSingleHopMode flag

Change History (93)

comment:1 Changed 2 years ago by teor

This is an alternate design and implementation of the "Stream latency is reduced on a 4-hop circuit" feature of #prop252. (It reduces the path length, but does it in a different way to the proposal, by building a one-hop path to hidden/onion service introduction points and rendezvous points.)

These one-hop introduction point and rendezvous paths are compatible with the existing hidden service implementation, and it works on the existing tor network without any changes to older relays or clients.

Some sites wish to scale up public ".onion" sites within the next 6 months, using the existing Tor network and Tor clients. This patch enables their use case, until #prop252 is implemented and deployed widely enough.

comment:2 Changed 2 years ago by teor

Status: newneeds_review

comment:3 Changed 2 years ago by nickm

Keywords: 028-triaged added

comment:4 Changed 2 years ago by nickm

Keywords: SponsorU removed
Sponsor: SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:5 Changed 2 years ago by nickm

Points: large

comment:6 Changed 2 years ago by dgoulet

Keywords: tor-hs added

comment:7 Changed 2 years ago by teor

Severity: Normal
Status: needs_reviewneeds_revision

The one-hop circuits that were created by this code weren't marked as one-hop, which caused a info-level bug log message in the pathbias code.

I've pushed a fixup to the branch hs-route-len-v2.
There's also a branch with the same changes based on Tor 0.2.6.10 at hs-route-len-02610.

I think this code needs a redesign (and a proposal) before it is merged, to:

  • get community consensus on the tradeoffs between efficiency, features, and splitting the hidden service anonymity set
  • collapse the two experimental options into a single option like OnionServerOneHop 1, which would do the equivalent of __OnionSrvIntroRouteLength 1 __OnionSrvRendRouteLength 0
  • (I also dislike that one-hop paths need to have one of these settings as 1, and the other as 0. This should be handled internally and not exposed to operators.)

comment:8 Changed 2 years ago by teor

I've created a new branch feature-17178-rsos that:

  • adds a new boolean option RendezvousSingleOnionServiceNonAnonymousServer
  • removes the previous options
  • sets the one-hop flag in the functions which launch intro and rendezvous circuits
  • simplifies the remaining code
  • rebase onto the latest 0.2.8

(There's also feature-17178-rsos-02610 based on Tor 0.2.6.10.)

This can be tested with the chutney branch feature-17178-rsos at https://github.com/teor2345/chutney.git using src/test/test-network.sh --flavor rsos-min.

This code still needs the following before it can be merged:

  • a proposal: to get community consensus on the tradeoffs between efficiency, features, and splitting the hidden service anonymity set, and
  • unit tests.

comment:9 Changed 2 years ago by teor

Please see the draft proposal in my rsos branch in https://gitbhub.com/torspec.git
I'll post it to tor-dev after another round of edits.

comment:10 Changed 2 years ago by asn

Cc: asn added

comment:11 Changed 2 years ago by teor

Status update:

The current implementation has one known issue:

  • when RendezvousSingleOnionServiceNonAnonymousServer is changed at runtime, tor attempts to re-use introduction points and introduction point circuits after a HUP. It should close the circuits and discard the intro points.

The proposal has been updated and posted to tor-dev.

The proposal is available in the branch “feature-17178-rsos” at https://github.com/teor2345/torspec.git as
torspec/proposals/ideas/xxx-rend-single-onion.txt

There is a basic implementation in the branch “feature-17178-rsos” at https://github.com/teor2345/tor.git

This can be tested with the chutney branch "feature-17178-rsos” at https://github.com/teor2345/chutney.git using the command:
src/test/test-network.sh --flavor rsos-min

comment:12 Changed 2 years ago by nickm

Priority: MediumHigh

comment:13 Changed 2 years ago by asn

Hello teor, what can I do here to help? What's the current status here? I can spend a few hours this week on this ticket.

comment:14 in reply to:  13 ; Changed 2 years ago by teor

Replying to asn:

Hello teor, what can I do here to help? What's the current status here? I can spend a few hours this week on this ticket.

The code works and has been tested using chutney.

It doesn't behave correctly when RendezvousSingleOnionServiceNonAnonymousServer Is changed at runtime (using torrc and a HUP, or over the control port). The current code attempts to re-use introduction points and introduction point circuits after a HUP. But if the value of RendezvousSingleOnionServiceNonAnonymousServer has changed, the circuits are the wrong length. Tor should close the circuits and discard the intro points (this needs to be coded), then post a fresh descriptor (this likely already happens anyway after a config change).

There aren't any specific unit tests, but any existing unit tests pass.

comment:15 in reply to:  14 ; Changed 2 years ago by asn

Replying to teor:

Replying to asn:

Hello teor, what can I do here to help? What's the current status here? I can spend a few hours this week on this ticket.

The code works and has been tested using chutney.

It doesn't behave correctly when RendezvousSingleOnionServiceNonAnonymousServer Is changed at runtime (using torrc and a HUP, or over the control port). The current code attempts to re-use introduction points and introduction point circuits after a HUP. But if the value of RendezvousSingleOnionServiceNonAnonymousServer has changed, the circuits are the wrong length. Tor should close the circuits and discard the intro points (this needs to be coded), then post a fresh descriptor (this likely already happens anyway after a config change).

This can be done yes, but it's some moderate engineering complexity. Are we sure we want RendezvousSingleOnionServiceNonAnonymousServer to be hotpluggable like that? We think HS operators would enjoy that, or can we just fail and warn the user if RSOS was enabled after a HUP?

And also there is the opposite direction. What happens if RSOS is disabled after a HUP? Then you need to kill all 1-hop circuits and make 3-hop ones? Do we want people to think it's so easy to switch between these two modes?

comment:16 in reply to:  15 Changed 2 years ago by teor

Replying to asn:

Replying to teor:

Replying to asn:

Hello teor, what can I do here to help? What's the current status here? I can spend a few hours this week on this ticket.

The code works and has been tested using chutney.

It doesn't behave correctly when RendezvousSingleOnionServiceNonAnonymousServer Is changed at runtime (using torrc and a HUP, or over the control port). The current code attempts to re-use introduction points and introduction point circuits after a HUP. But if the value of RendezvousSingleOnionServiceNonAnonymousServer has changed, the circuits are the wrong length. Tor should close the circuits and discard the intro points (this needs to be coded), then post a fresh descriptor (this likely already happens anyway after a config change).

This can be done yes, but it's some moderate engineering complexity. Are we sure we want RendezvousSingleOnionServiceNonAnonymousServer to be hotpluggable like that? We think HS operators would enjoy that, or can we just fail and warn the user if RSOS was enabled after a HUP?

That's a really good point. The existing design deliberately doesn't support mixing HS and RSOS for security reasons. (See below.)

And also there is the opposite direction. What happens if RSOS is disabled after a HUP? Then you need to kill all 1-hop circuits and make 3-hop ones?

If we were to close all connections, it would have to happen on both RSOS to HS, and HS to RSOS. But that's not secure.

Do we want people to think it's so easy to switch between these two modes?

You're right, we really can't allow hot-plugging securely and safely.

I think we need secure defaults here:

  1. a Tor instance can only run all RSOS, or all HS at one time (RendezvousSingleOnionServiceNonAnonymousServer is global)
  2. after an onion service is launched, a Tor instance can only run all RSOS, or all HS, until Tor is restarted (RendezvousSingleOnionServiceNonAnonymousServer is persistent on first RSOS/HS launch)

These rules protect operators from inadvertent disclosure by either simultaneously or sequentially running onion services with 1 and 3 hop paths on the same Tor instance. But they also support the ephemeral use-case, where the operator launches Tor, sets or unsets RendezvousSingleOnionServiceNonAnonymousServer, then launches an onion service.

I have implemented 1, but I haven't implemented 2 yet. To implement it, Tor needs to check: if RendezvousSingleOnionServiceNonAnonymousServer is changed, and there are any onion services active (excluding deleted/inactive services, if this is a feature of (ephemeral) services), then reject the change and warn the user. Tell them to restart if they really want to change it.

Can you help with that?

comment:17 Changed 2 years ago by asn

Yes. I aim to review the code and implement the missing logic this week. Let's see!

comment:18 Changed 2 years ago by teor

nickm says on IRC:

teor
Is it OK to make operators restart their tor instance to switch to/from (Rendezvous) Single Onion Services?

nickm
IMO absolutely yes; and in fact... the same datadirectory probably shouldn't let you do that without some kind of a "yes I know what I'm doing"
err,
the same hidden service directory

comment:19 Changed 2 years ago by asn

Hello, please see branch feature-17178-rsos in my repo for the code that blocks
RSOS hotplugging. That was easy to do.

Now if we want to do more fancy stuff like "don't allow transitioning between
RSOS and normal HS even after restart", then we need to do more stuff (like put
a notice in the hidden service dir that RSOS was enabled). I'm wondering if we
should get in this trouble.

BTW, I noticed there is no code that blocks cannibalization of rendezvous
circuits. I remember that this was a problem in the past, since you ended up
cannibalizing 3-hop circuits all the time. Is this something we don't care
anymore, or maybe it was never a problem?

comment:20 in reply to:  19 ; Changed 2 years ago by teor

Replying to asn:

Hello, please see branch feature-17178-rsos in my repo for the code that blocks
RSOS hotplugging. That was easy to do.

Now if we want to do more fancy stuff like "don't allow transitioning between
RSOS and normal HS even after restart", then we need to do more stuff (like put
a notice in the hidden service dir that RSOS was enabled). I'm wondering if we
should get in this trouble.

Ideally, we should mark every key that is used for a HS or RSOS on first use, and refuse to use it for the other flavour/purpose unless an option like PermitNonAnonymousMultiUseOnionServiceKeys is set.

But I don't like the idea of modifying keys. So instead, we could write a file to the directory that says whether the keys were last used for an anonymous or non-anonymous service.

I think the security benefits are worth the extra complexity, and I can't see how to make it work without an extra file in the onion service directory.

(As a comparison, Tor2Web will only run with a different binary (with a compilation flag) and specific torrc option.)

BTW, I noticed there is no code that blocks cannibalization of rendezvous
circuits. I remember that this was a problem in the past, since you ended up
cannibalizing 3-hop circuits all the time. Is this something we don't care
anymore, or maybe it was never a problem?

I improved the code so the one-hop flag is set on intro and rendezvous circuits. (The previous code modified the path length calculation.)
Circuits with the one-hop flag never cannibalize existing circuits, and aren't themselves cannibalized.

It makes for much simpler code, and the flag is set where the circuit is initiated, so it's more readable, too. (And setting the one-hop flag makes the rest of the code treat the circuits like it does other one-hop circuits, which is also a win.)

comment:21 Changed 2 years ago by teor

Keywords: TorCoreTeam201511 added

comment:22 in reply to:  20 ; Changed 2 years ago by asn

Status: needs_revisionneeds_review

Replying to teor:

Replying to asn:

Hello, please see branch feature-17178-rsos in my repo for the code that blocks
RSOS hotplugging. That was easy to do.

Now if we want to do more fancy stuff like "don't allow transitioning between
RSOS and normal HS even after restart", then we need to do more stuff (like put
a notice in the hidden service dir that RSOS was enabled). I'm wondering if we
should get in this trouble.

Ideally, we should mark every key that is used for a HS or RSOS on first use, and refuse to use it for the other flavour/purpose unless an option like PermitNonAnonymousMultiUseOnionServiceKeys is set.

But I don't like the idea of modifying keys. So instead, we could write a file to the directory that says whether the keys were last used for an anonymous or non-anonymous service.

I think the security benefits are worth the extra complexity, and I can't see how to make it work without an extra file in the onion service directory.

(As a comparison, Tor2Web will only run with a different binary (with a compilation flag) and specific torrc option.)

OK, I coded this feature and pushed it in branch feature-17178-rsos. Let me know if you like it.

I still feel a bit guilty for adding another 100 lines of rarely-used code to rendservice.c, but...

Also, I was not sure what to do about ephemeral hidden services who don't have an HS directory.

comment:23 in reply to:  22 Changed 2 years ago by teor

Replying to asn:

Replying to teor:

Replying to asn:

Hello, please see branch feature-17178-rsos in my repo for the code that blocks
RSOS hotplugging. That was easy to do.

Now if we want to do more fancy stuff like "don't allow transitioning between
RSOS and normal HS even after restart", then we need to do more stuff (like put
a notice in the hidden service dir that RSOS was enabled). I'm wondering if we
should get in this trouble.

Ideally, we should mark every key that is used for a HS or RSOS on first use, and refuse to use it for the other flavour/purpose unless an option like PermitNonAnonymousMultiUseOnionServiceKeys is set.

But I don't like the idea of modifying keys. So instead, we could write a file to the directory that says whether the keys were last used for an anonymous or non-anonymous service.

I think the security benefits are worth the extra complexity, and I can't see how to make it work without an extra file in the onion service directory.

OK, I coded this feature and pushed it in branch feature-17178-rsos. Let me know if you like it.

Thanks, I'll have a look at it this week.

I still feel a bit guilty for adding another 100 lines of rarely-used code to rendservice.c, but...

Also, I was not sure what to do about ephemeral hidden services who don't have an HS directory.

Ephemeral hidden services don't need this feature:

  • On HUP: RSOS / HS path lengths can't change, and
  • On restart: ephemeral hidden service keys aren't persistent (so there is no need to mark them as anonymous / non-anonymous).

If operators retrieve their own ephemeral keys, I think they can manage anonymity as well.

comment:24 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

Reviewing asn's HS/RSOS poisoning changes:

  • code looks good, but I wonder if we should make it generic, so we can use it with single onion services as well, by using more generic terms, like SOS and "non-anonymous onion service":
    #define SOS_POISON_FNAME "non_anonymous_onion_service"
    
  • There's a warning for ephemeral RSOS, but I think it's actually an acceptable use case. The keys aren't persistent, so there's no issue with reuse in anonymous/non-anonymous modes. Maybe make it a log_info and remove the "can't be"?
    log_warn(LD_GENERAL, "Ephemeral HS can't be started as RSOS.");
    

Running the changes:

  • chutney verify works using chutney rsos-min network in the chutney feature-17178-rsos branch
  • changing from RSOS to HS or HS to RSOS and then doing a HUP fails.
  • changing from RSOS to HS and then relaunching tor fails.

Changing from HS to RSOS in the torrc and relaunching doesn't fail. I think this is OK, because we need an upgrade path for hidden service operators to deliberately upgrade to a rendezvous single onion service if they want to. And I don't like the idea of making operators touch a file in the filesystem to use RSOS.

We already have an extensive warning the first time we start up with RSOS and poison the directory. Is this sufficient for the HS -> RSOS case?

[warn] RendezvousSingleOnionServiceNonAnonymousServer (RSOS) is set. Every hidden/onion service on this instance is NON-ANONYMOUS. Clients remain location-anonymous, but may be statistically distinguishable. If RSOS is disabled, Tor will refuse to  launch RSOS hidden services to protect against misconfiguration errors. This setting is for experimental use only.

comment:25 Changed 2 years ago by teor

Keywords: TorCoreTeam201601 added; TorCoreTeam201511 removed

This keeps throwing up child tickets as we discover interesting edge cases in the rest of the Tor codebase. I'd say January is more likely.

comment:26 in reply to:  24 Changed 2 years ago by asn

Replying to teor:

Reviewing asn's HS/RSOS poisoning changes:

  • code looks good, but I wonder if we should make it generic, so we can use it with single onion services as well, by using more generic terms, like SOS and "non-anonymous onion service":
    #define SOS_POISON_FNAME "non_anonymous_onion_service"
    
  • There's a warning for ephemeral RSOS, but I think it's actually an acceptable use case. The keys aren't persistent, so there's no issue with reuse in anonymous/non-anonymous modes. Maybe make it a log_info and remove the "can't be"?
    log_warn(LD_GENERAL, "Ephemeral HS can't be started as RSOS.");
    

OK I implemented both of the above changes and pushed them in my branch feature-17178-rsos.

Can someone test that ephemeral HSes play nicely with the RSOS feature in this latest branch? Will do it myself soon if someone beats me to it.

comment:27 Changed 2 years ago by asn

Status: needs_revisionneeds_review

comment:28 Changed 22 months ago by teor

Keywords: TorCoreTeam201602 added; TorCoreTeam201601 removed

I don't think this will finish in January, I have some non-tor work to do.

comment:29 Changed 22 months ago by teor

Status update:

  • I've reviewed and tested asn's changes in branch feature-17178-rsos, and they look good.
    • But I haven't tried ephemeral hidden services or ephemeral RSOS.
  • I need a code review on my changes in branch feature-17178-rsos
  • I need to fix the issues listed in the child tickets, I'm not sure whether to:
    • get this branch merged first, or
    • add each child ticket as a separate commit to this branch. I'm concerned this branch will become too big and hard to review.

comment:30 Changed 22 months ago by teor

Status: needs_reviewneeds_revision

I want to finish #17625, #17358, and #17788 on this branch, and then get it reviewed in Feb.

comment:31 Changed 22 months ago by teor

Status: needs_revisionneeds_review

I finished #17625 and #17358, but #17788 is large and I want to do it on a separate branch.

My branch feature-17178-rsos is ready for review on https://github.com/teor2345/tor.git

comment:32 Changed 22 months ago by teor

Summary: Single Onion Services: One-Hop Intro Point and RendezvousRendezvous Single Onion Services: One-Hop Intro Point and Rendezvous

comment:33 in reply to:  31 ; Changed 22 months ago by asn

Replying to teor:

I finished #17625 and #17358, but #17788 is large and I want to do it on a separate branch.

My branch feature-17178-rsos is ready for review on https://github.com/teor2345/tor.git

Looked at the code some more.

Please see my branch feature-17178-rsos for a unittest on the poisoning functionality.

Also, I feel a bit uneasy about code like this:

+      if (!options->RendezvousSingleOnionServiceNonAnonymousServer) {
+        service->next_upload_time += crypto_rand_int(2*rendpostperiod);
+      }

It's a bit like we are treating location-hidden services as a special case, whereas we should probably have it be the default case. I don't mind the specific snippet above, but maybe we could functionify the RSOS option check to also make it a bit nicer (since it's huge and unreadable). Maybe we could put it in a function service_has_no_location_hiding() (or some nicer name please).

Similarly I don't like:

 #ifndef NON_ANONYMOUS_MODE_ENABLED
-  tor_assert(!(circuit->build_state->onehop_tunnel));
+  if (!get_options()->RendezvousSingleOnionServiceNonAnonymousServer) {
+    tor_assert(!(circuit->build_state->onehop_tunnel));
+  }
 #endif

these asserts used to make the code look terrible, and now they are worse. The number of negative clauses in those asserts makes it even more confusing. Do you think we could functionify those asserts similar to assert_circuit_ok()? Also, shouldn't we assert that if we are in RSOS mode, it is a one hop tunnel?

comment:34 in reply to:  33 Changed 22 months ago by teor

Replying to asn:

Replying to teor:

My branch feature-17178-rsos is ready for review on https://github.com/teor2345/tor.git

Please see my branch feature-17178-rsos for a unittest on the poisoning functionality.

Cherry-picked to my branch feature-17178-rsos.

Also, I feel a bit uneasy about code like this:

+      if (!options->RendezvousSingleOnionServiceNonAnonymousServer) {
+        service->next_upload_time += crypto_rand_int(2*rendpostperiod);
+      }

It's a bit like we are treating location-hidden services as a special case, whereas we should probably have it be the default case. I don't mind the specific snippet above, but maybe we could functionify the RSOS option check to also make it a bit nicer (since it's huge and unreadable). Maybe we could put it in a function service_has_no_location_hiding() (or some nicer name please).

For this particular case, refactored into rend_reveal_startup_time(), which applies to RSOS and will apply to SOS.

Similarly I don't like:

 #ifndef NON_ANONYMOUS_MODE_ENABLED
-  tor_assert(!(circuit->build_state->onehop_tunnel));
+  if (!get_options()->RendezvousSingleOnionServiceNonAnonymousServer) {
+    tor_assert(!(circuit->build_state->onehop_tunnel));
+  }
 #endif

these asserts used to make the code look terrible, and now they are worse. The number of negative clauses in those asserts makes it even more confusing. Do you think we could functionify those asserts similar to assert_circuit_ok()?

Refactored into assert_circ_onehop_ok(), which uses the new function rend_allow_direct_connection(). rend_allow_direct_connection() is true when in Tor2web or RSOS modes. (But won't be true for SOS, as they accept incoming client extend requests rather than making requests.)

Also, shouldn't we assert that if we are in RSOS mode, it is a one hop tunnel?

No, the security property we're asserting is that standard hidden services (and clients) *must* make 3-hop paths for everything except directory connections. This keeps their location hidden.

When we don't care about location hiding (RSOS, Tor2web), then there may be other reasons to make 3-hop paths. For example, RSOS make 3-hop paths to HSDirs to avoid denial of service attacks.

comment:35 Changed 22 months ago by teor

I also cherry-picked 3 commits which implement #8976 and #18126.

This branch is ready to be reviewed / merged as-is, does anyone else want to review it?

comment:36 Changed 22 months ago by asn

Small review.

How come assert_circ_onehop_ok() is a macro? Having it as a macro makes it look kinda ugly and also it's hidden in a header file.

Also, I don't entirely understand:

    tor_assert((is_dir) || rend_allow_direct_connection((options)) || \
               (circ)->build_state->onehop_tunnel == 0); \

it's different than the assert that it replaced. I would suggest to spread it out and maybe add some comments? Or is it something trivial that I don't get?
Hmm, I got it now. Maybe this whole comment is moot.

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

comment:37 in reply to:  36 Changed 22 months ago by teor

Replying to asn:

Small review.

How come assert_circ_onehop_ok() is a macro? Having it as a macro makes it look kinda ugly and also it's hidden in a header file.

Because the assertions correspond to the lines in the source file where the macro is used.
Otherwise, I could modify it to be a macro that calls a function that takes __FILE__ and __LINE__. We do that with one existing function already.

Also, I don't entirely understand:

    tor_assert((is_dir) || rend_allow_direct_connection((options)) || \
               (circ)->build_state->onehop_tunnel == 0); \

it's different than the assert that it replaced. I would suggest to spread it out and maybe add some comments? Or is it something trivial that I don't get?
Hmm, I got it now. Maybe this whole comment is moot.

For the record, it's an abstraction of the different checks for valid one-hop connections:

  • directory connections are one-hop
  • (Some) RSOS and Tor2web connections are one-hop
  • otherwise, the connection must not be one-hop.

comment:38 Changed 22 months ago by dgoulet

Status: needs_reviewneeds_revision

commit ce251ea5e7d98a8e46079f2733006f8e718717d8

  • This should be a static const char *. It's not required but imo we should use type as much as we can which is much more helpful on the compiler side.
#define RSOS_POISON_FNAME "non_anonymous_hidden_service_rsos"

If you really don't want to, that's fine but the tor_asprintf doesn't need to use %s for it. Same for PATH_SEPARATOR. (tor_asprintf(&fname, "%s" PATH_SEPARATOR ...))

  • Nitpick: You can use tor_free(poison_fname); once after file_status() is called. Avoid two of them.

commit ff63c64c9cdebb7ea50354a3e72cb57758f9f939

  • Hrm that commit simply return 0. Can't we flag the HS that it's actually in RSOS mode? By that I mean, can we have two ephemeral HS, one in RSOS and the other one not ?

commit 1e0b54feb5629eb85e9b365db684e1df8073a516

  • rend_allow_direct_connection() comment mentions: "Returns true in Tor2web and RSOS modes.". But the code return 1 if one of them is enabled, not both. So I'm guessing typo here.

commit 80a041b9740fa69126f40ddc1c8bba9555c8a08b

  • In rend_client_get_random_intro_impl(), this is added:
    -    new_extend_info = extend_info_from_node(node, 0);
    +    new_extend_info = extend_info_from_node(node,
    +                                        rend_allow_direct_connection(options));
    

This is somehow worrying me. I get the Tor2Web mode but what if I use my HS server as a client, I loose anonymity? Am I seeing that right?

Same goes in find_rp_for_intro(), if the HS is somehow compiled with NON_ANONYMOUS_MODE_ENABLED (Tor2Web), it goes to the RP/IP with one hop?

---

That's it for now! :)

comment:39 in reply to:  38 ; Changed 22 months ago by teor

Status: needs_revisionneeds_review

Please see my branch feature-17178-rsos:

Added 5de1055acb37f9a448ef6e769a5587c9d00441e7 as a fixup for f519c356a17afec8dd732ec2e46315c5576283d0:

Make tor_addr_is_multicast accessible in address.h.

Replying to dgoulet:

commit ce251ea5e7d98a8e46079f2733006f8e718717d8

  • This should be a static const char *. It's not required but imo we should use type as much as we can which is much more helpful on the compiler side.
#define RSOS_POISON_FNAME "non_anonymous_hidden_service_rsos"

If you really don't want to, that's fine but the tor_asprintf doesn't need to use %s for it. Same for PATH_SEPARATOR. (tor_asprintf(&fname, "%s" PATH_SEPARATOR ...))

Fixed in 30969ca2abec80ebacb585767a88915a53293c01.

  • Nitpick: You can use tor_free(poison_fname); once after file_status() is called. Avoid two of them.

Fixed in 30969ca2abec80ebacb585767a88915a53293c01.

commit ff63c64c9cdebb7ea50354a3e72cb57758f9f939

  • Hrm that commit simply return 0. Can't we flag the HS that it's actually in RSOS mode? By that I mean, can we have two ephemeral HS, one in RSOS and the other one not ?

No, we decided that was a bad design, as the non-anonymous RSOS could expose the anonymous HS.
Whenever RSOS is set, it applies to all configured services.

commit 1e0b54feb5629eb85e9b365db684e1df8073a516

  • rend_allow_direct_connection() comment mentions: "Returns true in Tor2web and RSOS modes.". But the code return 1 if one of them is enabled, not both. So I'm guessing typo here.

One of the glorious ambiguities of English. I meant: "Returns true in Tor2web mode and returns true in RSOS mode."

Fixed in 70f44487b8ecdeef09681f02068ac30b6643a0e5.

commit 80a041b9740fa69126f40ddc1c8bba9555c8a08b

  • In rend_client_get_random_intro_impl(), this is added:
    -    new_extend_info = extend_info_from_node(node, 0);
    +    new_extend_info = extend_info_from_node(node,
    +                                        rend_allow_direct_connection(options));
    

This is somehow worrying me. I get the Tor2Web mode but what if I use my HS server as a client, I loose anonymity? Am I seeing that right?

Same goes in find_rp_for_intro(), if the HS is somehow compiled with NON_ANONYMOUS_MODE_ENABLED (Tor2Web), it goes to the RP/IP with one hop?

I think that you're right, if you use a Tor2web client with a HS, or run a standard tor client as a RSOS, you lose anonymity.

So let's prevent that. See bae2a4de61d6d440840411fb992ffc72ad04c660.

For the record, these code changes only control how the node's address is selected.
(In #17840, we change extend_info_from_node() to select addresses based on ReachableAddresses, ClientUseIPv4/6 and ClientPreferIPv6OR/DirPort.)

It's the code like this that makes the connections one-hop:

    if (rend_allow_direct_connection(options)) {
          flags = flags | CIRCLAUNCH_ONEHOP_TUNNEL;
    }

comment:40 Changed 21 months ago by nickm

Owner: set to teor
Status: needs_reviewassigned

setting owner

comment:41 Changed 21 months ago by nickm

Status: assignedneeds_review

comment:42 in reply to:  39 ; Changed 21 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to teor:

All fixes look good!

[snip]

I think that you're right, if you use a Tor2web client with a HS, or run a standard tor client as a RSOS, you lose anonymity.

So let's prevent that. See bae2a4de61d6d440840411fb992ffc72ad04c660.

Ok I see that this is extra protection since somehow a client using an RSOS tor instance is distinguishable in some ways? (comment in or.h):

+   * location-anonymous. However, client use of a RSOS may be statistically
+   * distinguishable.

As a last change, I would document that part in the manpage for the option RendezvousSingleOnionServiceNonAnonymousServer. It changes things a bit because now the server that host a RSOS needs a second tor for any other client usage which is not that easy to have in a standard Linux distro (systemd and all only manage one single tor). As long as the user as a way to learn that before setting it up, it's fine.

Thanks!

comment:43 in reply to:  42 Changed 21 months ago by teor

Status: needs_revisionneeds_review

Replying to dgoulet:

Replying to teor:

All fixes look good!

[snip]

I think that you're right, if you use a Tor2web client with a HS, or run a standard tor client as a RSOS, you lose anonymity.

So let's prevent that. See bae2a4de61d6d440840411fb992ffc72ad04c660.

Ok I see that this is extra protection since somehow a client using an RSOS tor instance is distinguishable in some ways? (comment in or.h):

+   * location-anonymous. However, client use of a RSOS may be statistically
+   * distinguishable.

As a last change, I would document that part in the manpage for the option RendezvousSingleOnionServiceNonAnonymousServer. It changes things a bit because now the server that host a RSOS needs a second tor for any other client usage which is not that easy to have in a standard Linux distro (systemd and all only manage one single tor). As long as the user as a way to learn that before setting it up, it's fine.

Thanks!

I added the manual page changes for RSOS and Tor2webMode in 4093eeb. I also fixed the option checking so that it takes into account the compile-time --enable-tor2web-mode setting, and not just the options at runtime.

comment:44 Changed 21 months ago by teor

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

Not going to make it into 0.2.9, we need to:

  • find a better name (I started a discussion on tor-onions, and asked alianthus to help us find a name that would make sense to the wider public),
  • fix #17788 so that RSOS (and relays) won't extend to their own addresses, or and RSOS, HS, and relays won't extend to private addresses.

comment:45 Changed 21 months ago by dgoulet

Keywords: 028-triaged TorCoreTeam201602 removed
Status: needs_reviewneeds_revision

comment:46 Changed 20 months ago by dgoulet

Sponsor: SponsorU

comment:47 Changed 18 months ago by isabela

Points: large6

comment:48 Changed 17 months ago by teor

Keywords: rsos TorCoreTeam201607 added
Status: needs_revisionneeds_review

Please see my tor branch feature-17178-v4 on https://github.com/teor2345/tor.git

It can be tested using my chutney branch feature-17178-v2 on https://github.com/teor2345/chutney.git

The following changes have been made since dgoulet reviewed "Make sure anonymous clients can't be configured with RSOS":

  • rebased to master (0.2.9)
  • a (squashed) fixup to that commit that improves the option description in the man page, and options checks for --enable-tor2web-mode
  • 372c449 Rename option to SingleOnionMode and add NonAnonymousMode.
  • eb76c18 Refactor is_sensitive_dir_purpose check & single onion options validation
  • 6a3a498 Downgrade Ephemeral Single Onion warning to info and LD_REND
  • 3ccc870 Add options_validate tests for SingleOnionMode

I think the single onion service code is feature-complete now. I'd like to fix #19163 and #17945 after this branch is merged.

comment:49 Changed 17 months ago by dgoulet

Reviewer: dgoulet

comment:50 Changed 17 months ago by dgoulet

Status: needs_reviewneeds_revision

comment:51 Changed 17 months ago by teor

Status: needs_revisionneeds_review

I commented on the review.
And I added commits to the branch feature-17178-v4.

Only one outstanding question:

David Goulet @dgoulet commented 3 days ago Master
So hrm... now services with Tor2web compiled in have now single hop?

Tim Wilson-Brown @teor commented 21 minutes ago
Yes, if you configure non-anonymous Single Onion or Tor2web, you can also configure the other one. And if you configure anonymous Hidden Service, you can also configure Anonymous Client. But you can't mix non-anonymous and anonymous.
Is there some scenario you'd like us to prevent here?

I am happy to split the option check into rend_allow_direct_connection_single_onion() and rend_allow_direct_connection_tor2web(), if you think that would be clearer. But it might be more confusing - most of the time we check if connections are one-hop, we actually want to check if it's either tor2web or single onion.

comment:52 Changed 17 months ago by nickm

Keywords: review-group-5 added

comment:53 Changed 17 months ago by dgoulet

Status: needs_reviewneeds_revision

More comments. Sorry about that teor, I'm throwing back some discussion about torrc names but give you a strong proposition :).

Also, a discussion on NonAnonymousMode.

comment:54 Changed 17 months ago by teor

Status: needs_revisionneeds_review

I understand the namespacing thing, and I am happy to comply, with one minor change.

I also see the confusion around NonAnonymousMode, so I disentangled the client and server checks (except where they're used in logging, or assertions).

Once you're happy, I will squash this branch for nick to review.

comment:55 Changed 17 months ago by teor

Please see my branch feature-17178-v5-rebased, which squashes this branch and rebases it onto the latest master.

I made a few fixes, they can be checked using git diff feature-17178-v4 feature-17178-v5:

  • split the changes file up into the fix for #19677, and the feature #17178
  • clarified the manual entry for OnionServiceNonAnonymousMode to say it's server only
  • removed some unnecessary changes:
    • left whitespace alone (including a set of parenthesis that didn't change the meaning)
    • kept an #if ENABLE_TOR2WEB_MODE block rather than refactoring it
    • avoided making Single Onion rend retries single-hop, so I didn't have to come back and fix it in #19663

comment:56 Changed 16 months ago by teor

I pushed some more fixes, because the fix to #19663 didn't cover reachable addresses. I also ended up borrowing some commits from #19163 to avoid merge conflicts.

  • HS rend opportunistically upgrades to ntor
  • Single Onion Services retry a 3-hop path when rend is unreachable

The squashed and rebased version is available at:
https://gitlab.com/teor/tor/merge_requests/3

comment:57 in reply to:  56 Changed 16 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to teor:

I pushed some more fixes, because the fix to #19663 didn't cover reachable addresses. I also ended up borrowing some commits from #19163 to avoid merge conflicts.

  • HS rend opportunistically upgrades to ntor
  • Single Onion Services retry a 3-hop path when rend is unreachable

The squashed and rebased version is available at:
https://gitlab.com/teor/tor/merge_requests/3

Some comments have been put there.

comment:58 Changed 16 months ago by teor

Points: 66.5
Status: needs_revisionneeds_review

I have fixed the comments and pushed changes to feature-17178-v5-rebased.

comment:59 Changed 16 months ago by asn

Hello, heard this code is almost about to be merge_ready so I did another review:
https://gitlab.com/asn/tor/merge_requests/2

The patch turned out to be much bigger than I expected, so my comments are mainly code quality issues.

comment:60 Changed 16 months ago by dgoulet

Status: needs_reviewneeds_revision

Considering asn's review and this unanswered comment, putting this one back in needs_revision.

https://gitlab.com/teor/tor/merge_requests/3#note_13440551

comment:61 Changed 16 months ago by nickm

Keywords: review-group-6 added

comment:62 Changed 16 months ago by asn

Keywords: TorCoreTeam201608 added; TorCoreTeam201607 removed

comment:63 Changed 16 months ago by nickm

Keywords: review-group-7 added; review-group-6 removed

All review-group-6 tickets have had at least one review; moving them to 7.

comment:64 Changed 15 months ago by teor

Actual Points: 10
Parent ID: #19923
Status: needs_revisionneeds_review

I removed some code due to dgoulet's review.
So the branch is now feature-17178-v6.

The new commits start at:
1a9d8a3 fixup! Single Onion Services retry a 3-hop path when rend is unreachable

dgoulet wrote a review on:
https://gitlab.com/teor/tor/merge_requests/3

In response to dgoulet's review, I split off opportunistic ntor upgrades into #19923.
There are a lot of dependencies, and I don't want to hold this feature up.

asn wrote a review on:
https://gitlab.com/asn/tor/merge_requests/2

asn seemed to agree with dgoulet about the code in #19923 being too complex. Let's deal with it separately.

In response to asn's review, I moved some Single Onion code into its own functions, refactored code so the logic was clearer, and added checks for single onion mode to functions that should only be called in that mode:
[feature-17178-v6 84d0ec7] fixup! Implement Prop #260: Single Onion Services
[feature-17178-v6 c72a46e] fixup! fixup! Implement Prop #260: Single Onion Services
[feature-17178-v6 790d39c] fixup! fixup! fixup! Implement Prop #260: Single Onion Services

The new gitlab merge request is at:
https://gitlab.com/teor/tor/merge_requests/6

comment:65 Changed 15 months ago by dgoulet

On the overall patch, I found minor things:

  • Long tab :)
    +    if (rend_service_use_direct_connection(options, rp)) {
    +          flags = flags | CIRCLAUNCH_ONEHOP_TUNNEL;
    +    }
    
  • rend_service_reveal_startup_time() is exactly the same thing as rend_service_allow_direct_connection() so making the reveal startup time function use rend_service_allow_direct_connection() could be a better choice as the requirement for now is for the service to allow direct connection. I like the separation of semantic here so something like this would be good I think:
int rend_service_reveal_startup_time(const or_options_t *options)
{
   return rend_service_allow_direct_connection(options);
}

I'm happy with the rest! I'll leave this in needs_review for asn to take a look at it.

comment:66 in reply to:  65 ; Changed 15 months ago by teor

Replying to dgoulet:

On the overall patch, I found minor things:

  • Long tab :)
    +    if (rend_service_use_direct_connection(options, rp)) {
    +          flags = flags | CIRCLAUNCH_ONEHOP_TUNNEL;
    +    }
    

That snuck in during a merge. Fixed along with an end-of-file EOL in:
[feature-17178-v6 f239a1e] Appease make check-spaces

  • rend_service_reveal_startup_time() is exactly the same thing as rend_service_allow_direct_connection() so making the reveal startup time function use rend_service_allow_direct_connection() could be a better choice as the requirement for now is for the service to allow direct connection. I like the separation of semantic here so something like this would be good I think:
int rend_service_reveal_startup_time(const or_options_t *options)
{
   return rend_service_allow_direct_connection(options);
}

Changed in [feature-17178-v6 0b964a5] fixup! fixup! fixup! fixup! Implement Prop #260: Single Onion Services

I'm happy with the rest! I'll leave this in needs_review for asn to take a look at it.

Over to asn then.

comment:67 Changed 15 months ago by teor

Parent ID: #19923

comment:68 in reply to:  66 ; Changed 15 months ago by asn

Replying to teor:

Replying to dgoulet:

On the overall patch, I found minor things:

  • Long tab :)
    +    if (rend_service_use_direct_connection(options, rp)) {
    +          flags = flags | CIRCLAUNCH_ONEHOP_TUNNEL;
    +    }
    

That snuck in during a merge. Fixed along with an end-of-file EOL in:
[feature-17178-v6 f239a1e] Appease make check-spaces

  • rend_service_reveal_startup_time() is exactly the same thing as rend_service_allow_direct_connection() so making the reveal startup time function use rend_service_allow_direct_connection() could be a better choice as the requirement for now is for the service to allow direct connection. I like the separation of semantic here so something like this would be good I think:
int rend_service_reveal_startup_time(const or_options_t *options)
{
   return rend_service_allow_direct_connection(options);
}

Changed in [feature-17178-v6 0b964a5] fixup! fixup! fixup! fixup! Implement Prop #260: Single Onion Services

I'm happy with the rest! I'll leave this in needs_review for asn to take a look at it.

Over to asn then.

OK did another pass over the branch, and suggested some mainly-aesthetic stuff in gitlab.
Code looks cleaner and easier to analyze than before.

BTW, why are these unix socket commits required for #17178? i.e. Count unix sockets when counting client listeners.

I hope to find some time to test the branch later today.

comment:69 in reply to:  68 ; Changed 15 months ago by dgoulet

Replying to asn:

BTW, why are these unix socket commits required for #17178? i.e. Count unix sockets when counting client listeners.

The way I understand this one is that if RSOS is enabled, client functionality MUST be disabled and we support Unix socket for client ports such as SocksPort so we have to count them as listeners in the RSOS case so we can know if we have a client enabled.

If I'm wrong here, hopefully teor can correct me and make it clearer in the commit message?

comment:70 in reply to:  69 Changed 15 months ago by teor

Actual Points: 1010.5

Replying to dgoulet:

Replying to asn:

BTW, why are these unix socket commits required for #17178? i.e. Count unix sockets when counting client listeners.

The way I understand this one is that if RSOS is enabled, client functionality MUST be disabled and we support Unix socket for client ports such as SocksPort so we have to count them as listeners in the RSOS case so we can know if we have a client enabled.

If I'm wrong here, hopefully teor can correct me and make it clearer in the commit message?

That's correct. I can redo the commit message next time I rebase the branch if you'd like.

I've made changes, but I'm holding off on pushing this branch until we resolve a more urgent issue that it depends on.

comment:71 Changed 15 months ago by teor

Actual Points: 10.511
Status: needs_reviewneeds_revision

This needs to be rebased and revised, as the changes in #19973 add a new flag to node selection.

comment:72 Changed 15 months ago by teor

Actual Points: 1111.5
Keywords: TorCoreTeam201609 added
Status: needs_revisionmerge_ready

I have revised and rebased this into my branch feature-17178-v7 on https://github.com/teor2345/tor.git

It can be tested using chutney branch feature-17178-v3 on https://github.com/teor2345/chutney.git with:

chutney/tools/test-network.sh --flavour single-onion

There haven't been any code changes from feature-17178-v6.
Here's what I did do:

  • rebased onto master
  • resolved some rebase merge conflicts
  • added an entry to the changes file
  • squashed some commits
  • reworded two commit messages

I'm putting this into merge_ready for nickm to look at, because asn and dgoulet seem happy with it to go there. If I have that wrong, please feel free to put it back, with suggestions.

comment:74 Changed 15 months ago by nickm

Status: merge_readyneeds_review

Actual-review-points += 0.5

Pfew, that was complicated and a bit stressful to review! (This is touching a lot of code where, if we mess up, the consequences are quite bad.)

I've made comments on the gitlab page. I think this isn't far from done.

When you respond to the comments, please make changes as separate fixup! or squash! commits on this same branch, so that I don't have to re-review everything I already reviewed.

comment:75 Changed 15 months ago by nickm

I should also clarify some of my comments about what things seem "error-prone". I'm (mostly) not suggesting that the code has errors now, but that I'm worried about whether we could introduce errors in the code down the line if we misunderstand what the code is supposed to mean. So the solution I'd hope for would be to try to make the code more idiot-proof.

comment:76 Changed 15 months ago by nickm

Two more concerns that just occurred to me:

  1. Should the 'poisoning' feature work in both directions? It seems to me that the damage from accidentally making an anonymous hidden service non-anonymous would be much much greater than the danger of accidentally anonymizing something that you didn't mean to anonymize.
  1. I anticipate that if all the options for making RSOS start with "OnionService" and all of the options for making anonymous onions services start with "HiddenService", then people will call RSOS "onion services" and continue to call anonymous onion services "hidden services." That's probably not what we had in mind.

comment:77 Changed 15 months ago by nickm

Status: needs_reviewneeds_revision

comment:78 Changed 15 months ago by nickm

Keywords: review-group-8 added; review-group-7 removed

comment:79 in reply to:  76 Changed 15 months ago by teor

Replying to nickm:

Two more concerns that just occurred to me:

  1. Should the 'poisoning' feature work in both directions? It seems to me that the damage from accidentally making an anonymous hidden service non-anonymous would be much much greater than the danger of accidentally anonymizing something that you didn't mean to anonymize.

So I think the design you're asking for is:

  • mark all anonymous services as anonymous,
  • mark all non-anonymous services as non-anonymous,
  • refuse to start if the current config is inconsistent with any of the services' previous usage,
  • and when there's no record of what the hidden service key has been used for:
    • assume it's been used for an anonymous service,
    • if we're in non-anonymous mode:
      • refuse to start, and
      • advise the user to use a newly created directory with a new key, or
      • provide a manual action ("create a file") that convinces tor that the key can be used for non-anonymous services.

I guess I'll work on this tomorrow. I've pushed the remainder of the changes to feature-17178-v7.

  1. I anticipate that if all the options for making RSOS start with "OnionService" and all of the options for making anonymous onions services start with "HiddenService", then people will call RSOS "onion services" and continue to call anonymous onion services "hidden services." That's probably not what we had in mind.

I believe dgoulet and asn plan to alias all existing HiddenService* options to OnionService*, and then change all the documentation. But they're waiting until prop224 merges.

comment:80 Changed 15 months ago by teor

Actual Points: 11.513

I redesigned single onion service key poisoning for key files in the following commits:

[feature-17178-v7 269b829] fixup! Allow the unit tests to pass a service list to rend_service_load_all_keys

[feature-17178-v7 919c54c] Refactor the hidden service code to use rend_service_path

[feature-17178-v7 6293a08] squash! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Implement Prop #260: Single Onion Services

Here's the description from that last commit:

When in OnionServiceSingleHopMode, each hidden service key is poisoned
(marked as non-anonymous) on creation by creating a poison file in the
hidden service directory.


Existing keys are considered non-anonymous if this file exists, and
anonymous if it does not.


Tor refuses to launch in OnionServiceSingleHopMode if any existing keys
are anonymous. Similarly, it refuses to launch in anonymous client mode
if any existing keys are non-anonymous.


Rewrite the unit tests to match and be more comprehensive.
Adds a bonus unit test for rend_service_load_all_keys().

Next step is to add the ephemeral ADD_ONION case:

  • Flags must contain OnionServiceSingleHopMode when that option is set, and must not if it is not set.
  • This will need a control spec patch as well, see #20094.

comment:81 Changed 15 months ago by teor

Actual Points: 1313.5
Status: needs_revisionneeds_review

The ADD_ONION flag is "NonAnonymous".

[feature-17178-v7 7406391] Ephemeral Single Onion Services must have the NonAnonymous ADD_ONION flag

There are no ADD_ONION unit tests, so I tested it using:

stem/tor-prompt --tor src/or/tor
ADD_ONION NEW:BEST Flags=DiscardPK Port=22
ADD_ONION NEW:BEST Flags=DiscardPK,NonAnonymous Port=22
tor/src/or/tor DataDirectory /tmp/tor.$$ ControlPort 2000 OnionServiceSingleHopMode 1 OnionServiceNonAnonymousMode 1 SOCKSPort 0
stem/tor-prompt -i 2000
ADD_ONION NEW:BEST Flags=DiscardPK Port=22
ADD_ONION NEW:BEST Flags=DiscardPK,NonAnonymous Port=22

And the responses are as specified in #20094.

All the existing tests passed after these changes, including:

  • unit tests,
  • chutney make test-network-all,
  • chutney single-onion, single-onion-indirect, single-onion-ipv6, and single-onion-client-ipv6 from #17622 / #20072,
  • chutney client-ipv6-only, hs-ipv6 and hs-client-ipv6 from #17812 / #20069.

I think we're done with changes here, the only one I declined was to change the option names to something containing "Hidden", mainly because it's hard to say "NonHiddenService" in a way that's understandable. I believe the correct fix for this is to alias all the other option names from HiddenService to OnionService, which will happen with prop224.

The existing GitLab merge request has had 29 commits added, typically one per review comment.
I have responded to each comment with the commit hash.
The fixup! chain gets quite ridiculous, sorry about that.
https://gitlab.com/teor/tor/merge_requests/8

I am happy to fixup/squash these before merge, once you're happy with the branch. I was careful to try to make sure that they all squashed cleanly, but there's always one...

comment:82 Changed 15 months ago by teor

A reminder that if we merge Single Onion Services (#17178) and Netflow Padding (#16861) in 0.2.9, we'll likely want a Consensus Parameter to Disable Netflow Padding For Single Onion Services (#17857) as well.

comment:83 Changed 15 months ago by nickm

Reviewer: dgouletnickm

this is back on my plate now

comment:84 Changed 15 months ago by teor

I have thought of some minor changes that could clarify our dual-option design, but I don't think they're that necessary (and I have patch fatigue!):

  • Refactor options_validate_single_onion() to use rend_service_non_anonymous_mode_enabled() and rend_service_allow_non_anonymous_connection(), rather than the raw options
  • Whenever we use one of rend_service_non_anonymous_mode_enabled() or rend_service_allow_non_anonymous_connection(), assert it's equal to the other
    • except in options_validate_single_onion(), where they're allowed to be different
  • Use rend_service_allow_non_anonymous_connection() for the places where we are specifically deciding to make a one-hop connection, and rend_service_non_anonymous_mode_enabled() otherwise (for example, when poisoning or checking ADD_ONION flags)

Do you think these would be useful, nickm?

comment:85 Changed 15 months ago by nickm

Thanks for the revisions! I've been over the gitlab page and made comments. There are only a couple of pending issues and questions.

I believe the correct fix for this is to alias all the other option names from HiddenService to OnionService, which will happen with prop224.

I'll bring this up for discussion more at the network team meeting (starting now-ish). I still need to consider your comment:84.

comment:86 Changed 15 months ago by nickm

On reflection then, I do think that the things in comment:84 would be useful.

comment:87 Changed 15 months ago by nickm

Status: needs_reviewneeds_revision

comment:88 Changed 15 months ago by dgoulet

My two cents. I know I proposed the torrc OnionService* prefix to teor so sorry about that. After discussion with nickm, we should have either only HiddenService* or OnionService* but not both. Proposal 224 should be the one that makes the switch (#17343) but this will happen at the very best 030 and more realistically at 031.

So for now, my choice would be:

HiddenServiceSingleHopMode 0|1
HiddenServiceNonAnonymousMode 0|1

Important thing imo is to keep the namespacing of the "HIDDEN SERVICE OPTIONS" section. RSOS is only about hidden service so we shouldn't try to confuse the user that it's something else. And having the NonAnonymousMode option is great way to have a strong semantic of what the user is about to do.

comment:89 Changed 14 months ago by teor

Actual Points: 13.514
Keywords: TorCoreTeam201608 removed
Status: needs_revisionneeds_review

I have made the changes nickm requested in his gitlab review, and added comments on gitlab with those commits.

The changes in my comment:84 are in:
[feature-17178-v7 16c1a3a] Refactor Single Onion code to improve consistency

The changes in dgoulet's comment:88 are in:
[feature-17178-v7 f4d938c] Replace OnionService* with HiddenService* in option names

So I think this is it. lgtm?
I am happy to do a not-so-auto-squash once everyone is happy.

comment:90 Changed 14 months ago by nickm

Status: needs_reviewmerge_ready

LGTM. I actually prefer to do the squashing myself when I can, since that lets me make sure I understand the code, and it makes it a little easier to make sure that What I Reviewed Is What I Merged.

I made a squashed feature17178-v7-squashed-v2, and used git diff feature-17178-v7 feature-17178-v7-squashed-v2 to make sure that it was exactly the same as your branch.

comment:91 Changed 14 months ago by nickm

Merged cleanly. Compiles okay (once I added 223747804568635a9b to bring the testing API usage up to date). Tests passed. Tests with expensive hardening found a memory leak in the tests (fixed as 831649f56eed00728b5). Pushed to master.

comment:92 Changed 14 months ago by nickm

(Woohoo!)

comment:93 Changed 14 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.