r11773@catbus: nickm | 2007-02-12 15:18:48 -0500

 Implement proposal 106: stop requiring clients to have certificates, and stop checking for nicknames in certificates.  [See proposal 106 for rationale.]  Also improve messages when checking TLS handshake, to re-resolve bug 382.


svn:r9568

ChangeLog

@@ -33,6 +33,14 @@ Changes in version 0.1.2.8-alpha - 2007-??-??
       advance warning.
     - Remove some never-implemented options.  Mark PathlenCoinWeight as
       obsolete.
+    - Implement proposal 106: Stop requiring clients to have well-formed
+      certificates; stop checking nicknames in certificates.  (Clients have
+      certificates so that they can look like Tor servers, but in the future
+      we might want to allow them to look like regular TLS clients instead.
+      Nicknames in certificates serve no purpose other than making our
+      protocol easier to recognize on the wire.)
+    - Revise messages on handshake failure again to be even more clear about
+      which are incoming connections and which are outgoing.
 
 
 Changes in version 0.1.2.7-alpha - 2007-02-06

doc/TODO

@@ -111,12 +111,14 @@ NR. Write path-spec.txt
       - recommend gaim.
       - unrecommend IE because of ftp:// bug.
 N   - we should add a preamble to tor-design saying it's out of date.
-N   - Document transport and natdport
-
-  - Forward compatibility fixes
-    - Caches should start trying to cache consensus docs?
-NR    - Design
-N     - Implement, if we think it's smart.
+N   . Document transport and natdport
+      o In man page
+      - In a good HOWTO.
+
+  . Forward compatibility fixes
+    D Caches should start trying to cache consensus docs?
+      D Design
+      D Implement, if we think it's smart.
     - Start uploading short and long descriptors; authorities should support
       URLs to retrieve long descriptors, and should discard short descriptors
       for now.  Later, once tools use the "long descriptor" URLs, authorities
@@ -124,9 +126,11 @@ N     - Implement, if we think it's smart.
       a descriptor.
 NR    - Design
 N     - Implement, if we think it's smart.
-    - Check for any outstanding checks we do on the form or number of client
+    o Check for any outstanding checks we do on the form or number of client
       certificates that would prevent us from executing certain
       blocking-resistance strategies.
+      o Design (proposal 106)
+      o Implement
 
 Things we'd like to do in 0.2.0:
   - Proposals:

doc/spec/proposals/000-index.txt

@@ -24,5 +24,5 @@ Proposals by number:
 103  Splitting identity key from regularly used signing key [OPEN]
 104  Long and Short Router Descriptors [OPEN]
 105  Version negotiation for the Tor protocol [OPEN]
-106  Checking fewer things during TLS handshakes [OPEN]
+106  Checking fewer things during TLS handshakes [FINISHED]
 

doc/spec/proposals/106-less-tls-constraint.txt

@@ -4,7 +4,7 @@ Version: $Revision: 12105 $
 Last-Modified: $Date: 2007-01-30T07:50:01.643717Z $
 Author: Nick Mathewson
 Created:
-Status: Accepted
+Status: Finished
 
 Overview:
 

src/common/tortls.c

@@ -672,56 +672,6 @@ tor_tls_peer_has_cert(tor_tls_t *tls)
   return 1;
 }
 
-/** Write the nickname (if any) that the peer connected on <b>tls</b>
- * claims to have into the first <b>buflen</b> characters of <b>buf</b>.
- * Truncate the nickname if it is longer than buflen-1 characters.  Always
- * NUL-terminate.  Return 0 on success, -1 on failure.
- */
-int
-tor_tls_get_peer_cert_nickname(int severity, tor_tls_t *tls,
-                               char *buf, size_t buflen)
-{
-  X509 *cert = NULL;
-  X509_NAME *name = NULL;
-  int nid;
-  int lenout;
-  int r = -1;
-
-  if (!(cert = SSL_get_peer_certificate(tls->ssl))) {
-    log_fn(severity, LD_PROTOCOL, "Peer has no certificate");
-    goto error;
-  }
-  if (!(name = X509_get_subject_name(cert))) {
-    log_fn(severity, LD_PROTOCOL, "Peer certificate has no subject name");
-    goto error;
-  }
-  if ((nid = OBJ_txt2nid("commonName")) == NID_undef)
-    goto error;
-
-  lenout = X509_NAME_get_text_by_NID(name, nid, buf, buflen);
-  if (lenout == -1)
-    goto error;
-  if (((int)strspn(buf, LEGAL_NICKNAME_CHARACTERS)) < lenout) {
-    log_fn(severity, LD_PROTOCOL,
-           "Peer certificate nickname %s has illegal characters.",
-           escaped(buf));
-    if (strchr(buf, '.'))
-      log_fn(severity, LD_PROTOCOL,
-             "  (Maybe it is not really running Tor at its "
-             "advertised OR port.)");
-    goto error;
-  }
-
-  r = 0;
-
- error:
-  if (cert)
-    X509_free(cert);
-
-  tls_log_errors(severity, "getting peer certificate nickname");
-  return r;
-}
-
 /** DOCDOC */
 static void
 log_cert_lifetime(X509 *cert, const char *problem)

src/or/circuitbuild.c

@@ -70,6 +70,11 @@ get_unique_circ_id_by_conn(or_connection_t *conn)
   uint16_t high_bit;
 
   tor_assert(conn);
+  if (conn->circ_id_type == CIRC_ID_TYPE_NEITHER) {
+    log_warn(LD_BUG, "Bug: Trying to pick a circuit ID for a connection from "
+             "a client with no identity.");
+    return 0;
+  }
   high_bit = (conn->circ_id_type == CIRC_ID_TYPE_HIGHER) ? 1<<15 : 0;
   do {
     /* Sequentially iterate over test_circ_id=1...1<<15-1 until we find a

src/or/connection_or.c

@@ -26,7 +26,7 @@ static int connection_or_empty_enough_for_dirserv_data(or_connection_t *conn);
 static digestmap_t *orconn_identity_map = NULL;
 
 /** If conn is listed in orconn_identity_map, remove it, and clear
- * conn->identity_digest. */
+ * conn->identity_digest.  Otherwise do nothing. */
 void
 connection_or_remove_from_identity_map(or_connection_t *conn)
 {
@@ -35,8 +35,13 @@ connection_or_remove_from_identity_map(or_connection_t *conn)
   if (!orconn_identity_map)
     return;
   tmp = digestmap_get(orconn_identity_map, conn->identity_digest);
-  if (!tmp)
+  if (!tmp) {
+    if (!tor_digest_is_zero(conn->identity_digest)) {
+      log_warn(LD_BUG, "Didn't found connection on identity map when trying "
+               "to remove it.");
+    }
     return;
+  }
   if (conn == tmp) {
     if (conn->next_with_same_id)
       digestmap_set(orconn_identity_map, conn->identity_digest,
@@ -81,7 +86,7 @@ connection_or_clear_identity_map(void)
 }
 
 /** Change conn->identity_digest to digest, and add conn into
- * orconn_digest_map. */
+ * orconn_digest_map.   */
 static void
 connection_or_set_identity_digest(or_connection_t *conn, const char *digest)
 {
@@ -93,14 +98,22 @@ connection_or_set_identity_digest(or_connection_t *conn, const char *digest)
     orconn_identity_map = digestmap_new();
   if (!memcmp(conn->identity_digest, digest, DIGEST_LEN))
     return;
-  if (tor_digest_is_zero(conn->identity_digest))
+
+  /* If the identity was set previously, remove the old mapping. */
+  if (! tor_digest_is_zero(conn->identity_digest))
     connection_or_remove_from_identity_map(conn);
 
   memcpy(conn->identity_digest, digest, DIGEST_LEN);
+
+  /* If we're setting the ID to zero, don't add a mapping.*/
+  if (tor_digest_is_zero(digest))
+    return;
+
   tmp = digestmap_set(orconn_identity_map, digest, conn);
   conn->next_with_same_id = tmp;
 
-#if 0
+#if 1
+  /*XXXX012 change this back to if 0. */
   /* Testing code to check for bugs in representation. */
   for (; tmp; tmp = tmp->next_with_same_id) {
     tor_assert(!memcmp(tmp->identity_digest, digest, DIGEST_LEN));
@@ -551,16 +564,21 @@ connection_or_nonopen_was_started_here(or_connection_t *conn)
   return !tor_tls_is_server(conn->tls);
 }
 
-/** Conn just completed its handshake. Return 0 if all is well, and
+/** <b>Conn</b> just completed its handshake. Return 0 if all is well, and
  * return -1 if he is lying, broken, or otherwise something is wrong.
  *
- * Make sure he sent a correctly formed certificate. If it has a
- * recognized ("named") nickname, make sure his identity key matches
- * it. If I initiated the connection, make sure it's the right guy.
+ * If we initiated this connection (<b>started_here</b> is true), make sure
+ * the other side sent sent a correctly formed certificate. If I initiated the
+ * connection, make sure it's the right guy.
  *
- * If we return 0, write a hash of the identity key into digest_rcvd,
- * which must have DIGEST_LEN space in it. (If we return -1 this
- * buffer is undefined.)
+ * Otherwise (if we _didn't_ initiate this connection), it's okay for
+ * the certificate to be weird or absent.
+ *
+ * If we return 0, and the certificate is as expected, write a hash of the
+ * identity key into digest_rcvd, which must have DIGEST_LEN space in it. (If
+ * we return -1 this buffer is undefined.)  If the certificate is invalid
+ * or missing on an incoming connection, we return 0 and set digest_rcvd to
+ * DIGEST_LEN 0 bytes.
  *
  * As side effects,
  * 1) Set conn->circ_id_type according to tor-spec.txt.
@@ -569,66 +587,68 @@ connection_or_nonopen_was_started_here(or_connection_t *conn)
  *    this guy; and note that this guy is reachable.
  */
 static int
-connection_or_check_valid_handshake(or_connection_t *conn, char *digest_rcvd)
+connection_or_check_valid_handshake(or_connection_t *conn, int started_here,
+                                    char *digest_rcvd)
 {
-  routerinfo_t *router;
   crypto_pk_env_t *identity_rcvd=NULL;
   char nickname[MAX_NICKNAME_LEN+1];
   or_options_t *options = get_options();
   int severity = server_mode(options) ? LOG_PROTOCOL_WARN : LOG_WARN;
-  int started_here = connection_or_nonopen_was_started_here(conn);
   const char *safe_address =
     started_here ? conn->_base.address : safe_str(conn->_base.address);
-  const char *peer_type = started_here ? "Router" : "Client or router";
+  const char *conn_type = started_here ? "outgoing" : "incoming";
+  int has_cert = 0, has_identity = 0;
 
   check_no_tls_errors();
-  if (! tor_tls_peer_has_cert(conn->tls)) {
-    log_info(LD_PROTOCOL,"%s (%s:%d) didn't send a cert! Closing.",
-             peer_type, safe_address, conn->_base.port);
-    return -1;
-  }
-  check_no_tls_errors();
-  if (tor_tls_get_peer_cert_nickname(severity, conn->tls, nickname,
-                                     sizeof(nickname))) {
-    log_fn(severity,LD_PROTOCOL,"%s (%s:%d) has a cert without a "
-           "valid nickname. Closing.",
-           peer_type, safe_address, conn->_base.port);
+  has_cert = tor_tls_peer_has_cert(conn->tls);
+  if (started_here && !has_cert) {
+    log_info(LD_PROTOCOL,"Tried connecting to router at %s:%d, but it didn't "
+             "send a cert! Closing.",
+             safe_address, conn->_base.port);
     return -1;
+  } else if (!has_cert) {
+    log_debug(LD_PROTOCOL,"Got incoming connection with no certificate. "
+              "That's ok.");
   }
   check_no_tls_errors();
-  log_debug(LD_OR, "%s (%s:%d) claims to be router '%s'",
-            peer_type, safe_address, conn->_base.port, nickname);
 
-  if (tor_tls_verify(severity, conn->tls, &identity_rcvd) < 0) {
-    log_fn(severity,LD_OR,"%s which claims to be router '%s' (%s:%d),"
-           " has a cert but it's invalid. Closing.",
-           peer_type, nickname, safe_address, conn->_base.port);
-    return -1;
+  if (has_cert) {
+    int v = tor_tls_verify(started_here?severity:LOG_INFO,
+                           conn->tls, &identity_rcvd);
+    if (started_here && v<0) {
+      log_fn(severity,LD_OR,"Tried connecting to router at %s:%d: It"
+             " has a cert but it's invalid. Closing.",
+             safe_address, conn->_base.port);
+      return -1;
+    } else if (v<0) {
+      log_info(LD_PROTOCOL,"Incoming connection gave us an invalid cert "
+               "chain; ignoring.");
+    } else {
+      log_debug(LD_OR,"The certificate seems to be valid on %s connection "
+                "with %s:%d", conn_type, safe_address, conn->_base.port);
+    }
+    check_no_tls_errors();
   }
-  check_no_tls_errors();
-  log_debug(LD_OR,"The router's cert is valid.");
-  crypto_pk_get_digest(identity_rcvd, digest_rcvd);
 
-  if (crypto_pk_cmp_keys(get_identity_key(), identity_rcvd)<0) {
-    conn->circ_id_type = CIRC_ID_TYPE_LOWER;
+  if (identity_rcvd) {
+    has_identity=1;
+    crypto_pk_get_digest(identity_rcvd, digest_rcvd);
+
+    if (crypto_pk_cmp_keys(get_identity_key(), identity_rcvd)<0) {
+      conn->circ_id_type = CIRC_ID_TYPE_LOWER;
+    } else {
+      conn->circ_id_type = CIRC_ID_TYPE_HIGHER;
+    }
+    crypto_free_pk_env(identity_rcvd);
   } else {
-    conn->circ_id_type = CIRC_ID_TYPE_HIGHER;
-  }
-  crypto_free_pk_env(identity_rcvd);
-
-  router = router_get_by_nickname(nickname, 0);
-  if (router && /* we know this nickname */
-      router->is_named && /* make sure it's the right guy */
-      memcmp(digest_rcvd, router->cache_info.identity_digest,DIGEST_LEN) !=0) {
-    log_fn(severity, LD_OR,
-           "Identity key not as expected for peer claiming to be "
-           "'%s' (%s:%d)",
-           nickname, safe_address, conn->_base.port);
-    return -1;
+    memset(digest_rcvd, 0, DIGEST_LEN);
+    conn->circ_id_type = CIRC_ID_TYPE_NEITHER;
   }
 
   if (started_here) {
     int as_advertised = 1;
+    tor_assert(has_cert);
+    tor_assert(has_identity);
     if (memcmp(digest_rcvd, conn->identity_digest, DIGEST_LEN)) {
       /* I was aiming for a particular digest. I didn't get it! */
       char seen[HEX_DIGEST_LEN+1];
@@ -637,8 +657,8 @@ connection_or_check_valid_handshake(or_connection_t *conn, char *digest_rcvd)
       base16_encode(expected, sizeof(expected), conn->identity_digest,
                     DIGEST_LEN);
       log_fn(severity, LD_OR,
-             "Identity key not as expected for router at %s:%d: wanted %s "
-             "but got %s",
+             "Tried connecting to router at %s:%d, but identity key was not "
+             "as expected wanted %s but got %s",
              conn->_base.address, conn->_base.port, expected, seen);
       entry_guard_register_connect_status(conn->identity_digest,0,time(NULL));
       router_set_status(conn->identity_digest, 0);
@@ -677,7 +697,7 @@ connection_tls_finish_handshake(or_connection_t *conn)
   int started_here = connection_or_nonopen_was_started_here(conn);
 
   log_debug(LD_OR,"tls handshake done. verifying.");
-  if (connection_or_check_valid_handshake(conn, digest_rcvd) < 0)
+  if (connection_or_check_valid_handshake(conn, started_here, digest_rcvd) < 0)
     return -1;
 
   if (!started_here) {

src/or/or.h

@@ -201,7 +201,8 @@
 /** DOCDOC */
 typedef enum {
   CIRC_ID_TYPE_LOWER=0,
-  CIRC_ID_TYPE_HIGHER=1
+  CIRC_ID_TYPE_HIGHER=1,
+  CIRC_ID_TYPE_NEITHER=2
 } circ_id_type_t;
 
 #define _CONN_TYPE_MIN 3
@@ -772,8 +773,9 @@ typedef struct connection_t {
 typedef struct or_connection_t {
   connection_t _base;
 
-  char identity_digest[DIGEST_LEN]; /**< Hash of the public RSA key for
-                                     * the other side's signing key. */
+  /** Hash of the public RSA key for the other side's identity key, or zero if
+   * the other side hasn't shown us a valid identity key. */
+  char identity_digest[DIGEST_LEN];
   char *nickname; /**< Nickname of OR on other side (if any). */
 
   tor_tls_t *tls; /**< TLS connection state */
@@ -787,9 +789,6 @@ typedef struct or_connection_t {
   int read_bucket; /**< When this hits 0, stop receiving. Every second we
                     * add 'bandwidthrate' to this, capping it at
                     * bandwidthburst. (OPEN ORs only) */
-  circ_id_type_t circ_id_type; /**< When we send CREATE cells along this
-                                * connection, which half of the space should
-                                * we use? */
   int n_circuits; /**< How many circuits use this connection as p_conn or
                    * n_conn ? */
   struct or_connection_t *next_with_same_id; /**< Next connection with same
@@ -797,6 +796,9 @@ typedef struct or_connection_t {
   /** Linked list of bridged dirserver connections that can't write until
    * this connection's outbuf is less full. */
   struct dir_connection_t *blocked_dir_connections;
+  circ_id_type_t circ_id_type:2; /**< When we send CREATE cells along this
+                                  * connection, which half of the space should
+                                  * we use? */
   uint16_t next_circ_id; /**< Which circ_id do we try to use next on
                           * this connection?  This is always in the
                           * range 0..1<<15-1. */