Repository: pion/dtls
Pull request #238 (2020-03-28 08:01:23)

Refactor State.clone()
Remove `error` from return value of `State.clone()`.
Fix `TestResume` which didn't used deserialized state.

Note: `State.MarshalBinary()` result is incompatible with the previous version.

Related to #237 


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

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

```diff
@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   76.93%   77.68%   +0.74%     
==========================================
  Files          76       76              
  Lines        4076     4046      -30     
==========================================
+ Hits         3136     3143       +7     
+ Misses        640      622      -18     
+ Partials      300      281      -19     
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `77.72% <88.33%> (+0.74%)` | :arrow_up: |
| #wasm | `72.61% <88.33%> (+0.73%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/238?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [resume.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-cmVzdW1lLmdv) | `60.00% <33.33%> (ø)` | |
| [flight5handler.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-ZmxpZ2h0NWhhbmRsZXIuZ28=) | `81.65% <83.33%> (+1.56%)` | :arrow_up: |
| [flight4handler.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-ZmxpZ2h0NGhhbmRsZXIuZ28=) | `81.90% <87.50%> (+3.10%)` | :arrow_up: |
| [state.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-c3RhdGUuZ28=) | `88.05% <89.65%> (+19.76%)` | :arrow_up: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `84.46% <100.00%> (+0.41%)` | :arrow_up: |
| [handshake\_message\_client\_hello.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-aGFuZHNoYWtlX21lc3NhZ2VfY2xpZW50X2hlbGxvLmdv) | `55.93% <100.00%> (+1.83%)` | :arrow_up: |
| [handshake\_message\_server\_hello.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-aGFuZHNoYWtlX21lc3NhZ2Vfc2VydmVyX2hlbGxvLmdv) | `54.90% <100.00%> (+2.07%)` | :arrow_up: |
| [handshake\_random.go](https://codecov.io/gh/pion/dtls/pull/238/diff?src=pr&el=tree#diff-aGFuZHNoYWtlX3JhbmRvbS5nbw==) | `90.00% <100.00%> (+1.76%)` | :arrow_up: |

------

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


Patch:
diff --git a/conn.go b/conn.go
index e054232..cdbea90 100644
--- a/conn.go
+++ b/conn.go
@@ -369,20 +369,14 @@ func (c *Conn) ExportKeyingMaterial(label string, context []byte, length int) ([
 		return nil, errReservedExportKeyingMaterial
 	}
 
-	localRandom, err := c.state.localRandom.Marshal()
-	if err != nil {
-		return nil, err
-	}
-	remoteRandom, err := c.state.remoteRandom.Marshal()
-	if err != nil {
-		return nil, err
-	}
+	localRandom := c.state.localRandom.marshalFixed()
+	remoteRandom := c.state.remoteRandom.marshalFixed()
 
 	seed := []byte(label)
 	if c.state.isClient {
-		seed = append(append(seed, localRandom...), remoteRandom...)
+		seed = append(append(seed, localRandom[:]...), remoteRandom[:]...)
 	} else {
-		seed = append(append(seed, remoteRandom...), localRandom...)
+		seed = append(append(seed, remoteRandom[:]...), localRandom[:]...)
 	}
 	return prfPHash(c.state.masterSecret, seed, length, c.state.cipherSuite.hashFunc())
 }
diff --git a/flight4handler.go b/flight4handler.go
index 19aa562..d10744a 100644
--- a/flight4handler.go
+++ b/flight4handler.go
@@ -75,15 +75,10 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 	}
 
 	if !state.cipherSuite.isInitialized() {
-		serverRandom, err := state.localRandom.Marshal()
-		if err != nil {
-			return 0, &alert{alertLevelFatal, alertInternalError}, err
-		}
-		clientRandom, err := state.remoteRandom.Marshal()
-		if err != nil {
-			return 0, &alert{alertLevelFatal, alertInternalError}, err
-		}
+		serverRandom := state.localRandom.marshalFixed()
+		clientRandom := state.remoteRandom.marshalFixed()
 
+		var err error
 		var preMasterSecret []byte
 		if cfg.localPSKCallback != nil {
 			var psk []byte
@@ -111,13 +106,13 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 				return 0, &alert{alertLevelFatal, alertInternalError}, err
 			}
 		} else {
-			state.masterSecret, err = prfMasterSecret(preMasterSecret, clientRandom, serverRandom, state.cipherSuite.hashFunc())
+			state.masterSecret, err = prfMasterSecret(preMasterSecret, clientRandom[:], serverRandom[:], state.cipherSuite.hashFunc())
 			if err != nil {
 				return 0, &alert{alertLevelFatal, alertInternalError}, err
 			}
 		}
 
-		if err := state.cipherSuite.init(state.masterSecret, clientRandom, serverRandom, false); err != nil {
+		if err := state.cipherSuite.init(state.masterSecret, clientRandom[:], serverRandom[:], false); err != nil {
 			return 0, &alert{alertLevelFatal, alertInternalError}, err
 		}
 	}
@@ -221,14 +216,8 @@ func flight4Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 			},
 		})
 
-		serverRandom, err := state.localRandom.Marshal()
-		if err != nil {
-			return nil, &alert{alertLevelFatal, alertInternalError}, err
-		}
-		clientRandom, err := state.remoteRandom.Marshal()
-		if err != nil {
-			return nil, &alert{alertLevelFatal, alertInternalError}, err
-		}
+		serverRandom := state.localRandom.marshalFixed()
+		clientRandom := state.remoteRandom.marshalFixed()
 
 		// Find compatible signature scheme
 		signatureHashAlgo, err := selectSignatureScheme(cfg.localSignatureSchemes, certificate.PrivateKey)
@@ -236,7 +225,7 @@ func flight4Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 			return nil, &alert{alertLevelFatal, alertInsufficientSecurity}, err
 		}
 
-		signature, err := generateKeySignature(clientRandom, serverRandom, state.localKeypair.publicKey, state.namedCurve, certificate.PrivateKey, signatureHashAlgo.hash)
+		signature, err := generateKeySignature(clientRandom[:], serverRandom[:], state.localKeypair.publicKey, state.namedCurve, certificate.PrivateKey, signatureHashAlgo.hash)
 		if err != nil {
 			return nil, &alert{alertLevelFatal, alertInternalError}, err
 		}
diff --git a/flight5handler.go b/flight5handler.go
index cf286fa..9816f5e 100644
--- a/flight5handler.go
+++ b/flight5handler.go
@@ -250,14 +250,10 @@ func initalizeCipherSuite(state *State, cache *handshakeCache, cfg *handshakeCon
 		return nil, nil
 	}
 
-	clientRandom, err := state.localRandom.Marshal()
-	if err != nil {
-		return &alert{alertLevelFatal, alertInternalError}, err
-	}
-	serverRandom, err := state.remoteRandom.Marshal()
-	if err != nil {
-		return &alert{alertLevelFatal, alertInternalError}, err
-	}
+	clientRandom := state.localRandom.marshalFixed()
+	serverRandom := state.remoteRandom.marshalFixed()
+
+	var err error
 
 	if state.extendedMasterSecret {
 		var sessionHash []byte
@@ -271,7 +267,7 @@ func initalizeCipherSuite(state *State, cache *handshakeCache, cfg *handshakeCon
 			return &alert{alertLevelFatal, alertIllegalParameter}, err
 		}
 	} else {
-		state.masterSecret, err = prfMasterSecret(state.preMasterSecret, clientRandom, serverRandom, state.cipherSuite.hashFunc())
+		state.masterSecret, err = prfMasterSecret(state.preMasterSecret, clientRandom[:], serverRandom[:], state.cipherSuite.hashFunc())
 		if err != nil {
 			return &alert{alertLevelFatal, alertInternalError}, err
 		}
@@ -290,7 +286,7 @@ func initalizeCipherSuite(state *State, cache *handshakeCache, cfg *handshakeCon
 			return &alert{alertLevelFatal, alertInsufficientSecurity}, errNoAvailableSignatureSchemes
 		}
 
-		expectedMsg := valueKeyMessage(clientRandom, serverRandom, h.publicKey, h.namedCurve)
+		expectedMsg := valueKeyMessage(clientRandom[:], serverRandom[:], h.publicKey, h.namedCurve)
 		if err = verifyKeySignature(expectedMsg, h.signature, h.hashAlgorithm, state.remoteCertificate); err != nil {
 			return &alert{alertLevelFatal, alertBadCertificate}, err
 		}
@@ -307,7 +303,7 @@ func initalizeCipherSuite(state *State, cache *handshakeCache, cfg *handshakeCon
 		}
 	}
 
-	if err = state.cipherSuite.init(state.masterSecret, clientRandom, serverRandom, true); err != nil {
+	if err = state.cipherSuite.init(state.masterSecret, clientRandom[:], serverRandom[:], true); err != nil {
 		return &alert{alertLevelFatal, alertInternalError}, err
 	}
 	return nil, nil
diff --git a/handshake_message_client_hello.go b/handshake_message_client_hello.go
index ac1e867..f61adb9 100644
--- a/handshake_message_client_hello.go
+++ b/handshake_message_client_hello.go
@@ -36,11 +36,8 @@ func (h *handshakeMessageClientHello) Marshal() ([]byte, error) {
 	out[0] = h.version.major
 	out[1] = h.version.minor
 
-	rand, err := h.random.Marshal()
-	if err != nil {
-		return nil, err
-	}
-	copy(out[2:], rand)
+	rand := h.random.marshalFixed()
+	copy(out[2:], rand[:])
 
 	out = append(out, 0x00) // SessionID
 
@@ -65,9 +62,9 @@ func (h *handshakeMessageClientHello) Unmarshal(data []byte) error {
 	h.version.major = data[0]
 	h.version.minor = data[1]
 
-	if err := h.random.Unmarshal(data[2 : 2+handshakeRandomLength]); err != nil {
-		return err
-	}
+	var random [handshakeRandomLength]byte
+	copy(random[:], data[2:])
+	h.random.unmarshalFixed(random)
 
 	// rest of packet has variable width sections
 	currOffset := handshakeMessageClientHelloVariableWidthStart
diff --git a/handshake_message_server_hello.go b/handshake_message_server_hello.go
index b5b4e00..5be0159 100644
--- a/handshake_message_server_hello.go
+++ b/handshake_message_server_hello.go
@@ -37,11 +37,8 @@ func (h *handshakeMessageServerHello) Marshal() ([]byte, error) {
 	out[0] = h.version.major
 	out[1] = h.version.minor
 
-	rand, err := h.random.Marshal()
-	if err != nil {
-		return nil, err
-	}
-	copy(out[2:], rand)
+	rand := h.random.marshalFixed()
+	copy(out[2:], rand[:])
 
 	out = append(out, 0x00) // SessionID
 
@@ -66,9 +63,9 @@ func (h *handshakeMessageServerHello) Unmarshal(data []byte) error {
 	h.version.major = data[0]
 	h.version.minor = data[1]
 
-	if err := h.random.Unmarshal(data[2 : 2+handshakeRandomLength]); err != nil {
-		return err
-	}
+	var random [handshakeRandomLength]byte
+	copy(random[:], data[2:])
+	h.random.unmarshalFixed(random)
 
 	currOffset := handshakeMessageServerHelloVariableWidthStart
 	currOffset += int(data[currOffset]) + 1 // SessionID
diff --git a/handshake_random.go b/handshake_random.go
index fe5f743..479132e 100644
--- a/handshake_random.go
+++ b/handshake_random.go
@@ -15,23 +15,18 @@ type handshakeRandom struct {
 	randomBytes [randomBytesLength]byte
 }
 
-func (h *handshakeRandom) Marshal() ([]byte, error) {
-	out := make([]byte, handshakeRandomLength)
+func (h *handshakeRandom) marshalFixed() [handshakeRandomLength]byte {
+	var out [handshakeRandomLength]byte
 
 	binary.BigEndian.PutUint32(out[0:], uint32(h.gmtUnixTime.Unix()))
 	copy(out[4:], h.randomBytes[:])
 
-	return out, nil
+	return out
 }
 
-func (h *handshakeRandom) Unmarshal(data []byte) error {
-	if len(data) != handshakeRandomLength {
-		return errBufferTooSmall
-	}
+func (h *handshakeRandom) unmarshalFixed(data [handshakeRandomLength]byte) {
 	h.gmtUnixTime = time.Unix(int64(binary.BigEndian.Uint32(data[0:])), 0)
 	copy(h.randomBytes[:], data[4:])
-
-	return nil
 }
 
 // populate fills the handshakeRandom with random values
diff --git a/resume.go b/resume.go
index d75c484..d2b50ca 100644
--- a/resume.go
+++ b/resume.go
@@ -7,15 +7,15 @@ import (
 
 // Export extracts dtls state and inner connection from an already handshaked dtls conn
 func (c *Conn) Export() (*State, net.Conn, error) {
-	state, err := c.state.clone()
-	if err != nil {
-		return nil, nil, err
-	}
+	state := c.state.clone()
 	return state, c.nextConn.Conn(), nil
 }
 
 // Resume imports an already stablished 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
+	}
 	c, err := createConn(context.Background(), conn, config, state.isClient, state)
 	if err != nil {
 		return nil, err
diff --git a/state.go b/state.go
index b3b1200..0a1ccf6 100644
--- a/state.go
+++ b/state.go
@@ -42,51 +42,31 @@ type State struct {
 type serializedState struct {
 	LocalEpoch            uint16
 	RemoteEpoch           uint16
-	LocalRandom           []byte
-	RemoteRandom          []byte
+	LocalRandom           [handshakeRandomLength]byte
+	RemoteRandom          [handshakeRandomLength]byte
 	CipherSuiteID         uint16
 	MasterSecret          []byte
 	SequenceNumber        uint64
 	SRTPProtectionProfile uint16
-	RemoteCertificate     []byte
+	RemoteCertificate     [][]byte
 	IsClient              bool
 }
 
-func (s *State) clone() (*State, error) {
-	serialized, err := s.serialize()
-	if err != nil {
-		return nil, err
-	}
+func (s *State) clone() *State {
+	serialized := s.serialize()
 	state := &State{}
-	if err := state.deserialize(*serialized); err != nil {
-		return nil, err
-	}
-	return state, nil
+	state.deserialize(*serialized)
+
+	return state
 }
 
-func (s *State) serialize() (*serializedState, error) {
+func (s *State) serialize() *serializedState {
 	// Marshal random values
-	localRnd, err := s.localRandom.Marshal()
-	if err != nil {
-		return nil, err
-	}
-	remoteRnd, err := s.remoteRandom.Marshal()
-	if err != nil {
-		return nil, err
-	}
-
-	// Marshal remote certificate
-	var cert []byte
-	if s.remoteCertificate != nil {
-		h := &handshakeMessageCertificate{s.remoteCertificate}
-		cert, err = h.Marshal()
-		if err != nil {
-			return nil, err
-		}
-	}
+	localRnd := s.localRandom.marshalFixed()
+	remoteRnd := s.remoteRandom.marshalFixed()
 
 	epoch := s.localEpoch.Load().(uint16)
-	serialized := serializedState{
+	return &serializedState{
 		LocalEpoch:            epoch,
 		RemoteEpoch:           s.remoteEpoch.Load().(uint16),
 		CipherSuiteID:         uint16(s.cipherSuite.ID()),
@@ -95,14 +75,12 @@ func (s *State) serialize() (*serializedState, error) {
 		LocalRandom:           localRnd,
 		RemoteRandom:          remoteRnd,
 		SRTPProtectionProfile: uint16(s.srtpProtectionProfile),
-		RemoteCertificate:     cert,
+		RemoteCertificate:     s.remoteCertificate,
 		IsClient:              s.isClient,
 	}
-
-	return &serialized, nil
 }
 
-func (s *State) deserialize(serialized serializedState) error {
+func (s *State) deserialize(serialized serializedState) {
 	// Set epoch values
 	epoch := serialized.LocalEpoch
 	s.localEpoch.Store(serialized.LocalEpoch)
@@ -114,51 +92,51 @@ func (s *State) deserialize(serialized serializedState) error {
 
 	// Set random values
 	localRandom := &handshakeRandom{}
-	if err := localRandom.Unmarshal(serialized.LocalRandom); err != nil {
-		return err
-	}
+	localRandom.unmarshalFixed(serialized.LocalRandom)
 	s.localRandom = *localRandom
+
 	remoteRandom := &handshakeRandom{}
-	if err := remoteRandom.Unmarshal(serialized.RemoteRandom); err != nil {
-		return err
-	}
+	remoteRandom.unmarshalFixed(serialized.RemoteRandom)
 	s.remoteRandom = *remoteRandom
 
 	s.isClient = serialized.IsClient
 
+	// Set master secret
+	s.masterSecret = serialized.MasterSecret
+
 	// Set cipher suite
 	s.cipherSuite = cipherSuiteForID(CipherSuiteID(serialized.CipherSuiteID))
-	var err error
-	if serialized.IsClient {
-		err = s.cipherSuite.init(serialized.MasterSecret, serialized.LocalRandom, serialized.RemoteRandom, true)
-	} else {
-		err = s.cipherSuite.init(serialized.MasterSecret, serialized.RemoteRandom, serialized.LocalRandom, false)
-	}
-	if err != nil {
-		return err
-	}
 
 	atomic.StoreUint64(&s.localSequenceNumber[epoch], serialized.SequenceNumber)
 	s.srtpProtectionProfile = SRTPProtectionProfile(serialized.SRTPProtectionProfile)
 
 	// Set remote certificate
-	if serialized.RemoteCertificate != nil {
-		h := &handshakeMessageCertificate{}
-		if err := h.Unmarshal(serialized.RemoteCertificate); err != nil {
-			return err
-		}
-		s.remoteCertificate = h.certificate
+	s.remoteCertificate = serialized.RemoteCertificate
+}
+
+func (s *State) initCipherSuite() error {
+	if s.cipherSuite.isInitialized() {
+		return nil
 	}
 
+	localRandom := s.localRandom.marshalFixed()
+	remoteRandom := s.remoteRandom.marshalFixed()
+
+	var err error
+	if s.isClient {
+		err = s.cipherSuite.init(s.masterSecret, localRandom[:], remoteRandom[:], true)
+	} else {
+		err = s.cipherSuite.init(s.masterSecret, remoteRandom[:], localRandom[:], false)
+	}
+	if err != nil {
+		return err
+	}
 	return nil
 }
 
 // MarshalBinary is a binary.BinaryMarshaler.MarshalBinary implementation
 func (s *State) MarshalBinary() ([]byte, error) {
-	serialized, err := s.serialize()
-	if err != nil {
-		return nil, err
-	}
+	serialized := s.serialize()
 
 	var buf bytes.Buffer
 	enc := gob.NewEncoder(&buf)
@@ -176,7 +154,8 @@ func (s *State) UnmarshalBinary(data []byte) error {
 		return err
 	}
 
-	if err := s.deserialize(serialized); err != nil {
+	s.deserialize(serialized)
+	if err := s.initCipherSuite(); err != nil {
 		return err
 	}
 	return nil


Test patch:
diff --git a/resume_test.go b/resume_test.go
index 77db7ff..81c5a0c 100644
--- a/resume_test.go
+++ b/resume_test.go
@@ -132,7 +132,7 @@ func DoTestResume(t *testing.T, newLocal, newRemote func(net.Conn, *Config) (*Co
 
 	// Resume dtls connection
 	var resumed net.Conn
-	resumed, err = Resume(state, localConn2, config)
+	resumed, err = Resume(deserialized, localConn2, config)
 	if err != nil {
 		fatal(t, errChan, err)
 	}