Repository: pion/dtls
Pull request #347 (2021-02-17 14:58:35)

Always include NegotationInfo in ServerHello
If ClientHello doesn't contain

Relates to #314


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/347?src=pr&el=h1) Report
> Merging [#347](https://codecov.io/gh/pion/dtls/pull/347?src=pr&el=desc) (9890736) into [master](https://codecov.io/gh/pion/dtls/commit/48fe23d6f7b09c5d4a3f85132af50b9f57863e46?el=desc) (48fe23d) will **decrease** coverage by `9.98%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
- Coverage   75.83%   65.85%   -9.99%     
==========================================
  Files          86       62      -24     
  Lines        3666     3104     -562     
==========================================
- Hits         2780     2044     -736     
- Misses        602      839     +237     
+ Partials      284      221      -63     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `65.85% <100.00%> (-0.05%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/347?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [flight0handler.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-ZmxpZ2h0MGhhbmRsZXIuZ28=) | `76.00% <ø> (-4.77%)` | :arrow_down: |
| [state.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-c3RhdGUuZ28=) | `89.47% <ø> (ø)` | |
| [flight4handler.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-ZmxpZ2h0NGhhbmRsZXIuZ28=) | `82.12% <100.00%> (-1.84%)` | :arrow_down: |
| [pkg/protocol/errors.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2Vycm9ycy5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/version.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL3ZlcnNpb24uZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/application\_data.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2FwcGxpY2F0aW9uX2RhdGEuZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/extension/srtp\_protection\_profile.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi9zcnRwX3Byb3RlY3Rpb25fcHJvZmlsZS5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/handshake/header.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2hhbmRzaGFrZS9oZWFkZXIuZ28=) | `0.00% <0.00%> (-86.67%)` | :arrow_down: |
| [pkg/protocol/compression\_method.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2NvbXByZXNzaW9uX21ldGhvZC5nbw==) | `13.33% <0.00%> (-73.34%)` | :arrow_down: |
| [pkg/protocol/handshake/handshake.go](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2hhbmRzaGFrZS9oYW5kc2hha2UuZ28=) | `0.00% <0.00%> (-70.84%)` | :arrow_down: |
| ... and [58 more](https://codecov.io/gh/pion/dtls/pull/347/diff?src=pr&el=tree-more) | |

------

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


Patch:
diff --git a/flight0handler.go b/flight0handler.go
index 77fa1ab..949d7c0 100644
--- a/flight0handler.go
+++ b/flight0handler.go
@@ -64,8 +64,6 @@ func flight0Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 			}
 		case *extension.ServerName:
 			state.serverName = e.ServerName // remote server name
-		case *extension.RenegotiationInfo:
-			state.remoteSupportsRenegotiation = true
 		}
 	}
 
diff --git a/flight4handler.go b/flight4handler.go
index d5d2450..ecdf03a 100644
--- a/flight4handler.go
+++ b/flight4handler.go
@@ -173,7 +173,9 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 }
 
 func flight4Generate(c flightConn, state *State, cache *handshakeCache, cfg *handshakeConfig) ([]*packet, *alert.Alert, error) {
-	extensions := []extension.Extension{}
+	extensions := []extension.Extension{&extension.RenegotiationInfo{
+		RenegotiatedConnection: 0,
+	}}
 	if (cfg.extendedMasterSecret == RequestExtendedMasterSecret ||
 		cfg.extendedMasterSecret == RequireExtendedMasterSecret) && state.extendedMasterSecret {
 		extensions = append(extensions, &extension.UseExtendedMasterSecret{
@@ -185,11 +187,6 @@ func flight4Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 			ProtectionProfiles: []SRTPProtectionProfile{state.srtpProtectionProfile},
 		})
 	}
-	if state.remoteSupportsRenegotiation {
-		extensions = append(extensions, &extension.RenegotiationInfo{
-			RenegotiatedConnection: 0,
-		})
-	}
 	if state.cipherSuite.AuthenticationType() == CipherSuiteAuthenticationTypeCertificate {
 		extensions = append(extensions, []extension.Extension{
 			&extension.SupportedEllipticCurves{
diff --git a/state.go b/state.go
index f681b49..53fca6a 100644
--- a/state.go
+++ b/state.go
@@ -13,12 +13,11 @@ import (
 
 // State holds the dtls connection state and implements both encoding.BinaryMarshaler and encoding.BinaryUnmarshaler
 type State struct {
-	localEpoch, remoteEpoch     atomic.Value
-	localSequenceNumber         []uint64 // uint48
-	localRandom, remoteRandom   handshake.Random
-	masterSecret                []byte
-	cipherSuite                 CipherSuite // nil if a cipherSuite hasn't been chosen
-	remoteSupportsRenegotiation bool        // did ClientHello contain a RenegotiationInfo
+	localEpoch, remoteEpoch   atomic.Value
+	localSequenceNumber       []uint64 // uint48
+	localRandom, remoteRandom handshake.Random
+	masterSecret              []byte
+	cipherSuite               CipherSuite // nil if a cipherSuite hasn't been chosen
 
 	srtpProtectionProfile SRTPProtectionProfile // Negotiated SRTPProtectionProfile
 	PeerCertificates      [][]byte


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 875bdea..b532926 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -1903,8 +1903,8 @@ func TestMultipleHelloVerifyRequest(t *testing.T) {
 	cancel()
 }
 
-// Assert that a DTLS Server only responds with RenegotiationInfo if
-// a ClientHello contained that extension
+// Assert that a DTLS Server always responds with RenegotiationInfo if
+// a ClientHello contained that extension or not
 func TestRenegotationInfo(t *testing.T) {
 	// Limit runtime in case of deadlocks
 	lim := test.TimeOut(10 * time.Second)
@@ -1917,8 +1917,8 @@ func TestRenegotationInfo(t *testing.T) {
 	resp := make([]byte, 1024)
 
 	for _, testCase := range []struct {
-		Name                    string
-		ExpectRenegotiationInfo bool
+		Name                  string
+		SendRenegotiationInfo bool
 	}{
 		{
 			"Include RenegotiationInfo",
@@ -1933,7 +1933,7 @@ func TestRenegotationInfo(t *testing.T) {
 		t.Run(test.Name, func(t *testing.T) {
 			sendClientHello := func(cookie []byte, ca net.Conn, sequenceNumber uint64) {
 				extensions := []extension.Extension{}
-				if test.ExpectRenegotiationInfo {
+				if test.SendRenegotiationInfo {
 					extensions = append(extensions, &extension.RenegotiationInfo{
 						RenegotiatedConnection: 0,
 					})
@@ -2011,15 +2011,15 @@ func TestRenegotationInfo(t *testing.T) {
 			}
 
 			serverHello := r.Content.(*handshake.Handshake).Message.(*handshake.MessageServerHello)
-			actualNegotationInfo := false
+			gotNegotationInfo := false
 			for _, v := range serverHello.Extensions {
 				if _, ok := v.(*extension.RenegotiationInfo); ok {
-					actualNegotationInfo = true
+					gotNegotationInfo = true
 				}
 			}
 
-			if test.ExpectRenegotiationInfo != actualNegotationInfo {
-				t.Fatalf("NegotationInfo state in ServerHello is incorrect: expected(%t) actual(%t)", test.ExpectRenegotiationInfo, actualNegotationInfo)
+			if !gotNegotationInfo {
+				t.Fatalf("Received ServerHello without RenegotiationInfo")
 			}
 		})
 	}