-
Notifications
You must be signed in to change notification settings - Fork 4
removed unused suffix from signature #21
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
base: SHARD-2023
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| export function verifyDetached(msg: string, sig: hexstring | Buffer, pk: publicKey | Buffer): boolean { | ||
| if (typeof msg !== 'string') { | ||
| throw new TypeError('Message to compare must be a string.') | ||
| } | ||
| const msgBuf = Buffer.from(msg, 'hex') | ||
| const sigBuf = _ensureBuffer(sig) | ||
| const pkBuf = _ensureBuffer(pk) | ||
|
|
||
| if (sigBuf.length !== sodium.crypto_sign_BYTES) { | ||
| throw new Error('Invalid signature length for detached signature.') | ||
| } | ||
|
|
||
| try { | ||
| const opened = Buffer.allocUnsafe(sigBuf.length - sodium.crypto_sign_BYTES) | ||
| sodium.crypto_sign_open(opened, sigBuf as Buffer, pkBuf as Buffer) | ||
| const verified = opened.toString('hex') | ||
| return verified === msg | ||
| return sodium.crypto_sign_verify_detached(sigBuf as Buffer, msgBuf, pkBuf as Buffer) | ||
| } catch (e) { | ||
| throw new Error('Unable to verify provided signature with provided public key.') | ||
| return false | ||
| } | ||
| } |
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.
Suggestion: Throwing an error for invalid signature length may allow attackers to distinguish between valid and invalid signatures, potentially leaking information. Instead, return false for invalid signature lengths to avoid information leakage. [security, importance: 8]
| export function verifyDetached(msg: string, sig: hexstring | Buffer, pk: publicKey | Buffer): boolean { | |
| if (typeof msg !== 'string') { | |
| throw new TypeError('Message to compare must be a string.') | |
| } | |
| const msgBuf = Buffer.from(msg, 'hex') | |
| const sigBuf = _ensureBuffer(sig) | |
| const pkBuf = _ensureBuffer(pk) | |
| if (sigBuf.length !== sodium.crypto_sign_BYTES) { | |
| throw new Error('Invalid signature length for detached signature.') | |
| } | |
| try { | |
| const opened = Buffer.allocUnsafe(sigBuf.length - sodium.crypto_sign_BYTES) | |
| sodium.crypto_sign_open(opened, sigBuf as Buffer, pkBuf as Buffer) | |
| const verified = opened.toString('hex') | |
| return verified === msg | |
| return sodium.crypto_sign_verify_detached(sigBuf as Buffer, msgBuf, pkBuf as Buffer) | |
| } catch (e) { | |
| throw new Error('Unable to verify provided signature with provided public key.') | |
| return false | |
| } | |
| } | |
| export function verifyDetached(msg: string, sig: hexstring | Buffer, pk: publicKey | Buffer): boolean { | |
| if (typeof msg !== 'string') { | |
| throw new TypeError('Message to compare must be a string.') | |
| } | |
| const msgBuf = Buffer.from(msg, 'hex') | |
| const sigBuf = _ensureBuffer(sig) | |
| const pkBuf = _ensureBuffer(pk) | |
| if (sigBuf.length !== sodium.crypto_sign_BYTES) { | |
| return false | |
| } | |
| try { | |
| return sodium.crypto_sign_verify_detached(sigBuf as Buffer, msgBuf, pkBuf as Buffer) | |
| } catch (e) { | |
| return false | |
| } | |
| } |
| * @param detached - If true (default), returns only the signature (64 bytes). If false, returns signature + message | ||
| */ | ||
| export function sign(input: hexstring | Buffer, sk: secretKey | Buffer): string { | ||
| export function sign(input: hexstring | Buffer, sk: secretKey | Buffer, detached = true): string { |
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.
Suggestion: The function currently returns a hex string for both detached and non-detached signatures, but the non-detached signature contains both the signature and the message. If the input is sensitive, ensure that returning the concatenated signature and message does not leak unintended information. Consider documenting this behavior clearly to prevent misuse. [general, importance: 6]
Refactors the signing and verification logic to default to detached signatures for improved security and efficiency. Adds functionality to manually convert non-detached signatures to detached signatures, ensuring compatibility and flexibility in signature management. Updates tests to reflect the new default behavior and conversion capabilities.
7714612 to
14f302d
Compare
PR Type
Enhancement, Tests
Description
Added full support for detached Ed25519 signatures (signing and verification)
Introduced new functions:
signDetached,verifyDetached,signObjDetached,verifyObjDetachedUpdated
signandsignObjto default to detached signatures, with backward compatibilityAdded comprehensive unit tests for detached and legacy signatures
Changes walkthrough 📝
sodium-native.d.ts
Add typings for detached signature sodium-native functionssodium-native.d.ts
crypto_sign_detachedandcrypto_sign_verify_detachedindex.ts
Implement detached signature support and update signing APIsrc/index.ts
signDetached,verifyDetached,signObjDetached,verifyObjDetachedfunctions
signandsignObjto support and default to detached signaturesfunctions
detachedSignatures.test.ts
Add unit tests for detached and legacy signature supporttest/unit/detachedSignatures.test.ts