Authentication of PeerIds in authority discovery records #10317
Conversation
wigy-opensource-developer
added
A3-in_progress
github-actions
bot
added
A0-please_review
wigy-opensource-developer
force-pushed
the
wigy-auth-auth-disco
branch
from
a04c3ea
to
4f55b6e
Compare
Unsigned records cannot be rejected yet, they just produce a warning in the log.
|
Is this ready for review? |
|
Now it is code complete in my view, especially if the CI passes. Where do I need to document the new command-line option? |
wigy-opensource-developer
changed the title
wigy-opensource-developer
changed the title
| serde_json::to_writer(&file, data)?; | ||
| file.flush()?; |
There was a problem hiding this comment.
Ooops. Forgot it in this changeset. Was thinking about if this reduces the race-condition that another process opens the file before the permissions are applied. It is a very thin chance, and did not want to push it 😟
|
|
||
| /// Reject authority discovery records that are not signed by their network identity (PeerId) | ||
| /// | ||
| /// Defaults to `false` to provide compatibility with old versions |
There was a problem hiding this comment.
IMO this should default to true to have new networks enabled this by default.
There was a problem hiding this comment.
That would cause silent breaking for existing networks, which use Default::default() in their sc-authority-discovery::Worker instantiation. I would be more conservative here and reduce gotchas in the ecosystem.
There was a problem hiding this comment.
I think no one uses this, but fine.
Then we should document this in the docs of the crate?
At the moment I have added tests that do not sign the record. The wire format should be the same as the old records. Should I record an encoded record from a test on master and try to decode it with the current proto definition in a test? |
Yes that was my idea, but if you are sure that the format is compatible. I'm also okay without any test. |
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Checks failed for cb009e0 |
…10317) * Consolidating test and production code * Signing/verifying authority discovery records with PeerId Unsigned records cannot be rejected yet, they just produce a warning in the log. * Upgrading to libp2p 0.40 * libp2p::identity and sp_core::crypto Ed25519 are compatible * Rejecting authority records unsigned by peer id can be configured * Fixes based on review comments * No command-line argument needed * info was still too much spam in the logs * Added tests for both strict and loose validation * Fixing based on review comments * Pierre preferred a signing method * Ooops, I need to slow down * Update bin/node/cli/src/service.rs * Reexport libp2p crypto used in sc-network * Added proto3 compatibility tests. And import noise. Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
…10317) * Consolidating test and production code * Signing/verifying authority discovery records with PeerId Unsigned records cannot be rejected yet, they just produce a warning in the log. * Upgrading to libp2p 0.40 * libp2p::identity and sp_core::crypto Ed25519 are compatible * Rejecting authority records unsigned by peer id can be configured * Fixes based on review comments * No command-line argument needed * info was still too much spam in the logs * Added tests for both strict and loose validation * Fixing based on review comments * Pierre preferred a signing method * Ooops, I need to slow down * Update bin/node/cli/src/service.rs * Reexport libp2p crypto used in sc-network * Added proto3 compatibility tests. And import noise. Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
This PR adds a proof that the authority controls the key behind the PeerId it advertises to be reachable at.
Closes #8833
libp2p::identity::PublicKeyandlibp2p::identity::error::SigningErrorfromsc-networkand use it insc-authority-discovery.SignedAuthorityRecordfrom the old format.skip check-dependent-cumulus