-
Notifications
You must be signed in to change notification settings - Fork 19
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.
Nice!
But can we have lots of comments so nobody does something stupid and changes something they shouldn't?
if err != nil { | ||
return nil, fmt.Errorf("unmarshalling public key failed: %s", err) | ||
} | ||
certKeyPub, err := x509.MarshalPKIXPublicKey(cert.PublicKey) |
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.
Is this deterministic?
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.
func keyToCertificate(sk ic.PrivKey) (crypto.PrivateKey, *x509.Certificate, error) { | ||
sn, err := rand.Int(rand.Reader, big.NewInt(1<<62)) | ||
func keyToCertificate(sk ic.PrivKey) (*tls.Certificate, error) { | ||
certKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) |
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.
Are there any other curves we can use...?
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.
The Go TLS implementation doesn't support P224, and P256 is curve that produces the smallest signatures. We could also use P384 and P512.
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.
Does it support curve25519
?
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.
keyToCertificate
does (as well as secp256k1), but the Go standard library doesn't. This was one of the reasons we had to invent this new handshake protocol.
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.
Ahh, I made a mistake and thought it was ecdh
not ecdsa
. (curve25519
is DH, ed25519
is signature).
6613471
to
8e61f12
Compare
8e61f12
to
11bbc4e
Compare
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.
Can't say I'm a crypto expert, but logically LGTM. Minor comments here, and I defer to @Kubuxu and @Stebalien for final approval. Thanks for the great work, @marten-seemann! 🎉
Co-Authored-By: marten-seemann <[email protected]>
57a5a79
to
d9950d0
Compare
use ChaCha if one of the peers doesn't have AES hardware support
3f4a541
to
663747a
Compare
use a prefix when signing the public key
When resuming a session using session tickets, no certificate chain is presented, and the callbacks needed to verify the peer identity would not be called.
@Kubuxu, @Stebalien I just merged the TLS spec. Can you please review this PR now? |
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.
SGWM
Please read libp2p/specs#151.