cluster: verify config signatures #1014
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
Codecov Report
@@ Coverage Diff @@
## main #1014 +/- ##
==========================================
- Coverage 54.16% 53.37% -0.80%
==========================================
Files 118 124 +6
Lines 13215 13836 +621
==========================================
+ Hits 7158 7385 +227
- Misses 5030 5404 +374
- Partials 1027 1047 +20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
xenowits
reviewed
Aug 23, 2022
Comment on lines
+157
to
+162
| if err := lock.Verify(); err != nil && !conf.NoVerify { | ||
| return errors.Wrap(err, "cluster lock signature verification failed. Run with --no-verify to bypass verification at own risk") | ||
| } else if err != nil && conf.NoVerify { | ||
| log.Warn(ctx, "Ignoring failed cluster lock signature verification due to --no-verify flag", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggested change
| if err := lock.Verify(); err != nil && !conf.NoVerify { | |
| return errors.Wrap(err, "cluster lock signature verification failed. Run with --no-verify to bypass verification at own risk") | |
| } else if err != nil && conf.NoVerify { | |
| log.Warn(ctx, "Ignoring failed cluster lock signature verification due to --no-verify flag", err) | |
| } | |
| err := lock.Verify(); err != nil { | |
| if !conf.NoVerify { | |
| return errors.Wrap(err, "cluster lock signature verification failed. Run with --no-verify to bypass verification at your own risk") | |
| } | |
| if conf.NoVerify { | |
| log.Warn(ctx, "Ignoring failed cluster lock signature verification due to --no-verify flag", err) | |
| } | |
There was a problem hiding this comment.
Not of a fan of this verbose style
There was a problem hiding this comment.
how is this "more" verbose? is there a go guideline that i can read upon?
There was a problem hiding this comment.
More lines and more if statements?
xenowits
reviewed
Aug 23, 2022
xenowits
reviewed
Aug 23, 2022
| } else if !ok { | ||
| return false, errors.Wrap(err, "config signature mismatch") | ||
| return errors.New("invalid operator config signature", z.Str("operator_address", o.Address)) |
There was a problem hiding this comment.
nit
Suggested change
| return errors.New("invalid operator config signature", z.Str("operator_address", o.Address)) | |
| return errors.New("invalid operator config signature", z.Str("operator_address", o.Address), z.Str("config_signature", string(digest[:]))) |
There was a problem hiding this comment.
It is the in the definition file, and humans cannot understand hex string in any case.
There was a problem hiding this comment.
think about signal to noise, logging more is seldom better. Log only what is actually useful.
xenowits
reviewed
Aug 23, 2022
| } else if !ok { | ||
| return false, errors.Wrap(err, "enr signature mismatch") | ||
| return errors.New("invalid operator enr signature", z.Str("operator_address", o.Address)) |
There was a problem hiding this comment.
Suggested change
| return errors.New("invalid operator enr signature", z.Str("operator_address", o.Address)) | |
| return errors.New("invalid operator enr signature", z.Str("operator_address", o.Address), z.Str("enr_signature", string(digest[:]))) |
xenowits
reviewed
Aug 23, 2022
| } else if !ok { | ||
| return false, errors.Wrap(err, "config signature mismatch") | ||
| return errors.New("invalid operator config signature", z.Str("operator_address", o.Address)) |
There was a problem hiding this comment.
discussion: should we also log the invalid and valid sigs?
There was a problem hiding this comment.
signal to noise, this would be noise
Co-authored-by: Abhishek Kumar <43061995+xenowits@users.noreply.github.com>
dB2510
approved these changes
Aug 23, 2022
xenowits
reviewed
Aug 23, 2022
cluster/helpers.go
Outdated
| @@ -54,6 +58,69 @@ func verifySig(addr string, digest []byte, sig []byte) (bool, error) { | |||
| return actual == expect, nil | |||
| } | |||
|
|
|||
| // signOperator return the operator with config hash and enr EIP712 signatures by the provided k1 private key. | |||
There was a problem hiding this comment.
Suggested change
| // signOperator return the operator with config hash and enr EIP712 signatures by the provided k1 private key. | |
| // signOperator returns the operator with signed config hash and EIP712 signed ENR using the provided k1 private key. |
There was a problem hiding this comment.
did something else
xenowits
reviewed
Aug 23, 2022
cluster/helpers.go
Outdated
| return b, nil | ||
| } | ||
|
|
||
| // signEIP712 signs the config hash digest and returns the signature. |
There was a problem hiding this comment.
Suggested change
| // signEIP712 signs the config hash digest and returns the signature. | |
| // signEIP712 signs the message and returns the signature. |
xenowits
reviewed
Aug 23, 2022
| func signEIP712(secret *ecdsa.PrivateKey, address string, message []byte) ([]byte, error) { | ||
| const nonce = 0 | ||
|
|
||
| digest, err := digestEIP712(address, message, nonce) |
There was a problem hiding this comment.
discussion: is nonce a required field?
There was a problem hiding this comment.
at the moment yes
xenowits
reviewed
Aug 23, 2022
Comment on lines
+183
to
+187
| if err != nil { | ||
| return errors.Wrap(err, "verify lock signature aggregate") | ||
| } else if !ok { | ||
| return errors.New("invalid lock signature aggregate") | ||
| } |
There was a problem hiding this comment.
nit
Suggested change
| if err != nil { | |
| return errors.Wrap(err, "verify lock signature aggregate") | |
| } else if !ok { | |
| return errors.New("invalid lock signature aggregate") | |
| } | |
| if err != nil { | |
| return errors.Wrap(err, "verify lock signature aggregate") | |
| } | |
| if !ok { | |
| return errors.New("invalid lock signature aggregate") | |
| } |
There was a problem hiding this comment.
I see it as the same check block, so prefer else to indicate that
xenowits
reviewed
Aug 23, 2022
tbls/tblsconv/tblsconv.go
Outdated
| @@ -86,6 +86,16 @@ func KeyToCore(key *bls_sig.PublicKey) (core.PubKey, error) { | |||
| return core.PubKeyFromBytes(b) | |||
| } | |||
|
|
|||
| // SigFromBytes converts a bytes into a kryptology bls signature. | |||
There was a problem hiding this comment.
Suggested change
| // SigFromBytes converts a bytes into a kryptology bls signature. | |
| // SigFromBytes converts secret bytes into a kryptology bls signature. |
There was a problem hiding this comment.
did something else
corverroos
added
the
merge when ready
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds
--no-verifyflags tocharon runandcharon dkg. Verify config otherwise.Note that
charon create clusterrequires--no-verifysince it doesn't sign the generated lock file.category: feature
ticket: #589