Repository: pion/dtls
Pull request #346 (2021-02-16 06:11:46)

Include RenegotiationInfo in ServerHello
Before if clients sent a ClientHello with RenegotiationInfo we would
just ignore. Now we properly respond that we don't support.

Relates to #314


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/346?src=pr&el=h1) Report
> Merging [#346](https://codecov.io/gh/pion/dtls/pull/346?src=pr&el=desc) (1981be0) into [master](https://codecov.io/gh/pion/dtls/commit/eee772b69694f7d4c5b57edc9a3eae19eb00710c?el=desc) (eee772b) will **decrease** coverage by `0.04%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
- Coverage   75.84%   75.80%   -0.05%     
==========================================
  Files          86       86              
  Lines        3660     3666       +6     
==========================================
+ Hits         2776     2779       +3     
- Misses        600      603       +3     
  Partials      284      284              
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `75.82% <100.00%> (-0.05%)` | :arrow_down: |
| wasm | `65.89% <100.00%> (+0.03%)` | :arrow_up: |

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/346?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [state.go](https://codecov.io/gh/pion/dtls/pull/346/diff?src=pr&el=tree#diff-c3RhdGUuZ28=) | `89.47% <ø> (ø)` | |
| [flight0handler.go](https://codecov.io/gh/pion/dtls/pull/346/diff?src=pr&el=tree#diff-ZmxpZ2h0MGhhbmRsZXIuZ28=) | `80.76% <100.00%> (+0.76%)` | :arrow_up: |
| [flight4handler.go](https://codecov.io/gh/pion/dtls/pull/346/diff?src=pr&el=tree#diff-ZmxpZ2h0NGhhbmRsZXIuZ28=) | `83.96% <100.00%> (+0.27%)` | :arrow_up: |
| [internal/net/dpipe/dpipe.go](https://codecov.io/gh/pion/dtls/pull/346/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbmV0L2RwaXBlL2RwaXBlLmdv) | `93.24% <0.00%> (-2.71%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/346/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `82.37% <0.00%> (-0.20%)` | :arrow_down: |

------

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

Sean-Der: @sipsorcery mind trying this PR? You can use `go mod edit` and do a replace. If you don't have the time don't worry about it! I think this will merge super quick.

@fffilimonov @daenney mind giving a quick review?
sipsorcery: Happy to test.

I'm having some challenges with setting the go module to the PR. Here are the steps I've taken:

 - Cloned this repo and checked out a branch with this PR,
 - In my [go.mod](https://github.com/sipsorcery/webrtc-echoes/blob/master/pion/client/go.mod) file of the application I'm testing I've added:
`replace github.com/pion/dtls => c:/dev/github/dtls`

Those steps don't seem to be getting the fix and I suspect there are some more steps needed to switch the module to my `dtls` repo.

daenney: I don't recall exactly, but I don't think that's going to work because you don't explicitly list dtls as a dependency for your module. It's a transitive dependency through webrtc, so you probably need to clone webrtc, use the dtls replace directive there and on this module use a replace for webrtc.
sipsorcery: Tried the below for my `go.mod` but doesn't seem to have used the dtls source from my repo.

````
Sean-Der: I say we just get this merged instead of fighting that. This is a good chang no matter what!
Sean-Der: @daenney awesome, mind approving when you get a chance? If you don't have bandwidth we can nag some others :) 
daenney: Done. For some reason my brain thought you had, but that can’t be since you raised the PR 😅

Patch:
diff --git a/flight0handler.go b/flight0handler.go
index 949d7c0..77fa1ab 100644
--- a/flight0handler.go
+++ b/flight0handler.go
@@ -64,6 +64,8 @@ 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 7b0d031..d5d2450 100644
--- a/flight4handler.go
+++ b/flight4handler.go
@@ -185,6 +185,11 @@ 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 53fca6a..f681b49 100644
--- a/state.go
+++ b/state.go
@@ -13,11 +13,12 @@ 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
+	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
 
 	srtpProtectionProfile SRTPProtectionProfile // Negotiated SRTPProtectionProfile
 	PeerCertificates      [][]byte


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 74bfc7d..875bdea 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -1902,3 +1902,125 @@ func TestMultipleHelloVerifyRequest(t *testing.T) {
 	}
 	cancel()
 }
+
+// Assert that a DTLS Server only responds with RenegotiationInfo if
+// a ClientHello contained that extension
+func TestRenegotationInfo(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(10 * time.Second)
+	defer lim.Stop()
+
+	// Check for leaking routines
+	report := test.CheckRoutines(t)
+	defer report()
+
+	resp := make([]byte, 1024)
+
+	for _, testCase := range []struct {
+		Name                    string
+		ExpectRenegotiationInfo bool
+	}{
+		{
+			"Include RenegotiationInfo",
+			true,
+		},
+		{
+			"No RenegotiationInfo",
+			false,
+		},
+	} {
+		test := testCase
+		t.Run(test.Name, func(t *testing.T) {
+			sendClientHello := func(cookie []byte, ca net.Conn, sequenceNumber uint64) {
+				extensions := []extension.Extension{}
+				if test.ExpectRenegotiationInfo {
+					extensions = append(extensions, &extension.RenegotiationInfo{
+						RenegotiatedConnection: 0,
+					})
+				}
+
+				packet, err := (&recordlayer.RecordLayer{
+					Header: recordlayer.Header{
+						Version:        protocol.Version1_2,
+						SequenceNumber: sequenceNumber,
+					},
+					Content: &handshake.Handshake{
+						Header: handshake.Header{
+							MessageSequence: uint16(sequenceNumber),
+						},
+						Message: &handshake.MessageClientHello{
+							Version:            protocol.Version1_2,
+							Cookie:             cookie,
+							CipherSuiteIDs:     cipherSuiteIDs(defaultCipherSuites()),
+							CompressionMethods: defaultCompressionMethods(),
+							Extensions:         extensions,
+						},
+					},
+				}).Marshal()
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if _, err = ca.Write(packet); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			ca, cb := dpipe.Pipe()
+			defer func() {
+				if err := ca.Close(); err != nil {
+					t.Error(err)
+				}
+			}()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			go func() {
+				if _, err := testServer(ctx, cb, &Config{}, true); !errors.Is(err, context.Canceled) {
+					t.Error(err)
+				}
+			}()
+
+			time.Sleep(50 * time.Millisecond)
+
+			sendClientHello([]byte{}, ca, 0)
+			n, err := ca.Read(resp)
+			if err != nil {
+				t.Fatal(err)
+			}
+			r := &recordlayer.RecordLayer{}
+			if err = r.Unmarshal(resp[:n]); err != nil {
+				t.Fatal(err)
+			}
+
+			helloVerifyRequest := r.Content.(*handshake.Handshake).Message.(*handshake.MessageHelloVerifyRequest)
+
+			sendClientHello(helloVerifyRequest.Cookie, ca, 1)
+			if n, err = ca.Read(resp); err != nil {
+				t.Fatal(err)
+			}
+
+			messages, err := recordlayer.UnpackDatagram(resp[:n])
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if err := r.Unmarshal(messages[0]); err != nil {
+				t.Fatal(err)
+			}
+
+			serverHello := r.Content.(*handshake.Handshake).Message.(*handshake.MessageServerHello)
+			actualNegotationInfo := false
+			for _, v := range serverHello.Extensions {
+				if _, ok := v.(*extension.RenegotiationInfo); ok {
+					actualNegotationInfo = true
+				}
+			}
+
+			if test.ExpectRenegotiationInfo != actualNegotationInfo {
+				t.Fatalf("NegotationInfo state in ServerHello is incorrect: expected(%t) actual(%t)", test.ExpectRenegotiationInfo, actualNegotationInfo)
+			}
+		})
+	}
+}