Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Rotate WebRTC Direct certificates #2997

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/transport-webrtc/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ export interface TransportCertificate {
* The hash of the certificate
*/
certhash: string
notAfter: number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

export type { WebRTCTransportDirectInit, WebRTCDirectTransportComponents }
Expand Down
57 changes: 44 additions & 13 deletions packages/transport-webrtc/src/private-to-public/listener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export interface WebRTCDirectListenerInit {
dataChannel?: DataChannelOptions
rtcConfiguration?: RTCConfiguration | (() => RTCConfiguration | Promise<RTCConfiguration>)
useLibjuice?: boolean
certificateDuration?: number
certificateExpiryThreshold?: number
}

export interface WebRTCListenerMetrics {
Expand Down Expand Up @@ -82,6 +84,15 @@ export class WebRTCDirectListener extends TypedEventEmitter<ListenerEvents> impl
}
}

private isCertificateExpiring (): boolean {
if (this.certificate == null) return true
const expiryDate = this.certificate.notAfter
const now = new Date()
const timeToExpiry = expiryDate - now.getTime()
Comment on lines +90 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to allocate a new Date object:

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

const threshold = this.init.certificateExpiryThreshold ?? 7 * 86400000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 * 86400000

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

return timeToExpiry < threshold
}

async listen (ma: Multiaddr): Promise<void> {
const { host, port } = ma.toOptions()

Expand Down Expand Up @@ -133,20 +144,9 @@ 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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.log.trace('creating TLS certificate')
const keyPair = await crypto.subtle.generateKey({
name: 'ECDSA',
namedCurve: 'P-256'
}, true, ['sign', 'verify'])

const certificate = await generateTransportCertificate(keyPair, {
days: 365 * 10
})

if (this.certificate == null) {
this.certificate = certificate
}
await this.createAndSetCertificate()
}

if (port === 0) {
Expand All @@ -171,6 +171,37 @@ export class WebRTCDirectListener extends TypedEventEmitter<ListenerEvents> impl
}
}

private async createAndSetCertificate (): Promise<void> {
const keyPair = await crypto.subtle.generateKey({
name: 'ECDSA',
namedCurve: 'P-256'
}, true, ['sign', 'verify'])

const certificate = await generateTransportCertificate(keyPair, {
days: this.init.certificateDuration ?? 365 * 10
})

this.certificate = certificate
this.setCertificateExpiryTimeout()
}

private setCertificateExpiryTimeout (): void {
if (this.certificate == null) {
return
}

const expiryDate = new Date(this.certificate.notAfter)
const now = new Date()
const timeToExpiry = expiryDate.getTime() - now.getTime()
Comment on lines +194 to +195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

const timeoutDuration = timeToExpiry - 7 * 86400000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the configurable threshold.

7 * 86400000

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


setTimeout(async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.log.trace('renewing TLS certificate')
await this.createAndSetCertificate()
this.safeDispatchEvent('listening')
}, timeoutDuration)
}

private async incomingConnection (ufrag: string, remoteHost: string, remotePort: number, signal: AbortSignal): Promise<void> {
const key = `${remoteHost}:${remotePort}:${ufrag}`
let peerConnection = this.connections.get(key)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export async function generateTransportCertificate (keyPair: CryptoKeyPair, opti
return {
privateKey: privateKeyPem,
pem: cert.toString('pem'),
certhash: base64url.encode((await sha256.digest(new Uint8Array(cert.rawData))).bytes)
certhash: base64url.encode((await sha256.digest(new Uint8Array(cert.rawData))).bytes),
notAfter: notAfter.getTime()
}
}
94 changes: 94 additions & 0 deletions packages/transport-webrtc/test/certificate.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { defaultLogger } from '@libp2p/logger'
import { Crypto } from '@peculiar/webcrypto'
import { expect } from 'aegir/utils/chai.js'
import sinon from 'sinon'
import { stubInterface } from 'sinon-ts'
import { WebRTCDirectListener, type WebRTCDirectListenerInit } from '../src/private-to-public/listener.js'
import { type WebRTCDirectTransportComponents } from '../src/private-to-public/transport.js'
import { generateTransportCertificate } from '../src/private-to-public/utils/generate-certificates.js'
import type { TransportCertificate } from '../src/index.js'
import type { TransportManager } from '@libp2p/interface-internal'

const crypto = new Crypto()

describe('WebRTCDirectListener', () => {
let listener: WebRTCDirectListener
let components: WebRTCDirectTransportComponents
let init: WebRTCDirectListenerInit

beforeEach(() => {
components = {
peerId: { toB58String: () => 'QmPeerId' } as any,
privateKey: {} as any,
logger: defaultLogger(),
transportManager: stubInterface<TransportManager>()
}

init = {
certificateDuration: 10,
upgrader: {} as any
}

listener = new WebRTCDirectListener(components, init)
})

afterEach(() => {
sinon.restore()
})

it('should generate a certificate with the configured duration', async () => {
const keyPair = await crypto.subtle.generateKey({
name: 'ECDSA',
namedCurve: 'P-256'
}, true, ['sign', 'verify'])

const certificate: TransportCertificate = await generateTransportCertificate(keyPair, {
days: init.certificateDuration!
})

expect(new Date(certificate.notAfter).getTime()).to.be.closeTo(
new Date().getTime() + init.certificateDuration! * 86400000,
1000
)
})

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
})
Comment on lines +55 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})