This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
More secure Signed implementation
#2963
Merged
Conversation
This file contains 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
`SignedFullStatement` now signals that the signature has already been checked.
Also get rid of Encode/Decode for Signed! *party*
eskimor
added
A0-please_review
bkchr
reviewed
Apr 30, 2021
bkchr
reviewed
Apr 30, 2021
drahnr
reviewed
May 3, 2021
There was a problem hiding this comment.
Generally this LGTM besides a few nits.
Iiuc this unifies the signature check into a type, and uses that to only verify it once and hence reduce the load.
The open question that is left is, if the path from the point of checking where it was to where it is now, if that opens up any issues. TBD
| @@ -72,7 +75,7 @@ pub struct ValidatorInfo { | |||
|
|
|||
| impl RuntimeInfo { | |||
| /// Create a new `RuntimeInfo` for convenient runtime fetches. | |||
| pub fn new(keystore: SyncCryptoStorePtr) -> Self { | |||
| pub fn new(keystore: Option<SyncCryptoStorePtr>) -> Self { | |||
There was a problem hiding this comment.
I don't quite see where the keystore is ever None 🙈
There was a problem hiding this comment.
In the collator protocol (collator side) we don't need it, because we are not a validator.
| signature: ValidatorSignature, | ||
| context: &SigningContext<H>, | ||
| key: &ValidatorId, | ||
| ) -> Option<Self> { |
There was a problem hiding this comment.
nit: Isn't this rather a Result with a unit struct SignatureVerificationError?
There was a problem hiding this comment.
It was this way before and I also think it is pretty obvious what None means in this case. To be honest you have a point, but if you do not object, I am going to be lazy and just merge if this is the only nit 😬
| statement: &SignedFullStatement, | ||
| ) -> std::result::Result<(), ()> { | ||
| statement: UncheckedSignedFullStatement, | ||
| ) -> std::result::Result<SignedFullStatement, UncheckedSignedFullStatement> { |
drahnr
approved these changes
May 3, 2021
andresilva
pushed a commit
that referenced
this pull request
May 4, 2021
* Remove signature verification in backing. `SignedFullStatement` now signals that the signature has already been checked. * Remove unused check_payload function. * Introduced unchecked signed variants. * Fix inclusion to use unchecked variant. * More unchecked variants. * Use unchecked variants in protocols. * Start fixing statement-distribution. * Fixup statement distribution. * Fix inclusion. * Fix warning. * Fix backing properly. * Fix bitfield distribution. * Make crypto store optional for `RuntimeInfo`. * Factor out utility functions. * get_group_rotation_info * WIP: Collator cleanup + check signatures. * Convenience signature checking functions. * Check signature on collator-side. * Fix warnings. * Fix collator side tests. * Get rid of warnings. * Better Signed/UncheckedSigned implementation. Also get rid of Encode/Decode for Signed! *party* * Get rid of dead code. * Move Signed in its own module. * into_checked -> try_into_checked * Fix merge.
ordian
added a commit
that referenced
this pull request
May 6, 2021
* master: Squashed 'bridges/' changes from 89a76998..b2099c5c Enable Pallet XCM for Kusama & Westend (#2970) kusama, westend: use proper parachain session keys (#2975) change statemint id (not system para) (#2974) runtime: remove random_seed from BlockBuilder API (#2968) runtime: remove beefy and mmr from westend (again) (#2972) Do peer connect later (as it happens in reality). (#2971) More secure `Signed` implementation (#2963) runtime: remove BabePalletPrefix impls for old migration (#2967) Companion for Substrate#8526 (#2845) Companion for Multi-phase elections solution resubmission (#2648)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #2944
Also:
CollatorProtocolhas changed as we now also transmit the relay parent for the signing context in order to actually check the signature.Important: This PR changes the wire protocol for collations.