Allocation-free operations by bal-e · Pull Request #349 · parallaxsecond/rust-cryptoki

bal-e · 2026-01-26T07:22:52Z

Starts addressing #346.

Question for the reviewers:

What do you think of the added documentation? Is it too much? Or should I add more to the related sign() functions for consistency?
I don't have a strict need to implement this for more operations, but I'm happy to do one or two sets more. Are there any in particular you'd like to see? I'm leaning towards key generation (which I need, but is not performance-sensitive) and encryption (which I guess more people would want to use).
Would you have any tips for adding these methods to the test suite? It'll have to be a bit algorithm-specific.

Signed-off-by: arya dradjica <arya@nlnetlabs.nl>

These '_into()' variants can be used when the caller knows the expected buffer size, and has allocated a buffer for the signature themselves. It requires fewer calls into the underlying library and can save on heap allocations. Signed-off-by: arya dradjica <arya@nlnetlabs.nl>

bal-e · 2026-01-26T07:31:10Z

I see working with c_ulong is a bit frustrating.

hug-dev

Thank you!
Really like the added documentation, I think that's good! My only suggestion is to simplify and return the original error as is :)

For testing I think it's fine trying one or a few variants. You can for example use the same one which is done in the test currently for the sign operation!

hug-dev · 2026-01-27T12:59:01Z

cryptoki/src/session/signing_macing.rs

+    ///
+    /// `data` should be a byte sequence representing the input message. It will
+    /// be signed using the specified key, and the resulting signature will be
+    /// written to the given output buffer.


maybe add the name of that output buffer to mirror how you did it with data, ie sig.

hug-dev · 2026-01-27T13:05:11Z

cryptoki/src/session/signing_macing.rs

+        match res {
+            Rv::Ok => {
+                // TODO: Should this be an Error of some kind?
+                assert!(sig_len <= sig_buf_len);
+
+                // 'sig_len <= sig_buf_len <= usize::MAX'.
+                Ok(sig_len as usize)
+            }
+
+            Rv::Error(RvError::BufferTooSmall) => {
+                // TODO: Should this be an Error of some kind?
+                assert!(sig_len > sig_buf_len);
+
+                // All reasonable buffer sizes fit in 'usize'.
+                let sig_len: usize = sig_len.try_into()?;
+
+                Err(Error::IncorrectBufferSize(sig_len))
+            }
+
+            // The error seems unrelated to the buffer size, so we don't check
+            // what 'sig_len' has been set to (if anything).
+            Rv::Error(err) => Err(Error::Pkcs11(err, Function::Sign)),
+        }


My personal opinion is that we can simply return return res as is, without the new error IncorrectBufferSize. I think the original PKCS11 error code BufferTooSmall is explicit enough for the caller 😸 Same for the multi steps variant!

If needed, we could also add a new method which would return the size of a signature with this specific key and mechanism!

Right, I had originally added IncorrectBufferSize because I wanted to enforce that the caller had provided exactly the right size (and not one that was too big). Since that is no longer the case, the error is not necessary. Good catch, I'll remove it.

wiktor-k

Looks great, a couple of nitpicks/clarifications. Thanks for your contribution! 🙇

wiktor-k · 2026-01-29T13:11:02Z

cryptoki/src/error/mod.rs

+    ///
+    /// The buffer may be too small or too large. The expected size (in bytes)
+    /// is returned.
+    IncorrectBufferSize(usize),


I think I'd make it a bit more explicit:

Suggested change

IncorrectBufferSize(usize),

IncorrectBufferSize {

expected_size: usize

},

wiktor-k · 2026-01-29T13:11:51Z

cryptoki/src/session/signing_macing.rs

 use cryptoki_sys::*;
 use std::convert::TryInto;

+/// # Generating and Verifying Signatures


Hmm... are these docs rendered anywhere? If not I'd move the docs to the bottom of docs for struct Session

wiktor-k · 2026-01-29T13:12:52Z

cryptoki/src/session/signing_macing.rs

+    /// (i.e. `sig[0..size]` contains the prepared signature). Otherwise,
+    /// [`Error::IncorrectBufferSize`] is returned.
+    ///
+    /// Use [`Self::sign()`] if (an upper bound for) the size of the signature


I'm not sure if these parens are working 🤔

Suggested change

/// Use [`Self::sign()`] if (an upper bound for) the size of the signature

/// Use [`Self::sign`] if (an upper bound for) the size of the signature

wiktor-k · 2026-01-29T13:15:10Z

cryptoki/src/session/signing_macing.rs

+        // Check the result and buffer size.
+        match res {
+            Rv::Ok => {
+                // TODO: Should this be an Error of some kind?


I don't think so, we promise in the docs to write only up to sig_len size so the assert is fine here. I'd add a little bit of docs to the assert...

wiktor-k · 2026-01-29T13:15:56Z

cryptoki/src/session/signing_macing.rs

+        match res {
+            Rv::Ok => {
+                // TODO: Should this be an Error of some kind?
+                assert!(sig_len <= sig_buf_len);


A random suggestion:

Suggested change

assert!(sig_len <= sig_buf_len);

assert!(sig_len <= sig_buf_len, "Signature size is bigger than the buffer size but the PKCS#11 module returned OK, this is not right.");

wiktor-k · 2026-01-29T13:16:53Z

cryptoki/src/session/signing_macing.rs

+            }
+
+            Rv::Error(RvError::BufferTooSmall) => {
+                // TODO: Should this be an Error of some kind?


Similarly to the case above with some clever wording :)

wiktor-k · 2026-01-29T13:20:33Z

What do you think of the added documentation? Is it too much? Or should I add more to the related sign() functions for consistency?

I think it looks fine as it is 👌

I don't have a strict need to implement this for more operations, but I'm happy to do one or two sets more. Are there any in particular you'd like to see? I'm leaning towards key generation (which I need, but is not performance-sensitive) and encryption (which I guess more people would want to use).

No pressure to add more, this may grow organically as use-cases appear. If you feel like doing additional work I'm not stopping you though 😅

Would you have any tips for adding these methods to the test suite? It'll have to be a bit algorithm-specific.

Yep, ideally the tests should exercise all paths, including error ones and check if returned errors match what we expect. Choosing one algo for tests is sufficient. (Ideally it should be implemented by both softhsm and kryoptic but I don't think you'll use anything exotic).

Thanks! 👋

Jakuje · 2026-02-02T15:01:36Z

cryptoki/src/session/signing_macing.rs

+    /// The output buffer should be large enough to store the signature.
+    /// If it is large enough, the number of filled bytes is returned
+    /// (i.e. `sig[0..size]` contains the prepared signature). Otherwise,
+    /// [`Error::IncorrectBufferSize`] is returned.


might make sense to be explicit here too that the returned IncorrectBufferSize has the expected size.

Jakuje · 2026-02-02T15:54:53Z

cryptoki/src/session/signing_macing.rs

+    //
+    // TODO: Is it possible to re-do this call if an incorrect buffer size
+    // was passed in? I think so?
+    pub fn sign_final_into(&mut self, sig: &mut [u8]) -> Result<usize> {


Is there a way to reuse the sign_final() we have? Or reuse this in the existing sign_final() function? It has a lot of duplicated code.

Similarly for the sign_into().

[error] Add 'Error::IncorrectBufferSize'

c3fc3bb

Signed-off-by: arya dradjica <arya@nlnetlabs.nl>

bal-e force-pushed the alloc-free-ops branch 2 times, most recently from 3c1d3ff to bfba84a Compare January 26, 2026 07:25

bal-e force-pushed the alloc-free-ops branch from bfba84a to c072317 Compare January 26, 2026 07:31

hug-dev reviewed Jan 27, 2026

View reviewed changes

wiktor-k reviewed Jan 29, 2026

View reviewed changes

Jakuje reviewed Feb 2, 2026

View reviewed changes

-    IncorrectBufferSize(usize),
+    IncorrectBufferSize {
+       expected_size: usize
+    },

	/// Use [`Self::sign()`] if (an upper bound for) the size of the signature
	/// Use [`Self::sign`] if (an upper bound for) the size of the signature

	assert!(sig_len <= sig_buf_len);
	assert!(sig_len <= sig_buf_len, "Signature size is bigger than the buffer size but the PKCS#11 module returned OK, this is not right.");

Conversation

bal-e commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bal-e commented Jan 26, 2026

Uh oh!

hug-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiktor-k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiktor-k commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bal-e commented Jan 26, 2026 •

edited

Loading