Replace rcgen with custom X.509 certificate generation #106
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'm not actually clear on why we want support for building a self-signed cert? In such cases shouldn't we just not use TLS?
| tokio = { version = "1.38.0", default-features = false, features = ["time", "signal", "rt-multi-thread"] } | ||
| tokio-rustls = { version = "0.26", default-features = false, features = ["ring"] } | ||
| rcgen = { version = "0.13", default-features = false, features = ["ring"] } | ||
| ring = { version = "0.17", default-features = false } |
There was a problem hiding this comment.
Given we're gonna ship a binary, I do very much wonder whether we shouldn't use native-tls across the stack in ldk-server. That would avoid us being responsible for shipping updates to ring and rustls in case of a security issue (though honestly I'm not entirely clear on why we want TLS for the RPC to begin with)
There was a problem hiding this comment.
Note that if we did so we could retain the cert-building logic here by switching to secp256k1 rather than secp256r1-via-ring.
| Ok(der_sequence(&san_ext)) | ||
| } | ||
|
|
||
| fn build_san_extension(hosts: &[String]) -> Result<Vec<u8>, String> { |
There was a problem hiding this comment.
given its self-signed anyway why are we building a SAN? Can't we just have a fixed CN of localhost and call it a day?
There was a problem hiding this comment.
There's been many times where I need to do this with lnd. A lot with docker containers where we need to add the container's name to the cert so other containers can call on the rpc
There was a problem hiding this comment.
Right but its a self-signed cert so the hostname is usually not validated? Or are there things that validate the hostname even for a self-signed cert?
There was a problem hiding this comment.
I am pretty sure the hostname is validated because I normally am not able to get anything to work unless I add it to the cert
There was a problem hiding this comment.
I mean that's up to the client. In browsers its definitely not. Given we're likely to mis-auto-detect the hostname (or eg someone might have an alias we're not aware of, eg via tailscale), we should probably prime the ecosystem to not validate for self-signed certs so that. IMO hostname validation on self-signed certs is marginally dangerous as people might treat it as gospel.
ldk-server/src/main.rs
Outdated
| let mut rng = rand::thread_rng(); | ||
| let mut key_bytes = [0u8; 32]; | ||
| rng.fill(&mut key_bytes); | ||
| getrandom::getrandom(&mut key_bytes) |
There was a problem hiding this comment.
rand as a dep isn't actually removed?
There was a problem hiding this comment.
yeah its still a transitive dep but if our underlying crates stop using it, we will be fine now
There was a problem hiding this comment.
it never was in there
There was a problem hiding this comment.
Uh huhhhhhhhhhhhhhhhh :)
diff --git a/ldk-server/Cargo.toml b/ldk-server/Cargo.toml
index 965c960..54f4626 100644
--- a/ldk-server/Cargo.toml
+++ b/ldk-server/Cargo.toml
@@ -1,40 +1,40 @@
[package]
name = "ldk-server"
version = "0.1.0"
edition = "2021"
[dependencies]
ldk-node = { git = "https://github.com/lightningdevkit/ldk-node", rev = "d1bbf978c8b7abe87ae2e40793556c1fe4e7ea49" }
serde = { version = "1.0.203", default-features = false, features = ["derive"] }
hyper = { version = "1", default-features = false, features = ["server", "http1"] }
http-body-util = { version = "0.1", default-features = false }
hyper-util = { version = "0.1", default-features = false, features = ["server-graceful"] }
tokio = { version = "1.38.0", default-features = false, features = ["time", "signal", "rt-multi-thread"] }
tokio-rustls = { version = "0.26", default-features = false, features = ["ring"] }
-rcgen = { version = "0.13", default-features = false, features = ["ring"] }
+ring = { version = "0.17", default-features = false }
+getrandom = { version = "0.2", default-features = false }
prost = { version = "0.11.6", default-features = false, features = ["std"] }
ldk-server-protos = { path = "../ldk-server-protos" }
bytes = { version = "1.4.0", default-features = false }
hex = { package = "hex-conservative", version = "0.2.1", default-features = false }
rusqlite = { version = "0.31.0", features = ["bundled"] }
-rand = { version = "0.8.5", default-features = false }
Implement self-signed certificate generation using ring directly, removing the rcgen dependency along with its transitive dependencies (yasna, time). ring was already in our dependency tree via tokio-rustls. Co-Authored-By: Claude Opus 4.5 <[email protected]>
benthecarman
force-pushed
the
rm-rcgen
branch
from
7b39d0b to
31e8feb
Compare
there are definitely cases where you want encryption but dont want to go through the hassle of the CA and can just share the certificate directly |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I didn't test this, but I blindly trust Claude to do things right the first time with no mistakes.
3 participants
Implement self-signed certificate generation using ring directly, removing the rcgen dependency along with its transitive dependencies (yasna, time). ring was already in our dependency tree via tokio-rustls.