Add PKCS#11 3.2 attribute to NSSDB storage by simo5 · Pull Request #310 · latchset/kryoptic

simo5 · 2025-07-31T13:05:37Z

Description

This catches up nssdb to the changes happenning in NSS upstream

Fixes #307

Checklist

Test suite updated with functionality tests
Test suite updated with negative tests
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

src/storage/nssdb/attrs.rs

Jakuje

I am wondering how the PQC algorithm tests worked before this with the NSS DB when the new attributes like Encapsulate, ParamSet and others could not have been stored in the database ....

src/storage/nssdb/mod.rs

simo5 · 2025-07-31T14:31:37Z

I am wondering how the PQC algorithm tests worked before this with the NSS DB when the new attributes like Encapsulate, ParamSet and others could not have been stored in the database ....

Good q, I think they simply always use the sqlite database, we'll have to figure out how to make sure most test are also tried on nssdb, perhaps a build target that has only the nssdb ?

Jakuje · 2025-07-31T14:34:41Z

I am wondering how the PQC algorithm tests worked before this with the NSS DB when the new attributes like Encapsulate, ParamSet and others could not have been stored in the database ....

Good q, I think they simply always use the sqlite database, we'll have to figure out how to make sure most test are also tried on nssdb, perhaps a build target that has only the nssdb ?

Thats not how I wrote that. When nssdb is enabled, it should default to NSSDB, unless something changed since I did this:

kryoptic/src/tests/mod.rs

Line 191 in 461010b

fn get_default_db(name: &str) -> (&'static str, String) {

simo5 · 2025-07-31T14:54:34Z

I think I changed that because at some point all CI jobs included nssdb, and that means sqlite was never tested, so I think I change something and then forgot about it.

Maybe we should open a separate issue to follow up on this?

simo5 · 2025-08-01T07:13:28Z

Marking this as draft because I think we need to add at least a test that will upgrade a database with the new attributes and work correctly after doing that

This catches up nssdb to the changes happenning in NSS upstream Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2025-09-26T17:41:21Z

The test_nssdb_key_cache test does upgrade the database we hold in testdata/nssdbdir and I can see that it does in fact get properly upgraded with new columns for all the PKCS#11 3.2 new attributes.

simo5 · 2025-09-26T17:42:33Z

Still keeping this in draft until I can get my hands on another test database generate by a new enough version of NSS that has the new attributes so I can ensure not only the upgrade path but also full compatibility with dbs generated by new NSS versions.

Jakuje · 2025-09-29T07:12:14Z

src/storage/nssdb/attrs.rs

-    CKA_UNIQUE_ID,
-    CKA_COPYABLE,
-    CKA_DESTROYABLE,


please, make sure we have tests with these attributes executed with the new nss db

why?
they are just regular attributes like any other

I think we might have some tests for them that are either excluded with the nssdb backend and if not, we should test they make it through at all cases.

I wonder if we should change the tests infra to look at an enve var in tests which will set what is the db to use for tests, and then have one of the builds test with forcing NSSDB on, thi way we have more control over what DB is used for testing regardless of how many DBs we've built into the binary ?

We already have the tests forcing one type of DBs based on enabled features (currently just NSS and SQLite):
https://github.com/latchset/kryoptic/blob/main/src/tests/mod.rs#L191
all of the code initialization goes through this function

Yes, but I think we should always pick sqlitedb by default, and have an environment variable to forcibly change the DB type, such that if the db type is not compiled and the variable is set it will error out as well.

This way we can be sure of what we are testing in CI, currently it is almost a guess which DB is tested and we can't test "the other" DB when multiple are compiled in which is not great because applications can chose which one to use and we are not testing if the behavior of "the other" dbs is not broken by config options enabled by whichever one is chosen at test time.

src/storage/nssdb/mod.rs

Implement a mechanism to check and upgrade the NSS SQLite database schema upon initialization. This change introduces a check that runs when the database is opened. It compares the columns in the `nssPublic` and `nssPrivate` tables against a list of known PKCS#11 attributes. If any columns for known attributes are missing, they are automatically added using `ALTER TABLE`. This ensures that existing databases can be seamlessly upgraded to support new attributes (e.g., from PKCS#11 3.2) without manual intervention. The logic for converting database rows to objects is also refactored to use a new column cache, improving efficiency and robustness. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2025-09-29T18:45:59Z

@Jakuje marking as ready to review because now I have a DB generated by NSSDB with the attributes we need (and a fix to handle a NSS bug ...)

src/tests/nssdb.rs

This was generated by NSS code and we use it to test compatibility woth NSS written DBs. Signed-off-by: Simo Sorce <simo@redhat.com>

Some versions of NSS have a bug where the CKA_PARAMETER_SET attribute is written to the database as a raw, platform-endian CK_ULONG (4 or 8 bytes) instead of the expected 4-byte big-endian database integer. This change adds a workaround to correctly read this attribute from malformed databases. It attempts to parse the stored blob as both a 32-bit and 64-bit integer, checking both little-endian and big-endian formats. Since the valid values for this attribute are small numbers, we can reliably recover the intended value, ensuring compatibility. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the nss_p11_3.2 branch from c368230 to 1363842 Compare July 31, 2025 13:16

simo5 requested a review from Jakuje July 31, 2025 13:16

Jakuje reviewed Jul 31, 2025

View reviewed changes

src/storage/nssdb/attrs.rs Outdated Show resolved Hide resolved

src/storage/nssdb/attrs.rs Show resolved Hide resolved

simo5 force-pushed the nss_p11_3.2 branch from 1363842 to 7bed5e7 Compare July 31, 2025 13:42

Jakuje reviewed Jul 31, 2025

View reviewed changes

src/storage/nssdb/mod.rs Show resolved Hide resolved

simo5 marked this pull request as draft August 1, 2025 07:12

simo5 force-pushed the nss_p11_3.2 branch from 7bed5e7 to 1fa40ef Compare September 17, 2025 19:12

Add PKCS#11 3.2 attribute to NSSDB storage

c731307

This catches up nssdb to the changes happenning in NSS upstream Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the nss_p11_3.2 branch 2 times, most recently from c2660a7 to 9c17377 Compare September 26, 2025 17:40

Jakuje reviewed Sep 29, 2025

View reviewed changes

simo5 force-pushed the nss_p11_3.2 branch from 9c17377 to 02be4a4 Compare September 29, 2025 18:43

simo5 marked this pull request as ready for review September 29, 2025 18:45

simo5 force-pushed the nss_p11_3.2 branch from 02be4a4 to 3f3291a Compare September 29, 2025 18:58

Jakuje approved these changes Sep 29, 2025

View reviewed changes

src/tests/nssdb.rs Show resolved Hide resolved

simo5 added 2 commits September 29, 2025 17:19

Add newer nssdb with ML-DA keys

929bc7f

This was generated by NSS code and we use it to test compatibility woth NSS written DBs. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the nss_p11_3.2 branch from 3f3291a to 5236604 Compare September 29, 2025 21:19

simo5 mentioned this pull request Sep 29, 2025

Allow to select which DB to use in tests via env var #345

Closed

simo5 merged commit 8f50a16 into latchset:main Sep 29, 2025
49 checks passed

Comments

Conversation

simo5 commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Reviewer's checklist:

Uh oh!

Uh oh!

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simo5 commented Jul 31, 2025

Uh oh!

Jakuje commented Jul 31, 2025

Uh oh!

simo5 commented Jul 31, 2025

Uh oh!

simo5 commented Aug 1, 2025

Uh oh!

simo5 commented Sep 26, 2025

Uh oh!

simo5 commented Sep 26, 2025

Uh oh!

Jakuje Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

simo5 Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Jakuje Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

simo5 Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Jakuje Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

simo5 Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simo5 commented Sep 29, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

simo5 commented Jul 31, 2025 •

edited

Loading

simo5 Sep 29, 2025 •

edited

Loading