feat: implement WebRTC certificate storage in the keychain #3048
Conversation
achingbrain
changed the title
There was a problem hiding this comment.
Thanks for opening this. As implemented it won't actually persist the certificate as it sets it as a _certificate field on a private key.
As of #3059 we can now generate and store ECDSA keypairs as PrivateKeys and as of #3062 we can store PEM-encoded x509 certificates in the keychain which you may have been missing.
Please can you update the implementation to use these new methods.
| try { | ||
| // Check if the private key has our expected certificate metadata | ||
| // @ts-expect-error - We're checking for custom properties added during storage | ||
| if (privateKey._certificate == null) { |
There was a problem hiding this comment.
This field will not be present when the key is deserialized by the keychain.
| // Create a node with certificate persistence enabled | ||
| const node = await createNode(null, { | ||
| certificates: [], // Empty array to not use any pre-defined certificates | ||
| useLibjuice: false, // Use WebRTC's built-in STUN/TURN |
There was a problem hiding this comment.
This option is ignored, it will only use libjuice now.
| useLibjuice: false, // Use WebRTC's built-in STUN/TURN |
| rtcConfiguration: { | ||
| iceTransportPolicy: 'all', | ||
| } |
There was a problem hiding this comment.
I don't think this config is relevant to the test.
| rtcConfiguration: { | |
| iceTransportPolicy: 'all', | |
| } |
| ], | ||
| services: { | ||
| keychain: async (components) => { | ||
| const keychainModule = await import('@libp2p/keychain') |
There was a problem hiding this comment.
Why is the dynamic import necessary?
| @@ -0,0 +1,176 @@ | |||
| /** | |||
There was a problem hiding this comment.
Because this test imports libp2p and other modules, it should be moved to /packages/integration-tests in this monorepo.
Only unit tests should be added here. Obviously the definition is hazy but if you're starting a libp2p node or having to jump through hoops to import other modules from the monorepo, it's probably an integration test.
| const validityDays = options.validityDays ?? DEFAULT_CERTIFICATE_VALIDITY_DAYS | ||
|
|
||
| log?.trace('generating new ECDSA P-256 key pair') | ||
| const keyPair = await crypto.subtle.generateKey({ |
There was a problem hiding this comment.
You can now import generateKeyPair from @libp2p/crypto/keys and do await generateKeyPair('ECDSA').
Then the keypair can be stored in the keychain and you can use privateKeyToCryptoKeyPair (also imported from @libp2p/crypto/keys) to get a WebCrypto-friendly keypair to generate the cert.
| connectionGater: { | ||
| denyDialMultiaddr: async () => false | ||
| }, | ||
| streamMuxers: [mplex()], | ||
| connectionEncrypters: [noise()], |
There was a problem hiding this comment.
Why do you need to supply a muxer/encrypter?
| webRTCDirect({ | ||
| ...options, | ||
| dataChannel: { | ||
| maxMessageSize: 1 << 16 | ||
| } | ||
| }) |
There was a problem hiding this comment.
Why do you need to set the message size?
| await node.start() | ||
|
|
||
| // Wait for addresses to be available | ||
| await new Promise(resolve => setTimeout(resolve, 1000)) |
There was a problem hiding this comment.
Please don't add arbitrary delays, they make the tests take a long time to run and can be flakey.
Instead, use something like p-wait-for and test for the thing you are waiting for.
| * This test checks that the WebRTC transport is at least attempting to persist and retrieve certificates. | ||
| * After fixing the createPrivateKeyFromCertificate implementation, it should now work properly. | ||
| */ | ||
| it('should attempt certificate persistence between restarts', async function() { |
There was a problem hiding this comment.
This test should just start a node, record any WebRTC-Direct multiaddrs it's listening on, then stop the node and start it again and assert that it's re-used the same cert. No need for deep stubbing of module internals.
Title
feat: Implement WebRTC certificate persistence
Description
This PR implements the WebRTC certificate persistence functionality. The following changes have been made:
certificate-store.tsto handle storing and retrieving certificates from the keychain.certificate-persistence.spec.ts.This addresses the issue of ensuring certificates can be stored and reused across node restarts, improving the reliability and efficiency of WebRTC connections.
Related #2988.
Notes & open questions
certificate-persistence.spec.tscontains two tests. One test passes successfully, while the second test related to persisting certificates between restarts still needs changes. Further debugging and adjustments are required to ensure all tests pass.Change checklist