From 4745431cdac9e68ffe66e617ce205a63e360320d Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@nym.se>
Date: Thu, 5 Mar 2015 15:58:16 +0100
Subject: [PATCH 1/2] Verify negotiated protocol bep/1.0

---
 cmd/syncthing/connections.go | 9 ++++++++-
 cmd/syncthing/main.go        | 4 +++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/cmd/syncthing/connections.go b/cmd/syncthing/connections.go
index 745d68dc..24bc90b5 100644
--- a/cmd/syncthing/connections.go
+++ b/cmd/syncthing/connections.go
@@ -41,7 +41,14 @@ func listenConnect(myID protocol.DeviceID, m *model.Model, tlsCfg *tls.Config) {
 
 next:
 	for conn := range conns {
-		certs := conn.ConnectionState().PeerCertificates
+		cs := conn.ConnectionState()
+		if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != bepProtocolName {
+			l.Infof("Peer %s did not negotiate bep/1.0", conn.RemoteAddr())
+			conn.Close()
+			continue
+		}
+
+		certs := cs.PeerCertificates
 		if cl := len(certs); cl != 1 {
 			l.Infof("Got peer certificate list of length %d != 1 from %s; protocol error", cl, conn.RemoteAddr())
 			conn.Close()
diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go
index ed6c5a2b..08d53236 100644
--- a/cmd/syncthing/main.go
+++ b/cmd/syncthing/main.go
@@ -72,6 +72,8 @@ const (
 	exitUpgrading          = 4
 )
 
+const bepProtocolName = "bep/1.0"
+
 var l = logger.DefaultLogger
 
 func init() {
@@ -461,7 +463,7 @@ func syncthingMain() {
 
 	tlsCfg := &tls.Config{
 		Certificates:           []tls.Certificate{cert},
-		NextProtos:             []string{"bep/1.0"},
+		NextProtos:             []string{bepProtocolName},
 		ClientAuth:             tls.RequestClientCert,
 		SessionTicketsDisabled: true,
 		InsecureSkipVerify:     true,

From aaaa6556f3be3d4926cc9d57918b8c9e1379eac8 Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@nym.se>
Date: Thu, 5 Mar 2015 16:09:20 +0100
Subject: [PATCH 2/2] Some commentary on the initial connection checks

---
 cmd/syncthing/connections.go | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/cmd/syncthing/connections.go b/cmd/syncthing/connections.go
index 24bc90b5..eb7c402e 100644
--- a/cmd/syncthing/connections.go
+++ b/cmd/syncthing/connections.go
@@ -42,12 +42,19 @@ func listenConnect(myID protocol.DeviceID, m *model.Model, tlsCfg *tls.Config) {
 next:
 	for conn := range conns {
 		cs := conn.ConnectionState()
+
+		// We should have negotiated the next level protocol "bep/1.0" as part
+		// of the TLS handshake. If we didn't, we're not speaking to another
+		// BEP-speaker so drop the connection.
 		if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != bepProtocolName {
 			l.Infof("Peer %s did not negotiate bep/1.0", conn.RemoteAddr())
 			conn.Close()
 			continue
 		}
 
+		// We should have received exactly one certificate from the other
+		// side. If we didn't, they don't have a device ID and we drop the
+		// connection.
 		certs := cs.PeerCertificates
 		if cl := len(certs); cl != 1 {
 			l.Infof("Got peer certificate list of length %d != 1 from %s; protocol error", cl, conn.RemoteAddr())
@@ -57,12 +64,21 @@ next:
 		remoteCert := certs[0]
 		remoteID := protocol.NewDeviceID(remoteCert.Raw)
 
+		// The device ID should not be that of ourselves. It can happen
+		// though, especially in the presense of NAT hairpinning, multiple
+		// clients between the same NAT gateway, and global discovery.
 		if remoteID == myID {
 			l.Infof("Connected to myself (%s) - should not happen", remoteID)
 			conn.Close()
 			continue
 		}
 
+		// We should not already be connected to the other party. TODO: This
+		// could use some better handling. If the old connection is dead but
+		// hasn't timed out yet we may want to drop *that* connection and keep
+		// this one. But in case we are two devices connecting to each other
+		// in parallell we don't want to do that or we end up with no
+		// connections still established...
 		if m.ConnectedTo(remoteID) {
 			l.Infof("Connected to already connected device (%s)", remoteID)
 			conn.Close()