-
Notifications
You must be signed in to change notification settings - Fork 76
generate ecdsa public key from an input public key #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not incorrect, but where do we need this? We don't have a corresponding method for RSA, for example.
I created this function because currently I'm working on a project where I need to generate a pubKey, err := ECDSAPublicKeyFromPubKey(someECDSAPubKey)
thePeerId, err := peer.IDFromPublicKey(pubKey) Should I implement a similar function for RSA and Ed25519 too? |
Ok, that's legit.
No need to. Can we have a test please? |
I've added the required test |
crypto/ecdsa.go
Outdated
@@ -67,6 +67,15 @@ func ECDSAKeyPairFromKey(priv *ecdsa.PrivateKey) (PrivKey, PubKey, error) { | |||
return &ECDSAPrivateKey{priv}, &ECDSAPublicKey{&priv.PublicKey}, nil | |||
} | |||
|
|||
// ECDSAPublicKeyFromPubKey generates a new ecdsa public key from an input public key | |||
func ECDSAPublicKeyFromPubKey(pub *ecdsa.PublicKey) (PubKey, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From some conversation during triage:
It is likely better to dereference they key before it's passed in to reduce the likelihood of hard to track down bugs. Also the keys aren't too big
crypto/ecdsa.go
Outdated
func ECDSAPublicKeyFromPubKey(pub *ecdsa.PublicKey) (PubKey, error) { | ||
if pub == nil { | ||
return nil, ErrNilPublicKey | ||
} | ||
|
||
return &ECDSAPublicKey{pub}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, the bug I see here is that someone could pass a key into it and since it's by reference modify it while libp2p is using it (libp2p expects key to be constant and not change).
If it's passed by value then a copy is made and this can't happen.
func ECDSAPublicKeyFromPubKey(pub *ecdsa.PublicKey) (PubKey, error) { | |
if pub == nil { | |
return nil, ErrNilPublicKey | |
} | |
return &ECDSAPublicKey{pub}, nil | |
} | |
func ECDSAPublicKeyFromPubKey(pub ecdsa.PublicKey) (PubKey, error) { | |
return &ECDSAPublicKey{pub:&pub}, nil | |
} |
Thank you for the code review, @aschmahmann and @Jorropo . I have done the requested changes. Should I open a separate PR for doing the same change in here?: // ECDSAKeyPairFromKey generates a new ecdsa private and public key from an input private key
func ECDSAKeyPairFromKey(priv *ecdsa.PrivateKey) (PrivKey, PubKey, error) {
if priv == nil {
return nil, nil, ErrNilPrivateKey
}
return &ECDSAPrivateKey{priv}, &ECDSAPublicKey{&priv.PublicKey}, nil
} |
@richard-ramos no you should rebase or amend commit. First, on your This will change this PR to a new commit that is just like you did it right the first time (sometime you want to do "history" based commits so you would just make a new commit on top of the old one and explain your thought process in the commit description, for something that small it's obvious you can just force push). |
@Jorropo but i did rebase this PR. Notice it's only a single commit containing the suggested changes |
No description provided.