Add ckm aes key wrap pad by abbra · Pull Request #448 · latchset/kryoptic

abbra · 2026-05-05T07:23:32Z

Implement CKM_AES_KEY_WRAP_PAD as defined in PKCS#11 v3.0 section 2.16. This mechanism applies PKCS#7 padding (block size 8) to input data before wrapping with the standard AES Key Wrap algorithm (RFC 3394 / NIST SP 800-38F Section 6.2). It uses the same OpenSSL cipher as CKM_AES_KEY_WRAP (AES-NNN-WRAP), with padding handled in the Kryoptic layer.

This is needed for FreeIPA migration from SoftHSMv2 to Kryoptic, where existing wrapped key material uses this mechanism.

Assisted-by: Claude Code, using Opus 4.6 model

Checklist

Test suite updated
Rustdoc string were added or updated
CHANGELOG and/or other documentation added or updated
This is not a code change

Reviewer's checklist:

Any issues marked for closing are fully addressed
There is a test suite reasonably covering new functionality or modifications
This feature/change has adequate documentation added
A changelog entry is added if the change is significant
Code conform to coding style that today cannot yet be enforced via the check style test
Commits have short titles and sensible text
Doc string are properly updated

gemini-code-assist

Code Review

This pull request implements support for the CKM_AES_KEY_WRAP_PAD mechanism, including documentation, FIPS indicators, and OpenSSL backend integration. However, the current implementation incorrectly handles multi-part operations by performing padding and wrapping/unwrapping within the update methods instead of buffering data for the final methods. Additionally, the padding validation logic in the decryption path is inefficient due to heap allocations and should be replaced with a more performant constant-time check.

Jakuje · 2026-05-05T09:27:18Z

+            let enc = ret_or_panic!(encrypt(
+                session,
+                handle,
+                data.as_slice(),
+                &mechanism,
+            ));
+            let dec = ret_or_panic!(decrypt(
+                session,
+                handle,
+                enc.as_slice(),
+                &mechanism,
+            ));


this tests the encrypt/decrypt opearations, but not the wrap/unwrap too?

Jakuje · 2026-05-05T09:27:45Z

                | CKF_DERIVE,
        },
+        FipsMechanism {
+            mechanism: CKM_AES_KEY_WRAP_PAD,


@simo5 is this mechanism OK from FIPS point of view?

Implement CKM_AES_KEY_WRAP_PAD as defined in PKCS#11 v3.0 section 2.16. This mechanism applies PKCS#7 padding (block size 8) to input before wrapping with the standard AES Key Wrap algorithm (RFC 3394 / NIST SP 800-38F Section 6.2), using the same OpenSSL cipher as CKM_AES_KEY_WRAP. - Register CKM_AES_KEY_WRAP_PAD with encrypt/decrypt/wrap/unwrap flags - Accept optional 8-byte IV parameter (same as CKM_AES_KEY_WRAP) - Buffer input in encrypt_update; apply PKCS#7 padding and encrypt in encrypt_final to correctly handle multi-part operations - Buffer ciphertext in decrypt_update; unwrap and strip PKCS#7 padding in decrypt_final using an allocation-free constant-time XOR loop - Update rustdoc for wrap/unwrap/encrypt_update/decrypt_update methods and add module-level SP 800-38F reference This is needed for FreeIPA migration from SoftHSMv2 to Kryoptic, where existing wrapped key material uses this mechanism. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

Register CKM_AES_KEY_WRAP_PAD in the FIPS approved mechanism table with encrypt/decrypt/wrap/unwrap operations permitted. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

Extend the existing wrap/unwrap mechanism loop to include CKM_AES_KEY_WRAP_PAD, and add a dedicated test for non-aligned input sizes (8, 9, 13, 15 bytes) to verify correct PKCS#7 padding round-trips. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

simo5 · 2026-05-05T14:44:55Z

Ok so the mechanism sounded familiar to me and the reason it was not implement is:

6.16.3 AES Key Wrap
[...]
* CKM_AES_KEY_WRAP_PAD
The CKM_AES_KEY_WRAP_PAD mechanism is deprecated. CKM_AES_KEY_WRAP_KWP resp. CKM_AES_KEY_WRAP_PKCS7 shall be used instead.

simo5 · 2026-05-05T14:45:20Z

@abbra any reason why you can't use CKM_AES_KEY_WRAP_PKCS7 instead ?

simo5 · 2026-05-05T14:50:20Z

Also PKCS#11 v3.0 section 2.16 does not exist ... and all the mechanisms are defined in section 6 anyway

simo5 · 2026-05-05T14:53:13Z

Also PKCS#11 v3.0 section 2.16 does not exist ... and all the mechanisms are defined in section 6 anyway

Nevermind we renamed those section in 3.1, it did exist in the old 3.0 ...

simo5 · 2026-05-05T14:56:22Z

That said I would accept a PR for CKM_AES_KEY_WRAP_PKCS7, but I am not so hot on accepting a PR for a deprecated mechanism. I would accept it after CKM_AES_KEY_WRAP_PKCS7 is implemented and only if the data stored using CKM_AES_KEY_WRAP_PAD can't be unwrapped with CKM_AES_KEY_WRAP_PKCS7.

Note that CKM_AES_KEY_WRAP_PKCS7 should eb binary compatible with CKM_AES_KEY_WRAP_PAD so we shouldn't need to implement it, except perhaps as an alias...

simo5

The main issue here seems that decryption depadding is not constant time.

simo5 · 2026-05-05T15:24:14Z

            CKM_AES_KEY_WRAP | CKM_AES_KEY_WRAP_KWP => (),
+            CKM_AES_KEY_WRAP_PAD => {
+                let buf_len = self.buffer.len();
+                let pad_len = AES_KWP_BLOCK - (buf_len % AES_KWP_BLOCK);


we should probably rename AES_KWP_BLOCK to something like AES_KEYWRAP_BLOCK or similar if we start using it for multiple mechanisms.

simo5 · 2026-05-05T15:26:19Z

+            CKM_AES_KEY_WRAP_PAD => {
+                let buf_len = self.buffer.len();
+                let pad_len = AES_KWP_BLOCK - (buf_len % AES_KWP_BLOCK);
+                self.buffer.resize(buf_len + pad_len, pad_len as u8);


pad_len as u8 is unsafe, use u8::try_feom() and handle the error

simo5 · 2026-05-05T16:24:59Z

+                        Err(CKR_DEVICE_ERROR)
+                    },
+                )?;
+                zeromem(self.buffer.as_mut_slice());


This should not be necessary, key wrap is single-shot only and we zeromem the buffer on deallocation

simo5 · 2026-05-05T16:33:20Z

                }
            }
+            CKM_AES_KEY_WRAP_PAD => {
+                self.buffer.extend_from_slice(cipher);


if the buffer length is not zero before the extend this should fail because wrapping is a one shot operation.

simo5 · 2026-05-05T16:36:35Z

            }
+            CKM_AES_KEY_WRAP_PAD => {
+                self.buffer.extend_from_slice(cipher);
+                cipher_offset = cipher_end;


this shouldn't even happen, if padding is deferred to _final then probably this mechanism should return w/o going though openssl encryption here.

simo5 · 2026-05-05T16:44:29Z

+                if outlen < AES_KWP_BLOCK {
+                    return Err(CKR_ENCRYPTED_DATA_INVALID)?;
+                }
+                let pad_byte = plain[outlen - 1];


The code here and below is not constant time, and will easily reveal the last byte resulting from decryption.

simo5 · 2026-05-05T17:08:55Z

                    }
                }
+                CKM_AES_KEY_WRAP_PAD => {
+                    ((data_len / AES_KWP_BLOCK) + 2) * AES_KWP_BLOCK


A coment that explains why +2 would be useful here

abbra mentioned this pull request May 5, 2026

WIP: migrate to kryoptic freeipa/freeipa#8392

Open

gemini-code-assist Bot reviewed May 5, 2026

View reviewed changes

Comment thread src/ossl/aes.rs

Comment thread src/ossl/aes.rs Outdated

Comment thread src/ossl/aes.rs Outdated

abbra force-pushed the add-ckm-aes-key-wrap-pad branch from 355bb86 to 41cc8bb Compare May 5, 2026 07:57

Jakuje reviewed May 5, 2026

View reviewed changes

abbra force-pushed the add-ckm-aes-key-wrap-pad branch from 41cc8bb to 8bbc7d8 Compare May 5, 2026 10:07

abbra force-pushed the add-ckm-aes-key-wrap-pad branch from 8bbc7d8 to 0aaa71f Compare May 5, 2026 10:36

abbra added 3 commits May 5, 2026 15:28

fips: add FIPS indicator entry for CKM_AES_KEY_WRAP_PAD

c0ea894

Register CKM_AES_KEY_WRAP_PAD in the FIPS approved mechanism table with encrypt/decrypt/wrap/unwrap operations permitted. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

CHANGELOG: document CKM_AES_KEY_WRAP_PAD mechanism addition

395cb2f

Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>

abbra force-pushed the add-ckm-aes-key-wrap-pad branch from 0aaa71f to 395cb2f Compare May 5, 2026 12:28

simo5 requested changes May 5, 2026

View reviewed changes

Conversation

abbra commented May 5, 2026

Checklist

Reviewer's checklist:

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

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!

simo5 commented May 5, 2026

Uh oh!

simo5 commented May 5, 2026

Uh oh!

simo5 commented May 5, 2026

Uh oh!

simo5 commented May 5, 2026

Uh oh!

simo5 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simo5 commented May 5, 2026 •

edited

Loading