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

add sign and verify test vectors #292

Merged
merged 8 commits into from
Aug 20, 2024
Merged

add sign and verify test vectors #292

merged 8 commits into from
Aug 20, 2024

Conversation

nitro-neal
Copy link
Contributor

Updating web5-spec and adding ed25519 test vectors

@nitro-neal
Copy link
Contributor Author

this pr goes along with this one - decentralized-identity/web5-spec#172

private val rustCoreVerifier: RustCoreEd25519Verifier

constructor(privateKey: Jwk) {
this.rustCoreVerifier = RustCoreEd25519Verifier(privateKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think with the latest on main you'll need to rebase and then this'll be...

Suggested change
this.rustCoreVerifier = RustCoreEd25519Verifier(privateKey)
this.rustCoreVerifier = RustCoreEd25519Verifier(privateKey.rustCoreJwkData)

val key: TestVectorJwk,
)

data class TestVectorJwk(
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we're not testing the JWK in this test, but ideally we could use the real Jwk, but to your point on the other PR, we may need to add #297 first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup after 297 is in we can use it

Copy link
Contributor

@KendallWeihe KendallWeihe left a comment

Choose a reason for hiding this comment

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

well done! I added one comment with respect to rebasing onto main which needs addressed but after that this is good to go!

let signature = result.expect("Signing should not fail");

// Convert the signature to a hex string
let signature_hex = byte_array_to_hex_string(&signature);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this works. ed25519 signatures are different every time because they use a random nonce to prevent computing the private key if the same private key is used for multiple signatures

Copy link
Contributor Author

@nitro-neal nitro-neal Aug 20, 2024

Choose a reason for hiding this comment

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

oh no..
This works in every sdk
Are we doing something wrong lol, let me check on this..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

see Section 4.2.5 on Deterministic Nonce Generation

Ed25519 uses deterministic signing which removes the need for fresh random numbers during the signing process. This does not lead to any particular consequences for our security analysis since we model the key derivation function as a random oracle. However, it is well known not to reduce security


let result = verifier.verify(&data, &signature);

let is_valid = result.expect("Verification should not fail");
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between Err() and Ok(false) for verifier.verify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chad says:
Ok(false): The operation itself succeeded, but the verification result is false. This means that the signature is valid in terms of format and process, but it does not match the data or is otherwise invalid according to the verification logic.

and yea then err is an exception basically if I'm understnading rust correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh and as far as the implementation yea just verified false is invalid signature vs catastrophic failure

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI in #300 I'm moving in favor of the monad pattern so verify returns a void and utilizes the error case for cryptographic failure

no actionable items in this PR, just making you aware (and I'll be sure to remediate test vectors prior to merging #300 )

@nitro-neal nitro-neal merged commit 7042127 into main Aug 20, 2024
15 checks passed
@nitro-neal nitro-neal deleted the ed25519-test-vectors branch August 20, 2024 21:44
diehuxx pushed a commit that referenced this pull request Aug 21, 2024
* main:
  Use error for Verifier::verify(), Add ed25519 unit tests, Add Web5Error::Crypto (#300)
  Test Secp256k1Generator (#302)
  Generate X25519 keys (#301)
  Support X25519 in did:dht verification method (#299)
  add sign and verify test vectors (#292)
  Add feature complete Jwk (#294)
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.

4 participants