Repository: pion/dtls
Pull request #237 (2020-03-27 07:19:29)

Add ConnectionState to Conn to return State
The RemoteCertificate function is now deprecated and the
PeerCertificates property of State can be used to get the peer
certificates.


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/237?src=pr&el=h1) Report
> Merging [#237](https://codecov.io/gh/pion/dtls/pull/237?src=pr&el=desc) into [master](https://codecov.io/gh/pion/dtls/commit/3caecff8298b6b091c45a1914e9bf54f3b2a7e7e&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `65.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   76.93%   77.03%   +0.09%     
==========================================
  Files          76       76              
  Lines        4076     4076              
==========================================
+ Hits         3136     3140       +4     
+ Misses        640      637       -3     
+ Partials      300      299       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `77.07% <65.00%> (+0.09%)` | :arrow_up: |
| #wasm | `71.85% <65.00%> (-0.03%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/237?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [flight4handler.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-ZmxpZ2h0NGhhbmRsZXIuZ28=) | `78.80% <37.50%> (ø)` | |
| [flight5handler.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-ZmxpZ2h0NWhhbmRsZXIuZ28=) | `80.09% <66.66%> (ø)` | |
| [state.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-c3RhdGUuZ28=) | `68.29% <83.33%> (ø)` | |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `84.40% <100.00%> (+0.34%)` | :arrow_up: |
| [flight3handler.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-ZmxpZ2h0M2hhbmRsZXIuZ28=) | `90.65% <100.00%> (ø)` | |
| [crypto\_gcm.go](https://codecov.io/gh/pion/dtls/pull/237/diff?src=pr&el=tree#diff-Y3J5cHRvX2djbS5nbw==) | `69.23% <0.00%> (+3.84%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/237?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/237?src=pr&el=footer). Last update [3caecff...41ecacd](https://codecov.io/gh/pion/dtls/pull/237?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

daenney: I think this makes sense. It brings our API a bit closer to crypto/tls, and the naming is more correct. RemoteCertificate() suggests there's only one, but that's usually not the case.
jdbruijn: Yeah, just a little step at a time :wink: 
I'm not sure what wyh there is a patch diff. I don't think this should have changed anything in coverage.
at-wat: >  I'm not sure what wyh there is a patch diff. I don't think this should have changed anything in coverage.

Patch coverage is a test coverage of the edited lines in the PR.
If-statements tested only one condition is treated as partially covered (not counted as hit).
jdbruijn: Yeah, but my changes (mosty rename of `remoteCertificate` to `PeerCertificates` doesn't change anything in whether or not a branch of an if is triggered.

Let's take `flight4handler` line 31.
- Previous commit doesn't hit it: https://codecov.io/gh/pion/dtls/src/3caecff8298b6b091c45a1914e9bf54f3b2a7e7e/flight4handler.go#L31
- This commit also doesn't hit it: https://codecov.io/gh/pion/dtls/commit/41ecacd6288b464ad44c0679c6a3944b4806b4a5#D2-31

I understand what the patch coverage represents, but don't get why it wouldnt be a higher percentage. Maybe I'm just not seeing it.
jdbruijn: This is now rebased on master to include #238 and I'm returning the cloned state, thanks again for refactoring that @at-wat .

As discussed I've tried removing the Export (https://github.com/pion/dtls/blob/refactor/peer-certificates/resume.go#L9), but I'm a bit overwhelmed with the test output errors I got from that.

I saw the only usage was in [resume_test](https://github.com/pion/dtls/blob/refactor/peer-certificates/resume_test.go#L114), so I replaced that with `ConnectionState` and tried to add `local.Close()` as well, but still got a some errors. I honestly don't know where to start to figure out what is going happening here, other than it has to do with `DoTestResume`.

@at-wat could you please help me figure out what is going on here?

```
$ CGO_ENABLED=0 go test
--- FAIL: TestResumeClient (10.41s)
    resume_test.go:53: EOF
    util.go:38: Unexpected routines on test end: 
        goroutine 40619 [chan send]:
        github.com/pion/dtls/v2.DoTestResume.func2(0x6fae08, 0xc00015ee40, 0xc00009a000, 0xc000056900)
                /home/jdebruijn/projects/dtls/resume_test.go:82 +0x1ee
        created by github.com/pion/dtls/v2.DoTestResume
                /home/jdebruijn/projects/dtls/resume_test.go:61 +0x393
--- FAIL: TestResumeServer (10.20s)
    util.go:38: Unexpected routines on test startup: 
        goroutine 40619 [chan send]:
        github.com/pion/dtls/v2.DoTestResume.func2(0x6fae08, 0xc00015ee40, 0xc00009a000, 0xc000056900)
                /home/jdebruijn/projects/dtls/resume_test.go:82 +0x1ee
        created by github.com/pion/dtls/v2.DoTestResume
                /home/jdebruijn/projects/dtls/resume_test.go:61 +0x393
FAIL
exit status 1
FAIL    github.com/pion/dtls/v2 32.005s
```

This is the diff I used to create above test output.
```diff
+++ b/resume.go
@@ -5,13 +5,7 @@ import (
        "net"
 )
 
-// Export extracts dtls state and inner connection from an already handshaked dtls conn
-func (c *Conn) Export() (*State, net.Conn, error) {
-       state := c.state.clone()
-       return state, c.nextConn.Conn(), nil
-}
-
-// Resume imports an already stablished dtls connection using a specific dtls state
+// Resume imports an already established dtls connection using a specific dtls state
 func Resume(state *State, conn net.Conn, config *Config) (*Conn, error) {
        if err := state.initCipherSuite(); err != nil {
                return nil, err
diff --git a/resume_test.go b/resume_test.go
index 81c5a0c..b207124 100644
--- a/resume_test.go
+++ b/resume_test.go
@@ -108,18 +108,14 @@ func DoTestResume(t *testing.T, newLocal, newRemote func(net.Conn, *Config) (*Co
                fatal(t, errChan, fmt.Errorf("messages missmatch: %s != %s", message, recv[:n]))
        }
 
-       // Export dtls connection
-       var state *State
+       // // Export dtls connection
        var innerConn net.Conn
-       state, innerConn, err = local.Export()
-       if err != nil {
-               fatal(t, errChan, err)
-       }
        if err = innerConn.Close(); err != nil {
                fatal(t, errChan, err)
        }
 
        // Serialize and deserialize state
+       state := local.ConnectionState()
        var b []byte
        b, err = state.MarshalBinary()
        if err != nil {
```
at-wat: ```diff
diff --git a/resume_test.go b/resume_test.go
index 81c5a0c..e72aad9 100644
--- a/resume_test.go
+++ b/resume_test.go
@@ -108,18 +108,12 @@ func DoTestResume(t *testing.T, newLocal, newRemote func(net.Conn, *Config) (*Co
                fatal(t, errChan, fmt.Errorf("messages missmatch: %s != %s", message, recv[:n]))
        }
 
-       // Export dtls connection
-       var state *State
-       var innerConn net.Conn
-       state, innerConn, err = local.Export()
-       if err != nil {
-               fatal(t, errChan, err)
-       }
-       if err = innerConn.Close(); err != nil {
+       if err = localConn1.Close(); err != nil {
                fatal(t, errChan, err)
        }
 
        // Serialize and deserialize state
+       state := local.ConnectionState()
        var b []byte
        b, err = state.MarshalBinary()
        if err != nil {
```
should work.
jdbruijn: Thanks! pushed that,

What do I need to do about the codecov/patch issue?
at-wat: Patch coverage state is for reference. No problem for this PR.
igolaizola: Do you mind not merging this until monday? I have some projects depending on the current `Resume` `Export` APIs that I would like to review to give you some feedback.
jdbruijn: Just as a reference: see #238 for discussion on removal/renaming of `Export`.
igolaizola: > Just as a reference: see #238 for discussion on removal/renaming of `Export`.

Following the discussion...

Previous implementation of `Export` (now `ConnectionState`) returned inner `net.Conn`. This was useful for example to close the inner connection (UDP or other) and save the current DLTS state at the same time without closing the DTLS part. However, this can still be done even if `ConnectionState` doesn't return the inner connection. The caller just needs to maintain a reference to this inner `net.Conn` instead of depending on `Export` returning it.
The only inconvenient will be if the caller uses `dtls.Dial` because he won't have access to the inner connection. This can be easily fixed using `dtls.Client` instead and creating the UDP connection outside.

In short, the change looks good to me.
daenney: Thanks for the extensive review on the API and the consequences of the changes @igolaizola, it's much appreciated. Good to know we can move forward.
jdbruijn: > Could you move ExportKeyingMaterial from *Conn to *State?
Exposing other members of State will not break API, but moving ExportKeyingMaterial breaks, so, it should be done before v2.0.0 release.

@at-wat I believe this is the last piece open in this PR. I feel like a complete seperate change so propose I create a sperate PR for that once I've merged this PR. I can work on it right away when merged and doesn't look like too much work so can get it done today!
jdbruijn: If you're okay with that and the changes in this PR, I'll merged it and will create a PR for moving `ExportKeyingMaterial`.

Patch:
diff --git a/conn.go b/conn.go
index cdbea90..bf503b9 100644
--- a/conn.go
+++ b/conn.go
@@ -335,11 +335,12 @@ func (c *Conn) Close() error {
 	return err
 }
 
-// RemoteCertificate exposes the remote certificate
-func (c *Conn) RemoteCertificate() [][]byte {
+// ConnectionState returns basic DTLS details about the connection.
+// Note that this replaced the `Export` function of v1.
+func (c *Conn) ConnectionState() State {
 	c.lock.RLock()
 	defer c.lock.RUnlock()
-	return c.state.remoteCertificate
+	return *c.state.clone()
 }
 
 // SelectedSRTPProtectionProfile returns the selected SRTPProtectionProfile
diff --git a/examples/listen/psk/main.go b/examples/listen/psk/main.go
index d3a6b02..72a6c23 100644
--- a/examples/listen/psk/main.go
+++ b/examples/listen/psk/main.go
@@ -58,7 +58,7 @@ func main() {
 
 			// `conn` is of type `net.Conn` but may be casted to `dtls.Conn`
 			// using `dtlsConn := conn.(*dtls.Conn)` in order to to expose
-			// functions like `RemoteCertificate` etc.
+			// functions like `ConnectionState` etc.
 
 			// Register the connection with the chat hub
 			if err == nil {
diff --git a/examples/listen/selfsign/main.go b/examples/listen/selfsign/main.go
index 1d812be..2df4ac4 100644
--- a/examples/listen/selfsign/main.go
+++ b/examples/listen/selfsign/main.go
@@ -59,7 +59,7 @@ func main() {
 
 			// `conn` is of type `net.Conn` but may be casted to `dtls.Conn`
 			// using `dtlsConn := conn.(*dtls.Conn)` in order to to expose
-			// functions like `RemoteCertificate` etc.
+			// functions like `ConnectionState` etc.
 
 			// Register the connection with the chat hub
 			if err == nil {
diff --git a/examples/listen/verify/main.go b/examples/listen/verify/main.go
index 3eb9109..96f5c0d 100644
--- a/examples/listen/verify/main.go
+++ b/examples/listen/verify/main.go
@@ -68,7 +68,7 @@ func main() {
 
 			// `conn` is of type `net.Conn` but may be casted to `dtls.Conn`
 			// using `dtlsConn := conn.(*dtls.Conn)` in order to to expose
-			// functions like `RemoteCertificate` etc.
+			// functions like `ConnectionState` etc.
 
 			// Register the connection with the chat hub
 			hub.Register(conn)
diff --git a/flight3handler.go b/flight3handler.go
index 8949dd9..fda824a 100644
--- a/flight3handler.go
+++ b/flight3handler.go
@@ -64,7 +64,7 @@ func flight3Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 	}
 
 	if h, ok := msgs[handshakeTypeCertificate].(*handshakeMessageCertificate); ok {
-		state.remoteCertificate = h.certificate
+		state.PeerCertificates = h.certificate
 	}
 
 	if h, ok := msgs[handshakeTypeServerKeyExchange].(*handshakeMessageServerKeyExchange); ok {
diff --git a/flight4handler.go b/flight4handler.go
index d10744a..5149f6d 100644
--- a/flight4handler.go
+++ b/flight4handler.go
@@ -23,11 +23,11 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 	}
 
 	if h, hasCert := msgs[handshakeTypeCertificate].(*handshakeMessageCertificate); hasCert {
-		state.remoteCertificate = h.certificate
+		state.PeerCertificates = h.certificate
 	}
 
 	if h, hasCertVerify := msgs[handshakeTypeCertificateVerify].(*handshakeMessageCertificateVerify); hasCertVerify {
-		if state.remoteCertificate == nil {
+		if state.PeerCertificates == nil {
 			return 0, &alert{alertLevelFatal, alertNoCertificate}, errCertificateVerifyNoCertificate
 		}
 
@@ -54,24 +54,24 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 			return 0, &alert{alertLevelFatal, alertInsufficientSecurity}, errNoAvailableSignatureSchemes
 		}
 
-		if err := verifyCertificateVerify(plainText, h.hashAlgorithm, h.signature, state.remoteCertificate); err != nil {
+		if err := verifyCertificateVerify(plainText, h.hashAlgorithm, h.signature, state.PeerCertificates); err != nil {
 			return 0, &alert{alertLevelFatal, alertBadCertificate}, err
 		}
 		var chains [][]*x509.Certificate
 		var err error
 		var verified bool
 		if cfg.clientAuth >= VerifyClientCertIfGiven {
-			if chains, err = verifyClientCert(state.remoteCertificate, cfg.clientCAs); err != nil {
+			if chains, err = verifyClientCert(state.PeerCertificates, cfg.clientCAs); err != nil {
 				return 0, &alert{alertLevelFatal, alertBadCertificate}, err
 			}
 			verified = true
 		}
 		if cfg.verifyPeerCertificate != nil {
-			if err := cfg.verifyPeerCertificate(state.remoteCertificate, chains); err != nil {
+			if err := cfg.verifyPeerCertificate(state.PeerCertificates, chains); err != nil {
 				return 0, &alert{alertLevelFatal, alertBadCertificate}, err
 			}
 		}
-		state.remoteCertificateVerified = verified
+		state.peerCertificatesVerified = verified
 	}
 
 	if !state.cipherSuite.isInitialized() {
@@ -137,18 +137,18 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 
 	switch cfg.clientAuth {
 	case RequireAnyClientCert:
-		if state.remoteCertificate == nil {
+		if state.PeerCertificates == nil {
 			return 0, &alert{alertLevelFatal, alertNoCertificate}, errClientCertificateRequired
 		}
 	case VerifyClientCertIfGiven:
-		if state.remoteCertificate != nil && !state.remoteCertificateVerified {
+		if state.PeerCertificates != nil && !state.peerCertificatesVerified {
 			return 0, &alert{alertLevelFatal, alertBadCertificate}, errClientCertificateNotVerified
 		}
 	case RequireAndVerifyClientCert:
-		if state.remoteCertificate == nil {
+		if state.PeerCertificates == nil {
 			return 0, &alert{alertLevelFatal, alertNoCertificate}, errClientCertificateRequired
 		}
-		if !state.remoteCertificateVerified {
+		if !state.peerCertificatesVerified {
 			return 0, &alert{alertLevelFatal, alertBadCertificate}, errClientCertificateNotVerified
 		}
 	}
diff --git a/flight5handler.go b/flight5handler.go
index 9816f5e..8ee63ba 100644
--- a/flight5handler.go
+++ b/flight5handler.go
@@ -287,17 +287,17 @@ func initalizeCipherSuite(state *State, cache *handshakeCache, cfg *handshakeCon
 		}
 
 		expectedMsg := valueKeyMessage(clientRandom[:], serverRandom[:], h.publicKey, h.namedCurve)
-		if err = verifyKeySignature(expectedMsg, h.signature, h.hashAlgorithm, state.remoteCertificate); err != nil {
+		if err = verifyKeySignature(expectedMsg, h.signature, h.hashAlgorithm, state.PeerCertificates); err != nil {
 			return &alert{alertLevelFatal, alertBadCertificate}, err
 		}
 		var chains [][]*x509.Certificate
 		if !cfg.insecureSkipVerify {
-			if chains, err = verifyServerCert(state.remoteCertificate, cfg.rootCAs, cfg.serverName); err != nil {
+			if chains, err = verifyServerCert(state.PeerCertificates, cfg.rootCAs, cfg.serverName); err != nil {
 				return &alert{alertLevelFatal, alertBadCertificate}, err
 			}
 		}
 		if cfg.verifyPeerCertificate != nil {
-			if err = cfg.verifyPeerCertificate(state.remoteCertificate, chains); err != nil {
+			if err = cfg.verifyPeerCertificate(state.PeerCertificates, chains); err != nil {
 				return &alert{alertLevelFatal, alertBadCertificate}, err
 			}
 		}
diff --git a/resume.go b/resume.go
index d2b50ca..40e55e4 100644
--- a/resume.go
+++ b/resume.go
@@ -5,13 +5,7 @@ import (
 	"net"
 )
 
-// Export extracts dtls state and inner connection from an already handshaked dtls conn
-func (c *Conn) Export() (*State, net.Conn, error) {
-	state := c.state.clone()
-	return state, c.nextConn.Conn(), nil
-}
-
-// Resume imports an already stablished dtls connection using a specific dtls state
+// Resume imports an already established dtls connection using a specific dtls state
 func Resume(state *State, conn net.Conn, config *Config) (*Conn, error) {
 	if err := state.initCipherSuite(); err != nil {
 		return nil, err
diff --git a/state.go b/state.go
index 0a1ccf6..0c7abb0 100644
--- a/state.go
+++ b/state.go
@@ -17,7 +17,7 @@ type State struct {
 	cipherSuite               cipherSuite // nil if a cipherSuite hasn't been chosen
 
 	srtpProtectionProfile SRTPProtectionProfile // Negotiated SRTPProtectionProfile
-	remoteCertificate     [][]byte
+	PeerCertificates      [][]byte
 
 	isClient bool
 
@@ -34,7 +34,7 @@ type State struct {
 	localCertificatesVerify    []byte // cache CertificateVerify
 	localVerifyData            []byte // cached VerifyData
 	localKeySignature          []byte // cached keySignature
-	remoteCertificateVerified  bool
+	peerCertificatesVerified   bool
 
 	replayDetector []replaydetector.ReplayDetector
 }
@@ -48,7 +48,7 @@ type serializedState struct {
 	MasterSecret          []byte
 	SequenceNumber        uint64
 	SRTPProtectionProfile uint16
-	RemoteCertificate     [][]byte
+	PeerCertificates      [][]byte
 	IsClient              bool
 }
 
@@ -75,7 +75,7 @@ func (s *State) serialize() *serializedState {
 		LocalRandom:           localRnd,
 		RemoteRandom:          remoteRnd,
 		SRTPProtectionProfile: uint16(s.srtpProtectionProfile),
-		RemoteCertificate:     s.remoteCertificate,
+		PeerCertificates:      s.PeerCertificates,
 		IsClient:              s.isClient,
 	}
 }
@@ -111,7 +111,7 @@ func (s *State) deserialize(serialized serializedState) {
 	s.srtpProtectionProfile = SRTPProtectionProfile(serialized.SRTPProtectionProfile)
 
 	// Set remote certificate
-	s.remoteCertificate = serialized.RemoteCertificate
+	s.PeerCertificates = serialized.PeerCertificates
 }
 
 func (s *State) initCipherSuite() error {


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 82916e2..b764500 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -810,7 +810,8 @@ func TestClientCertificate(t *testing.T) {
 				if res.err != nil {
 					t.Errorf("Client failed(%v)", res.err)
 				}
-				actualClientCert := server.RemoteCertificate()
+
+				actualClientCert := server.ConnectionState().PeerCertificates
 				if tt.serverCfg.ClientAuth == RequireAnyClientCert || tt.serverCfg.ClientAuth == RequireAndVerifyClientCert {
 					if actualClientCert == nil {
 						t.Errorf("Client did not provide a certificate")
@@ -826,7 +827,7 @@ func TestClientCertificate(t *testing.T) {
 					}
 				}
 
-				actualServerCert := res.c.RemoteCertificate()
+				actualServerCert := res.c.ConnectionState().PeerCertificates
 				if actualServerCert == nil {
 					t.Errorf("Server did not provide a certificate")
 				}
diff --git a/resume_test.go b/resume_test.go
index 81c5a0c..e72aad9 100644
--- a/resume_test.go
+++ b/resume_test.go
@@ -108,18 +108,12 @@ func DoTestResume(t *testing.T, newLocal, newRemote func(net.Conn, *Config) (*Co
 		fatal(t, errChan, fmt.Errorf("messages missmatch: %s != %s", message, recv[:n]))
 	}
 
-	// Export dtls connection
-	var state *State
-	var innerConn net.Conn
-	state, innerConn, err = local.Export()
-	if err != nil {
-		fatal(t, errChan, err)
-	}
-	if err = innerConn.Close(); err != nil {
+	if err = localConn1.Close(); err != nil {
 		fatal(t, errChan, err)
 	}
 
 	// Serialize and deserialize state
+	state := local.ConnectionState()
 	var b []byte
 	b, err = state.MarshalBinary()
 	if err != nil {