Repository: pion/dtls
Pull request #335 (2021-01-13 22:09:48)

Add PSK Client Hint to conn state
#### Description
My issue started when using the `go-coap` project where we had the need o access the PSK Client Hint after the DTLS +PSK handshake, more specifically, inside a path handler or middleware. Today that is already possible for the DTLS + PKI secure method, where we can have access to the Client Certificate after the handshake.

This PR basically adds the field `IdentityHint` to the connection State struct following the same idea of the `PeerCertificates` field on the same struct.

Here is the issue with more details: https://github.com/plgd-dev/go-coap/issues/196

#### Reference issue
Fixes https://github.com/plgd-dev/go-coap/issues/196


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/335?src=pr&el=h1) Report
> Merging [#335](https://codecov.io/gh/pion/dtls/pull/335?src=pr&el=desc) (daa978c) into [master](https://codecov.io/gh/pion/dtls/commit/4b0ca8ea13aca9861dfabdd42276a3abd929a3d4?el=desc) (4b0ca8e) will **decrease** coverage by `0.87%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
- Coverage   73.83%   72.96%   -0.88%     
==========================================
  Files          78       76       -2     
  Lines        3535     3458      -77     
==========================================
- Hits         2610     2523      -87     
- Misses        648      660      +12     
+ Partials      277      275       -2     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `72.96% <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/335?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [flight3handler.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-ZmxpZ2h0M2hhbmRsZXIuZ28=) | `89.47% <100.00%> (+0.09%)` | :arrow_up: |
| [flight4handler.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-ZmxpZ2h0NGhhbmRsZXIuZ28=) | `79.80% <100.00%> (+0.09%)` | :arrow_up: |
| [state.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-c3RhdGUuZ28=) | `89.47% <100.00%> (+0.28%)` | :arrow_up: |
| [errors.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-ZXJyb3JzLmdv) | `76.08% <0.00%> (-8.70%)` | :arrow_down: |
| [config.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-Y29uZmlnLmdv) | `89.47% <0.00%> (-5.27%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `78.41% <0.00%> (-2.38%)` | :arrow_down: |
| [errors\_errno.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-ZXJyb3JzX2Vycm5vLmdv) | | |
| [internal/net/dpipe/dpipe.go](https://codecov.io/gh/pion/dtls/pull/335/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbmV0L2RwaXBlL2RwaXBlLmdv) | | |

------

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

Sean-Der: This LGTM to me @alvarowolfx !

Would you mind making your commit message Body+Subject (with a newline between them) you can run the scripts locally like [this](https://github.com/pion/webrtc/wiki/Contributing#run-all-automated-tests-and-checks-before-submitting) when that goes green will merge and tag
jkralik: @alvarowolfx Could you add/extend simple test whether the `Identity Hint` is filled ?
alvarowolfx: @Sean-Der I made some changes to pass all CI checks and also added a test like @jkralik suggested to check that the new field is being filled out. Let me know if more changes are needed or any other suggestion.
jkralik: @alvarowolfx Good job 👍 
daenney: I'll merge it in a day or so, to give other contributors a chance to review. But it looks good to me.
daenney: Alright, that's enough people then :smile: 

Patch:
diff --git a/README.md b/README.md
index 86e19d8..20b2c5c 100644
--- a/README.md
+++ b/README.md
@@ -148,6 +148,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [Carson Hoffman](https://github.com/CarsonHoffman)
 * [Vadim Filimonov](https://github.com/fffilimonov)
 * [Jim Wert](https://github.com/bocajim)
+* [Alvaro Viebrantz](https://github.com/alvarowolfx)
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text
diff --git a/flight3handler.go b/flight3handler.go
index 309a887..dbea45f 100644
--- a/flight3handler.go
+++ b/flight3handler.go
@@ -103,7 +103,7 @@ func handleServerKeyExchange(_ flightConn, state *State, cfg *handshakeConfig, h
 		if psk, err = cfg.localPSKCallback(h.identityHint); err != nil {
 			return &alert{alertLevelFatal, alertInternalError}, err
 		}
-
+		state.IdentityHint = h.identityHint
 		state.preMasterSecret = prfPSKPreMasterSecret(psk)
 	} else {
 		if state.localKeypair, err = generateKeypair(h.namedCurve); err != nil {
diff --git a/flight4handler.go b/flight4handler.go
index 1221e2f..9f4d9af 100644
--- a/flight4handler.go
+++ b/flight4handler.go
@@ -85,7 +85,7 @@ func flight4Parse(ctx context.Context, c flightConn, state *State, cache *handsh
 			if psk, err = cfg.localPSKCallback(clientKeyExchange.identityHint); err != nil {
 				return 0, &alert{alertLevelFatal, alertInternalError}, err
 			}
-
+			state.IdentityHint = clientKeyExchange.identityHint
 			preMasterSecret = prfPSKPreMasterSecret(psk)
 		} else {
 			preMasterSecret, err = prfPreMasterSecret(clientKeyExchange.publicKey, state.localKeypair.privateKey, state.localKeypair.curve)
diff --git a/state.go b/state.go
index 33d2105..0c0c016 100644
--- a/state.go
+++ b/state.go
@@ -18,6 +18,7 @@ type State struct {
 
 	srtpProtectionProfile SRTPProtectionProfile // Negotiated SRTPProtectionProfile
 	PeerCertificates      [][]byte
+	IdentityHint          []byte
 
 	isClient bool
 
@@ -49,6 +50,7 @@ type serializedState struct {
 	SequenceNumber        uint64
 	SRTPProtectionProfile uint16
 	PeerCertificates      [][]byte
+	IdentityHint          []byte
 	IsClient              bool
 }
 
@@ -76,6 +78,7 @@ func (s *State) serialize() *serializedState {
 		RemoteRandom:          remoteRnd,
 		SRTPProtectionProfile: uint16(s.srtpProtectionProfile),
 		PeerCertificates:      s.PeerCertificates,
+		IdentityHint:          s.IdentityHint,
 		IsClient:              s.isClient,
 	}
 }
@@ -112,6 +115,7 @@ func (s *State) deserialize(serialized serializedState) {
 
 	// Set remote certificate
 	s.PeerCertificates = serialized.PeerCertificates
+	s.IdentityHint = serialized.IdentityHint
 }
 
 func (s *State) initCipherSuite() error {


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 87cc27c..b8c88e9 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -485,6 +485,12 @@ func TestPSK(t *testing.T) {
 			if err != nil {
 				t.Fatalf("TestPSK: Server failed(%v)", err)
 			}
+
+			actualPSKIdentityHint := server.ConnectionState().IdentityHint
+			if !bytes.Equal(actualPSKIdentityHint, clientIdentity) {
+				t.Errorf("TestPSK: Server ClientPSKIdentity Mismatch '%s': expected(%v) actual(%v)", test.Name, clientIdentity, actualPSKIdentityHint)
+			}
+
 			defer func() {
 				_ = server.Close()
 			}()