feat(webrtc): certificate, can now be managed like node secret key#12621
Open
gab8i wants to merge 2 commits into
Open
feat(webrtc): certificate, can now be managed like node secret key#12621gab8i wants to merge 2 commits into
gab8i wants to merge 2 commits into
Conversation
The WebRTC certificate, which determines the node's WebRTC `/certhash`, can now be managed like the node secret key. A new `key generate-webrtc-certificate` subcommand generates a certificate, writes it to a file (or stdout) and prints the corresponding certhash to stderr, mirroring `key generate-node-key`. Additionally, `--experimental-webrtc` now accepts an optional certificate file path (`--experimental-webrtc=<CERT_FILE>`): the certificate is loaded from the file if it exists, or generated and persisted there otherwise. Without a path the behavior is unchanged, the certificate is loaded from or generated at `<base_path>/chains/<chain_id>/network/webrtc_certificate`.
Contributor
Author
|
/cmd prdoc |
dmitry-markin
approved these changes
Jul 10, 2026
dmitry-markin
left a comment
dmitry-markin
left a comment
Contributor
There was a problem hiding this comment.
Looks good with a couple of nits:
- We should use dedicated error types for
Result::Errinstead ofStringeverywhere, so it's possible to plug these types into higher-level error handling (e.g. use as[from]inthiserror::Error, use withanyhow!, etc.).#[derive(thiserror::Error)]is a convention for error types in substrate. - I don't think we need to encode data as hex on disk at all.
Comment on lines
+82
to
+90
| /// Optionally takes a path to the WebRTC DTLS certificate file | ||
| /// (`--experimental-webrtc=<CERT_FILE>`), which determines the node's `/certhash`. | ||
| /// The certificate is loaded from the file if it exists, otherwise a new certificate | ||
| /// is generated and persisted there. It can be generated ahead of time with the | ||
| /// `key generate-webrtc-certificate` subcommand, which accepts both hex and raw | ||
| /// binary formats. When no path is given, the certificate is loaded from or | ||
| /// generated at `<base_path>/chains/<chain_id>/network/webrtc_certificate`. | ||
| #[arg(long, value_name = "CERT_FILE", num_args = 0..=1, require_equals = true)] | ||
| pub experimental_webrtc: Option<Option<PathBuf>>, |
Contributor
There was a problem hiding this comment.
I would add a dedicated --webrtc-certificate=<CERT_FILE> switch from the very beginning. This way we avoid ambiguity with a switch having uncertain number of arguments, and once --experimental-webrtc is decomissioned, the --webrtc-certificate stays.
| (None, None) => { | ||
| log::warn!( | ||
| target: LOG_TARGET, | ||
| "WebRtc enabled but no networking path specified, using an ephemeral certificate" |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| "WebRtc enabled but no networking path specified, using an ephemeral certificate" | |
| "WebRTC enabled but no networking path specified, using an ephemeral certificate" |
|
|
||
| /// Compute the `/certhash/<hash>` multiaddress component of a certificate. | ||
| pub fn certhash(certificate: &DtlsCertificate) -> String { | ||
| let hash = Code::Sha2_256.digest(certificate.as_parts().0); |
Contributor
There was a problem hiding this comment.
Shouldn't this be part of DtlsCertificate on litep2p side to avoid errors?
Comment on lines
+112
to
+115
|
|
||
| let hex = array_bytes::bytes2hex("", &encoded); | ||
| let decoded = decode_certificate(hex.as_bytes()).unwrap(); | ||
| assert_eq!(decoded.as_parts(), certificate.as_parts()); |
Contributor
There was a problem hiding this comment.
This is testing array_bytes :)
Suggested change
| let hex = array_bytes::bytes2hex("", &encoded); | |
| let decoded = decode_certificate(hex.as_bytes()).unwrap(); | |
| assert_eq!(decoded.as_parts(), certificate.as_parts()); |
|
|
||
| #[test] | ||
| fn read_or_generate_persists_certificate() { | ||
| let dir = tempfile::tempdir().unwrap(); |
Contributor
There was a problem hiding this comment.
This will likely create dir, so won't test the parent directory creation by our function.
| .ok() | ||
| .or_else(|| { | ||
| let hex = std::str::from_utf8(bytes).ok()?; | ||
| let raw = array_bytes::hex2bytes(hex.trim()).ok()?; |
Contributor
There was a problem hiding this comment.
Why do we decode hex if encode_certificate is not encoding hex? Encoding/decoding should be symmetric without hidden details.
| .ok() | ||
| .or_else(|| { | ||
| let hex = std::str::from_utf8(bytes).ok()?; | ||
| let raw = array_bytes::hex2bytes(hex.trim()).ok()?; |
Contributor
There was a problem hiding this comment.
Why do we need hex encoding at the file level at all and not just use binary data?
Comment on lines
+46
to
+50
| /// The output is in raw binary format. | ||
| /// If not given, the output is written as an hex encoded string. | ||
| #[arg(long)] | ||
| bin: bool, | ||
|
|
Contributor
There was a problem hiding this comment.
IMO we should drop hexifying completely and store binary data only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The WebRTC certificate, which determines the node's WebRTC
/certhash, can now be managed like the node secret key.Integration
A new
key generate-webrtc-certificatesubcommand generates a certificate, writes it to a file (or stdout) and prints the corresponding certhash to stderr, mirroringkey generate-node-key.Additionally,
--experimental-webrtcnow accepts an optional certificate file path (--experimental-webrtc=<CERT_FILE>): the certificate is loaded from the file if it exists, or generated and persisted there otherwise.Without a path the behavior is unchanged, the certificate is loaded from or generated at
<base_path>/chains/<chain_id>/network/webrtc_certificate.