randomize ports and query_id, #23

CHANGELOG.md

@@ -2,6 +2,10 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
+## unreleased
+### Fixed
+- Randomized ports for client connections and message ids, #23
+
 ## 0.7.0 2016-06-20
 ### Added
 - Added recovery from journal to named startup

Cargo.lock

@@ -11,6 +11,7 @@ dependencies = [
  "mio 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "openssl 0.7.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "openssl-sys 0.7.8 (registry+https://github.com/rust-lang/crates.io-index)",
+ "rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)",
  "rusqlite 0.7.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)",
  "time 0.1.35 (registry+https://github.com/rust-lang/crates.io-index)",

Cargo.toml

@@ -7,7 +7,7 @@ authors = ["Benjamin Fry <benjaminfry@me.com>"]
 # uploaded to crates.io (aka this is not markdown)
 description = """
 TRust-DNS is a safe and secure DNS server and client with DNSec support.
- Eentually this could be a replacement for BIND9. DNSSec on the client side,
+ Eventually this could be a replacement for BIND9. DNSSec on the client side,
  with NSEC validation for negative records, is complete. The client and
  server both support dynamic DNS with authenticated requests.
 """
@@ -73,6 +73,7 @@ log = "^0.3.5"
 mio = "^0.5.1"
 openssl = "^0.7.8"
 openssl-sys = "^0.7.8"
+rand = "^0.3"
 rustc-serialize = "^0.3.18"
 rusqlite = "^0.7.3"
 time = "^0.1.35"

src/client/client.rs

@@ -12,13 +12,14 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::cell::{Cell, RefCell};
+use std::cell::RefCell;
 use std::collections::HashSet;
 use std::sync::Arc as Rc;
 
 use chrono::UTC;
 use data_encoding::base32hex;
 use openssl::crypto::pkey::Role;
+use rand;
 
 use ::error::*;
 use ::rr::{DNSClass, RecordType, Record, RData};
@@ -34,7 +35,6 @@ use ::client::ClientConnection;
 ///  disallow TCP in some cases, so if TCP double check if UDP works.
 pub struct Client<C: ClientConnection> {
   client_connection: RefCell<C>,
-  next_id: Cell<u16>,
   trust_anchor: TrustAnchor,
 }
 
@@ -46,7 +46,6 @@ impl<C: ClientConnection> Client<C> {
   /// * `client_connection` - the client_connection to use for all communication
   pub fn new(client_connection: C) -> Client<C> {
     Client{ client_connection: RefCell::new(client_connection),
-            next_id: Cell::new(1037),
             trust_anchor: TrustAnchor::default() }
   }
 
@@ -59,7 +58,6 @@ impl<C: ClientConnection> Client<C> {
   ///                    root public_key.
   pub fn with_trust_anchor(client_connection: C, trust_anchor: TrustAnchor) -> Client<C> {
     Client{ client_connection: RefCell::new(client_connection),
-            next_id: Cell::new(1037),
             trust_anchor: trust_anchor }
   }
 
@@ -494,7 +492,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    let id = self.next_id();
+    let id: u16 = rand::random();
     // TODO make recursion a parameter
     message.id(id).message_type(MessageType::Query).op_code(OpCode::Query).recursion_desired(true);
 
@@ -566,7 +564,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     let mut prerequisite = Record::with(record.get_name().clone(), record.get_rr_type(), 0);
@@ -642,7 +640,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     if must_exist {
@@ -729,7 +727,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     // make sure the record is what is expected
@@ -816,7 +814,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     // the class must be none for delete
@@ -893,7 +891,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     // the class must be none for an rrset delete
@@ -962,7 +960,7 @@ impl<C: ClientConnection> Client<C> {
 
     // build the message
     let mut message: Message = Message::new();
-    message.id(self.next_id()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
+    message.id(rand::random()).message_type(MessageType::Query).op_code(OpCode::Update).recursion_desired(false);
     message.add_zone(zone);
 
     // the TTL shoudl be 0
@@ -1018,13 +1016,6 @@ impl<C: ClientConnection> Client<C> {
 
     Ok(response)
   }
-
-  /// increments the next_id for use in messages
-  fn next_id(&self) -> u16 {
-    let id = self.next_id.get();
-    self.next_id.set(id + 1);
-    id
-  }
 }
 
 #[cfg(test)]

src/lib.rs

@@ -33,6 +33,7 @@ extern crate data_encoding;
 extern crate mio;
 extern crate openssl;
 extern crate openssl_sys;
+extern crate rand;
 extern crate rusqlite;
 extern crate rustc_serialize;
 extern crate time;

src/tcp/tcp_client_connection.rs

@@ -46,6 +46,8 @@ impl TcpClientConnection {
   ///
   /// * `name_server` - address of the name server to use for queries
   pub fn new(name_server: SocketAddr) -> ClientResult<Self> {
+    // TODO: randomize local port binding issue #23
+    //  probably not necessary for TCP...
     debug!("connecting to {:?}", name_server);
     let stream = try!(TcpStream::connect(&name_server));
 

src/udp/udp_client_connection.rs

@@ -20,6 +20,8 @@ use std::fmt;
 
 use mio::udp::UdpSocket;
 use mio::{Token, EventLoop, Handler, EventSet, PollOpt}; // not * b/c don't want confusion with std::net
+use rand::Rng;
+use rand;
 
 use ::error::*;
 use client::ClientConnection;
@@ -34,6 +36,23 @@ pub struct UdpClientConnection {
 }
 
 impl UdpClientConnection {
+  fn next_bound_local_address() -> ClientResult<UdpSocket> {
+    let mut rand = rand::thread_rng();
+
+    let mut error = Err(ClientErrorKind::Message("could not bind address in 10 tries").into());
+    for _ in 0..10 {
+      let zero_addr = ("0.0.0.0", rand.gen_range(1025_u16, u16::max_value())).to_socket_addrs().expect("could not parse 0.0.0.0 address").
+                                     next().expect("no addresses parsed from 0.0.0.0");
+
+      match UdpSocket::bound(&zero_addr) {
+        Ok(socket) => return Ok(socket),
+        Err(err) => error = Err(err.into()),
+      }
+    }
+
+    error
+  }
+
   /// Creates a new client connection.
   ///
   /// *Note* this has side affects of binding the socket to 0.0.0.0 and starting the listening
@@ -43,11 +62,9 @@ impl UdpClientConnection {
   ///
   /// * `name_server` - address of the name server to use for queries
   pub fn new(name_server: SocketAddr) -> ClientResult<Self> {
+    // TODO: allow the bind address to be specified...
     // client binds to all addresses... this shouldn't ever fail
-    let zero_addr = ("0.0.0.0", 0).to_socket_addrs().expect("could not parse 0.0.0.0 address").
-                                   next().expect("no addresses parsed from 0.0.0.0");
-
-    let socket = try!(UdpSocket::bound(&zero_addr));
+    let socket = try!(Self::next_bound_local_address());
     let mut event_loop: EventLoop<Response> = try!(EventLoop::new());
     // TODO make the timeout configurable, 5 seconds is the dig default
     // TODO the error is private to mio, which makes this awkward...