Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Authority-discovery should sign its validator key with its PeerIds #8833

Closed
tomaka opened this issue May 17, 2021 · 6 comments · Fixed by #10317
Closed

Authority-discovery should sign its validator key with its PeerIds #8833

tomaka opened this issue May 17, 2021 · 6 comments · Fixed by #10317
Assignees
Labels
I3-bug The node fails to follow expected behavior.

Comments

@tomaka
Copy link
Contributor

tomaka commented May 17, 2021

Failing to do so means that it's possible for a malicious validator to cause disruptions on the network.
It would be quite complicated to actually exploit this, plus any validator actually doing that would be easily identified and can't profit from this in any way, so the chances of attack are extremely low, but it doesn't hurt to fix this.

@tomaka tomaka added the I3-bug The node fails to follow expected behavior. label May 17, 2021
@stale
Copy link

stale bot commented Jul 7, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
@tomaka
Copy link
Contributor Author

tomaka commented Jul 8, 2021

Issue still relevant and important.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 8, 2021
@wigy-opensource-developer
Copy link
Contributor

I have checked the sc-authority-discovery implementation, and was wondering if there will be authorities on the network with and without this patch. I suspect there will be, so we need a 2-phase deployment: First we warn if signatures are missing from found DHT records, and later we reject/ignore those DHT records. Do we have a good mechanism for those kinds of "authority-voted" hard forks?

@wigy-opensource-developer
Copy link
Contributor

@bkchr At the moment the private key for PeerId is parsed from CLI params into a NetworkConfiguration, then passed to build_transport, where it is buried in the Noise-based authentication layer. There are several ways to expose the key to sc-authority-discovery, which all have different long-term consequences:

  1. Wrap all DHT records put into Kademlia with PeerId/signature (DHT is only used by sc-authority-discovery in our codebase) and keep private key not exposed from sc-network
  2. Expose a synchronous sign/verify interface on NetworkService so sc-authority-discovery can sign/verify records it puts into Kademlia
  3. Add the network identity keys to LocalKeyStore and let the transport take it from there.
  4. Pass the network identity key to authority discovery worker, too.
  5. Other

My personal preference would be 2 or 3, but I might be missing pieces of code that suggest another design.

@bkchr
Copy link
Member

bkchr commented Nov 22, 2021

Add the network identity keys to LocalKeyStore and let the transport take it from there.

Sounds reasonable to me 👍

We probably could create a follow up issue to move sc-networking itself to use LocalKeyStore to get the key and "burry" it in noise.

You should also create a new KeyType for it.

@burdges
Copy link

burdges commented Feb 1, 2022

We need back certs kinda everywhere else too. I'd envisioned doing the session key ones in #7398 but of course transport is different.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I3-bug The node fails to follow expected behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants