Set ulIvBits and more graceful error handling #249
Conversation
…re graceful error handling Signed-off-by: Jason Parker <[email protected]>
jaeparker22
force-pushed
the
ulivbits-optional-assign
branch
from
05673ff to
6524352
Compare
wiktor-k
left a comment
wiktor-k
left a comment
There was a problem hiding this comment.
Looks good, a couple of nits.
cryptoki/src/mechanism/aead.rs
Outdated
| /// This function panics if the length of `iv` or `aad` does not | ||
| /// fit into an [Ulong]. | ||
| pub fn new(iv: &'a mut [u8], aad: &'a [u8], tag_bits: Ulong) -> Self { | ||
| pub fn new(iv: &'a mut [u8], aad: &'a [u8], tag_bits: Ulong) -> Result<Self, &'a str> { |
There was a problem hiding this comment.
I think I'd leave the "Panics" section but rename it to "Errors" since then it'd be clear when this function fails.
While &'a str is not ideal (a custom error would be better) it's not super-bad IMO (but maybe the lifetime of that should be 'static since it's just static messages? It's not tied to iv in any way).
There was a problem hiding this comment.
There is a TryFromIntError in the crate, which I initially thought to use and is the natural error generated. Only reason I didn't was because I wanted to maintain the original error message.
There was a problem hiding this comment.
Only reason I didn't was because I wanted to maintain the original error message
Do you mean your custom error message?
I think it would be perfect to use TryFromIntError here 😃 I think users would understand based on the context/line number?
There was a problem hiding this comment.
I think users would understand based on the context/line number?
Happy to be corrected here but AFAIK Errors generally don't provide any kind of backtrace when they're captured (it is possible to manually capture a Backtrace but I didn't find any mention of it being automatic in std::error::Error). This is one of the benefits of panics (but of course, not really applicable to production code).
I'm OK with just using TryFromIntError for now (even if that means it'll be harder to find the cause).
There was a problem hiding this comment.
I'll set it to return a TryFromIntError then.
Signed-off-by: Jason Parker <[email protected]>
Signed-off-by: Jason Parker <[email protected]>
Co-authored-by: Wiktor Kwapisiewicz <[email protected]> Signed-off-by: Jason Parker <[email protected]>
Signed-off-by: Jason Parker <[email protected]>
jaeparker22
force-pushed
the
ulivbits-optional-assign
branch
from
d2e1a2e to
bd41a6b
Compare
Signed-off-by: Jason Parker <[email protected]>
hug-dev
left a comment
hug-dev
left a comment
There was a problem hiding this comment.
Thanks a lot! It looks all good for me, I would though be more in favour of re-using our Error type
cryptoki/src/mechanism/aead.rs
Outdated
| /// This function panics if the length of `iv` or `aad` does not | ||
| /// fit into an [Ulong]. | ||
| pub fn new(iv: &'a mut [u8], aad: &'a [u8], tag_bits: Ulong) -> Self { | ||
| pub fn new(iv: &'a mut [u8], aad: &'a [u8], tag_bits: Ulong) -> Result<Self, &'a str> { |
There was a problem hiding this comment.
Only reason I didn't was because I wanted to maintain the original error message
Do you mean your custom error message?
I think it would be perfect to use TryFromIntError here 😃 I think users would understand based on the context/line number?
Signed-off-by: Jason Parker <[email protected]>
…ring. Signed-off-by: Jason Parker <[email protected]>
jaeparker22
force-pushed
the
ulivbits-optional-assign
branch
from
51568e9 to
a3a791f
Compare
|
Looks really great. I'd be nice if all these were squashed (since the code looks really simple now) but... I guess we don't need to block merging for that. Thanks for sticking with us throughout the entire process. Deeply appreciated! 🙇 |
3 participants
Author: Jason Parker
Email: [email protected]
Modify GcmParams::new to return a Result rather than panic!() and auto-set ulIvBits to support those HSMs that require it, such as Thales. If the IV length is such that ulIvBits is too long, it will be set to 0 as previously. Either the HSM doesn't care and can work without it, or the operation would fail either way.
Tests using GCM params have been updated to reflect the function now returning a Result and a test has been added for the new operation with ulIvBits.