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

security: Add ed25519 signer #92

Merged
merged 4 commits into from
Jan 2, 2025
Merged

security: Add ed25519 signer #92

merged 4 commits into from
Jan 2, 2025

Conversation

zjkmxy
Copy link
Member

@zjkmxy zjkmxy commented Jan 1, 2025

Fix: #91

@zjkmxy
Copy link
Member Author

zjkmxy commented Jan 1, 2025

Validator is already there

// EddsaValidate verifies the signature with a known ed25519 public key.
// ndn-cxx's PIB does not support this, but a certificate is supposed to use ASN.1 DER format.
// Use x509.ParsePKIXPublicKey to parse. Note: ed25519.PublicKey is defined to be a pointer type without '*'.
func EddsaValidate(sigCovered enc.Wire, sig ndn.Signature, pubKey ed25519.PublicKey) bool {
if sig.SigType() != ndn.SignatureEd25519 {
return false
}
return ed25519.Verify(pubKey, sigCovered.Join(), sig.SigValue())
}

Don't have a unit test for now (WIP), but it should work.

Copy link
Member

@yoursunny yoursunny left a comment

Choose a reason for hiding this comment

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

Need test cases:

  • Sign then verify.
  • Import the public key from a self-signed Ed25519 certificate and verify the certificate packet. The sample certificate is linked from Ed25519 signature type #91 (comment)

std/security/signers_test.go Show resolved Hide resolved
std/security/ed-signer.go Show resolved Hide resolved
@zjkmxy
Copy link
Member Author

zjkmxy commented Jan 2, 2025

I have not finished addressing Junxiao's comments but thank Varun for reviewing.
Please do not merge for now :)

std/security/ed-signer.go Outdated Show resolved Hide resolved
std/security/ed-signer.go Outdated Show resolved Hide resolved
@zjkmxy zjkmxy merged commit e6a1870 into named-data:main Jan 2, 2025
9 checks passed
@zjkmxy zjkmxy deleted the eddsa branch January 2, 2025 17:46
@@ -9,38 +9,17 @@ import (
enc "github.com/named-data/ndnd/std/encoding"
basic_engine "github.com/named-data/ndnd/std/engine/basic"
"github.com/named-data/ndnd/std/ndn"
"github.com/named-data/ndnd/std/utils"
)

// eccSigner is a signer that uses ECC key to sign packets.
type eccSigner struct {
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking on the naming: "ECC" is a very broad category of cryptography but this code is specifically for ECDSA. Saying "ecc signing" or "ecc key" is fairly meaningless (what algorithm?). Moreover, EdDSA also uses elliptic curves (twisted Edwards curves), so this naming could be confusing. The comment above is also meaningless for similar reasons.

)

// edSigner is a signer that uses Ed25519 key to sign packets.
type edSigner struct {
Copy link
Member

Choose a reason for hiding this comment

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

also nitpicking but much less important than the previous comment: EdDSA can be instantiated with different curves and hash algorithms. Ed25519 is just one instance (arguably the most common one) but the name edSigner could potentially indicate that this is for EdDSA generically. Thankfully, the comment is clear that it's for Ed25519, so this is not too important at the end of the day. It could become awkward when/if we introduce the Ed448 signature type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ed25519 signature type
4 participants