feat: Rotate WebRTC Direct certificates #2997

Aryakoste · 2025-02-23T18:56:09Z

Title

Rotating web RTC certificate when expiring

Description

Fixes #2989

Notes & open questions

I know this PR is not be perfect. Please provide me suggestions or changes that needed to be done.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation if necessary (this includes comments as well)
I have added tests that prove my fix is effective or that my feature works

achingbrain

Thanks for opening this, comments inline.

achingbrain · 2025-03-05T16:02:47Z

packages/transport-webrtc/src/private-to-public/listener.ts

@@ -154,20 +164,19 @@ export class WebRTCDirectListener extends TypedEventEmitter<ListenerEvents> impl
      server: Promise.resolve()
        .then(async (): Promise<StunServer> => {
          // ensure we have a certificate
-          if (this.certificate == null) {
+          if (this.certificate == null || this.isCertificateExpiring()) {


This check will only run when the node is started.

If the certificate expires while the node is running it will continue using it and connections will start to fail.

Instead a timeout should be set that fires a short(ish) time before the certificate expiry that creates a new certificate, restarts the server with the new cert and emits the "listening" event.

achingbrain · 2025-03-05T16:05:16Z

packages/transport-webrtc/src/index.ts

@@ -280,6 +280,7 @@ export interface TransportCertificate {
   * The hash of the certificate
   */
  certhash: string
+  notAfter: string


This would be better as a number so we don't have to convert the string back to a date in order to see if the cert is about to expire, instead it can just be compared to Date.now() - threshold .

achingbrain · 2025-03-05T16:05:26Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const expiryDate = new Date(this.certificate.notAfter)
+    const now = new Date()
+    const timeToExpiry = expiryDate.getTime() - now.getTime()
+    const threshold = 30 * 24 * 60 * 60 * 1000


threshold should be configurable.

achingbrain · 2025-03-05T16:06:13Z

packages/transport-webrtc/test/certificate.spec.ts

+      peerId: { toB58String: () => 'QmPeerId' } as any,
+      privateKey: {} as any,
+      logger: {
+        forComponent: () => ({
+          trace: () => {},
+          error: () => {}
+        })
+      } as any,


These fields can be filled using actual values, see other tests here for an example.

achingbrain · 2025-03-05T16:06:38Z

packages/transport-webrtc/test/certificate.spec.ts

+          error: () => {}
+        })
+      } as any,
+      transportManager: {} as any


You can use stubInterface<TransportManager>() from sinon-ts here to avoid casting to any.

Aryakoste · 2025-03-16T08:13:38Z

Making this changes

achingbrain · 2025-03-18T13:35:12Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const now = new Date()
+    const timeToExpiry = expiryDate - now.getTime()


No need to allocate a new Date object:

Suggested change

const now = new Date()

const timeToExpiry = expiryDate - now.getTime()

const timeToExpiry = expiryDate - Date.now()

achingbrain · 2025-03-18T13:36:04Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const expiryDate = this.certificate.notAfter
+    const now = new Date()
+    const timeToExpiry = expiryDate - now.getTime()
+    const threshold = this.init.certificateExpiryThreshold ?? 7 * 86400000


7 * 86400000

Please make this value a constant at the top of the file.

achingbrain · 2025-03-18T13:38:05Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const timeToExpiry = expiryDate.getTime() - now.getTime()
+    const timeoutDuration = timeToExpiry - 7 * 86400000
+
+    setTimeout(async () => {


If node.stop() is called, this timeout will prevent the process from stopping until it fires when the certificate expires long into the future.

Instead you should store a ref to the returned value and pass it to clearTimeout when the listener is stopped.

achingbrain · 2025-03-18T13:40:09Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const now = new Date()
+    const timeToExpiry = expiryDate.getTime() - now.getTime()


No need to allocate a new Date object:

Suggested change

const now = new Date()

const timeToExpiry = expiryDate.getTime() - now.getTime()

const timeToExpiry = expiryDate.getTime() - Date.now()

achingbrain · 2025-03-18T13:41:57Z

packages/transport-webrtc/src/private-to-public/listener.ts

+    const expiryDate = new Date(this.certificate.notAfter)
+    const now = new Date()
+    const timeToExpiry = expiryDate.getTime() - now.getTime()
+    const timeoutDuration = timeToExpiry - 7 * 86400000


This should use the configurable threshold.

7 * 86400000

Please make/reuse this default value as a constant at the top of the file.

achingbrain · 2025-03-18T13:50:38Z

packages/transport-webrtc/test/certificate.spec.ts

+  it('should re-emit listening event when a new certificate is generated', async () => {
+    const emitSpy = sinon.spy(listener as any, 'safeDispatchEvent')
+    const generateCertificateSpy = sinon.spy(generateTransportCertificate)
+
+    await (listener as any).startUDPMuxServer('127.0.0.1', 0)
+
+    expect(generateCertificateSpy.called).to.be.true
+    expect(emitSpy.calledWith('listening')).to.be.true
+  })
+
+  it('should generate a new certificate before expiry with default threshold', async () => {
+    init.certificateExpiryThreshold = undefined
+    listener = new WebRTCDirectListener(components, init)
+    ;(listener as any).certificate = {
+      notAfter: new Date(Date.now() + 5 * 86400000).getTime()
+    }
+
+    const isCertificateExpiringSpy = sinon.spy(listener as any, 'isCertificateExpiring')
+    const generateCertificateSpy = sinon.spy(listener as any, 'createAndSetCertificate')
+
+    await (listener as any).startUDPMuxServer('127.0.0.1', 0)
+
+    expect(isCertificateExpiringSpy.returned(true)).to.be.true
+    expect(generateCertificateSpy.called).to.be.true
+  })
+
+  it('should generate a new certificate before expiry with custom threshold', async () => {
+    (listener as any).certificate = {
+      notAfter: new Date(Date.now() + 4 * 86400000).getTime()
+    }
+
+    const isCertificateExpiringSpy = sinon.spy(listener as any, 'isCertificateExpiring')
+    const generateCertificateSpy = sinon.spy(listener as any, 'createAndSetCertificate')
+
+    await (listener as any).startUDPMuxServer('127.0.0.1', 0)
+
+    expect(isCertificateExpiringSpy.returned(true)).to.be.true
+    expect(generateCertificateSpy.called).to.be.true
+  })


For these tests please do not depend on internal state (e.g. private fields and methods), it makes the tests fragile and which then makes them hard to maintain.

Instead, make the WebRTCDirectListener.certificate field public, configure short timeouts for certificate expiry/renewal thresholds (e.g. under 100ms), use delay and assert on the certificate status after time has passed.

achingbrain · 2025-03-18T13:51:09Z

packages/transport-webrtc/src/index.ts

@@ -283,6 +283,7 @@ export interface TransportCertificate {
   * The hash of the certificate
   */
  certhash: string
+  notAfter: number


Please add a jsdoc comment explaining what this field is for.

feat: Rotate WebRTC Direct certificates

4666793

Aryakoste requested a review from a team as a code owner February 23, 2025 18:56

changes to the test file

2c545d8

achingbrain requested changes Mar 5, 2025

View reviewed changes

Aryakoste added 2 commits March 16, 2025 15:44

feat: configuratle threshold and timeout before certificate expiry

e219b6b

fixed merge conflicts

0828261

achingbrain requested changes Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rotate WebRTC Direct certificates #2997

feat: Rotate WebRTC Direct certificates #2997

Aryakoste commented Feb 23, 2025

achingbrain left a comment

achingbrain Mar 5, 2025

achingbrain Mar 5, 2025

achingbrain Mar 5, 2025

achingbrain Mar 5, 2025

achingbrain Mar 5, 2025

Aryakoste commented Mar 16, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

achingbrain Mar 18, 2025

		const now = new Date()
		const timeToExpiry = expiryDate - now.getTime()

	const now = new Date()
	const timeToExpiry = expiryDate - now.getTime()
	const timeToExpiry = expiryDate - Date.now()

		const now = new Date()
		const timeToExpiry = expiryDate.getTime() - now.getTime()

feat: Rotate WebRTC Direct certificates #2997

Are you sure you want to change the base?

feat: Rotate WebRTC Direct certificates #2997

Conversation

Aryakoste commented Feb 23, 2025

Title

Description

Notes & open questions

Change checklist

achingbrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aryakoste commented Mar 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment