Add support for the simple "sigs-based auth" VSS scheme #755
Conversation
TheBlueMatt
commented
Jan 15, 2026
TheBlueMatt
commented
At lightningdevkit/vss-server#79 we added a new, trivial, VSS authentication scheme that ensures client isolation without much else. This is great for testing, and we expect some to do new-account-rate-limiting via other means, so might well become a common default. Here we add support to it in ldk-node.
When we added the trivial sigs-based authentication scheme in VSS, we made it the default if no other authentication scheme was configured and default features are enabled. This broke our integration tests as we were expecting no authentication to be required in such a case. Here we fix this by switching to the new sigs-based auth scheme, removing `store_id`s to demonstrate client isolation while we're at it.
|
I've assigned @tnull as a reviewer! |
|
Depends on lightningdevkit/vss-client#54 |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, some comments. Mind adding an intermittent commit that bumps the dependency to your branch of vss-client so we can see if CI passes?
| .build_with_vss_store( | ||
| config_a.node_entropy, | ||
| vss_base_url.clone(), | ||
| "node_1_store".to_string(), |
There was a problem hiding this comment.
Why are we changing the store_id here and below?
There was a problem hiding this comment.
To demonstrate/test client isolation in VSS by default.
There was a problem hiding this comment.
Hmm, not sure it's worth the changes, but if you think it somehow preferable, sure..
There was a problem hiding this comment.
Entirely up to you. I did it cause coverage is coverage, even if the coverage is in totally the wrong repo. If you don't want coverage in the wrong repo, that's also totally fine :).
|
|
||
| let rand_suffix: String = | ||
| (0..7).map(|_| rng().sample(rand::distr::Alphanumeric) as char).collect(); | ||
| let store_id = format!("v0_compat_test_{}", rand_suffix); |
There was a problem hiding this comment.
No, please leave this in place, otherwise running this test repeatedly against the same backend won't start from scratch every time.
There was a problem hiding this comment.
Oops, sorry, missed that they're using a static seed. I randomized the seed.
There's not really any reason to force devs to select a `store_id` in the default `build_with_vss_store` anymore - we use a key derived from the node entropy to key the storage on the service side which should prevent anything else from accessing the same data unless its very deliberate. Thus, we simply drop the `store_id` parameter to `build_with_vss_store`.
|
Oops, fixed an issue but also pushed a new commit to drop the |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, would still be good to see a vss-client dependency bump (preferably before the next vss-client release so we get a chance to amend bugs).
I don't mind the test changes, but leaning NACK on the fixating the store_id suddenly, see below.
| .build_with_vss_store( | ||
| config_a.node_entropy, | ||
| vss_base_url.clone(), | ||
| "node_1_store".to_string(), |
There was a problem hiding this comment.
Hmm, not sure it's worth the changes, but if you think it somehow preferable, sure..
| &self, node_entropy: NodeEntropy, vss_url: String, fixed_headers: HashMap<String, String>, | ||
| ) -> Result<Node, BuildError> { | ||
| let logger = setup_logger(&self.log_writer_config, &self.config)?; | ||
| let store_id = "ldk-node".to_owned(); |
There was a problem hiding this comment.
I don't think we should do this. Adding pubkey-auth is just another authentication mechanism, which is otherwise unrelated to the VSS API contract. Note that we had users pick store_ids freely already and they are running in production. Suddenly fixating the store_id breaks the VSS API contract but also disallows them to switch to pubkey-auth from whatever they are running right now.
There was a problem hiding this comment.
I'm not quite sure I understand the argument around the API contract - there's an API contract defined in vss-client and between the VSS client and server, but I'm not really sure that applies to new instances of ldk-node using VSS. Do we really think they're going to re-derive the VSS auth key from their entropy in order to instantiate another instance of the VSS client with the same auth to store other stuff there and need to put it in the same store_id? If not, making them pass another argument just seems like yet more cognitive burden on devs to figure out what a store_id is.
disallows them to switch to pubkey-auth from whatever they are running right now.
Fwiw they can manually instantiate the vss-client VssHeaderProvider instance and call build_with_vss_store_and_header_provider, though we could also add a separate method if we anticipate users switching authentication mechanisms but keeping their existing VSS instance. It wasn't clear that we anticipate that kind of migration.
3 participants
