Repository: pion/dtls
Pull request #357 (2021-03-26 15:22:35)

Add missing *rsa.PrivateKey into validating config certs private keys
#### Description

validates private keys of certs to be one of RSA, ECDSA or ED25519
everywhere else the RSA is correctly supported but the config validation

#### Reference issue
No issues


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/357?src=pr&el=h1) Report
> Merging [#357](https://codecov.io/gh/pion/dtls/pull/357?src=pr&el=desc) (3888c2a) into [master](https://codecov.io/gh/pion/dtls/commit/4665c3cde869e2ae9dd1cbfce827d853a6c3274f?el=desc) (4665c3c) will **decrease** coverage by `9.96%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
- Coverage   75.87%   65.90%   -9.97%     
==========================================
  Files          86       62      -24     
  Lines        3672     3115     -557     
==========================================
- Hits         2786     2053     -733     
- Misses        601      840     +239     
+ Partials      285      222      -63     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `65.90% <100.00%> (+0.01%)` | :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/357?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [config.go](https://codecov.io/gh/pion/dtls/pull/357/diff?src=pr&el=tree#diff-Y29uZmlnLmdv) | `90.00% <100.00%> (-10.00%)` | :arrow_down: |
| [pkg/protocol/errors.go](https://codecov.io/gh/pion/dtls/pull/357/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/357/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/357/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/357/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/357/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/357/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/357/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2hhbmRzaGFrZS9oYW5kc2hha2UuZ28=) | `0.00% <0.00%> (-70.84%)` | :arrow_down: |
| [pkg/protocol/extension/extension.go](https://codecov.io/gh/pion/dtls/pull/357/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi9leHRlbnNpb24uZ28=) | `11.36% <0.00%> (-61.37%)` | :arrow_down: |
| [pkg/protocol/extension/use\_master\_secret.go](https://codecov.io/gh/pion/dtls/pull/357/diff?src=pr&el=tree#diff-cGtnL3Byb3RvY29sL2V4dGVuc2lvbi91c2VfbWFzdGVyX3NlY3JldC5nbw==) | `0.00% <0.00%> (-53.85%)` | :arrow_down: |
| ... and [56 more](https://codecov.io/gh/pion/dtls/pull/357/diff?src=pr&el=tree-more) | |

------

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

misak113: Do you have idea how to fix lint error
```
  Error: SA1019: package crypto/dsa is deprecated: DSA is a legacy algorithm, and modern alternatives such as Ed25519 (implemented by package crypto/ed25519) should be used instead. Keys with 1024-bit moduli (L1024N160 parameters) are cryptographically weak, while bigger keys are not widely supported. Note that FIPS 186-5 no longer approves DSA for signature generation.  (staticcheck)
```

DSA is used only in tests on purpose to validate wrong input certificate private key.
Sean-Der: Hey @misak113 

thank you so much for the PR! You can add a `//nolint` to the line in question 
misak113: Ok, PR is hopefully ready for review.

Patch:
diff --git a/README.md b/README.md
index 62cac7c..9f7e4a0 100644
--- a/README.md
+++ b/README.md
@@ -150,6 +150,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [Jim Wert](https://github.com/bocajim)
 * [Alvaro Viebrantz](https://github.com/alvarowolfx)
 * [Kegan Dougal](https://github.com/Kegsay)
+* [Michael Zabka](https://github.com/misak113)
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text
diff --git a/config.go b/config.go
index 5ad42da..d94fbd6 100644
--- a/config.go
+++ b/config.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/ecdsa"
 	"crypto/ed25519"
+	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
 	"io"
@@ -182,6 +183,7 @@ func validateConfig(config *Config) error {
 			switch cert.PrivateKey.(type) {
 			case ed25519.PrivateKey:
 			case *ecdsa.PrivateKey:
+			case *rsa.PrivateKey:
 			default:
 				return errInvalidPrivateKey
 			}
diff --git a/go.mod b/go.mod
index f7f93dd..d3c660a 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ require (
 	github.com/pion/transport v0.12.3
 	github.com/pion/udp v0.1.1
 	golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2
-	golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4
+	golang.org/x/net v0.0.0-20210331212208-0fccb6fa2b5c
 	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
 )
 
diff --git a/go.sum b/go.sum
index 186eccf..df09e4c 100644
--- a/go.sum
+++ b/go.sum
@@ -21,13 +21,13 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn
 golang.org/x/net v0.0.0-20201201195509-5d6afe98e0b7/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU=
 golang.org/x/net v0.0.0-20210119194325-5f4716e94777/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
 golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
-golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4 h1:b0LrWgu8+q7z4J+0Y3Umo5q1dL7NXBkKBWkaVkAq17E=
-golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4/go.mod h1:RBQZq4jEuRlivfhVLdyRGr576XBO4/greRjx4P4O3yc=
+golang.org/x/net v0.0.0-20210331212208-0fccb6fa2b5c h1:KHUzaHIpjWVlVVNh65G3hhuj3KB1HnjY6Cq5cTvRQT8=
+golang.org/x/net v0.0.0-20210331212208-0fccb6fa2b5c/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.0.0-20210315160823-c6e025ad8005/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=


Test patch:
diff --git a/config_test.go b/config_test.go
index 695cbcc..a5a0772 100644
--- a/config_test.go
+++ b/config_test.go
@@ -1,9 +1,10 @@
 package dtls
 
 import (
+	"crypto/dsa" //nolint
+	"crypto/rand"
+	"crypto/rsa"
 	"crypto/tls"
-	"crypto/x509"
-	"encoding/pem"
 	"errors"
 	"testing"
 
@@ -68,14 +69,20 @@ func TestValidateConfig(t *testing.T) {
 	}
 
 	// Invalid private key
-	block, _ := pem.Decode([]byte(rawPrivateKey))
-	rsaKey, err := x509.ParsePKCS1PrivateKey(block.Bytes)
+	dsaPrivateKey := &dsa.PrivateKey{}
+	err = dsa.GenerateParameters(&dsaPrivateKey.Parameters, rand.Reader, dsa.L1024N160)
 	if err != nil {
-		t.Fatalf("TestValidateConfig: Config validation error(%v), parsing RSA private key", err)
+		t.Fatalf("TestValidateConfig: Config validation error(%v), DSA parameters not generated", err)
+		return
+	}
+	err = dsa.GenerateKey(dsaPrivateKey, rand.Reader)
+	if err != nil {
+		t.Fatalf("TestValidateConfig: Config validation error(%v), DSA private key not generated", err)
+		return
 	}
 	config = &Config{
 		CipherSuites: []CipherSuiteID{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
-		Certificates: []tls.Certificate{{Certificate: cert.Certificate, PrivateKey: rsaKey}},
+		Certificates: []tls.Certificate{{Certificate: cert.Certificate, PrivateKey: dsaPrivateKey}},
 	}
 	if err = validateConfig(config); !errors.Is(err, errInvalidPrivateKey) {
 		t.Fatalf("TestValidateConfig: Client error exp(%v) failed(%v)", errInvalidPrivateKey, err)
@@ -97,9 +104,14 @@ func TestValidateConfig(t *testing.T) {
 	}
 
 	// Valid config
+	rsaPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("TestValidateConfig: Config validation error(%v), RSA private key not generated", err)
+		return
+	}
 	config = &Config{
 		CipherSuites: []CipherSuiteID{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
-		Certificates: []tls.Certificate{cert},
+		Certificates: []tls.Certificate{cert, {Certificate: cert.Certificate, PrivateKey: rsaPrivateKey}},
 	}
 	if err = validateConfig(config); err != nil {
 		t.Fatalf("TestValidateConfig: Client error exp(%v) failed(%v)", nil, err)