Repository: pion/webrtc
Pull request #1742 (2021-04-01 15:55:42)

Add CertificateFromPEM and PEM() to Certificate
Allow for easier usage of a pre-existing certificate

Sean-Der: Are you depending on the ability to extract the Certificate from the PeerConnection with `(c Certificate) PEM()` ?

We can't duplicate/port this to WASM unfortunately. I am 100% for `CertificateFromPEM`, but `PEM()` will cause some portability issues. Happy to add if it is a show stopper for you!
codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1742?src=pr&el=h1) Report
> Merging [#1742](https://codecov.io/gh/pion/webrtc/pull/1742?src=pr&el=desc) (b88230d) into [master](https://codecov.io/gh/pion/webrtc/commit/e1c6662934c55f202d6fc92c761c4c0c9914a114?el=desc) (e1c6662) will **decrease** coverage by `4.93%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master    #1742      +/-   ##
==========================================
- Coverage   75.12%   70.18%   -4.94%     
==========================================
  Files          80       58      -22     
  Lines        5780     2160    -3620     
==========================================
- Hits         4342     1516    -2826     
+ Misses       1063      540     -523     
+ Partials      375      104     -271     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.18% <ø> (ø)` | |

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/webrtc/pull/1742?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-cnRwY29kZWMuZ28=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `5.26% <0.00%> (-57.90%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree#diff-ZHRsc3JvbGUuZ28=) | `68.96% <0.00%> (-31.04%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/pion/webrtc/pull/1742/diff?src=pr&el=tree-more) | |

------

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

daonb: I'm almost finished with the signaling server and one of the things it's doing is keeping the peers' fingerprints in the db and pass them together with the offer/answer. If the server's certificate is not persistent there is no way to verify the server's identity so it is a show stopper for me. 

That is unless you have a way of keeping a persistent certificate.

BTW - are you sure it doesn't work on WASM? I've tried running the test using `GOOS=js GOARCH=wasm go test -exec="./test-wasm/go_js_wasm_exec" -run PEM .` and it passes.
Sean-Der: Right now certificate_test.go isn't executed because it starts with `// +build !js`. 

The issue is that the browser doesn't give us the private key for a `RTCCertificate` so we can't provide the `PEM` API. 

So you goal is to generate one certificate and share between all PeerConnections on the server? Would this work?

* Generate certificate yourself (basically copy `GenerateCertificate`) and save to PEM yourself
* Call `GetFingerprints` on that certificate and save to DB
* For each connection call `CertificateFromPEM`
* Pass that into the `Configuration`

daonb: You're right, I can do all of this in my program. Problem is, the next user that needs certificate consistency will have to figure it out himself and it's hard work. 

Ceritifcates are one of those dark corners of code where there are too many standards, documentation and conflicting terms. I started this PR hoping to help future users save time and frustration.

I understand the browser's limitation - in terminal7 I use indexDB to get the fingerprint - and as pion uses the builtin RTCCertificate there's no way to access the private key. However, if certificate.go is not compiled under wasm I don't see the harm in providing a couple of helper functions. It's already a superset of w3c's interface and I don't see any harm if making it more super. 
Sean-Der: @daonb that's a good point :) here is what I am thinking. If you disagree/think I am worried about things that don't matter I say we merge!

* I don't want to embrace/extend/extinguish the WebRTC API. I want users to be able to switch between Pion and other implementations. If we add a non-standard API it will make it harder to switch and/or force others to implement non-standard things.
* I don't want to make a security mistake. They made a concious decision to not make the PrivateKey exportable in the browser. If we do that and something bad happens will look bad for Pion.

If you think my fears are unfounded merge away!
daonb: Thanks for letting me decide. I am a firm believer in the python proverb: "We're all consenting adults" and that's why I want you to merge. 

To recap, this PR is about adding certificate serialization. webexec uses it for persistent certificate but it doesn't matter. How the program uses it is up to the programmer. He's an adult and we should treat him as such.

Regarding WASM, it's a different world. In the browser we're not treated as adults. In the browser I had to go through hoops and use low-level IndexDB to get the public key. The private one is well hidden and javascript can't access it.

In the server world, we have a file system and a program can read the private key. When the user runs a program he makes it an "adult" and gives it access to a system wider and deeper than the browser. These capabilities can lead programmers to need more interfaces from pion. WASM compatbility shouldn't stop us from supporting them.

Sean-Der: @daonb I am 100% in support then! 

Mind fixing the merge conflict, then you can approve+merge. I added you to the Pion organization. I can't merge this PR since I am the one that opened it.
daonb: Sure. While I'm at it, I want to add mysekf to the authors. Mind if I copy
akl te names to a AUTHERS.txt

בתאריך יום ב׳, 5 באפר׳ 2021, 18:04, מאת Sean DuBois ‏<
***@***.***>:

> @daonb <https://github.com/daonb> I am 100% in support then!
>
> Mind fixing the merge conflict, then you can approve+merge. I added you to
> the Pion organization. I can't merge this PR since I am the one that opened
> it.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pion/webrtc/pull/1742#issuecomment-813441443>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAI75BJQGKQL54DOYCBXU3THHGOLANCNFSM42HLIXWA>
> .
>

Sean-Der: @daonb yes please! You will need to also

* Remove https://github.com/pion/.goassets/blob/master/ci/.github/workflows/lint.yaml#L25-L26
* Modify https://github.com/pion/.goassets/blob/master/ci/.github/assert-contributors.sh to generate `AUTHORS.txt` (not used by CI though)
* Push a new tag

Then you can see all the CI assets update PRs https://github.com/search?q=org%3Apion+type%3Apr+author%3Apionbot+is%3Aopen&type=Issues

We can split the PRs of moving from README.md -> AUTHORS.md
daonb: I'll be happy to do some house cleaning - my projects needs better linting and such and it looks like you got it covered.

 just fixed the conflict and tried to push to pion/webrtc and got permission denied. Maybe the branch is protected?
Sean-Der: @daonb sorry you have to hit the `Rebase and merge` button on this page. Just to make sure all the tests pass. If you force push to `pem-certificate` branch then we should be good to go!
daonb: Only problem is, I never accepted the invitation to join pion and now it's expired. Please invite me again.
Sean-Der: Done! 

Patch:
diff --git a/README.md b/README.md
index 0161cbef..5fc91791 100644
--- a/README.md
+++ b/README.md
@@ -270,6 +270,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [David Zhao](https://github.com/davidzhao)
 * [Nam V. Do](https://github.com/namvdo)
 * [Markus Tzoe](https://github.com/zyxar)
+* [Benny Daon](https://github.com/daonb)
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text
diff --git a/certificate.go b/certificate.go
index bf106d6a..b30515ad 100644
--- a/certificate.go
+++ b/certificate.go
@@ -11,8 +11,10 @@ import (
 	"crypto/x509/pkix"
 	"encoding/base64"
 	"encoding/hex"
+	"encoding/pem"
 	"fmt"
 	"math/big"
+	"strings"
 	"time"
 
 	"github.com/pion/dtls/v2/pkg/crypto/fingerprint"
@@ -145,7 +147,7 @@ func GenerateCertificate(secretKey crypto.PrivateKey) (*Certificate, error) {
 		BasicConstraintsValid: true,
 		NotBefore:             time.Now(),
 		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
-		NotAfter:              time.Now().AddDate(0, 1, 0),
+		NotAfter:              time.Now().AddDate(10, 0, 0),
 		SerialNumber:          serialNumber,
 		Version:               2,
 		Subject:               pkix.Name{CommonName: hex.EncodeToString(origin)},
@@ -183,3 +185,57 @@ func (c Certificate) collectStats(report *statsReportCollector) error {
 	report.Collect(stats.ID, stats)
 	return nil
 }
+
+// CertificateFromPEM creates a fresh certificate based on a string containing
+// pem blocks fort the private key and x509 certificate
+func CertificateFromPEM(pems string) (*Certificate, error) {
+	// decode & parse the certificate
+	block, more := pem.Decode([]byte(pems))
+	if block == nil || block.Type != "CERTIFICATE" {
+		return nil, errCertificatePEMFormatError
+	}
+	certBytes := make([]byte, base64.StdEncoding.DecodedLen(len(block.Bytes)))
+	n, err := base64.StdEncoding.Decode(certBytes, block.Bytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode ceritifcate: %w", err)
+	}
+	cert, err := x509.ParseCertificate(certBytes[:n])
+	if err != nil {
+		return nil, fmt.Errorf("failed parsing ceritifcate: %w", err)
+	}
+	// decode & parse the private key
+	block, _ = pem.Decode(more)
+	if block == nil || block.Type != "PRIVATE KEY" {
+		return nil, errCertificatePEMFormatError
+	}
+	privateKey, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse private key: %w", err)
+	}
+	x := CertificateFromX509(privateKey, cert)
+	return &x, nil
+}
+
+// PEM returns the certificate encoded as two pem block: once for the X509
+// certificate and the other for the private key
+func (c Certificate) PEM() (string, error) {
+	// First write the X509 certificate
+	var o strings.Builder
+	xcertBytes := make(
+		[]byte, base64.StdEncoding.EncodedLen(len(c.x509Cert.Raw)))
+	base64.StdEncoding.Encode(xcertBytes, c.x509Cert.Raw)
+	err := pem.Encode(&o, &pem.Block{Type: "CERTIFICATE", Bytes: xcertBytes})
+	if err != nil {
+		return "", fmt.Errorf("failed to pem encode the X certificate: %w", err)
+	}
+	// Next write the private key
+	privBytes, err := x509.MarshalPKCS8PrivateKey(c.privateKey)
+	if err != nil {
+		return "", fmt.Errorf("failed to marshal private key: %w", err)
+	}
+	err = pem.Encode(&o, &pem.Block{Type: "PRIVATE KEY", Bytes: privBytes})
+	if err != nil {
+		return "", fmt.Errorf("failed to encode private key: %w", err)
+	}
+	return o.String(), nil
+}
diff --git a/errors.go b/errors.go
index a9daf16d..2b8c9784 100644
--- a/errors.go
+++ b/errors.go
@@ -217,4 +217,6 @@ var (
 	errStatsICECandidateStateInvalid = errors.New("cannot convert to StatsICECandidatePairStateSucceeded invalid ice candidate state")
 
 	errICETransportNotInNew = errors.New("ICETransport can only be called in ICETransportStateNew")
+
+	errCertificatePEMFormatError = errors.New("bad Certificate PEM format")
 )


Test patch:
diff --git a/certificate_test.go b/certificate_test.go
index 4168d08c..873bee33 100644
--- a/certificate_test.go
+++ b/certificate_test.go
@@ -99,3 +99,18 @@ func TestGenerateCertificateExpires(t *testing.T) {
 	assert.NotNil(t, x509Cert)
 	assert.Contains(t, x509Cert.statsID, "certificate")
 }
+
+func TestPEM(t *testing.T) {
+	sk, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	assert.Nil(t, err)
+	cert, err := GenerateCertificate(sk)
+	assert.Nil(t, err)
+
+	pem, err := cert.PEM()
+	assert.Nil(t, err)
+	cert2, err := CertificateFromPEM(pem)
+	assert.Nil(t, err)
+	pem2, err := cert2.PEM()
+	assert.Nil(t, err)
+	assert.Equal(t, pem, pem2)
+}