[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: Patch: Adding country codes to *Nodes config options



On Monday 22 September 2008 21:02:41 Nick Mathewson wrote:
> {Please take pity on us old-school purists and wrap email to under 75
> colums, so that it doesn't overflow our terminals when we reply}
>
> Hi, Robert!  This patch is a good start.  I can clean it up if you
> would like, or you can do another version; let me know what you'd
> prefer.
>

OK, I think I've got my wrapping and most of the patch sorted. I've added XXXX 
comments wherever I'm unsure of something. Hopefully won't require too much 
cleaning up!


Index: src/or/config.c
===================================================================
--- src/or/config.c	(revision 16958)
+++ src/or/config.c	(working copy)
@@ -194,11 +194,11 @@
   V(DNSListenAddress,            LINELIST, NULL),
   V(DownloadExtraInfo,           BOOL,     "0"),
   V(EnforceDistinctSubnets,      BOOL,     "1"),
-  V(EntryNodes,                  STRING,   NULL),
+  V(EntryNodes,                  ROUTERSET,   NULL),
   V(TestingEstimatedDescriptorPropagationTime, INTERVAL, "10 minutes"),
   V(ExcludeNodes,                ROUTERSET, NULL),
   V(ExcludeExitNodes,            ROUTERSET, NULL),
-  V(ExitNodes,                   STRING, NULL),
+  V(ExitNodes,                   ROUTERSET, NULL),
   V(ExitPolicy,                  LINELIST, NULL),
   V(ExitPolicyRejectPrivate,     BOOL,     "1"),
   V(FallbackNetworkstatusFile,   FILENAME,
@@ -817,13 +817,22 @@
   return _version;
 }
 
+/** Release additional memory allocated in options
+ */
+static void
+or_options_free(void)
+{
+  routerset_free(global_options->_ExcludeExitNodesUnion);
+  config_free(&options_format, global_options);
+}
+
 /** Release all memory and resources held by global configuration structures.
  */
 void
 config_free_all(void)
 {
   if (global_options) {
-    config_free(&options_format, global_options);
+    or_options_free();
     global_options = NULL;
   }
   if (global_state) {
@@ -1336,11 +1345,21 @@
 #endif
     geoip_load_file(actual_fname, options);
     tor_free(actual_fname);
+
+    /* XXXX Would iterating through all option_var's routersets be better? */
+    if (options->EntryNodes)
+      routerset_refresh_countries(options->EntryNodes);
+    if (options->ExitNodes)
+      routerset_refresh_countries(options->ExitNodes);
+    if (options->ExcludeNodes)
+      routerset_refresh_countries(options->ExcludeNodes);
+    if (options->ExcludeExitNodes)
+      routerset_refresh_countries(options->ExcludeExitNodes);
   }
   /* Check if we need to parse and add the EntryNodes config option. */
   if (options->EntryNodes &&
       (!old_options ||
-       !opt_streq(old_options->EntryNodes, options->EntryNodes)))
+      (!routerset_equal(old_options->EntryNodes,options->EntryNodes))))
     entry_nodes_should_be_added();
 
   /* Since our options changed, we might need to regenerate and upload our
@@ -1701,7 +1720,6 @@
   case CONFIG_TYPE_LINELIST_S:
     config_line_append((config_line_t**)lvalue, c->key, c->value);
     break;
-
   case CONFIG_TYPE_OBSOLETE:
     log_warn(LD_CONFIG, "Skipping obsolete configuration option '%s'", 
c->key);
     break;
@@ -2964,17 +2982,17 @@
   }
 
   if (options->StrictExitNodes &&
-      (!options->ExitNodes || !strlen(options->ExitNodes)) &&
+      (!options->ExitNodes) &&
       (!old_options ||
        (old_options->StrictExitNodes != options->StrictExitNodes) ||
-       (!opt_streq(old_options->ExitNodes, options->ExitNodes))))
+       (!routerset_equal(old_options->ExitNodes,options->ExitNodes))))
     COMPLAIN("StrictExitNodes set, but no ExitNodes listed.");
 
   if (options->StrictEntryNodes &&
-      (!options->EntryNodes || !strlen(options->EntryNodes)) &&
+      (!options->EntryNodes) &&
       (!old_options ||
        (old_options->StrictEntryNodes != options->StrictEntryNodes) ||
-       (!opt_streq(old_options->EntryNodes, options->EntryNodes))))
+       (!routerset_equal(old_options->EntryNodes,options->EntryNodes))))
     COMPLAIN("StrictEntryNodes set, but no EntryNodes listed.");
 
   if (options->AuthoritativeDir) {
@@ -3334,10 +3352,6 @@
   if (options->UseEntryGuards && ! options->NumEntryGuards)
     REJECT("Cannot enable UseEntryGuards with NumEntryGuards set to 0");
 
-  if (check_nickname_list(options->ExitNodes, "ExitNodes", msg))
-    return -1;
-  if (check_nickname_list(options->EntryNodes, "EntryNodes", msg))
-    return -1;
   if (check_nickname_list(options->MyFamily, "MyFamily", msg))
     return -1;
   for (cl = options->NodeFamilies; cl; cl = cl->next) {
Index: src/or/routerlist.c
===================================================================
--- src/or/routerlist.c	(revision 16958)
+++ src/or/routerlist.c	(working copy)
@@ -4707,6 +4707,7 @@
   /** An address policy for routers in the set.  For implementation reasons,
    * a router belongs to the set if it is _rejected_ by this policy. */
   smartlist_t *policies;
+  bitarray_t *countries;
 };
 
 /** Return a new empty routerset. */
@@ -4718,9 +4719,62 @@
   result->names = strmap_new();
   result->digests = digestmap_new();
   result->policies = smartlist_create();
+  result->countries = bitarray_init_zero(geoip_get_n_countries());
   return result;
 }
 
+/** Add the GeoIP database's integer index (+1) of a valid two-character
+ * country code to the routerset's <b>countries</b> bitarray. Return the
+ * integer index if the country code is valid, zero otherwise.*/
+static int
+routerset_add_country(const char *c)
+{
+  char *country;
+  int cc;
+
+  /* XXXX: Country codes must be of the form \{[a-z\?]{2}\} but this accepts
+     \{[.]{2}\}. Do we need to be strict? */
+  if (!(*c=='{') || !(*(c+3)=='}'))
+    return 0;
+
+  if (!geoip_is_loaded()) {
+    log(LOG_WARN, LD_CONFIG, "GeoIP Database Not Loaded: Cannot add country"
+                             "entry %s, ignoring.", c);
+    return 0;
+  }
+
+  country=tor_strdup(c);
+  tor_strstrip(country,"}");
+  tor_strstrip(country,"{");
+  tor_strlower(country);
+
+  if ((cc=geoip_is_valid_country(country))==-1) {
+    log(LOG_WARN, LD_CONFIG, "Country Code '%s' is not valid, ignoring.",
+          country);
+  }
+  tor_free(country);
+  return cc;
+}
+
+/** Update the routerset's <b>countries</b> bitarray_t. Called whenever
+ * the GeoIP database is reloaded.
+ */
+void
+routerset_refresh_countries(routerset_t *target)
+{
+  int cc;
+  /* XXX: Is this the proper check for a bitarray_t that is empty? */
+  if (!target->countries)
+    return;
+  bitarray_free(target->countries);
+  target->countries=bitarray_init_zero(geoip_get_n_countries());
+  SMARTLIST_FOREACH(target->list, char *, nick, {
+    if ((cc=routerset_add_country(nick))!=-1) {
+        bitarray_set(target->countries,cc);
+    }
+  });
+}
+
 /** Parse the string <b>s</b> to create a set of routerset entries, and add
  * them to <b>target</b>.  In log messages, refer to the string as
  * <b>description</b>.  Return 0 on success, -1 on failure.
@@ -4732,7 +4786,7 @@
 int
 routerset_parse(routerset_t *target, const char *s, const char *description)
 {
-  int r = 0;
+  int r = 0,cc=0;
   smartlist_t *list = smartlist_create();
   smartlist_split_string(list, s, ",",
                          SPLIT_SKIP_SPACE | SPLIT_IGNORE_BLANK, 0);
@@ -4748,6 +4802,10 @@
       } else if (is_legal_nickname(nick)) {
         log_debug(LD_CONFIG, "Adding nickname %s to %s", nick, description);
         strmap_set_lc(target->names, nick, (void*)1);
+      } else if ((cc=routerset_add_country(nick))!=-1) {
+        log_debug(LD_CONFIG, "Adding country %s to %s", nick,
+                  description);
+        bitarray_set(target->countries,cc);
       } else if ((strchr(nick,'.') || strchr(nick, '*')) &&
                  (p = router_parse_addr_policy_item_from_string(
                                      nick, ADDR_POLICY_REJECT))) {
@@ -4795,6 +4853,10 @@
   if (addr && compare_tor_addr_to_addr_policy(addr, orport, set->policies)
       == ADDR_POLICY_REJECTED)
     return 1;
+  if (addr &&
+     bitarray_is_set(set->countries,
+                    geoip_get_country_by_ip(tor_addr_to_ipv4h(addr))))
+    return 1;
   return 0;
 }
 
@@ -4856,14 +4918,16 @@
         smartlist_add(out, router);
     }
   });
-  if (smartlist_len(routerset->policies)) {
+  if (smartlist_len(routerset->policies) || (routerset->countries)) {
     routerlist_t *rl = router_get_routerlist();
     SMARTLIST_FOREACH(rl->routers, routerinfo_t *, router,
-      if (compare_addr_to_addr_policy(router->addr, router->or_port,
-               routerset->policies) == ADDR_POLICY_REJECT) {
+      if ((compare_addr_to_addr_policy(router->addr, router->or_port,
+          routerset->policies) == ADDR_POLICY_REJECT) ||
+          (bitarray_is_set(routerset->countries,
+                           geoip_get_country_by_ip(router->addr)))) {
         if (!running_only || router->is_running)
           smartlist_add(out, router);
-      });
+  });
   }
 }
 
@@ -4892,6 +4956,29 @@
   return smartlist_join_strings(set->list, ",", 0, NULL);
 }
 
+/** Helper: return true iff old and new are both NULL, or both non-NULL
+ * equal routersets. */
+int
+routerset_equal(const routerset_t *old, const routerset_t *new)
+{
+  /* XXXX: This won't work if the names/digests are identical but in a
+     different order. Checking for exact equality would be heavy going,
+     is it worth it? */
+  if (sizeof(old->names) != sizeof(new->names))
+    return 0;
+  if (memcmp(old->names,new->names,sizeof(new->names)))
+    return 0;
+  if (sizeof(old->digests) != sizeof(new->digests))
+    return 0;
+  if (memcmp(old->digests,new->digests,sizeof(new->digests)))
+    return 0;
+  if (sizeof(old->countries) != sizeof(new->countries))
+    return 0;
+  if (memcmp(old->countries,new->countries,sizeof(new->countries)))
+    return 0;
+  return 1;
+}
+
 /** Free all storage held in <b>routerset</b>. */
 void
 routerset_free(routerset_t *routerset)
@@ -4904,7 +4991,7 @@
 
   strmap_free(routerset->names, NULL);
   digestmap_free(routerset->digests, NULL);
-
+  bitarray_free(routerset->countries);
   tor_free(routerset);
 }
 
Index: src/or/or.h
===================================================================
--- src/or/or.h	(revision 16958)
+++ src/or/or.h	(working copy)
@@ -1394,7 +1394,8 @@
   time_t last_reachable;
   /** When did we start testing reachability for this OR? */
   time_t testing_since;
-
+  /** According to the geoip db what country is this router in? */
+  int country;
 } routerinfo_t;
 
 /** Information needed to keep and cache a signed extra-info document. */
@@ -2090,17 +2091,25 @@
   char *Address; /**< OR only: configured address for this onion router. */
   char *PidFile; /**< Where to store PID of Tor process. */
 
-  char *ExitNodes; /**< Comma-separated list of nicknames of ORs to consider
-                    * as exits. */
-  char *EntryNodes; /**< Comma-separated list of nicknames of ORs to consider
-                     * as entry points. */
+  struct routerset_t *ExitNodes; /**< Structure containing nicknames,
+                                   * digests, country codes and IP address
+                                   * patterns of ORs to consider as exits. */
+  struct routerset_t *EntryNodes;/**< Structure containing nicknames,
+                                   * digests, country codes and IP address
+                                   * patterns of ORs to consider as entry
+                                   * points. */
   int StrictExitNodes; /**< Boolean: When none of our ExitNodes are up, do we
                         * stop building circuits? */
   int StrictEntryNodes; /**< Boolean: When none of our EntryNodes are up, do 
we
                          * stop building circuits? */
-  struct routerset_t *ExcludeNodes; /**< Comma-separated list of nicknames of
-                       * ORs not to use in circuits. */
-  struct routerset_t *ExcludeExitNodes; /**<DODOC */
+  struct routerset_t *ExcludeNodes;/**< Structure containing nicknames,
+                                   * digests, country codes and IP address
+                                   * patterns of ORs not to use in
+                                   * circuits. */
+  struct routerset_t *ExcludeExitNodes;/**< Structure containing nicknames,
+                                   * digests, country codes and IP address
+                                   * patterns of ORs not to consider as exits.
+                                   */
 
   /** Union of ExcludeNodes and ExcludeExitNodes */
   struct routerset_t *_ExcludeExitNodesUnion;
@@ -3468,6 +3477,7 @@
 int geoip_get_n_countries(void);
 const char *geoip_get_country_name(int num);
 int geoip_is_loaded(void);
+int geoip_is_valid_country(const char *country);
 /** Indicates an action that we might be noting geoip statistics on.
  * Note that if we're noticing CONNECT, we're a bridge, and if we're noticing
  * the others, we're not.
@@ -4292,6 +4302,8 @@
 void routerset_subtract_routers(smartlist_t *out,
                                 const routerset_t *routerset);
 char *routerset_to_string(const routerset_t *routerset);
+void routerset_refresh_countries(routerset_t *target);
+int routerset_equal(const routerset_t *old, const routerset_t *new);
 void routerset_free(routerset_t *routerset);
 
 int hid_serv_get_responsible_directories(smartlist_t *responsible_dirs,
Index: src/or/geoip.c
===================================================================
--- src/or/geoip.c	(revision 16958)
+++ src/or/geoip.c	(working copy)
@@ -42,6 +42,23 @@
 /** A list of all known geoip_entry_t, sorted by ip_low. */
 static smartlist_t *geoip_entries = NULL;
 
+/** Return the index of the <b>country</b>'s entry in the GeoIP DB
+ * if it is a valid 2-letter country code, otherwise return zero.
+ */
+int
+geoip_is_valid_country(const char *country)
+{
+  void *_idxplus1;
+  intptr_t idx;
+
+  _idxplus1 = strmap_get_lc(country_idxplus1_by_lc_code, country);
+  if (!_idxplus1)
+    return -1;
+
+  idx = ((uintptr_t)_idxplus1)-1;
+  return (int)idx;
+}
+
 /** Add an entry to the GeoIP table, mapping all IPs between <b>low</b> and
  * <b>high</b>, inclusive, to the 2-letter country code <b>country</b>.
  */
Index: src/or/circuitbuild.c
===================================================================
--- src/or/circuitbuild.c	(revision 16958)
+++ src/or/circuitbuild.c	(working copy)
@@ -1278,7 +1278,7 @@
            n_pending_connections);
 
   preferredexits = smartlist_create();
-  add_nickname_list_to_smartlist(preferredexits,options->ExitNodes,1);
+  routerset_get_all_routers(preferredexits,options->ExitNodes,1);
 
   sl = smartlist_create();
 
@@ -1399,6 +1399,24 @@
   return NULL;
 }
 
+/** 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_router_excluded(const extend_info_t *exit)
+{
+  or_options_t *options = get_options();
+  routerinfo_t *ri = router_get_by_digest(exit->identity_digest);
+
+  if (!ri || !options->_ExcludeExitNodesUnion)
+    return;
+
+  if (routerset_contains_router(options->_ExcludeExitNodesUnion, ri))
+    log_warn(LD_CIRC,"Requested exit node '%s' is in ExcludeNodes, "
+             "or ExcludeExitNodes, using anyway.",exit->nickname);
+
+  return;
+}
+
 /** Decide a suitable length for circ's cpath, and pick an exit
  * router (or use <b>exit</b> if provided). Store these in the
  * cpath. Return 0 if ok, -1 if circuit should be closed. */
@@ -1419,6 +1437,7 @@
   }
 
   if (exit) { /* the circuit-builder pre-requested one */
+    warn_if_router_excluded(exit);
     log_info(LD_CIRC,"Using requested exit node '%s'", exit->nickname);
     exit = extend_info_dup(exit);
   } else { /* we have to decide one */
@@ -1832,7 +1851,7 @@
   else if (options->UseBridges && ri->purpose != ROUTER_PURPOSE_BRIDGE)
     *reason = "not a bridge";
   else if (!options->UseBridges && !ri->is_possible_guard &&
-           !router_nickname_is_in_list(ri, options->EntryNodes))
+           !routerset_contains_router(options->EntryNodes,ri))
     *reason = "not recommended as a guard";
   else if (routerset_contains_router(options->ExcludeNodes, ri))
     *reason = "excluded";
@@ -1856,7 +1875,6 @@
     control_event_guard(e->nickname, e->identity, "GOOD");
     changed = 1;
   }
-
   return changed;
 }
 
@@ -2346,8 +2364,9 @@
     return;
   }
 
-  log_info(LD_CIRC,"Adding configured EntryNodes '%s'.",
-           options->EntryNodes);
+  if (options->EntryNodes)
+    log_info(LD_CIRC,"Adding configured EntryNodes '%s'.",
+             routerset_to_string(options->EntryNodes));
 
   entry_routers = smartlist_create();
   entry_fps = smartlist_create();
@@ -2355,7 +2374,7 @@
   old_entry_guards_not_on_list = smartlist_create();
 
   /* Split entry guards into those on the list and those not. */
-  add_nickname_list_to_smartlist(entry_routers, options->EntryNodes, 0);
+  routerset_get_all_routers(entry_routers, options->EntryNodes, 0);
   SMARTLIST_FOREACH(entry_routers, routerinfo_t *, ri,
                     smartlist_add(entry_fps,ri->cache_info.identity_digest));
   SMARTLIST_FOREACH(entry_guards, entry_guard_t *, e, {
Index: contrib/checkSpace.pl
===================================================================
--- contrib/checkSpace.pl	(revision 16958)
+++ contrib/checkSpace.pl	(working copy)
@@ -73,7 +73,7 @@
                 s!//.*!!;
             }
             ## Warn about braces preceded by non-space.
-            if (/([^\s])\{/) {
+            if (/([^\s'])\{/) {
                 print "       $1\{:$fn:$.\n";
             }
             ## Warn about multiple internal spaces.
Index: doc/tor.1.in
===================================================================
--- doc/tor.1.in	(revision 16958)
+++ doc/tor.1.in	(working copy)
@@ -422,28 +422,28 @@
 .LP
 .TP
 \fBExcludeNodes \fR\fInode\fR,\fInode\fR,\fI...\fP
-A list of identity fingerprints, nicknames, and address patterns of
-nodes to never use when building a circuit.  (Example: ExcludeNodes
-SlowServer, $ABCDEFFFFFFFFFFFFFFF, 255.254.0.0/8)
+A list of identity fingerprints, nicknames, country codes and address patterns
+of nodes to never use when building a circuit.  (Example: ExcludeNodes
+SlowServer, $ABCDEFFFFFFFFFFFFFFF, {cc}, 255.254.0.0/8)
 .LP
 .TP
 \fBExcludeExitNodes \fR\fInode\fR,\fInode\fR,\fI...\fP
-A list of identity fingerprints, nicknames, and address patterns of
-nodes to never use when picking an exit node.  Note that any node
+A list of identity fingerprints, nicknames, country codes and address patterns
+of nodes to never use when picking an exit node.  Note that any node
 listed in ExcludeNodes is automatically considered to be part of this
 list.
 .LP
 .TP
 \fBEntryNodes \fR\fInode\fR,\fInode\fR,\fI...\fP
-A list of identity fingerprints or nicknames of preferred nodes to use for the
-first hop in the circuit.
+A list of identity fingerprints, nicknames, country codes and address patterns
+of nodes to use for the first hop in the circuit.
 These are treated only as preferences unless StrictEntryNodes (see
 below) is also set.
 .LP
 .TP
 \fBExitNodes \fR\fInode\fR,\fInode\fR,\fI...\fP
-A list of identity fingerprints or nicknames of preferred nodes to use for the
-last hop in the circuit.
+A list of identity fingerprints, nicknames, country codes and address patterns
+of nodes to use for the last hop in the circuit.
 These are treated only as preferences unless StrictExitNodes (see
 below) is also set.
 .LP