Repository: pion/dtls
Pull request #115 (2019-08-29 07:04:15)

Fix signature verification not being called
`InsecureSkipVerify` should not skip the signature verification

#### Description

`InsecureSkipVerify` should only skip the verification of certificate properties such as server name, expiration, matching CAs... but not the signature verification.

In addition crypto/tls also seems to do it this way:
https://github.com/golang/go/blob/master/src/crypto/tls/handshake_client.go#L823


codecov-io: # [Codecov](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=h1) Report
> Merging [#115](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=desc) into [master](https://codecov.io/gh/pion/dtls/commit/0e96d71518834b9045215ba50b49bab074c5d040?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `20%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/dtls/pull/115/graphs/tree.svg?width=650&token=bcCB2jB88I&height=150&src=pr)](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   72.41%   72.41%           
=======================================
  Files          53       53           
  Lines        3070     3070           
=======================================
  Hits         2223     2223           
  Misses        597      597           
  Partials      250      250
```


| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [server\_handlers.go](https://codecov.io/gh/pion/dtls/pull/115/diff?src=pr&el=tree#diff-c2VydmVyX2hhbmRsZXJzLmdv) | `84.91% <0%> (ø)` | :arrow_up: |
| [client\_handlers.go](https://codecov.io/gh/pion/dtls/pull/115/diff?src=pr&el=tree#diff-Y2xpZW50X2hhbmRsZXJzLmdv) | `82.36% <33.33%> (ø)` | :arrow_up: |
| [internal/udp/conn.go](https://codecov.io/gh/pion/dtls/pull/115/diff?src=pr&el=tree#diff-aW50ZXJuYWwvdWRwL2Nvbm4uZ28=) | `75.45% <0%> (-1.82%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/115/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `80.36% <0%> (+0.6%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=footer). Last update [0e96d71...d7e053d](https://codecov.io/gh/pion/dtls/pull/115?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/client_handlers.go b/client_handlers.go
index fbe2d3c..7311714 100644
--- a/client_handlers.go
+++ b/client_handlers.go
@@ -38,11 +38,11 @@ func initalizeCipherSuite(c *Conn, h *handshakeMessageServerKeyExchange) (*alert
 	}
 
 	if c.localPSKCallback == nil {
+		expectedHash := valueKeySignature(clientRandom, serverRandom, h.publicKey, h.namedCurve, h.hashAlgorithm)
+		if err := verifyKeySignature(expectedHash, h.signature, h.hashAlgorithm, c.state.remoteCertificate); err != nil {
+			return &alert{alertLevelFatal, alertBadCertificate}, err
+		}
 		if !c.insecureSkipVerify {
-			expectedHash := valueKeySignature(clientRandom, serverRandom, h.publicKey, h.namedCurve, h.hashAlgorithm)
-			if err := verifyKeySignature(expectedHash, h.signature, h.hashAlgorithm, c.state.remoteCertificate); err != nil {
-				return &alert{alertLevelFatal, alertBadCertificate}, err
-			}
 			if err := verifyServerCert(c.state.remoteCertificate, c.rootCAs, c.serverName); err != nil {
 				return &alert{alertLevelFatal, alertBadCertificate}, err
 			}
diff --git a/examples/listen-psk/main.go b/examples/listen-psk/main.go
index 308a701..e1955a8 100644
--- a/examples/listen-psk/main.go
+++ b/examples/listen-psk/main.go
@@ -33,7 +33,7 @@ func main() {
 	listener, err := dtls.Listen("udp", addr, config)
 	util.Check(err)
 	defer func() {
-		util.Check(listener.Close())
+		util.Check(listener.Close(5 * time.Second))
 	}()
 
 	fmt.Println("Listening")
diff --git a/examples/listen/main.go b/examples/listen/main.go
index cef8baf..ebe2760 100644
--- a/examples/listen/main.go
+++ b/examples/listen/main.go
@@ -33,7 +33,7 @@ func main() {
 	listener, err := dtls.Listen("udp", addr, config)
 	util.Check(err)
 	defer func() {
-		util.Check(listener.Close())
+		util.Check(listener.Close(5 * time.Second))
 	}()
 
 	fmt.Println("Listening")
diff --git a/internal/udp/conn.go b/internal/udp/conn.go
index d94d325..bbf0844 100644
--- a/internal/udp/conn.go
+++ b/internal/udp/conn.go
@@ -9,6 +9,7 @@ import (
 )
 
 const receiveMTU = 8192
+const closeRetryInterval = 100 * time.Millisecond
 
 var errClosedListener = errors.New("udp: listener closed")
 
@@ -39,7 +40,7 @@ func (l *Listener) Accept() (*Conn, error) {
 
 // Close closes the listener.
 // Any blocked Accept operations will be unblocked and return errors.
-func (l *Listener) Close() error {
+func (l *Listener) Close(shutdownTimeout time.Duration) error {
 	l.lock.Lock()
 	defer l.lock.Unlock()
 
@@ -47,12 +48,26 @@ func (l *Listener) Close() error {
 	l.doneOnce.Do(func() {
 		l.accepting = false
 		close(l.doneCh)
-		err = l.cleanup()
+		err = l.cleanupWithTimeout(shutdownTimeout)
 	})
 
 	return err
 }
 
+func (l *Listener) cleanupWithTimeout(shutdownTimeout time.Duration) error {
+	timeoutTimer := time.NewTimer(shutdownTimeout)
+	for {
+		select {
+		case <-time.After(closeRetryInterval):
+			if len(l.conns) == 0 {
+				return l.cleanup()
+			}
+		case <-timeoutTimer.C:
+			return l.cleanup()
+		}
+	}
+}
+
 // cleanup closes the packet conn if it is no longer used
 // The caller should hold the read lock.
 func (l *Listener) cleanup() error {
diff --git a/listener.go b/listener.go
index 0a4a4a1..e36719d 100644
--- a/listener.go
+++ b/listener.go
@@ -3,6 +3,7 @@ package dtls
 import (
 	"errors"
 	"net"
+	"time"
 
 	"github.com/pion/dtls/internal/udp"
 )
@@ -41,8 +42,8 @@ func (l *Listener) Accept() (net.Conn, error) {
 // Close closes the listener.
 // Any blocked Accept operations will be unblocked and return errors.
 // Already Accepted connections are not closed.
-func (l *Listener) Close() error {
-	return l.parent.Close()
+func (l *Listener) Close(shutdownTimeout time.Duration) error {
+	return l.parent.Close(shutdownTimeout)
 }
 
 // Addr returns the listener's network address.
diff --git a/server_handlers.go b/server_handlers.go
index 5688405..3a53925 100644
--- a/server_handlers.go
+++ b/server_handlers.go
@@ -80,17 +80,15 @@ func serverHandshakeHandler(c *Conn) (*alert, error) {
 				handshakeCachePullRule{handshakeTypeClientKeyExchange, true},
 			)
 
+			if err := verifyCertificateVerify(plainText, h.hashAlgorithm, h.signature, c.state.remoteCertificate); err != nil {
+				return &alert{alertLevelFatal, alertBadCertificate}, err
+			}
 			verified := false
-			if !c.insecureSkipVerify && c.clientAuth >= RequireAnyClientCert {
-				if err := verifyCertificateVerify(plainText, h.hashAlgorithm, h.signature, c.state.remoteCertificate); err != nil {
+			if !c.insecureSkipVerify && c.clientAuth >= VerifyClientCertIfGiven {
+				if err := verifyClientCert(c.state.remoteCertificate, c.rootCAs); err != nil {
 					return &alert{alertLevelFatal, alertBadCertificate}, err
 				}
-				if c.clientAuth >= VerifyClientCertIfGiven {
-					if err := verifyClientCert(c.state.remoteCertificate, c.rootCAs); err != nil {
-						return &alert{alertLevelFatal, alertBadCertificate}, err
-					}
-					verified = true
-				}
+				verified = true
 			}
 			if c.verifyPeerCertificate != nil {
 				if err := c.verifyPeerCertificate(c.state.remoteCertificate, verified); err != nil {


Test patch:
diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go
index 4ef45b7..fdc6edd 100644
--- a/e2e/e2e_test.go
+++ b/e2e/e2e_test.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"io"
 	"net"
+	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -34,7 +35,7 @@ func randomPort(t testing.TB) int {
 	}
 }
 
-func simpleReadWrite(errChan chan error, outChan chan string, conn io.ReadWriteCloser, listener io.Closer, messageRecvCount *uint64) {
+func simpleReadWrite(errChan chan error, outChan chan string, conn io.ReadWriter, messageRecvCount *uint64) {
 	go func() {
 		buffer := make([]byte, 8192)
 		n, err := conn.Read(buffer)
@@ -57,27 +58,19 @@ func simpleReadWrite(errChan chan error, outChan chan string, conn io.ReadWriteC
 
 		time.Sleep(messageRetry)
 	}
-
-	maybePushError := func(err error) {
-		select {
-		case errChan <- err: // Do we care about these errors?
-		default:
-		}
-	}
-
-	if listener != nil {
-		maybePushError(listener.Close())
-	}
-	maybePushError(conn.Close())
 }
 
-func assertE2ECommunication(clientConfig, serverConfig *dtls.Config, t *testing.T) {
+func assertE2ECommunication(clientConfig, serverConfig *dtls.Config, serverPort int, t *testing.T) {
 	errChan := make(chan error)
 	clientChan := make(chan string)
 	serverChan := make(chan string)
 	var messageRecvCount uint64 // Counter to make sure both sides got a message
+	var clientMutex sync.Mutex
+	var clientConn net.Conn
+	var serverMutex sync.Mutex
+	var serverConn net.Conn
+	var serverListener *dtls.Listener
 
-	serverPort := randomPort(t)
 	serverReady := make(chan struct{})
 
 	// DTLS Client
@@ -88,7 +81,12 @@ func assertE2ECommunication(clientConfig, serverConfig *dtls.Config, t *testing.
 		case <-time.After(time.Second):
 			errChan <- errors.New("waiting on serverReady err: timeout")
 		}
-		conn, err := dtls.Dial("udp",
+
+		clientMutex.Lock()
+		defer clientMutex.Unlock()
+
+		var err error
+		clientConn, err = dtls.Dial("udp",
 			&net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: serverPort},
 			clientConfig,
 		)
@@ -96,12 +94,17 @@ func assertE2ECommunication(clientConfig, serverConfig *dtls.Config, t *testing.
 			errChan <- err
 			return
 		}
-		simpleReadWrite(errChan, clientChan, conn, nil, &messageRecvCount)
+
+		simpleReadWrite(errChan, clientChan, clientConn, &messageRecvCount)
 	}()
 
 	// DTLS Server
 	go func() {
-		listener, err := dtls.Listen("udp",
+		serverMutex.Lock()
+		defer serverMutex.Unlock()
+
+		var err error
+		serverListener, err = dtls.Listen("udp",
 			&net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: serverPort},
 			serverConfig,
 		)
@@ -110,13 +113,32 @@ func assertE2ECommunication(clientConfig, serverConfig *dtls.Config, t *testing.
 			return
 		}
 		serverReady <- struct{}{}
-		conn, err := listener.Accept()
+		serverConn, err = serverListener.Accept()
 		if err != nil {
 			errChan <- err
 			return
 		}
 
-		simpleReadWrite(errChan, serverChan, conn, listener, &messageRecvCount)
+		simpleReadWrite(errChan, serverChan, serverConn, &messageRecvCount)
+	}()
+
+	defer func() {
+		clientMutex.Lock()
+		serverMutex.Lock()
+		defer clientMutex.Unlock()
+		defer serverMutex.Unlock()
+
+		if err := clientConn.Close(); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := serverConn.Close(); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := serverListener.Close(2 * time.Second); err != nil {
+			t.Fatal(err)
+		}
 	}()
 
 	func() {
@@ -163,6 +185,8 @@ func TestPionE2ESimple(t *testing.T) {
 	report := test.CheckRoutines(t)
 	defer report()
 
+	serverPort := randomPort(t)
+
 	for _, cipherSuite := range []dtls.CipherSuiteID{
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
@@ -177,11 +201,11 @@ func TestPionE2ESimple(t *testing.T) {
 			PrivateKey:         key,
 			CipherSuites:       []dtls.CipherSuiteID{cipherSuite},
 			InsecureSkipVerify: true,
+			ConnectTimeout:     dtls.ConnectTimeoutOption(2 * time.Second),
 		}
-		assertE2ECommunication(cfg, cfg, t)
+		assertE2ECommunication(cfg, cfg, serverPort, t)
 
 	}
-
 }
 
 func TestPionE2ESimplePSK(t *testing.T) {
@@ -191,6 +215,8 @@ func TestPionE2ESimplePSK(t *testing.T) {
 	report := test.CheckRoutines(t)
 	defer report()
 
+	serverPort := randomPort(t)
+
 	for _, cipherSuite := range []dtls.CipherSuiteID{
 		dtls.TLS_PSK_WITH_AES_128_CCM_8,
 		dtls.TLS_PSK_WITH_AES_128_GCM_SHA256,
@@ -201,7 +227,8 @@ func TestPionE2ESimplePSK(t *testing.T) {
 			},
 			PSKIdentityHint: []byte{0x01, 0x02, 0x03, 0x04, 0x05},
 			CipherSuites:    []dtls.CipherSuiteID{cipherSuite},
+			ConnectTimeout:  dtls.ConnectTimeoutOption(2 * time.Second),
 		}
-		assertE2ECommunication(cfg, cfg, t)
+		assertE2ECommunication(cfg, cfg, serverPort, t)
 	}
 }
diff --git a/internal/udp/conn_test.go b/internal/udp/conn_test.go
index 11d4365..1434815 100644
--- a/internal/udp/conn_test.go
+++ b/internal/udp/conn_test.go
@@ -15,13 +15,14 @@ import (
 //	defer lim.Stop()
 //
 //	nettest.TestConn(t, func() (c1, c2 net.Conn, stop func(), err error) {
-//		c1, c2, err = pipe()
+//		listener, c1, c2, err = pipe()
 //		if err != nil {
 //			return nil, nil, nil, err
 //		}
 //		stop = func() {
 //			c1.Close()
 //			c2.Close()
+//			listener.Close(1 * time.Second)
 //		}
 //		return
 //	})
@@ -41,7 +42,7 @@ func TestStressDuplex(t *testing.T) {
 }
 
 func stressDuplex(t *testing.T) {
-	ca, cb, err := pipe()
+	listener, ca, cb, err := pipe()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -55,6 +56,10 @@ func stressDuplex(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
+		err = listener.Close(5 * time.Second)
+		if err != nil {
+			t.Fatal(err)
+		}
 	}()
 
 	opt := test.Options{
@@ -68,54 +73,74 @@ func stressDuplex(t *testing.T) {
 	}
 }
 
-func pipe() (*Conn, *net.UDPConn, error) {
+func TestListenerCloseTimeout(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	// Check for leaking routines
+	report := test.CheckRoutines(t)
+	defer report()
+
+	listener, ca, _, err := pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = listener.Close(500 * time.Millisecond)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Close client after server closes to cleanup
+	err = ca.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func pipe() (*Listener, *Conn, *net.UDPConn, error) {
 	// Start listening
 	network, addr := getConfig()
 	listener, err := Listen(network, addr)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to listen: %v", err)
+		return nil, nil, nil, fmt.Errorf("failed to listen: %v", err)
 	}
 
 	// Open a connection
 	var dConn *net.UDPConn
 	dConn, err = net.DialUDP(network, nil, listener.Addr().(*net.UDPAddr))
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to dial: %v", err)
+		return nil, nil, nil, fmt.Errorf("failed to dial: %v", err)
 	}
 
 	// Write to the connection to initiate it
 	handshake := "hello"
 	_, err = dConn.Write([]byte(handshake))
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to write to dialed Conn: %v", err)
+		return nil, nil, nil, fmt.Errorf("failed to write to dialed Conn: %v", err)
 	}
 
 	// Accept the connection
 	var lConn *Conn
 	lConn, err = listener.Accept()
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to accept Conn: %v", err)
+		return nil, nil, nil, fmt.Errorf("failed to accept Conn: %v", err)
 	}
 
 	buf := make([]byte, len(handshake))
 	n := 0
 	n, err = lConn.Read(buf)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to read handshake: %v", err)
+		return nil, nil, nil, fmt.Errorf("failed to read handshake: %v", err)
 	}
 
 	result := string(buf[:n])
 	if handshake != result {
-		return nil, nil, fmt.Errorf("handshake failed: %s != %s", handshake, result)
-	}
-
-	// Close the listener
-	err = listener.Close()
-	if err != nil {
-		return nil, nil, fmt.Errorf("failed close listener: %v", err)
+		return nil, nil, nil, fmt.Errorf("handshake failed: %s != %s", handshake, result)
 	}
 
-	return lConn, dConn, nil
+	return listener, lConn, dConn, nil
 }
 
 func getConfig() (string, *net.UDPAddr) {
@@ -126,7 +151,7 @@ func getConfig() (string, *net.UDPAddr) {
 // 	lim := test.TimeOut(time.Second * 5)
 // 	defer lim.Stop()
 //
-// 	ca, cb, err := pipe()
+// 	listener, ca, cb, err := pipe()
 // 	if err != nil {
 // 		t.Fatal(err)
 // 	}
@@ -138,4 +163,8 @@ func getConfig() (string, *net.UDPAddr) {
 // 	if err != nil {
 // 		t.Fatalf("Failed to close B side: %v\n", err)
 // 	}
+//  err = listener.Close(1 * time.Second)
+//  if err != nil {
+//  	t.Fatalf("Failed to close listener: %v\n", err)
+//  }
 // }