Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load pem file for p2p signing #4478

Merged
merged 17 commits into from
Sep 22, 2022
Merged

Conversation

ssd04
Copy link
Contributor

@ssd04 ssd04 commented Sep 19, 2022

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

  • Load pem file for p2p signing instead of using math.Rand for generating seeder private key

Proposed Changes

  • load pem file based on cli flag; if there is no file specified generate a private key as before

Testing procedure

  • Standard system testing procedure + half-network upgrade for p2p

@ssd04 ssd04 self-assigned this Sep 19, 2022
@ssd04 ssd04 marked this pull request as ready for review September 19, 2022 10:59
@codecov-commenter
Copy link

Codecov Report

Base: 73.87% // Head: 73.85% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (9566e28) compared to base (05f6f1d).
Patch coverage: 53.57% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.4.0    #4478      +/-   ##
=============================================
- Coverage      73.87%   73.85%   -0.02%     
=============================================
  Files            692      692              
  Lines          88308    88331      +23     
=============================================
+ Hits           65234    65240       +6     
- Misses         18160    18174      +14     
- Partials        4914     4917       +3     
Impacted Files Coverage Δ
node/nodeRunner.go 0.00% <0.00%> (ø)
p2p/libp2p/p2pSigner.go 86.66% <ø> (ø)
factory/networkComponents.go 55.71% <50.00%> (-0.85%) ⬇️
p2p/libp2p/netMessenger.go 79.74% <66.66%> (-0.55%) ⬇️
statusHandler/statusMetricsProvider.go 97.09% <0.00%> (-0.83%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@iulianpascalau iulianpascalau self-requested a review September 19, 2022 11:26
}

func generateP2pKey(list []key) ([]key, error) {
privateKey, publicKey, err := libp2pCrypto.GenerateSecp256k1Key(rand.Reader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from crypto/rand


log = logger.GetOrCreate("keygenerator")

validatorPubKeyConverter, _ = pubkeyConverter.NewHexPubkeyConverter(blsPubkeyLen)
p2pPubKeyConverter, _ = pubkeyConverter.NewHexPubkeyConverter(blsPubkeyLen)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not use the hex pub key converter here as we might want to see in the pem file something like:

-----BEGIN PRIVATE KEY for 16Uiu2HAm7deAWdBU41D6SUgtUEKC2VpXuRay4QqWnnPLqa8358Yv-----

https://github.com/ElrondNetwork/elrond-go/blob/5aed6c6ac41ded6321323a5195a03e6329fa50d0/p2p/libp2p/p2pSigner.go#L23
suggestion to check how is it done in the

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a separate converter for p2p encoding

if err != nil {
return nil, err
}

prvKey, _ := ecdsa.GenerateKey(btcec.S256(), randReader)
log.Warn("createP2PPrivKey: using an already existing privary key for p2p signing")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use WARN here, a plain INFO will do the job. Also, we might display the ID generated from the key. There is a typo privary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the comment; since i don't have access to the peer ID here yet i did not displayed it later since it's already visible in the listening addr string

@@ -152,7 +153,7 @@ func newNetworkMessenger(args ArgsNetworkMessenger, messageSigning messageSignin
return nil, fmt.Errorf("%w when creating a new network messenger", p2p.ErrNilPeersRatingHandler)
}

p2pPrivKey, err := createP2PPrivKey(args.P2pConfig.Node.Seed)
p2pPrivKey, err := createP2PPrivKey(args.P2pPrivKeyBytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the cleanup for the p2p/libp2p/rand package? We should delete that package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted rand package

iulianpascalau
iulianpascalau previously approved these changes Sep 19, 2022

// Decode does nothing
func (p *p2pConverter) Decode(humanReadable string) ([]byte, error) {
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might return ErrNotImplemented or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added not implemented error on return

@AdoAdoAdo AdoAdoAdo self-requested a review September 20, 2022 07:02
func createP2PPrivKey(seed string) (*libp2pCrypto.Secp256k1PrivateKey, error) {
randReader, err := randFactory.NewRandFactory(seed)
func createP2PPrivKey(p2pPrivKeyBytes []byte) (libp2pCrypto.PrivKey, error) {
if len(p2pPrivKeyBytes) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also log when you generate a p2p key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

AdoAdoAdo
AdoAdoAdo previously approved these changes Sep 20, 2022
iulianpascalau
iulianpascalau previously approved these changes Sep 20, 2022
iulianpascalau
iulianpascalau previously approved these changes Sep 20, 2022
AdoAdoAdo
AdoAdoAdo previously approved these changes Sep 20, 2022
@ssd04 ssd04 dismissed stale reviews from AdoAdoAdo and iulianpascalau via a0af5ab September 22, 2022 12:24
Copy link
Contributor

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System test passed.

@iulianpascalau iulianpascalau merged commit c414b68 into rc/v1.4.0 Sep 22, 2022
@iulianpascalau iulianpascalau deleted the load-private-key-for-p2p branch September 22, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants