Opened 10 years ago

Closed 8 years ago

Last modified 4 years ago

#1090 closed defect (fixed)

Warning about using an excluded node for exit

Reported by: Sebastian Owned by: nickm
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: 0.2.1.19
Severity: Keywords:
Cc: Sebastian, karsten, nickm, arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

Quite a few people have reported warnings in their logs when using
Exclude*Nodes in their torrc. We should track down why this happens,
and fix.

I opened the bug so we can keep track of ideas/problem reports. A
typical log line would be
[Warning] Requested exit node '..' is in ExcludeNodes or ExcludeExitNodes.. Using anyway.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (79)

comment:1 Changed 10 years ago by Sebastian

[rotor] my fix changes any circs with begin_dir to internal.
[rotor] problem is circs, if those alreadry exist nothing stop from
using it, onehop is safe but circs for dir req of rend desc not safe.

comment:2 Changed 10 years ago by Sebastian

Patch by rotor to be found at git://git.torproject.org/~sebastian/git/tor branch bug1090

also
[rotor] http://pastebin.com/d51430358 related to 1090 with fixed and hided warn only.
(note minor patch corruption due to pastebin madness)

[rotator] actualy fix do not fixes anything for 1090, but it prevent a real use excluded nodes if it can happen.

comment:3 Changed 10 years ago by Sebastian

Next patch pushed.

--- circuitbuild.c	Fri Jun 19 06:13:52 2009
+++ circuitbuild.c.fix	Fri Sep 11 10:18:32 2009
@@ -1436,13 +1436,15 @@
 /** Log a warning if the user specified an exit for the circuit that
  * has been excluded from use by ExcludeNodes or ExcludeExitNodes. */
 static void
-warn_if_last_router_excluded(uint8_t purpose, const extend_info_t *exit)
+warn_if_last_router_excluded(origin_circuit_t *circ, const extend_info_t *exit)
 {
   or_options_t *options = get_options();
   routerset_t *rs = options->ExcludeNodes;
   const char *description;
   int severity;
   int domain = LD_CIRC;
+  uint8_t purpose = circ->_base.purpose;
+  int is_internal = circ->build_state->is_internal;
 
   switch (purpose)
     {
@@ -1457,7 +1459,10 @@
     case CIRCUIT_PURPOSE_C_GENERAL:
       description = "Requested exit node";
       rs = options->_ExcludeExitNodesUnion;
-      severity = LOG_WARN;
+      if (is_internal)
+        severity = LOG_INFO;
+      else 
+        severity = LOG_WARN;
       break;
     case CIRCUIT_PURPOSE_C_INTRODUCING:
     case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT:
@@ -1521,7 +1526,7 @@
   }
 
   if (exit) { /* the circuit-builder pre-requested one */
-    warn_if_last_router_excluded(circ->_base.purpose, exit);
+    warn_if_last_router_excluded(circ, exit);
     log_info(LD_CIRC,"Using requested exit node '%s'", exit->nickname);
     exit = extend_info_dup(exit);
   } else { /* we have to decide one */
@@ -1568,6 +1573,7 @@
 circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit)
 {
   int err_reason = 0;
+  warn_if_last_router_excluded(circ, exit);
   circuit_append_new_exit(circ, exit);
   circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_BUILDING);
   if ((err_reason = circuit_send_next_onion_skin(circ))<0) {
Last edited 4 years ago by arma (previous) (diff)

comment:4 Changed 10 years ago by Sebastian

Some more information which I can't parse right now:

[rotor] circuit_find_to_cannibalize() returns one-hop circ.
[rotor] actualy most time it is dirty
[rotor] but it connection_ap_attach_pending() depends and contain of conn list
[rotor] actualy for now it's safe for general circ, and rend too. because onehop non internal for while.
[Sebastian] if the internal requirement doesn't make it safe
[rotor] but after 1090 if it becames internal so rend is not safe
[rotor] client's intro circ I think
[rotor] checking for onehop_tunnel just generalize the func ever.
[Sebastian] I feel that this is over my head for now
[Sebastian] Maybe at some point nick or roger have enough time to look at it
[Sebastian] Do you have a better suggestion?
[rotor] if onehop circ planed as resource for rebuilds so need fix 1090 another way.

comment:5 Changed 10 years ago by harris

This bug is beginning to do the rounds in various privacy forums and will be in open water soon enough. From what I

can gather there are instances where this flaw is serious. It's reported that it's particularly prevalent where
users have tried to exclude known bad exits run by authorities, eg jalopy. It's being mooted that as this "bug" has

been around for a while, this is an intentional back door left open in tor by the developers!!!

My own experiences are that there is no way to effectively exclude a group of nodes by country and one is resigned

to specifying entry and exit nodes as in the old days...

This bug should be elevated to high priority and medium severity as it has the potential to serious undermine tor's
user base.

If a patch exists it needs to be provided in a manner that can allow any non technical user to easily implement it.

The notes above and url referrals are not at all usable in this respect.

Finally thanks to those tryin to address this.

comment:6 Changed 10 years ago by nickm

17:55 <@arma> ok. there are two general classes of bugs here, around 1090
17:55 <@arma> the first is that we need to make sure when we're selecting an

exit node for a non-internal non-onehop circuit, we don't pick
one in excludeexitnodes

17:55 <@arma> i think we do ok at that, but i'm not sure. i would have to

check. there are probably some edge cases, like if we've built a
circuit and then they setconf excludeexitnodes, where we fail.

17:56 <@arma> the second class of bug is that we have this

warn-if-the-last-hop-looks-like-it's-in-excludedexitnodes
function, which warns too eagerly and makes users upset.

17:56 <@arma> we're already kind of in bad shape here, because vidalia shows

circuits and doesn't distinguish internal from non-internal, so
even if we fix this second category of bugs, we'll still have
upset users who have no idea that internal circuits exist.

[...]
18:26 <@arma> also, warn_if_last_router_excluded() should stop saying anything

about the log-info ones. they are not bugs. not even at log-info.

18:27 <@arma> i think we should fix warn_if_last_router_excluded so 1) it

doesn't log if not an exit, 2) it doesn't log if internal or
one-hop. 3) it describes the circuit better when it does log.

18:27 <@arma> then we should fix the circuit-is-acceptable logic to check

excludeexitnodes and excludenodes and skip the circuit if it's
bad.

18:29 <@arma> if (exitrouter && !connection_ap_can_use_exit(conn,

exitrouter)) {

18:29 <@arma> /* can't exit from this router */
18:31 <@arma> we should amend connection_ap_can_use_exit() to say "no thanks"

if the user has set excludestuff

comment:7 Changed 10 years ago by Sebastian

First part of a fix (don't log when we aren't using that exit to connect
to outside world) pushed as
113ba0e7270147b6eed10668970e1719139c4f27

comment:8 Changed 10 years ago by Sebastian

Next part pushed, see http://gitweb.torproject.org/?p=tor/sebastian.git;a=commitdiff;h=dc3229313b6d2aaff437c6fc7fa55ead4409e93d

This means that even if someone says "bla.com.node.exit", we won't use that exit and
refuse the connection.

comment:9 Changed 10 years ago by OTU

This means that someone nothing says ever takes bunches of warnings:
"Requested exit point .. would refuse request"
just because used Excludefakes and such directory was selected.
Also suggest for every user's complain about non-accessible HS
ask about Exclude* options.

comment:10 Changed 10 years ago by nickm

IMO we should check circuits for suitability again when we are about to build streams through them.

This way we can:

a) Be really really sure that we really have not messed up on any circuits that are used for user streams,

or detect it for sure if we have.

b) Be able to explain why we're using an excluded node (say for a hidden service).

comment:11 Changed 10 years ago by OTU

And again: can't general circ be internal if last hop predefined.
After reuse prebuilt general circ into some other circuit for rendezvous point
or intro point that purpose changes from CIRCUIT_PURPOSE_C_GENERAL to other.
Do that more generalizing? So why only for broken kludges?

Whole logic of circuit_find_to_cannibalize() and circ's properties seems broken.
No need play with kludges and another smartest math stat,
while basis is *weak and buggy*.

comment:12 Changed 10 years ago by nickm

Folks should have a look at arma's strictnodes branch (in git://git.torproject.org/~arma/git/tor .) He's basically
rewriting the way {Exclude}{Entry|Exit}Nodes works, and making a StrictNodes option that is willing to stop Tor from
working when doing so would use nodes the user said not to.

comment:13 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:14 Changed 9 years ago by nickm

Description: modified (diff)

arma, what's the status of your strictnodes branch? Is it mergeable? Should it get reviewed? Is it already merged?

What your the status here?

comment:15 Changed 9 years ago by arma

Most of my strictnodes branch is merged, but not all of it. I should merge what remains.

Before that, I should look at proposal 170 and see why I thought it disagreed with my original plan, which is:

i think the trajectory i was heading toward was to make tor do the smart thing, which i think is smart and some users will no doubt be shocked at,
unless you set strictnodes, in which case tor will do the stupid thing, and will break in ways that some users will be shocked at

comment:16 Changed 9 years ago by arma

Triage: this shouldn't block 0.2.2.x-rc, but it should block 0.2.2 stable.

comment:17 Changed 9 years ago by arma

Priority: minormajor

comment:18 Changed 9 years ago by nickm

Owner: set to arma
Status: newassigned

comment:19 Changed 9 years ago by nickm

Component: Tor ClientTor Check

It's been 3 months here. If we're going to get this done for 0.2.2.x, it needs to get done soon. I am making a note to myself here to re-read all the documentation about this issue and try to force some progress. Steps for me, unless arma gets to it first, are:

  • Read relevant threads here and on or-dev and or-talk
  • Look through relevant IRC stuff, if I can find it
  • Try to write up a short statement what the design for a fix definitely is, in spec form. Get roger to vet it. Look for consensus.

Steps for arma, unless he can't do it, in which case I will, are:

  • Identify areas that still need fixing.

Steps after that are:

  • Triage those areas. In a very short timeframe, we should either document them as a known issue in 0.2.2, or fix them in 0.2.2. Once we know what they are, I can give it a try.

comment:20 Changed 9 years ago by nickm

Component: Tor CheckTor Client

comment:21 Changed 9 years ago by arma

I talked to nickm about this at the dev mtg. My basic suggested design is to have EntryNodes be a list of nodes you must use in your first hop of circuits (except one-hop circuits). If none of your entrynodes are available, you build no circuits. Exitnodes is a list of nodes you must use in your last hop of non-internal (as defined in path-spec) circuits. Similarly, if no exitnodes work, no circuits for you.

ExcludeExitNodes is a list of nodes to never use as the last hop of a non-internal circuit. Nodes in both exitnodes and excludeexitnodes are excluded.

ExcludeNodes is the tricky one. There are a variety of cases where a) your Tor functionality will break if you don't use it, and b) we feel it really isn't that bad to use it. Generally these come with c) the user will have no clue that her ExcludeNodes choice is the reason why Tor sucks for them. Examples include:
1) the introduction point when connecting to a hidden service
2) the rendezvous point when the hidden service answers
3) connecting to the appropriate node to store/fetch a hidden service descriptor

If the user sets StrictNodes, we'll avoid the nodes in ExcludeNodes even when it mysteriously breaks Tor's functionality.

Borderline cases that need more thought include:
1) An exit relay that exit enclaves to the IP address the user just asked for. I guess it can't hurt to exclude it (though imo it also can't hurt to use it).
2) the directory mirror we randomly chose for fetching directory info. Probably ExcludeNodes should exclude it but ExcludeExitNodes shouldn't.

I bet we'll find more edge cases to ponder as we implement. Nickm suggested a code redesign where every time we ask for a router, we specify why we're asking for it (or at least, what constraints it should obey). Then we simply don't get back routers that don't obey the constraints we specify. That approach also makes the code more explicit for each case about what result we should expect.

People should also look at my commit messages in arma/strictnodes to get a sense of the various edge cases for EntryNodes and Exitnodes and how I decided to handle them. (I believe they are all handled appropriately now, but what do I know.)

See also #2114 and #2411 to reduce the number of Vidalia users who are confused about why ExcludeExitNodes or ExcludeNodes doesn't do what they expect.

comment:22 Changed 9 years ago by nickm

Owner: changed from arma to nickm

ExcludeExitNodes is a list of nodes to never use as the last hop of a non-internal circuit. Nodes in both exitnodes and excludeexitnodes are excluded.

This is a tricksy bit: we need to define what we mean by an "internal" circuit. ISTR based on the conversation with Roger that he did not think that ExcludeExitNodes should apply to the following:

  • Directory-only circuits
  • Testing and build-time measurement circuits
  • Circuits related to hidden services (introduction points, rendezvous points)


In other words, "ExcludeExitNodes" applies only to circuits where we attach AP streams. It means, "I don't want these servers able to see my plaintext." It explicitly *does not* mean, "If my entry is observed and a correlation attack mounted against me, I don't trust these servers not to participate in it."

Another nonintuitive point in the above is that ExcludeExitNodes does not have its meaning change when we do StrictNodes 1.

Another nonintuitivie part is that EntryNodes never changes its meaning when we do StrictNodes 1.

Fortunately, one nice piece of my "use a bunch of functions" design for dealing with this is that it is relatively easy to change this stuff in the future if we decide we've got it a bit wrong.

comment:23 Changed 9 years ago by nickm

I started writing up this list of candidate flags:

/** Usage flag: a node to relay user traffic. */
#define NODE_USAGE_RELAY (1<<0)
/** Usage flag: a note to use as the first hop for a multi-hop circuit. */
#define NODE_USAGE_ENTRY (1<<1)
/** Usage flag: a note to use as the only hop for a one-hop circuit. */
#define NODE_USAGE_ONEHOP (1<<2)
/** Usage flag: a node to use for attaching application streams. */
#define NODE_USAGE_GEN_EXIT  (1<<3)
/** Usage flag: a node used as a client to introduce to a hidden service */
#define NODE_USAGE_C_INTRO (1<<4)
/** Usage flag: a node used aa a rendezvous point for a hidden service we're
 * connecting to. */
#define NODE_USAGE_C_RENDPT (1<<5)
/** Usage flag: a node used as an intro point for our own hidden service */
#define NODE_USAGE_S_INTRO (1<<6)
/** Usage flag: a rend point requested by a client for our hidden service */
#define NODE_USAGE_S_RENDPT (1<<7)
/** Usage flag: a node used for a non-anonymous directory fetch. */
#define NODE_USAGE_ONEHOP_DIRFETCH (1<<8)
/** Usage flag: a node used as a directory for an anonymous directory
 * fetch. */
#define NODE_USAGE_ANON_DIRFETCH (1<<9)
/** Usage flag: a node used for a non-anonymous directory upload. */
#define NODE_USAGE_ONEHOP_DIRUPLOAD (1<<10)
/** Usage flag: a node used for an anonymous directory upload. */
#define NODE_USAGE_ANON_DIRUPLOAD (1<<11)
/** Usage flag: a node used for testing ourself */
#define NODE_USAGE_SELFTEST (1<<12)
/** Usage flag: a node used for testing itself, or testing other nodes */
#define NODE_USAGE_TEST (1<<13)

but then I realized that what I really want here is a two-tuple of {circuit purpose, node position}. But then I realized that circuit purposes don't completely fit my aim here, since they include too much intro about rendezvous state (a typical intro circuit changes purpose a few times), and they don't include ideas like "for directory use." :p

comment:24 Changed 9 years ago by nickm

And FWICT, the functions to extend/replace/audit include:

  • everything that returns a routerinfo_t*
  • everything that returns a routerstatus_t*
  • everything that returns an extend_info_t*
  • everything that adds a node to a circuit
  • everything that attaches a stream to a circuit
  • everything that returns a circuit

It might be smarter to do a stopgap fix ("try to find and fix all the broken parts") for 0.2.2 and get the well-designed fix into 0.2.3.1-alpha: we *already* replaced most of these in the node_t migration in 0.2.3.0-alpha-dev.

comment:25 in reply to:  22 Changed 9 years ago by arma

Replying to nickm:

ExcludeExitNodes is a list of nodes to never use as the last hop of a non-internal circuit. Nodes in both exitnodes and excludeexitnodes are excluded.

This is a tricksy bit: we need to define what we mean by an "internal" circuit.

I was intending to use the definition from path-spec.txt:
"An "internal" circuit, on the other hand, is one where the final node is chosen just like a middle node (ignoring its exit policy)."

We should probably clarify that to simply "is chosen without regard to its exit policy", but I believe the term "internal" is used consistently in path-spec.txt to mean what I mean.

ISTR based on the conversation with Roger that he did not think that ExcludeExitNodes should apply to the following:

  • Directory-only circuits
  • Testing and build-time measurement circuits
  • Circuits related to hidden services (introduction points, rendezvous points)

Yep. In all of those cases, the final hop of the circuit is chosen without regard for that node's exit policy.

In other words, "ExcludeExitNodes" applies only to circuits where we attach AP streams.

It's conceivable that this is an equivalent definition. At first glance I think it is.

It means, "I don't want these servers able to see my plaintext." It explicitly *does not* mean, "If my entry is observed and a correlation attack mounted against me, I don't trust these servers not to participate in it."

Correct. ExcludeExitNodes is about *exiting*, not about what gets put on one end of a circuit or the other.

Another nonintuitive point in the above is that ExcludeExitNodes does not have its meaning change when we do StrictNodes 1.

Another nonintuitivie part is that EntryNodes never changes its meaning when we do StrictNodes 1.

Yep. We could rename it to StrictExcludeNodes or something like that if we wanted to do a shell game with the names yet again.

Fortunately, one nice piece of my "use a bunch of functions" design for dealing with this is that it is relatively easy to change this stuff in the future if we decide we've got it a bit wrong.

Sounds good.

comment:26 in reply to:  23 ; Changed 9 years ago by arma

Replying to nickm:

but then I realized that what I really want here is a two-tuple of {circuit purpose, node position}. But then I realized that circuit purposes don't completely fit my aim here, since they include too much intro about rendezvous state (a typical intro circuit changes purpose a few times), and they don't include ideas like "for directory use." :p

While we're thinking about code reorg like this, an idea I've had floating in my mind the past few years is to add a 'role' value used in onion_extend_cpath() so it isn't so hard-coded in what relay roles go where in the path. So then we could specify that a typical 3-hop circuit is made up of NODE_USAGE_ENTRY, NODE_USAGE_MIDDLE, NODE_USAGE_EXIT, and then the "populate the cpath" logic would just loop over the circuit choosing a suitable relay for each hop based on that hop's intended role.

Notice how I have a NODE_USAGE_MIDDLE here -- I'm not sure if that's the same as your NODE_USAGE_RELAY. One way they could be different is because when we're choosing a middle hop, we want to weight our load balancing away from entry and exit hops proportional to the weights in the consensus. Did you envision using these NODE_USAGE values pervasively enough in the code that we also use them to tell routerlist_sl_choose_by_bandwidth(), router_choose_random_node(), etc the role of the node in the circuit? My guess is that you do?

Generalizing this way means that when somebody down the road uses the Tor codebase to build their overlay network that uses two middle hops and an exit node, they don't have to hunt through as much of the code to get all the weightings and exclusion fixes right.

Actually storing the role in the struct that remembers the cpath info could come in handy later too, where when we cannibalize circuits we have rules for which role-to-role transitions are acceptable and which aren't.

Is this crazy-talk that's way more invasive than what you were imagining, or just fleshing out the same ideas?

comment:27 in reply to:  24 Changed 9 years ago by arma

Replying to nickm:

It might be smarter to do a stopgap fix ("try to find and fix all the broken parts") for 0.2.2 and get the well-designed fix into 0.2.3.1-alpha: we *already* replaced most of these in the node_t migration in 0.2.3.0-alpha-dev.

That's fine by me -- especially because the node_t changes in 0.2.3.x will mean significant forward-porting effort from 0.2.2.x that is mostly a waste. But we should be aware that we will inevitably miss a few, people will get angry at us for the conspiracy, etc etc. So be it.

comment:28 in reply to:  26 Changed 9 years ago by nickm

Replying to arma:

Notice how I have a NODE_USAGE_MIDDLE here -- I'm not sure if that's the same as your NODE_USAGE_RELAY. One way they could be different is because when we're choosing a middle hop, we want to weight our load balancing away from entry and exit hops proportional to the weights in the consensus. Did you envision using these NODE_USAGE values pervasively enough in the code that we also use them to tell routerlist_sl_choose_by_bandwidth(), router_choose_random_node(), etc the role of the node in the circuit? My guess is that you do?

Well, my "NODE_USAGE_" idea was a bit orthogonal to this: it tried to subsume position and circuit type into one thing; NODE_USAGE_RELAY would have been the usage for the middle node of every type of multihop circuit.

I think that having "position within circuit" and "type of circuit" as separate things is a good idea.

Is this crazy-talk that's way more invasive than what you were imagining, or just fleshing out the same ideas?

It's a logical next-step here, I think.

Replying to arma:

That's fine by me -- especially because the node_t changes in 0.2.3.x will mean significant forward-porting effort from 0.2.2.x that is mostly a waste.

Agreed. Now I'm just trying to tell whether it makes more sense to code the 0.2.3.x thing first on the theory that doing so will point out all the cases in 0.2.2.x where we need to change the code, or to do the 0.2.2.x thing first on the theory that some of the code will surely be forward-portable. Decisions, decisions!

comment:29 Changed 9 years ago by nickm

Dropping a note here so I remember: there's a mismatch between circuit purpose and the flags I described above. Here's my mapping from origin circuit purpose to the node purpose flags above. For each origin circuit purpose, I give a 3-tuple of required flags for first, middle, and last node:

C_MEASURE_TIMEOUT
  -> TEST,TEST,TEST ?

S_ESTABLISH_INTRO
S_INTRO
   -> ENTRY|RELAY?, RELAY?, RELAY?|S_INTRO

S_CONNECT_REND
S_REND_JOINED
   -> ENTRY|RELAY, RELAY, RELAY|S_REND

TESTING
   -> SELFTEST,SELFTEST,SELFTEST ?

CONTROLLER
   -> ANY, ANY, ANY

The unused flags are the onehop node purpose flags and directory-oriented flags: we don't currently distinguish those by circuit purpose. Maybe we need to start.

comment:30 Changed 9 years ago by nickm

Oh, I omitted a few above:

C_GENERAL
   ENTRY|RELAY, RELAY, GEN_EXIT|RELAY

C_INTRODUCING
C_INTRODUCE_ACK_WAIT
C_INTRODUCE_ACKED
   ENTRY|RELAY?, RELAY?, RELAY?|C_INTRO

C_ESTABLISH_REND
C_REND_READY
C_REND_READY_INTRO_ACKED
C_REND_JOINED
   ENTRY|RELAY, RELAY, RELAY|C_REND

comment:31 Changed 9 years ago by nickm

I have tried to write up a description for the revised, revised, revised behavior as a manpage patch. I've put it in my public repository in a branch called "desired_node_behavior".

Again, this is what the behavior *should* IMO be.

Here's the relevant part:

**ExcludeNodes** __node__,__node__,__...__::
    A list of identity fingerprints, nicknames, country codes and address
    patterns of nodes to avoid when building a circuit.
    (Example:
    ExcludeNodes SlowServer, $    EFFFFFFFFFFFFFFF, \{cc}, 255.254.0.0/8) +
+
    By default, this options is treated as a preference that Tor is allowed
    to override in order to keep working.
    For example, if you try to connect to a hidden service,
    but you have excluded all of the hidden service's introduction points,
    Tor will connect to one of them anyway.  If you do not want this
    behavior, set the StrictNodes option (documented below).  +
+
    Note also that if you are a relay, this (and the other node selection
    options below) only affects your own circuits that Tor builds for you.
    Clients can still build circuits through you to any node.  Controllers
    can tell Tor to build circuits through any node.

**ExcludeExitNodes** __node__,__node__,__...__::
    A list of identity fingerprints, nicknames, country codes and address
    patterns of nodes to never use when picking an exit node---that is, a
    node that delivers traffic for you outside the Tor network.   Note that any
    node listed in ExcludeNodes is automatically considered to be part of this
    list too.

**ExitNodes** __node__,__node__,__...__::
    A list of identity fingerprints, nicknames, country codes and address
    patterns of nodes to use as exit node---that is, a
    node that delivers traffic for you outside the Tor network. +
+
    Note that if you list too few nodes here, or if you exclude too many exit
    nodes with ExcludeExitNodes, you can degrade functionality.  For example,
    if none of the exits you list allows traffic on port 80 or 443, you won't
    be able to browse the web. +
+
    Note also that not every circuit is used to deliver traffic outside of
    the Tor network.  It is normal to see non-exit circuits (such as those
    used to connect to hidden services, those that do directory fetches,
    those used for self-tests, and so on) that end at a non-exit node.  To
    keep a node from being used entirely, see ExcludeNodes and StrictNodes.

**EntryNodes** __node__,__node__,__...__::
    A list of identity fingerprints, nicknames and address patterns of nodes
    to use for the first hop in your normal circuits.  This includes all
    circuits except for direct connections to directory servers.  The Bridge
    option overrides this option; if you have configured bridges and
    UseBridges is 1, the Bridges are used as your entry nodes.

**StrictNodes** **0**|**1**::
    If StrictNodes is set to 1, Tor will treat the ExcludeNodes option as a
    requirement to follow for all the circuits you generate, even if doing so
    will break functionality for you.  If StrictNodes is set to 0, Tor will
    still try to avoid nodes in the ExcludeNodes list, but it will err on the
    side of avoiding unexpected errors.  Specifically, StrictNodes 0 tells
    Tor that it is okay to use an excluded node when necessary to connect to
    a hidden service, provide a hidden service to a client, fulfil a .exit
    request, upload directory information, or download directory information.
    (Default: 0)

comment:32 Changed 9 years ago by Sebastian

That manpage suggestion looks good to me. I think we also want to have a pointer to how the options change exit enclaving (if they do).

comment:33 in reply to:  31 Changed 9 years ago by rransom

Replying to nickm:

By default, this options is treated as a preference that Tor is allowed

s/options/option/ # (on line 496 of the revised man page source file)

comment:34 in reply to:  14 ; Changed 9 years ago by arma

Replying to nickm:

arma, what's the status of your strictnodes branch? Is it mergeable? Should it get reviewed? Is it already merged?

It is now branch bug1090-part1 in my arma.

It is mergeable (on maint-0.2.2).

It should get reviewed, primarily to see if it agrees with our nascent consensus on what we want Tor to do. I think it does -- but I would, wouldn't I. :)

It is also not a complete set of all things that need to change for bug1090.

comment:35 in reply to:  34 ; Changed 9 years ago by Sebastian

It is also not a complete set of all things that need to change for bug1090.

Where is that complete list?

comment:36 in reply to:  34 Changed 9 years ago by arma

Replying to arma:

It is also not a complete set of all things that need to change for bug1090.

I added a bunch more patches to bug1090-part1, including some XXX022's on what else needs doing.

I no longer know of any cases that aren't handled or XXX'ed. (But I should probably sleep a few times before saying that more confidently.)

comment:37 in reply to:  35 Changed 9 years ago by arma

Replying to Sebastian:

It is also not a complete set of all things that need to change for bug1090.

Where is that complete list?

There isn't one, I believe.

comment:38 Changed 9 years ago by arma

Status: assignedneeds_review

comment:39 Changed 9 years ago by nickm

Okay, let's start reading though this. Only 800 lines; how bad could it be?

general observation 1: we should make it so that routerset_contains_foo(set, foo) returns false if set is NULL. That would simplify our code a fair bit. [oh wait, it does! We should just simplify all the "if (excludeFoo && routerstet_contains(excludeFoo, foo)" lines to "if (routerset_contains(excludeFoo, foo))".

general observation 2: setting StrictNodes will get you a whole lot of warnings. That's probably fair.

re 470005bca: "refuse excluded hidserv nodes if strictnodes": I think that the approach of removing hidden service introduction points from the service descriptor is wrong: If the user changes their ExcludeNodes or StrictNodes settings, their hidden service won't start working.

re d924435c: changing the interface to routerset_get_all is needless; we already have routerset_subtract and routerset_get_disjunction.

Also, this exposes a hole in my documentation: it didn't say what should happen when every member of EntryNodes or ExitNodes is excluded and StrictNodes is 0. I think that warning the user and giving up is a fine thing to do in this case.

re 65dae3aa: the warning messages should probably say what directory activity we were thinking of doing when we ran into the excluded node.

The behavior of directory_initiate_command_routerstatus_rend is kinda broken and perhaps we should treat it as a bug. In fact, we should backtrack to figure out why we might pick a router for a given directory operation if that router would be excluded.

The router_pick_directory_server_impl and router_pick_trusteddirserver_impl functions do the wrong thing if every potential directory is excluded and StrictNodes is 0.

re e9f91b2f5291cd9c: I say that we just "#if 0" circuit_conforms_to_options for 0.2.2. In 0.2.3, it should live, and we should enlarge the set of circuit purposes to the point where it's actually able to work right.

re c16378a83cc1051: looks good.

re bac8bdb400eff: seems okay

Aside: We don't support IPs and countries in the EntryNodes list. We should fix that in 0.2.3.x. In 0.2.2, we should fix my manpage writeup to say so.

re 5535f0791d8d: This seems plausible, but it deviates from my writeup: disallowing general exits from ExcludeExitNodesUnion means that ExcludeNodes and ExcludeExitNodes are given the same force here regardless of the value of StrictNodes.

re 81c069f21a4039: same as above.

re commits that only add an XXX022: we currently have 34 xxx022s in maint-0.2.2. This adds 8. I suggest that we tag them with XXX022-strictnodes so we can grep for those in particular.

comment:40 in reply to:  39 Changed 9 years ago by Sebastian

Replying to nickm:

re 470005bca: "refuse excluded hidserv nodes if strictnodes": I think that the approach of removing hidden service introduction points from the service descriptor is wrong: If the user changes their ExcludeNodes or StrictNodes settings, their hidden service won't start working.

re d924435c: changing the interface to routerset_get_all is needless; we already have routerset_subtract and routerset_get_disjunction.

Also, this exposes a hole in my documentation: it didn't say what should happen when every member of EntryNodes or ExitNodes is excluded and StrictNodes is 0. I think that warning the user and giving up is a fine thing to do in this case.

I agree wrt what we should do if every node is excluded. Should we tell the user that some of their entrynodes are covered by excludenodes? This could be helpful if they wonder why their entrynodes isn't obeyed, but it could be quite spammy too once we allow countrys/IP ranges in entrynodes.

re bac8bdb400eff: seems okay

I think the "be flexible about families" is going to get us nasty looks. I think we should always fail or we should honor StrictNodes here.

re commits that only add an XXX022: we currently have 34 xxx022s in maint-0.2.2. This adds 8. I suggest that we tag them with XXX022-strictnodes so we can grep for those in particular.

About using ourselves for a reachability test: Yes, let's make it fail. If someone complains that they can't be a relay because they set excludeexitnodes, we can easily tell them to run a second Tor instance for their client needs.

comment:41 Changed 8 years ago by nickm

I've started doing some fixups here in branch "bug1090-part1" in my public repository. So far I've only got through the introduction points issues I noted above, and changing most of the "XXX022" instances to "XXX022-1090" instances, to make them easier to grep for.

comment:42 in reply to:  39 Changed 8 years ago by nickm

Replying to nickm:

Okay, let's start reading though this. Only 800 lines; how bad could it be?

general observation 1: we should make it so that routerset_contains_foo(set, foo) returns false if set is NULL. That would simplify our code a fair bit. [oh wait, it does! We should just simplify all the "if (excludeFoo && routerstet_contains(excludeFoo, foo)" lines to "if (routerset_contains(excludeFoo, foo))".

I'm going to add this as a new ticket: #2797

general observation 2: setting StrictNodes will get you a whole lot of warnings. That's probably fair.

re 470005bca: "refuse excluded hidserv nodes if strictnodes": I think that the approach of removing hidden service introduction points from the service descriptor is wrong: If the user changes their ExcludeNodes or StrictNodes settings, their hidden service won't start working.

I've got a patch for this in my branch.

re d924435c: changing the interface to routerset_get_all is needless; we already have routerset_subtract and routerset_get_disjunction.

Also, this exposes a hole in my documentation: it didn't say what should happen when every member of EntryNodes or ExitNodes is excluded and StrictNodes is 0. I think that warning the user and giving up is a fine thing to do in this case.

I've added a patch to my manpage branch explaining that ExcludeNodes overrides EntryNodes.

re 65dae3aa: the warning messages should probably say what directory activity we were thinking of doing when we ran into the excluded node.

The behavior of directory_initiate_command_routerstatus_rend is kinda broken and perhaps we should treat it as a bug. In fact, we should backtrack to figure out why we might pick a router for a given directory operation if that router would be excluded.

The router_pick_directory_server_impl and router_pick_trusteddirserver_impl functions do the wrong thing if every potential directory is excluded and StrictNodes is 0.

re e9f91b2f5291cd9c: I say that we just "#if 0" circuit_conforms_to_options for 0.2.2. In 0.2.3, it should live, and we should enlarge the set of circuit purposes to the point where it's actually able to work right.

re c16378a83cc1051: looks good.

re bac8bdb400eff: seems okay

On review, I think Sebastian's objection is right. This isn't a new change in arma's patch, though. I've added an XXX022-strict for it.

Aside: We don't support IPs and countries in the EntryNodes list. We should fix that in 0.2.3.x. In 0.2.2, we should fix my manpage writeup to say so.

Added this as bug2798, clarified in my desired_node_behavior branch.

re 5535f0791d8d: This seems plausible, but it deviates from my writeup: disallowing general exits from ExcludeExitNodesUnion means that ExcludeNodes and ExcludeExitNodes are given the same force here regardless of the value of StrictNodes.

re 81c069f21a4039: same as above.

Changed the tor.1.txt in my desired_node_behavior branch to note this.

re commits that only add an XXX022: we currently have 34 xxx022s in maint-0.2.2. This adds 8. I suggest that we tag them with XXX022-strictnodes so we can grep for those in particular.

I've changed these to "XXX022-1090".

Thus, at this point, as of the head of my bug1090-part1 branch, so far as I know, the problems are entirely in things labelled XXX022-1090.

comment:43 Changed 8 years ago by nickm

So, there are 8 XXX022-1090 issues as of the head of my XXX1090-branch today:

  • In circuitbuild.c, choose_good_exit_server_general(): Currently, people who set "ExitNodes" do not get restrictions based on router_is_unreliable. This creates a partitioning opportunity.
  • In circuitbuild.c, warn_if_last_router_excluded(): The "using anyway" warning fires too often, confuses people, and generally freaks everybody out.
  • In circuitbuild.c, choose_random_entry(): If we have a chosen exit that is in the same family with all our bridges, we currently ignore the family rules. Instead we should probably pick a new exit if we can, or give up and try a new circuit, or something.
  • In circuitbuild.c, launch_direct_descriptor_fetch(), and probably elsewhere, it's not consistent what we do if we ExcludeNodes a node and then list it as a bridge. My intuition is to not use that bridge: it seems to be what we have said we'd do in the case of conflicts between EntryNodes and ExcludeNodes.
  • In connection_edge.c, connection_ap_handshake_rewrite_and_attach(), we need to deal with foo.exit requests when foo is excluded or excluded as an exit.
  • In connection_edge.c, connection_ap_can_use_exit(), we need to untangle some logic, figure out what the function's used for, and generally mash it into shape.
  • In router.c, in consider_testing_reachability(), we need to decide what happens to reachability tests if we have excluded ourselves and set StrictNodes. I lean to saying "Hey, you excluded yourself. What the hey?" and never learning that you are reachable. If we've excluded ourselves and *not* set StrictNodes, then I say this should work.

comment:44 in reply to:  43 Changed 8 years ago by nickm

Replying to nickm:

  • In router.c, in consider_testing_reachability(), we need to decide what happens to reachability tests if we have excluded ourselves and set StrictNodes. I lean to saying "Hey, you excluded yourself. What the hey?" and never learning that you are reachable. If we've excluded ourselves and *not* set StrictNodes, then I say this should work.

Added a patch to the head of my bug1090-part1 branch to implement this, and another to the head of desired_node_behavior to explain that self-testing is another thing that "StrictNodes 0" allows.

comment:45 in reply to:  43 Changed 8 years ago by nickm

Replying to nickm:

So, there are 8 XXX022-1090 issues as of the head of my XXX1090-branch today:

I guess I should say what I think we should do about these too.

  • In circuitbuild.c, choose_good_exit_server_general(): Currently, people who set "ExitNodes" do not get restrictions based on router_is_unreliable. This creates a partitioning opportunity.

I think the answer here is just to take out the "if you have ExitNodes set, then ignore router_is_unreliable" logic. We already have code later in the function that retries choose_good_exit_server_general with need_uptime and need_capacity set to 0 if at least one of them was set and no nodes were found.

This way, if the only exits we could consider are unreliable, we use an unreliable exit, but otherwise we use a reliable one.

I've checked in a patch for this.

  • In circuitbuild.c, warn_if_last_router_excluded(): The "using anyway" warning fires too often, confuses people, and generally freaks everybody out.

The purpose of this warning is twofold:

  • To try to detect bugs. (This bug, to be specific)
  • To let people know that if they have set StrictNodes 0, they will sometimes get Excluded nodes used.

These two kinds of issues want really different log messages.

-- gotta go for now; more on this later.

Also, I added 2 more XXX022-1090 items related to changing or reclassifying circuits.

comment:46 Changed 8 years ago by nickm

My most recent bug1090-part1 branch has even more fixes for the remaining issues. Three are left. I'd like feedback on my patches, and on the three remaining XXX022-1090 instances.

comment:47 Changed 8 years ago by nickm

I'm confused about the XXX022-1090 comment on connection_ap_can_use_exit: what is the actual bug here?

comment:48 Changed 8 years ago by Sebastian

reviewed all new commits, have a tiny grammar patch in my 1090-part1 branch.

    /* XXXX022-1090 Should we also allow foo.bar.exit if ExitNodes is set and
       Bar is not listed in it?  I say yes, but our revised manpage branch
       implies no. */

I think yes is the answer here. .exit is disabled by default, so (IMO) we don't have to worry about people doing attacks here anymore, and anyone enabling the feature asked for it. If I understand correctly, this means that we can also use mapaddress to map an otherwise excluded relay for use with one particular host, which might be valuable for some people too.

Replying to nickm:

I'm confused about the XXX022-1090 comment on connection_ap_can_use_exit: what is the actual bug here?

This was added in c85eb64cdc982. Judging from the commit msg, arma only added it because it is a place where we look at strictnodes and exits; I don't spot an issue with it either.

For the last remaining 1090 XXX, I rewrote the using anyway log message and added a new LD_BUG message that triggers if we're using a node that is excluded and StrictNodes is set. I think this should now never happen, so we should learn about it.

comment:49 Changed 8 years ago by nickm

Thanks! I've merged your changes into mine.

I'm still not happy with the LD_BUG message: I worry that it's doing the wrong thing some times. For example, I'm pretty sure that controller-chosen paths override everything else, including strictnodes. Also, we might be getting a bit too overconfident in saying that we're using the excluded node because no other nodes were available: we might be using it because of a bug. I am not sure I know what the message should say instead, though.

comment:50 Changed 8 years ago by arma

       if (options->ExcludeNodes && options->StrictNodes &&
           routerset_contains_routerstatus(options->ExcludeNodes, rs)) {
-        log_warn(LD_DIR, "Wanted to publish to '%s', but it's in our "
-                 "ExcludedNodes list and StrictNodes is set. Skipping.",
+        log_warn(LD_DIR, "Wanted to contact authority '%s' for %s, but "
+                 "it's in our ExcludedNodes list and StrictNodes is set. "
+                 "Skipping.",
+                 dir_conn_purpose_to_string(dir_purpose),
                  ds->nickname);

The args here are reversed.

comment:51 Changed 8 years ago by arma

+  /* If not, see and StrictNodes is not set, see if we can return any old node

has an extra "see".

comment:52 Changed 8 years ago by arma

c45819dc98 is tricky, because there are some places in the code that
check if intro_nodes is empty to decide if the rend descriptor isn't
worth trying to connect to. I think these are the three:

rend_cache_lookup_entry() checks

  /* XXX023 hack for now, to return "not found" if there are no intro
   * points remaining. See bug 997. */
  if (smartlist_len((*e)->parsed->intro_nodes) == 0)
    return 0;

and rend_client_remove_intro_point() checks

  if (smartlist_len(ent->parsed->intro_nodes) == 0) {
    log_info(LD_REND,
             "No more intro points remain for %s. Re-fetching descriptor.",
             escaped_safe_str_client(rend_query->onion_address));
    rend_client_refetch_v2_renddesc(rend_query);
[...]

and rend_client_desc_trynow() which checks

    if (rend_cache_lookup_entry(conn->rend_data->onion_address, -1,
                                &entry) == 1 &&
        smartlist_len(entry->parsed->intro_nodes) > 0) {
      /* either this fetch worked, or it failed but there was a
       * valid entry from before which we should reuse */
      log_info(LD_REND,"Rend desc is usable. Launching circuits.");
      conn->_base.state = AP_CONN_STATE_CIRCUIT_WAIT;
[...]

(Note how the first and third examples here are actually redundant,
i.e. the third case checks something that will always be true based on
the first.)

I think we can replace these "> 0" checks with a call to something that
tries to pick one and returns failure if it wouldn't pick any. Maybe
rend_client_get_random_intro_impl() then wants to get handed

rend_cache_entry_t *entry

instead of

const rend_data_t *rend_query

comment:53 Changed 8 years ago by nickm

I've added fixup commits for the swapped arg issue and the extra "see".

I'm a little worried about the case where we re-fetch the descriptor based on having all our intro nodes excluded. If they were excluded to begin with, there's no point refetching the descriptor forever in an infinite loop. Is there something to stop that? I wrote up an implementation of the thing you suggest in branch "bug1090-intro_point_exclude", but I think it's broken for that reason.

Any other issues?

comment:54 Changed 8 years ago by Sebastian

The fixups look good.

For the refetching of descriptors, we'd need some kind of logic to check that we don't refetch a descriptor if the one we have looks recent and there are no other events that might cause us to like it now (we got a new consensus).

As for reviewing that commit, it looks like in rend_client_get_random_intro_impl() you wanted that warnings argument to get rid of the StrictNodes check inside (which is a good thing), but then neither documented it nor actually got rid of the StrictNodes check.

comment:55 in reply to:  54 Changed 8 years ago by arma

Replying to Sebastian:

For the refetching of descriptors, we'd need some kind of logic to check that we don't refetch a descriptor if the one we have looks recent and there are no other events that might cause us to like it now (we got a new consensus).

The origin design here (back before v2 hidserv protocol existed) was "if you get a .onion request and you have a cached not-too-old descriptor, use it. if it runs out of intro points, fetch a new one. if the new one you fetched is equal to the old one you had cached, fail all pending .onion requests and stop. else goto step 1."

So we could try reusing that same idea here -- if you do a fetch and you got the thing you already gave up on, give up.

comment:56 Changed 8 years ago by arma

+    /* XXXX022-1090 Should we also allow foo.bar.exit if ExitNodes is set and
+       Bar is not listed in it?  I say yes, but our revised manpage branch
+       implies no. */

I say yes as well.

(Though it's a pretty small point because we're trying to phase .exit
out in general.)

comment:57 Changed 8 years ago by arma

+#if 0
+    /* Removing this retry logic: if we only allow one exit, and it is in the
+       same family as all our entries, then we are just plain not going to win
+       here. */

The problem here is that we mean two different things by "family". One is that MyFamily has been set explicitly by the operator. The other is "it's nearby on the network". I agree that we want to honor the former, but if we've configured a bridge that happens to be nearby the exit relay we picked, and the circuit is about to fail, I think it shouldn't fail. Part of the problem here is that we're picking the exit node first without regard for where our bridge might be.

This topic is also related to the "do you want your bridges to keep you safe or do you want your bridges to maximize your reachability" tension that we're seeing in other contexts.

comment:58 Changed 8 years ago by arma

To clarify, I believe I added d0943987 when we had a test network all running on the same /16, and it couldn't handle my bridge. Since then, we changed routerlist_add_family() to not add any network-nearby family members if EnforceDistinctSubnets is 0, and it is 0 if you set TestingTorNetwork. So it would not be the end of the world to #ifdef out of the "try again ignoring families" logic, but I think if we #ifdef it out we will run across weird edge cases that only happen to bridge users, and I worry that 0.2.2 won't see enough testing for those cases.

As an alternate fix, how about we change

    if (!r && entry_list_is_constrained(options) && consider_exit_family) {

to

    if (!r && options->UseBridges && consider_exit_family) {

?

That way we would skip the behavior when you set EntryNodes (good), but do the behavior if you set UseBridges (also good). After all, people aren't going to successfully be putting both their exit relay and their bridge in the same family (which might be a separate bug we should ponder).

comment:59 Changed 8 years ago by arma

Also, I think we'll encounter unexpected failures if you set node Foo as your EntryNode but then attempt to go to a destination whose IP address is the same as Foo's IP address and it exit enclaves.

comment:60 Changed 8 years ago by nickm

I just convinced roger that the in the case where exit enclaves, restricted entries, and families collide, the solution is *not* "Ignore families when StrictNodes=0". It is maaaaybe something closer to "back off on the exit enclave if the node in question is in the same family as every running bridge". But in any case, it's a different bug.

(More rationale: we got into this rabbit-hole by creating underspecified carve-outs in configuration options in order to keep running in the face of contradictory requirements. We shouldn't do more of that; any carve-outs should be considered, specified, consistent, and disable-able. Further, StrictNodes only applies to *Nodes options: making it apply to families too in just one case would be odd and surprising.)

comment:61 in reply to:  56 Changed 8 years ago by nickm

Replying to arma:

+    /* XXXX022-1090 Should we also allow foo.bar.exit if ExitNodes is set and
+       Bar is not listed in it?  I say yes, but our revised manpage branch
+       implies no. */

I say yes as well.

(Though it's a pretty small point because we're trying to phase .exit
out in general.)

Actually, .exit is still used for TrackExitHosts and MapAddress. For MapAddress, I'm fine with saying that it overrides ExitNodes. For TrackExitHosts, we *do* want to drop tracked hosts if they become excluded. addressmap_clear_excluded_trackexithosts is supposed to solve that problem.

In any case, I believe that this behavior isn't what's on the (revised) manpage, and I'm not sure where to put it.

comment:62 in reply to:  60 Changed 8 years ago by arma

Replying to nickm:

I just convinced roger that the in the case where exit enclaves, restricted entries, and families collide, the solution is *not* "Ignore families when StrictNodes=0". It is maaaaybe something closer to "back off on the exit enclave if the node in question is in the same family as every running bridge". But in any case, it's a different bug.

I opened my new bug as #2998.

comment:63 Changed 8 years ago by arma

    /* XXXX022 HEY!  Shouldn't this look at ent->new_address? */

Yes, I agree. See #437 for the original issue. See git commit 5c03f82a650e where we did the wrong thing:

     if (conn->chosen_exit_retries) {
       if (--conn->chosen_exit_retries == 0) { /* give up! */
-        /* XXX020rc unregister maps from foo to
-         * foo.chosen_exit_name.exit \forall foo. -RD */
+        clear_trackexithost_mappings(edge_conn->chosen_exit_name);
         tor_free(edge_conn->chosen_exit_name); /* clears it */
         /* if this port is dangerous, warn or reject it now that we don't
          * think it'll be using an enclave. */

I wonder if this wants its own bug number. It will want its own changes file in any case.

comment:64 in reply to:  63 Changed 8 years ago by nickm

Replying to arma:

    /* XXXX022 HEY!  Shouldn't this look at ent->new_address? */

Yes, I agree. See #437 for the original issue. See git commit 5c03f82a650e where we did the wrong thing:

...

I wonder if this wants its own bug number. It will want its own changes file in any case.

It's now #2999.

comment:65 Changed 8 years ago by arma

Has addressmap_clear_excluded_trackexithosts() been tested in any way? E.g. triggered to happen, or unit tests, or something. It looks like the sort of thing that won't get reliable testing in the wild (see #2999 for a similar example).

comment:66 in reply to:  65 Changed 8 years ago by nickm

Replying to arma:

Has addressmap_clear_excluded_trackexithosts() been tested in any way?

Nope. Some unit tests would be nice here.

comment:67 Changed 8 years ago by arma

+        if (options->ExcludeNodes) {
+          /* Make sure no existing nodes in the circuit is excluded for
+           * general use.  (This is possible if StrictNodes is 0, and we
+           * thought we needed to use an otherwise excluded node for, say, a
+           * directory operation.) */
+          crypt_path_t *hop = circ->cpath;
+          do {
+            if (routerset_contains_extendinfo(options->ExcludeNodes,
+                                              hop->extend_info))
+              goto next;
+          } while (hop != circ->cpath);
+        }

A) "is excluded" -> "are excluded"

B) Is this situation really possible? What are these directory operations that alter our path selection for general open non-dirty circuits? But that said, it looks fine as a defensive step.

C) This is an infinite loop. You want a hop=hop->next somewhere.

comment:68 Changed 8 years ago by arma

git commit 78c6b416402 doesn't look dangerous, but I think it might be unnecessary too: intro circuits will be internal circuits (that is, the last hop is chosen without regard for its exit policy), and thus ExitNodes and ExcludeExits should not apply. ExcludeNodes is already applied in
rend_services_introduce() when we pick the last hop of the intro circuit.

Keep the clause in for defensive programming, or leave it out for code cleanliness? I lean toward keep-it-in.

comment:69 in reply to:  68 Changed 8 years ago by arma

Replying to arma:

Keep the clause in for defensive programming, or leave it out for code cleanliness? I lean toward keep-it-in.

But while keeping it in I might vote to remove the ExitNodes and ExcludeExitNodes reference. We could also tor_assert build_state->is_internal if we want to be sure.

comment:70 Changed 8 years ago by nickm

I committed a couple more changes on my branch for the last couple of issues.

comment:71 in reply to:  49 Changed 8 years ago by arma

Replying to nickm:

I'm still not happy with the LD_BUG message: I worry that it's doing the wrong thing some times. For example, I'm pretty sure that controller-chosen paths override everything else, including strictnodes. Also, we might be getting a bit too overconfident in saying that we're using the excluded node because no other nodes were available: we might be using it because of a bug. I am not sure I know what the message should say instead, though.

I just pushed 83718adc268 to my bug1090-part1 branch with some alternate text.

comment:72 Changed 8 years ago by arma

My bug1090-part1 branch also has a patch to warn if a given stream causes us to launch 10 circuits, to help catch inconsistencies in our logic.

It also resolves the XXX022-1090 around the excluded_means_no hack.

comment:73 in reply to:  53 Changed 8 years ago by arma

Replying to nickm:

I'm a little worried about the case where we re-fetch the descriptor based on having all our intro nodes excluded. If they were excluded to begin with, there's no point refetching the descriptor forever in an infinite loop. Is there something to stop that? I wrote up an implementation of the thing you suggest in branch "bug1090-intro_point_exclude", but I think it's broken for that reason.

Ha. It turns out there is a thing to stop the infinite downloads:

/** Determine the responsible hidden service directories for <b>desc_id</b>
 * and fetch the descriptor belonging to that ID from one of them. Only
 * send a request to hidden service directories that we did not try within
 * the last REND_HID_SERV_DIR_REQUERY_PERIOD seconds; on success, return 1,
 * in the case that no hidden service directory is left to ask for the
 * descriptor, return 0, and in case of a failure -1. <b>query</b> is only
 * passed for pretty log statements. */
static int
directory_get_from_hs_dir(const char *desc_id, const rend_data_t *rend_query)

It only tries any given hidden service directory at most every 15 minutes. See also #997.

I can't say it's super elegant, but I bet it'll do for now. We should then put this topic on the plate of hidserv things that rransom should take a closer look at.

comment:74 Changed 8 years ago by nickm

sounds reasonable; i'll merge bug1090-intro_point_exclude into bug1090-part1.

comment:75 Changed 8 years ago by nickm

Branch bug1090-part1-squashed now has all pending fixes. Its head of 748350ace11f corresponds to 55f7628f19 on bug1090-part1 .

My current plan is to sleep, then write changes files as fixup commits for bug1090-part1-squashed. After that, it should get squashed again, then get desired_node_behavior merged in, and then it should get merged into maint-0.2.2 and master. Merging onto master will take some fixups because of the node_t conversion, so I want to be fresh for that.

comment:76 in reply to:  54 Changed 8 years ago by arma

Replying to Sebastian:

As for reviewing that commit, it looks like in rend_client_get_random_intro_impl() you wanted that warnings argument to get rid of the StrictNodes check inside (which is a good thing), but then neither documented it nor actually got rid of the StrictNodes check.

I just added a comment for <b>warning</b> to my bug1090-part1-squashed.

comment:77 Changed 8 years ago by nickm

Merged onto 0.2.2. merging onto master.

comment:78 Changed 8 years ago by nickm

Resolution: Nonefixed
Status: needs_reviewclosed

Okay, we've basically rewritten all the code here. There may be bugs remaining, but if there are, they are not "bug 1090": please open new bug reports as appropriate.

comment:79 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.