-
Notifications
You must be signed in to change notification settings - Fork 1
nebula v1.10 compat #26
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
Conversation
nbrownus
left a comment
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.
@johnmaguire did you have any concerns? I know there was some prior complaints about the PEM banners.
keys/crypto.go
Outdated
| } | ||
|
|
||
| // newNebulaX25519KeypairPEM returns a new Nebula keypair (X25519) in PEM format. | ||
| // newNebulaX25519KeypairPEM returns a new Nebula key-agreement keypair (ED25519) in PEM format. |
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.
suuuuuper nit, but i think it's Ed25519. i don't think the D stands for anything.
what does key-agreement mean in this context exactly? what other types of Nebula keypairs exist?
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.
Actually, I'm doubly confused. The function name is X25519, not Ed25519. why was the comment updated?
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.
I think "key-agreement" is correct, but the parens should've stayed X25519? I think Ed25519 is signing, not key-agreement? I always get this stuff mixed up.
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.
X25519 - ECDH - diffie helmen aka key agreement
Ed25519 (Edwards curve) - ECDSA (EdDSA) - signing
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 public keys we use for CAs are "signing keys", not "key agreement keys". You're technically not allowed to interchange them.
keys/crypto.go
Outdated
| return nil, nil, err | ||
| } | ||
| pubkey, privkey = cert.MarshalX25519PublicKey(pubkey), cert.MarshalX25519PrivateKey(privkey) | ||
| pubkey, privkey = cert.MarshalPublicKeyToPEM(cert.Curve_CURVE25519, pubkey), cert.MarshalPrivateKeyToPEM(cert.Curve_P256, privkey) |
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.
I don't think this should be P256.
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.
yep this is wrong, thank you!!!
johnmaguire
left a comment
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.
I think the P256 thing needs fixed.
No description provided.