Repository: pion/dtls
Pull request #417 (2022-01-26 03:07:02)

Choose cipher suite matching the certificate
cherry-pick of https://github.com/pion/dtls/pull/399

lukaslihotzki: Maybe the problem is that the cipher suites are cleared completely if the certificate is not found. In the original branch (https://github.com/Famedly/dtls/commit/f37ebb3609e6b7cd8742e9dddaa61445a37e84b3), I've amended the commit to keep all cipher suites if no certificate is found.
lukaslihotzki: `go test` passes on my machine with my amended commit, but not with my original commit.
codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#417](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (b8b66b6) into [master](https://codecov.io/gh/pion/dtls/commit/8f0a63079a965465ea09a7922404ee8e6325f3bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (8f0a630) will **decrease** coverage by `9.66%`.
> The diff coverage is `87.50%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/dtls/pull/417/graphs/tree.svg?width=650&height=150&src=pr&token=bcCB2jB88I&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   75.89%   66.22%   -9.67%     
==========================================
  Files          88       64      -24     
  Lines        3692     4477     +785     
==========================================
+ Hits         2802     2965     +163     
- Misses        605     1266     +661     
+ Partials      285      246      -39     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `66.22% <87.50%> (+0.29%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [conn.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29ubi5nbw==) | `77.63% <87.50%> (-4.77%)` | :arrow_down: |
| [pkg/protocol/errors.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2Vycm9ycy5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/version.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL3ZlcnNpb24uZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/application\_data.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2FwcGxpY2F0aW9uX2RhdGEuZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/extension/srtp\_protection\_profile.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi9zcnRwX3Byb3RlY3Rpb25fcHJvZmlsZS5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/protocol/handshake/header.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2hhbmRzaGFrZS9oZWFkZXIuZ28=) | `0.00% <0.00%> (-86.67%)` | :arrow_down: |
| [pkg/protocol/handshake/handshake.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2hhbmRzaGFrZS9oYW5kc2hha2UuZ28=) | `0.00% <0.00%> (-70.84%)` | :arrow_down: |
| [pkg/protocol/compression\_method.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2NvbXByZXNzaW9uX21ldGhvZC5nbw==) | `16.00% <0.00%> (-70.67%)` | :arrow_down: |
| [pkg/protocol/extension/extension.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi9leHRlbnNpb24uZ28=) | `10.90% <0.00%> (-61.82%)` | :arrow_down: |
| [pkg/protocol/extension/use\_master\_secret.go](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi91c2VfbWFzdGVyX3NlY3JldC5nbw==) | `0.00% <0.00%> (-53.85%)` | :arrow_down: |
| ... and [81 more](https://codecov.io/gh/pion/dtls/pull/417/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [8f0a630...b8b66b6](https://codecov.io/gh/pion/dtls/pull/417?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

lukaslihotzki: So the added lines need to be factored out into a separate function. You or me?
Sean-Der: Hey @lukaslihotzki! Thanks for the super fast response. Sorry it took us so long to look at this :( 

I added you to the Pion organization, please push to the branch w/e you want! 
Sean-Der: If you can add a test this is a instant merge from me. I just don't want it to regress in the future (unless it is covered by tests already)
Sean-Der: @daenney @at-wat @lukaslihotzki I just forced push to delete that RSA cipher from the ED25519 test. 
This PR LGTM to me code wise.

I would love to see a E2E test (or even just a unit test) to make sure that a refactor doesn't break this functionality in the future. @lukaslihotzki Do you have the time to do that? If not I can write one tonight.
lukaslihotzki: @Sean-Der I have modeled the original problem with OpenSSL Client (#398) as E2E test.
lukaslihotzki: @Sean-Der, why have you removed the E2E test addition?
Sean-Der: Hey @lukaslihotzki 

Sorry for the churn. These are the changes I made/why I made them.

* Added error handling to `filterCipherSuitesForCertificate` call, before we ignored the error.

* Removed the E2E test and replaced with an explicit test for the behavior. I was worried that if someone regressed this feature it would be hard to deduced from the E2E test what exactly went wrong. It also would be easy to remove the check from the E2E test. Since it isn't targeted at testing a specific behavior it might be easy to miss in a review.

Fantastic work on the PR! Really appreciate your involvement.  

Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index b43156d..1fe3541 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -27,6 +27,8 @@ Jozef Kralik <jojo.lwin@gmail.com>
 Julien Salleyron <julien.salleyron@gmail.com>
 Kegan Dougal <kegan@matrix.org>
 Lander Noterman <lander.noterman@basalte.be>
+Len <len@hpcnt.com>
+Lukas Lihotzki <lukas@lihotzki.de>
 Michael Zabka <zabka.michael@gmail.com>
 Michiel De Backker <mail@backkem.me>
 Robert Eperjesi <eperjesi@uber.com>
diff --git a/cipher_suite.go b/cipher_suite.go
index 3dc1e5b..1301a8e 100644
--- a/cipher_suite.go
+++ b/cipher_suite.go
@@ -1,6 +1,10 @@
 package dtls
 
 import (
+	"crypto/ecdsa"
+	"crypto/ed25519"
+	"crypto/rsa"
+	"crypto/tls"
 	"fmt"
 	"hash"
 
@@ -222,3 +226,24 @@ func parseCipherSuites(userSelectedSuites []CipherSuiteID, customCipherSuites fu
 
 	return cipherSuites[:i], nil
 }
+
+func filterCipherSuitesForCertificate(cert *tls.Certificate, cipherSuites []CipherSuite) []CipherSuite {
+	if cert == nil || cert.PrivateKey == nil {
+		return cipherSuites
+	}
+	var certType clientcertificate.Type
+	switch cert.PrivateKey.(type) {
+	case ed25519.PrivateKey, *ecdsa.PrivateKey:
+		certType = clientcertificate.ECDSASign
+	case *rsa.PrivateKey:
+		certType = clientcertificate.RSASign
+	}
+
+	filtered := []CipherSuite{}
+	for _, c := range cipherSuites {
+		if c.AuthenticationType() != CipherSuiteAuthenticationTypeCertificate || certType == c.CertificateType() {
+			filtered = append(filtered, c)
+		}
+	}
+	return filtered
+}
diff --git a/conn.go b/conn.go
index c342c3f..dd78a5a 100644
--- a/conn.go
+++ b/conn.go
@@ -176,6 +176,12 @@ func createConn(ctx context.Context, nextConn net.Conn, config *Config, isClient
 		sessionStore:                config.SessionStore,
 	}
 
+	cert, err := hsCfg.getCertificate(serverName)
+	if err != nil && !errors.Is(err, errNoCertificates) {
+		return nil, err
+	}
+	hsCfg.localCipherSuites = filterCipherSuitesForCertificate(cert, cipherSuites)
+
 	var initialFlight flightVal
 	var initialFSMState handshakeState
 
diff --git a/pkg/crypto/selfsign/selfsign.go b/pkg/crypto/selfsign/selfsign.go
index 7b707af..581e0a2 100644
--- a/pkg/crypto/selfsign/selfsign.go
+++ b/pkg/crypto/selfsign/selfsign.go
@@ -7,6 +7,7 @@ import (
 	"crypto/ed25519"
 	"crypto/elliptic"
 	"crypto/rand"
+	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/hex"
@@ -54,6 +55,8 @@ func WithDNS(key crypto.PrivateKey, cn string, sans ...string) (tls.Certificate,
 		pubKey = k.Public()
 	case *ecdsa.PrivateKey:
 		pubKey = k.Public()
+	case *rsa.PrivateKey:
+		pubKey = k.Public()
 	default:
 		return tls.Certificate{}, errInvalidPrivateKey
 	}


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 46cbf49..2ff9b5e 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -3,7 +3,11 @@ package dtls
 import (
 	"bytes"
 	"context"
+	"crypto"
+	"crypto/ecdsa"
+	cryptoElliptic "crypto/elliptic"
 	"crypto/rand"
+	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/hex"
@@ -2299,3 +2303,84 @@ func (ms *memSessStore) Del(key []byte) error {
 
 	return nil
 }
+
+// Assert that the server only uses CipherSuites with a hash+signature that matches
+// the certificate. As specified in rfc5246#section-7.4.3
+func TestCipherSuiteMatchesCertificateType(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	// Check for leaking routines
+	report := test.CheckRoutines(t)
+	defer report()
+
+	for _, test := range []struct {
+		Name           string
+		cipherList     []CipherSuiteID
+		expectedCipher CipherSuiteID
+		generateRSA    bool
+	}{
+		{
+			Name:           "ECDSA Certificate with RSA CipherSuite first",
+			cipherList:     []CipherSuiteID{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
+			expectedCipher: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+		},
+		{
+			Name:           "RSA Certificate with ECDSA CipherSuite first",
+			cipherList:     []CipherSuiteID{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+			expectedCipher: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+			generateRSA:    true,
+		},
+	} {
+		test := test
+		t.Run(test.Name, func(t *testing.T) {
+			clientErr := make(chan error, 1)
+			client := make(chan *Conn, 1)
+
+			ca, cb := dpipe.Pipe()
+			go func() {
+				c, err := testClient(context.TODO(), ca, &Config{CipherSuites: test.cipherList}, false)
+				clientErr <- err
+				client <- c
+			}()
+
+			var (
+				priv crypto.PrivateKey
+				err  error
+			)
+
+			if test.generateRSA {
+				if priv, err = rsa.GenerateKey(rand.Reader, 2048); err != nil {
+					t.Fatal(err)
+				}
+			} else {
+				if priv, err = ecdsa.GenerateKey(cryptoElliptic.P256(), rand.Reader); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			serverCert, err := selfsign.SelfSign(priv)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if s, err := testServer(context.TODO(), cb, &Config{
+				CipherSuites: test.cipherList,
+				Certificates: []tls.Certificate{serverCert},
+			}, false); err != nil {
+				t.Fatal(err)
+			} else if err = s.Close(); err != nil {
+				t.Fatal(err)
+			}
+
+			if c, err := <-client, <-clientErr; err != nil {
+				t.Fatal(err)
+			} else if err := c.Close(); err != nil {
+				t.Fatal(err)
+			} else if c.ConnectionState().cipherSuite.ID() != test.expectedCipher {
+				t.Fatalf("Expected(%s) and Actual(%s) CipherSuite do not match", test.expectedCipher, c.ConnectionState().cipherSuite.ID())
+			}
+		})
+	}
+}
diff --git a/e2e/e2e_openssl_test.go b/e2e/e2e_openssl_test.go
index 09fdff9..62b40d2 100644
--- a/e2e/e2e_openssl_test.go
+++ b/e2e/e2e_openssl_test.go
@@ -174,7 +174,7 @@ func ciphersOpenSSL(cfg *dtls.Config) string {
 		dtls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:   "ECDHE-RSA-AES256-GCM-SHA384",
 
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA: "ECDHE-ECDSA-AES256-SHA",
-		dtls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA:   "ECDHE-RSA-AES128-SHA",
+		dtls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA:   "ECDHE-RSA-AES256-SHA",
 
 		dtls.TLS_PSK_WITH_AES_128_CCM:        "PSK-AES128-CCM",
 		dtls.TLS_PSK_WITH_AES_128_CCM_8:      "PSK-AES128-CCM8",
diff --git a/e2e/e2e_v113_test.go b/e2e/e2e_v113_test.go
index aa81241..b8220f2 100644
--- a/e2e/e2e_v113_test.go
+++ b/e2e/e2e_v113_test.go
@@ -30,7 +30,6 @@ func testPionE2ESimpleED25519(t *testing.T, server, client func(*comm)) {
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8,
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
-		dtls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
 		dtls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
 	} {
 		cipherSuite := cipherSuite