[network, crypto] Compressed serialization, key conversions and message signing #1129

huitseeker · 2021-08-12T03:41:31Z

This:

implements compressed (32/33 bytes instead of 64) (de)serialization for crypto.PublicKey.
- This is something we want to work on transitioning to more widely next cycle,
- it's a milestone towards #5739
re-activates message signing and verification, closes #5634
~~checks the OriginID contained in a message with respect to the (now cryptographically-verified) libp2p source, closes [Network] Verify originID against libp2p ID #1115~~

Edit: Splitting this in 2 PRs, to work on the concurrency issues w/ @vishalchangrani : this is the straightwforward crypto PR that will go easier

network/p2p/keyTranslator.go

network/p2p/readSubscription.go

network/test/testUtil.go

network/p2p/readSubscription.go

network/p2p/middleware.go

codecov-commenter · 2021-08-12T20:36:27Z

Codecov Report

Merging #1129 (a08b3c7) into master (2d5faf1) will increase coverage by 0.18%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   53.11%   53.30%   +0.18%     
==========================================
  Files         323      323              
  Lines       21857    21928      +71     
==========================================
+ Hits        11610    11688      +78     
+ Misses       8667     8648      -19     
- Partials     1580     1592      +12

Flag	Coverage Δ
unittests	`53.30% <47.05%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
network/p2p/libp2pNode.go	`62.91% <33.33%> (ø)`
network/p2p/keyTranslator.go	`63.41% <46.66%> (-10.18%)`	⬇️
network/p2p/libp2pUtils.go	`53.24% <100.00%> (ø)`
fvm/transactionEnv.go	`44.94% <0.00%> (-0.38%)`	⬇️
engine/consensus/dkg/reactor_engine.go	`75.96% <0.00%> (-0.19%)`	⬇️
fvm/scriptEnv.go	`35.87% <0.00%> (-0.11%)`	⬇️
module/signature/signer_store.go	`0.00% <0.00%> (ø)`
fvm/state/accounts.go	`45.27% <0.00%> (+0.60%)`	⬆️
...sus/approvals/assignment_collector_statemachine.go	`51.92% <0.00%> (+1.92%)`	⬆️
module/chunks/chunkVerifier.go	`68.90% <0.00%> (+4.14%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d5faf1...a08b3c7. Read the comment docs.

tarakby · 2021-08-16T20:17:00Z

crypto/bls.go

+
+// decodePublicKeyCompressed decodes a slice of bytes into a public key.
+// This function includes a membership check in G2 and rejects the infinity point.
+func (a *blsBLS12381Algo) decodePublicKeyCompressed(publicKeyBytes []byte) (PublicKey, error) {


This might be a bit misleading because the default choice is made here. This means we can enable an uncompressed setting while function's name is still ..Compressed.
I suggest we just keep decodePublicKey.

We could decide in the future to either:

remove the possibility to have uncompressed settings entirely.

support both compression formats dynamically.

OK. I inverted the delegation made by the compressed functions in 1dd2bf7 Note that the coherence check on Encode is a bit fugly.

crypto/bls.go

crypto/ecdsa.go

crypto/sign.go

crypto/ecdsa_test.go

crypto/sign_test_utils.go

network/p2p/keyTranslator.go

tarakby · 2021-08-17T00:14:18Z

In the PR summary, I think we can also remove the line below since it hasn't been addressed yet.

re-activates message signing and verification, closes #5634

crypto/ecdsa.go

crypto/ecdsa_test.go

yhassanzadeh13 · 2021-08-17T00:41:55Z

network/p2p/keyTranslator.go

+	case lcrypto_pb.KeyType_ECDSA:
+		pubB, err := lpk.Raw()
+		if err != nil {
+			return nil, lcrypto.ErrBadKeyType


Maybe making more distinct error types helps to spot the issue upon happening, i.e., this error type is the same as the next case.

This error and the next will not actually happen: I'm marshalling the key material and then reinterpreting it as bytes using the underlying x509 library manually, because libp2p deprecated its Bytes function. This is just bypassing lack of access to private members.

huitseeker · 2021-08-17T14:59:59Z

@tarakby

In the PR summary, I think we can also remove the line below since it hasn't been addressed yet.

Message signing and verification is re-activated in the commit titled "[network] re-activate message signing & verification", included in this PR.

tarakby · 2021-08-17T17:41:38Z

Message signing and verification is re-activated in the commit titled "[network] re-activate message signing & verification", included in this PR.

My bad, I missed the change! Thanks

synzhu · 2021-08-17T20:02:52Z

Looks good to me, but will leave to @tarakby to more closely review the crypto changes

tarakby

Thanks for adding the compressed form to the interface 🏄

crypto/ecdsa.go

crypto/ecdsa_test.go

crypto/ecdsa.go

crypto/bls.go

This will allow us to perform identity checks for e.g. matching protocol-level identities to the proven network-level IDs

…rves

…r API

ensure signerIDs are populated in pubSub tests

…port secp256k1

- check for parametrization of compression

as existing DecodeCompressed implementations already check for validity

1138: skip connecting to invalid identities in the identity table and log error instead of fatal r=vishalchangrani a=vishalchangrani Found one more place where it was `log.fatal` on a bad identity. 1163: [network] Check the OriginID of a libp2p message corresponds to its authenticated source r=huitseeker a=huitseeker Contributes to #1115. Follow-up of #1129. Edit: there was no problem with the integration tests, 🤦 However, TestMiddlewareTestSuit/TestUnsubscribe is still flaky based on the delay in the last step (sending the message once a mesh has formed). But it is problematic on master, see [[1]](https://github.com/onflow/flow-go/runs/3357056741) [[2]](https://github.com/onflow/flow-go/runs/3356508175), so this is not relevant to this PR per se. Co-authored-by: Vishal <1117327+vishalchangrani@users.noreply.github.com> Co-authored-by: François Garillot <francois.garillot@dapperlabs.com>

1028: adding secure-rpc-addr to access node systemd file r=vishalchangrani a=vishalchangrani 1163: [network] Check the OriginID of a libp2p message corresponds to its authenticated source r=huitseeker a=huitseeker Contributes to #1115. Follow-up of #1129. Edit: there was no problem with the integration tests, 🤦 However, TestMiddlewareTestSuit/TestUnsubscribe is still flaky based on the delay in the last step (sending the message once a mesh has formed). But it is problematic on master, see [[1]](https://github.com/onflow/flow-go/runs/3357056741) [[2]](https://github.com/onflow/flow-go/runs/3356508175), so this is not relevant to this PR per se. Co-authored-by: vishal <1117327+vishalchangrani@users.noreply.github.com> Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com> Co-authored-by: Vishal <1117327+vishalchangrani@users.noreply.github.com> Co-authored-by: François Garillot <francois.garillot@dapperlabs.com>

1163: [network] Check the OriginID of a libp2p message corresponds to its authenticated source r=huitseeker a=huitseeker Contributes to #1115. Follow-up of #1129. Edit: there was no problem with the integration tests, 🤦 However, TestMiddlewareTestSuit/TestUnsubscribe is still flaky based on the delay in the last step (sending the message once a mesh has formed). But it is problematic on master, see [[1]](https://github.com/onflow/flow-go/runs/3357056741) [[2]](https://github.com/onflow/flow-go/runs/3356508175), so this is not relevant to this PR per se. Co-authored-by: François Garillot <francois.garillot@dapperlabs.com>

1180: [bootstrap] Key Generation for Unstaked Access Nodes r=huitseeker a=huitseeker This introduces a generator for the unstaked Access Nodes, which by convention only have positive secp256k1 keys. The keys in question have a specific format (positive ECDSA Secp256k1 keys) because: - they are meant to serve ephemeral nodes, - allow a bijection between flow.NodeID, flow.NetworkPublicKey, libp2p.PeerID, libp2p.PublicKey - hence allowing us to only retain the minimum amount of information for these nodes. For this bijection, see #1165 and #1129. This is to be used in a bootstrap of the unstaked Access Node post #1133. For now, having this in master allows the direct bootstrapping of network in tests by creating suitable libp2p peers. Co-authored-by: François Garillot <francois.garillot@dapperlabs.com>

huitseeker requested review from tarakby, vishalchangrani and yhassanzadeh13 as code owners August 12, 2021 03:41

huitseeker requested a review from synzhu August 12, 2021 03:41

huitseeker marked this pull request as draft August 12, 2021 04:02

huitseeker force-pushed the keyTranslator_inverse_ branch from 51cca1a to 92f77d1 Compare August 12, 2021 12:46

synzhu reviewed Aug 12, 2021

View reviewed changes

network/p2p/keyTranslator.go Outdated Show resolved Hide resolved

synzhu reviewed Aug 12, 2021

View reviewed changes

huitseeker force-pushed the keyTranslator_inverse_ branch from 92f77d1 to 64ded2e Compare August 12, 2021 20:15

huitseeker force-pushed the keyTranslator_inverse_ branch 5 times, most recently from abb4a80 to 7fd970e Compare August 13, 2021 16:55

huitseeker changed the title ~~[network, crypto] Various crypto & network synergies~~ [network, crypto] Compressed serialization, key conversions and message signing Aug 16, 2021

huitseeker force-pushed the keyTranslator_inverse_ branch from 7fd970e to 173f71f Compare August 16, 2021 19:14

huitseeker marked this pull request as ready for review August 16, 2021 19:44

tarakby self-assigned this Aug 16, 2021

huitseeker force-pushed the keyTranslator_inverse_ branch 2 times, most recently from fadabd0 to 4907ea0 Compare August 16, 2021 21:21

tarakby reviewed Aug 17, 2021

View reviewed changes

yhassanzadeh13 approved these changes Aug 17, 2021

View reviewed changes

huitseeker force-pushed the keyTranslator_inverse_ branch 4 times, most recently from cc18b84 to cb9aef6 Compare August 17, 2021 17:03

tarakby approved these changes Aug 17, 2021

View reviewed changes

crypto/ecdsa.go Show resolved Hide resolved

crypto/ecdsa_test.go Outdated Show resolved Hide resolved

crypto/ecdsa.go Show resolved Hide resolved

crypto/bls.go Outdated Show resolved Hide resolved

crypto/bls.go Outdated Show resolved Hide resolved

huitseeker added 10 commits August 17, 2021 18:10

[network] translate libp2p.PubKey -> flow ECDSA PubKey

15d36d7

This will allow us to perform identity checks for e.g. matching protocol-level identities to the proven network-level IDs

[crypto] Implement Compressed De/Serialization for supported ECDSA cu…

253ef55

…rves

[crypto] Add EncodeCompressed, DecodepublicKeyCompressed to the signe…

571668f

…r API

[network] Simplify libp2p -> flow after adding DecodepublicKeyCompressed

be0f651

[network] re-activate message signing & verification

a6ebfd9

ensure signerIDs are populated in pubSub tests

[network] Rename functions in keyTranslator for clarity

141211d

[network] Lint UnsubscribeTest

e4c0bc8

[crypto] Add a test proving elliptic.UnmarshalCompressed does not sup…

47a0f2a

…port secp256k1

[crypto] Invert (encode|decode)PublicKey delegation for BLS

b1ee91a

- check for parametrization of compression

[crypto] partial revert of checkPublicKeyValid

d1e58f6

as existing DecodeCompressed implementations already check for validity

huitseeker force-pushed the keyTranslator_inverse_ branch from a08b3c7 to d1e58f6 Compare August 17, 2021 22:10

huitseeker merged commit 1b33269 into onflow:master Aug 17, 2021

huitseeker mentioned this pull request Aug 18, 2021

[network] Check the OriginID of a libp2p message corresponds to its authenticated source #1163

Merged

huitseeker mentioned this pull request Aug 20, 2021

[bootstrap] Key Generation for Unstaked Access Nodes #1180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[network, crypto] Compressed serialization, key conversions and message signing #1129

[network, crypto] Compressed serialization, key conversions and message signing #1129

huitseeker commented Aug 12, 2021 •

edited

codecov-commenter commented Aug 12, 2021 •

edited

tarakby Aug 16, 2021

huitseeker Aug 17, 2021

tarakby commented Aug 17, 2021

yhassanzadeh13 Aug 17, 2021

huitseeker Aug 17, 2021

huitseeker commented Aug 17, 2021 •

edited

tarakby commented Aug 17, 2021

synzhu commented Aug 17, 2021

tarakby left a comment

[network, crypto] Compressed serialization, key conversions and message signing #1129

[network, crypto] Compressed serialization, key conversions and message signing #1129

Conversation

huitseeker commented Aug 12, 2021 • edited

codecov-commenter commented Aug 12, 2021 • edited

Codecov Report

tarakby Aug 16, 2021

Choose a reason for hiding this comment

huitseeker Aug 17, 2021

Choose a reason for hiding this comment

tarakby commented Aug 17, 2021

yhassanzadeh13 Aug 17, 2021

Choose a reason for hiding this comment

huitseeker Aug 17, 2021

Choose a reason for hiding this comment

huitseeker commented Aug 17, 2021 • edited

tarakby commented Aug 17, 2021

synzhu commented Aug 17, 2021

tarakby left a comment

Choose a reason for hiding this comment

huitseeker commented Aug 12, 2021 •

edited

codecov-commenter commented Aug 12, 2021 •

edited

huitseeker commented Aug 17, 2021 •

edited