Repository: pion/dtls
Pull request #475 (2022-05-24 20:27:53)

Make the Elliptic curves and order configurable
This allows the client/server to chose which elliptic curves they wish to advertise, and in which order.

Fixes #474

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/475?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#475](https://codecov.io/gh/pion/dtls/pull/475?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (7941328) into [master](https://codecov.io/gh/pion/dtls/commit/fa5afe38c3513203446b16e958f154e6d652a4bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (fa5afe3) will **decrease** coverage by `10.27%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head 7941328 differs from pull request most recent head 29994a7. Consider uploading reports for the commit 29994a7 to get more accurate results

```diff
@@             Coverage Diff             @@
##           master     #475       +/-   ##
===========================================
- Coverage   76.06%   65.79%   -10.28%     
===========================================
  Files          96       66       -30     
  Lines        5608     4742      -866     
===========================================
- Hits         4266     3120     -1146     
- Misses       1009     1353      +344     
+ Partials      333      269       -64     
```

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

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/475?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [handshaker.go](https://codecov.io/gh/pion/dtls/pull/475/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aGFuZHNoYWtlci5nbw==) | `74.31% <ø> (-1.64%)` | :arrow_down: |
| [config.go](https://codecov.io/gh/pion/dtls/pull/475/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uZmlnLmdv) | `93.10% <100.00%> (-6.90%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/475/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29ubi5nbw==) | `76.67% <100.00%> (-4.03%)` | :arrow_down: |
| [flight1handler.go](https://codecov.io/gh/pion/dtls/pull/475/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-ZmxpZ2h0MWhhbmRsZXIuZ28=) | `87.85% <100.00%> (ø)` | |
| [pkg/crypto/elliptic/elliptic.go](https://codecov.io/gh/pion/dtls/pull/475/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL2NyeXB0by9lbGxpcHRpYy9lbGxpcHRpYy5nbw==) | `20.93% <100.00%> (-34.96%)` | :arrow_down: |
| [pkg/protocol/errors.go](https://codecov.io/gh/pion/dtls/pull/475/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/475/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/475/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/475/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/475/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%> (-84.22%)` | :arrow_down: |
| ... and [65 more](https://codecov.io/gh/pion/dtls/pull/475/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/475?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/475?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [fa5afe3...29994a7](https://codecov.io/gh/pion/dtls/pull/475?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).

daenney: Working on redoing the tests, since I realised that changing config in validateConfig wasn't the right place to do it.

Patch:
diff --git a/config.go b/config.go
index 7f68c03..1ebf76d 100644
--- a/config.go
+++ b/config.go
@@ -10,6 +10,7 @@ import (
 	"io"
 	"time"
 
+	"github.com/pion/dtls/v2/pkg/crypto/elliptic"
 	"github.com/pion/logging"
 )
 
@@ -130,6 +131,12 @@ type Config struct {
 
 	// List of application protocols the peer supports, for ALPN
 	SupportedProtocols []string
+
+	// List of Elliptic Curves to use
+	//
+	// If an ECC ciphersuite is configured and EllipticCurves is empty
+	// it will default to X25519, P-256, P-384 in this specific order.
+	EllipticCurves []elliptic.Curve
 }
 
 func defaultConnectContextMaker() (context.Context, func()) {
@@ -145,6 +152,8 @@ func (c *Config) connectContextMaker() (context.Context, func()) {
 
 const defaultMTU = 1200 // bytes
 
+var defaultCurves = []elliptic.Curve{elliptic.X25519, elliptic.P256, elliptic.P384} //nolint:gochecknoglobals
+
 // PSKCallback is called once we have the remote's PSKIdentityHint.
 // If the remote provided none it will be nil
 type PSKCallback func([]byte) ([]byte, error)
diff --git a/conn.go b/conn.go
index aff5a95..ce46740 100644
--- a/conn.go
+++ b/conn.go
@@ -154,6 +154,11 @@ func createConn(ctx context.Context, nextConn net.Conn, config *Config, isClient
 		serverName = ""
 	}
 
+	curves := config.EllipticCurves
+	if len(curves) == 0 {
+		curves = defaultCurves
+	}
+
 	hsCfg := &handshakeConfig{
 		localPSKCallback:            config.PSK,
 		localPSKIdentityHint:        config.PSKIdentityHint,
@@ -175,6 +180,7 @@ func createConn(ctx context.Context, nextConn net.Conn, config *Config, isClient
 		initialEpoch:                0,
 		keyLogWriter:                config.KeyLogWriter,
 		sessionStore:                config.SessionStore,
+		ellipticCurves:              curves,
 	}
 
 	// rfc5246#section-7.4.3
diff --git a/flight1handler.go b/flight1handler.go
index 3e14edd..7cfcea8 100644
--- a/flight1handler.go
+++ b/flight1handler.go
@@ -74,7 +74,7 @@ func flight1Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 	if setEllipticCurveCryptographyClientHelloExtensions {
 		extensions = append(extensions, []extension.Extension{
 			&extension.SupportedEllipticCurves{
-				EllipticCurves: []elliptic.Curve{elliptic.X25519, elliptic.P256, elliptic.P384},
+				EllipticCurves: cfg.ellipticCurves,
 			},
 			&extension.SupportedPointFormats{
 				PointFormats: []elliptic.CurvePointFormat{elliptic.CurvePointFormatUncompressed},
diff --git a/handshaker.go b/handshaker.go
index 1c7b9ff..5b5e95e 100644
--- a/handshaker.go
+++ b/handshaker.go
@@ -9,6 +9,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/pion/dtls/v2/pkg/crypto/elliptic"
 	"github.com/pion/dtls/v2/pkg/crypto/signaturehash"
 	"github.com/pion/dtls/v2/pkg/protocol/alert"
 	"github.com/pion/dtls/v2/pkg/protocol/handshake"
@@ -106,6 +107,7 @@ type handshakeConfig struct {
 	clientCAs                   *x509.CertPool
 	retransmitInterval          time.Duration
 	customCipherSuites          func() []CipherSuite
+	ellipticCurves              []elliptic.Curve
 
 	onFlightState func(flightVal, handshakeState)
 	log           logging.LeveledLogger
diff --git a/pkg/crypto/elliptic/elliptic.go b/pkg/crypto/elliptic/elliptic.go
index e3b6b18..023bf90 100644
--- a/pkg/crypto/elliptic/elliptic.go
+++ b/pkg/crypto/elliptic/elliptic.go
@@ -5,6 +5,7 @@ import (
 	"crypto/elliptic"
 	"crypto/rand"
 	"errors"
+	"fmt"
 
 	"golang.org/x/crypto/curve25519"
 )
@@ -57,6 +58,18 @@ const (
 	X25519 Curve = 0x001d
 )
 
+func (c Curve) String() string {
+	switch c {
+	case P256:
+		return "P-256"
+	case P384:
+		return "P-384"
+	case X25519:
+		return "X25519"
+	}
+	return fmt.Sprintf("%#x", uint16(c))
+}
+
 // Curves returns all curves we implement
 func Curves() map[Curve]bool {
 	return map[Curve]bool{


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 9810971..d54553b 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -2724,3 +2724,82 @@ func TestMultipleServerCertificates(t *testing.T) {
 		})
 	}
 }
+
+func TestEllipticCurveConfiguration(t *testing.T) {
+	// Check for leaking routines
+	report := test.CheckRoutines(t)
+	defer report()
+
+	for _, test := range []struct {
+		Name            string
+		ConfigCurves    []elliptic.Curve
+		HadnshakeCurves []elliptic.Curve
+	}{
+		{
+			Name:            "Curve defaulting",
+			ConfigCurves:    nil,
+			HadnshakeCurves: defaultCurves,
+		},
+		{
+			Name:            "Single curve",
+			ConfigCurves:    []elliptic.Curve{elliptic.X25519},
+			HadnshakeCurves: []elliptic.Curve{elliptic.X25519},
+		},
+		{
+			Name:            "Multiple curves",
+			ConfigCurves:    []elliptic.Curve{elliptic.P384, elliptic.X25519},
+			HadnshakeCurves: []elliptic.Curve{elliptic.P384, elliptic.X25519},
+		},
+	} {
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+		defer cancel()
+
+		ca, cb := dpipe.Pipe()
+		type result struct {
+			c   *Conn
+			err error
+		}
+		c := make(chan result)
+
+		go func() {
+			client, err := testClient(ctx, ca, &Config{CipherSuites: []CipherSuiteID{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}, EllipticCurves: test.ConfigCurves}, true)
+			c <- result{client, err}
+		}()
+
+		server, err := testServer(ctx, cb, &Config{CipherSuites: []CipherSuiteID{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}, EllipticCurves: test.ConfigCurves}, true)
+		if err != nil {
+			t.Fatalf("Server error: %v", err)
+		}
+
+		if len(test.ConfigCurves) == 0 && len(test.HadnshakeCurves) != len(server.fsm.cfg.ellipticCurves) {
+			t.Fatalf("Failed to default Elliptic curves, expected %d, got: %d", len(test.HadnshakeCurves), len(server.fsm.cfg.ellipticCurves))
+		}
+
+		if len(test.ConfigCurves) != 0 {
+			if len(test.HadnshakeCurves) != len(server.fsm.cfg.ellipticCurves) {
+				t.Fatalf("Failed to configure Elliptic curves, expect %d, got %d", len(test.HadnshakeCurves), len(server.fsm.cfg.ellipticCurves))
+			}
+			for i, c := range test.ConfigCurves {
+				if c != server.fsm.cfg.ellipticCurves[i] {
+					t.Fatalf("Failed to maintain Elliptic curve order, expected %s, got %s", c, server.fsm.cfg.ellipticCurves[i])
+				}
+			}
+		}
+
+		res := <-c
+		if res.err != nil {
+			t.Fatalf("Client error; %v", err)
+		}
+
+		defer func() {
+			err = server.Close()
+			if err != nil {
+				t.Fatal(err)
+			}
+			err = res.c.Close()
+			if err != nil {
+				t.Fatal(err)
+			}
+		}()
+	}
+}
diff --git a/handshaker_test.go b/handshaker_test.go
index a3b6cbd..8135efe 100644
--- a/handshaker_test.go
+++ b/handshaker_test.go
@@ -1,3 +1,4 @@
+//nolint:dupl
 package dtls
 
 import (
@@ -264,6 +265,7 @@ func TestHandshaker(t *testing.T) {
 				cfg := &handshakeConfig{
 					localCipherSuites:     cipherSuites,
 					localCertificates:     []tls.Certificate{clientCert},
+					ellipticCurves:        defaultCurves,
 					localSignatureSchemes: signaturehash.Algorithms(),
 					insecureSkipVerify:    true,
 					log:                   logger,
@@ -296,6 +298,7 @@ func TestHandshaker(t *testing.T) {
 				cfg := &handshakeConfig{
 					localCipherSuites:     cipherSuites,
 					localCertificates:     []tls.Certificate{clientCert},
+					ellipticCurves:        defaultCurves,
 					localSignatureSchemes: signaturehash.Algorithms(),
 					insecureSkipVerify:    true,
 					log:                   logger,
diff --git a/pkg/crypto/elliptic/elliptic_test.go b/pkg/crypto/elliptic/elliptic_test.go
new file mode 100644
index 0000000..0011a30
--- /dev/null
+++ b/pkg/crypto/elliptic/elliptic_test.go
@@ -0,0 +1,24 @@
+package elliptic
+
+import "testing"
+
+func TestString(t *testing.T) {
+	tests := []struct {
+		in  Curve
+		out string
+	}{
+		{X25519, "X25519"},
+		{P256, "P-256"},
+		{P384, "P-384"},
+		{0, "0x0"},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.out, func(t *testing.T) {
+			if tt.in.String() != tt.out {
+				t.Fatalf("Expected: %s, got: %s", tt.out, tt.in.String())
+			}
+		})
+	}
+}