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.

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!

cryptoki/src/session/signing_macing.rs

wiktor-k

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

cryptoki/src/error/mod.rs

cryptoki/src/session/signing_macing.rs

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! 👋

cryptoki/src/session/signing_macing.rs

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-02-18T10:14:18Z

Added tests, resolved review comments, and switched to &self instead of &mut self.

Jakuje · 2026-02-18T10:12:17Z

cryptoki/src/session/signing_macing.rs

+        assert!(
+            sig_len <= sig_buf_len,
+            "'C_Sign' succeeded but increased 'sig_len', possibly indicating out-of-bounds accesses"
+        );


I think we should not use assert!() throughout the code, just in tests. It is either something that can not happen and then the check should not be there or it can happen and we should report proper error, regardless how much unlikely it would be or how much the pkcs11 module would have to be broken.

I initially had a TODO comment here discussing whether an assert was appropriate. We could add some kind of ImplementationError variant to crate::error::Error, maybe that would be helpful for other functions too. @hug-dev, what do you think?

I don't think that PKCS11 implementation should increase the sig_len value while returning CK_OK (as per the specs). If they do then I would say that it's an error in the implementation but maybe not something that we should check in runtime.

So I would also be in favor of removing those asserts!

My only concern is that user code will assume this property to be upheld. It would be better for cryptoki to forcibly crash than to introduce a buffer overflow in user code (e.g. if the user does &sig[..sig_len], which loses bounds checks in release mode).

AFAIK Rust still inserts bounds checks in release mode (source https://news.ycombinator.com/item?id=39261604) unless the optimizer can deduct that it's safe to remove (eg. in for loops).

Maybe yet another possibility would be to use debug_assert? 🤔

I still think returning an error is more appropriate than assert in any production code though.

Yeah, asserts in production code are especially tricky. For really rare occasions even stdlib has panics but maybe we can "have a cookie and eat it too" here by introducing some error variant with just a string description that'd be a catch-all for all these assert-like bugs? 🤔

My only concern is that user code will assume this property to be upheld.

I think that is fair to assume (we can even write a warning notice about that in the readme). If we can't trust that the PKCS11 library implements the spec correctly then there is not much we can do 😢

Yeah, asserts in production code are especially tricky.

I would normally agree, but I think "we think UB has occurred, but we don't want to panic, maybe everything is fine" is concerning. I don't know what the ideal solution is. I'm happy to implement whatever you (the maintainers) think should happen.

I'm happy to implement whatever you (the maintainers) think should happen.

That's great, we just have a little split brain problem here so it may take a while until we have a consensus:)

Personally I'm okay with an assert of a very unlikely scenario. I'm also fine with a new Error enum variant if that makes @Jakuje calm.

The "no check at all" scenario is the worse one IMO.

I agree so don't let me hold the PR back as it is a great improvement! I agree that this case really means the underlying token is broken so maybe the assert is adequate. I just know how annoying is when somewhere deep in the dependency chain of your built package you get assert and there is nothing you can do but rebuild.

cryptoki/src/session/signing_macing.rs

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

cryptoki/src/session/signing_macing.rs Outdated Show resolved Hide resolved

cryptoki/src/session/signing_macing.rs Outdated Show resolved Hide resolved

wiktor-k reviewed Jan 29, 2026

View reviewed changes

Jakuje reviewed Feb 2, 2026

View reviewed changes

cryptoki/src/session/signing_macing.rs Outdated Show resolved Hide resolved

cryptoki/src/session/signing_macing.rs Outdated Show resolved Hide resolved

bal-e force-pushed the alloc-free-ops branch from c072317 to cbf4015 Compare February 17, 2026 14:02

bal-e requested a review from hug-dev February 17, 2026 14:05

bal-e force-pushed the alloc-free-ops branch from cbf4015 to 489e10d Compare February 18, 2026 10:13

Jakuje reviewed Feb 18, 2026

View reviewed changes

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!

Uh oh!

Uh oh!

wiktor-k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wiktor-k commented Jan 29, 2026

Uh oh!

Uh oh!

Uh oh!

bal-e commented Feb 18, 2026

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bal-e commented Jan 26, 2026 •

edited

Loading