Conversation
Two issues: (1) short ciphertext (29-43 bytes) passes the length check but causes OverflowException inside TweetNaCl from a negative array allocation. Tighten the minimum to require BoxBoxZeroBytes (16) after the header. (2) Auth failures throw InvalidCipherTextException which extends Exception, not NKeysException, so callers catching NKeysException miss decryption errors. Wrap the TweetNaCl call to normalize all exceptions to NKeysException.
Tighten three existing tests from ThrowsAny<Exception> to Throws<NKeysException> now that Open() normalizes exceptions. Add a theory test verifying short inputs (0, 28, 29, 43 bytes) are rejected with NKeysException.
There was a problem hiding this comment.
Pull request overview
This PR hardens KeyPair.Open() for Curve (xkey) decryption by preventing short ciphertexts from reaching TweetNaCl in a way that can trigger runtime exceptions, and by normalizing decryption/authentication failures to NKeysException so callers can reliably catch NKeys-specific failures.
Changes:
- Tightened the minimum sealed payload length check in
Open()to include NaCl box overhead. - Wrapped the TweetNaCl
CryptoBoxOpencall to convert non-NKeysExceptionfailures intoNKeysException. - Updated/added tests to assert
NKeysExceptionfor wrong sender/receiver, tampering, and short inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| NATS.NKeys/KeyPair.cs | Strengthens input validation and wraps TweetNaCl exceptions during Open() to normalize error types. |
| NATS.NKeys.Tests/NKeysTest.cs | Adjusts existing blackbox tests to expect NKeysException and adds coverage for short-input rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| } | ||
| catch (TweetNaCl.NKeyNaclException) | ||
| { | ||
| throw new NKeysException("Decryption failed"); |
There was a problem hiding this comment.
wouldn't it be useful to also know what the actual error was from the underlying library?
There was a problem hiding this comment.
good point. I can add it as inner
aricart
left a comment
aricart
left a comment
There was a problem hiding this comment.
LGTM - but possibly better to let the caller know why it failed- it may have the key length, or something corrupt, etc.
Pass the caught NKeyNaclException as InnerException so callers can inspect the underlying failure reason.
Open() had two issues: short ciphertext (29-43 bytes) passed the length check but caused OverflowException from a negative array allocation in TweetNaCl, and auth failures threw InvalidCipherTextException (not a subclass of NKeysException) so callers catching NKeysException would miss decryption errors. Tightened the minimum input length and wrapped the TweetNaCl call to normalize exceptions.
Open_rejects_short_input(0, 28, 29, 43 bytes all throw NKeysException)DecodePubCurveKey_blackbox_tampered_ciphertext_fails(tampered ciphertext throws NKeysException)DecodePubCurveKey_blackbox_wrong_sender_fails_open(wrong sender throws NKeysException)DecodePubCurveKey_blackbox_wrong_receiver_fails_open(wrong receiver throws NKeysException)