refactor the HSM server and keystore to handle a #48

jpbland1 · 2024-06-18T12:10:38Z

seperate cache for larger keys so customers with limited memory can still store a limited number of large keys alongside their smaller keys.

jpbland1 · 2024-06-18T12:14:20Z

Addressing the issue @bigbrett brought up where we need to support both large RSA keys while still allowing a large number of smaller keys, by making a separate cache for large keys like RSA and the cmac struct customers with limited memory can still use a large number of smaller keys. Considered changing the cache to a heap but we want the cache to be completely deterministic as to the number of keys you can store, rather than having the customer calculate the cachesize they need they should just be able to pick the number of big and small keys they plan to use.

bigbrett

Nice. A few things and request for clarification but looks solid.

bigbrett · 2024-06-28T00:39:20Z

src/wh_server_keystore.c

+    if (i >= WOLFHSM_NUM_RAMKEYS) {
+        for (i = 0; i < WOLFHSM_NUM_BIG_RAMKEYS; i++) {
+            if (server->bigCache[i].meta->id == keyId) {
+                slotCommited = &server->cache[i].commited;


copy pasta error:? shouldn't this be:

Suggested change

slotCommited = &server->cache[i].commited;

slotCommited = &server->bigCache[i].commited;

bigbrett · 2024-06-28T00:44:31Z

src/wh_server_crypto.c

+    uint16_t keySz = WOLFHSM_KEYCACHE_BIG_BUFSIZE;
+#else
+    uint16_t keySz = WOLFHSM_KEYCACHE_BUFSIZE;


pedantic: make this const if not meant to be changed

bigbrett · 2024-06-28T00:50:34Z

src/wh_server_crypto.c

    word32 privSz = CURVE25519_KEYSIZE;
    word32 pubSz = CURVE25519_KEYSIZE;
    whKeyId keyId = WOLFHSM_KEYTYPE_CRYPTO;
    /* get a free slot */
-    ret = slotIdx = hsmCacheFindSlot(server);
-    if (ret >= 0) {
+    ret = hsmCacheFindSlotAndZero(server, CURVE25519_KEYSIZE * 2, &cacheBuf,


can we make the val * 2 a local const variable with a self-describing name just to make it extremely obvious what is going on?

bigbrett · 2024-06-28T01:05:45Z

src/wh_server_crypto.c

+    /* get the lengths since they vary by ecc size, expect BUFFER_E since
+     * LENGTH_ONLY_E isn't supported. Note that this is a dangerous operation 
+     * but by having size of 0 or buffer of NULL we avoid NULL checks when
+     * getting the length */


looks like those args are NULL checked in that wolfCrypt function, so shouldn't necessarily be dangerous - can you elaborate on what you mean here?

It's dangerous because I have to pass in a non-NULL nonsense pointer for the length I'm trying to get, the operation will fail as long as at least one of the lengths, qx or qy, is NULL so it should be good but it warrants a comment, might be wise a refactor ecc to allow a LENGTH_ONLY_E return

bigbrett · 2024-06-28T01:09:35Z

src/wh_server_crypto.c

+    if (ret == 0) {
+        keySz = cacheMeta->len / 3;
+        ret = wc_ecc_import_unsigned(key, cacheBuf, cacheBuf + keySz,
+            cacheBuf + keySz * 2, curveId);


while we're here, can we constify the magic numbers with self describing names just to make it explicitly clear what is going on at a glance

this one can't be const because it depends on the length of the particular ecc key pulled from the metadata, will make a descriptive variable for it though

bigbrett · 2024-06-28T01:17:12Z

src/wh_server_crypto.c

+#ifdef WOLFHSM_SEPARATE_BIG_CACHE
+            /* evict the aes sized key in the normal cache */
+            if (moveToBigCache == 1) {
+                ret = hsmEvictKey(server, keyId);
+            }
+#endif


Not super clear why we are doing this? Could you explain plz

if a user pre-caches an aes key to use with cmac it will be in the regular cache initially, when we cache the cmac struct it will need to go into big cache so the now defunct aes key will be hanging if we don't evict it

billphipps · 2024-07-02T05:19:22Z

Love this idea. Any chance we can make this option always enabled and simply use a 0 num of big keys if they are not needed? This would give the same result without a bunch of conditional defines.

Also, I'd like to have the server context use pointers to the cache areas with a config member that indicates the number of slots. this will keep the server context size more consistent and cause the user to allocate the space themselves, as it may make sense to have this cache area somewhere other than in the middle of the context struct. just a thought.

bigbrett · 2024-07-02T14:47:03Z

Any chance we can make this option always enabled and simply use a 0 num of big keys if they are not needed? This would give the same result without a bunch of conditional defines.

Big +1 for this plan. The less code that is macro-ed out, the better! @jpbland1 Could you take a stab?

Also, I'd like to have the server context use pointers to the cache areas with a config member that indicates the number of slots. this will keep the server context size more consistent and cause the user to allocate the space themselves, as it may make sense to have this cache area somewhere other than in the middle of the context struct. just a thought.

Probably food for a follow-up PR IMO

bigbrett · 2024-07-15T16:01:53Z

@jpbland1 noticed this went stale, could you possibly address Bill's point so we can get it merged:

Any chance we can make this option always enabled and simply use a 0 num of big keys if they are not needed? This would give the same result without a bunch of conditional defines.

billphipps · 2024-07-15T16:06:22Z

And PR#49 changes the names of config values and provides an info message response as well. Need to add 2 more entries in the info message and API's to support large keys. WOLFHSM_CFG_KEYCACHE_LARGE_COUNT/BUFSIZE?

bigbrett

need to address merge conflicts, as well as Bill's points below:

Any chance we can make this option always enabled and simply use a 0 num of big keys if they are not needed? This would give the same result without a bunch of conditional defines.

and

And PR#49 changes the names of config values and provides an info message response as well. Need to add 2 more entries in the info message and API's to support large keys. WOLFHSM_CFG_KEYCACHE_LARGE_COUNT/BUFSIZE?

bigbrett

Might have noticed an issue with the coupling between CMAC and enablement of the big cache, lmk ur thoughts

bigbrett · 2024-08-02T17:45:20Z

wolfhsm/wh_settings.h

@@ -94,9 +94,19 @@
 #define WOLFHSM_CFG_SERVER_KEYCACHE_COUNT  8
 #endif

+/* Number of big RAM keys */
+#ifndef WOLFHSM_CFG_SERVER_KEYCACHE_BIG_COUNT
+#define WOLFHSM_CFG_SERVER_KEYCACHE_BIG_COUNT  1


Should we default to 0 for size here? So it is just an opt-in feature for the user? @billphipps what do you think?

src/wh_server_keystore.c

src/wh_server_crypto.c

seperate cache for larger keys so customers with limited memory can still store a limited number of large keys alongside their smaller keys.

for key lengths

billphipps

Got some reservations about this scheme in general, but I'm going to merge this because it does help with handling large keys. Thanks for putting this together!

billphipps · 2024-08-07T13:40:35Z

src/wh_server_keystore.c

+        (meta->len > WOLFHSM_CFG_SERVER_KEYCACHE_BUFSIZE
+        && meta->len > WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE


If BIG_BUFSIZE > BUF_SIZE, then this test could be simplified. Also, the different sizes should only be tested if the number of entries for that size >0.

Recommend eliminate the size test here (which incorrectly returns BADARGS for a len that is too large) and simply test if there is a slot somewhere that has sufficient size. If not, then return NOSPACE. The client won't necessarily know the buf sizes.

refactor the HSM server and keystore to handle a

jpbland1 requested review from bigbrett and billphipps June 18, 2024 12:10

jpbland1 assigned jpbland1, billphipps and bigbrett Jun 18, 2024

bigbrett requested changes Jun 28, 2024

View reviewed changes

jpbland1 requested a review from bigbrett June 28, 2024 03:06

bigbrett requested changes Jul 23, 2024

View reviewed changes

jpbland1 force-pushed the separate-big-key branch from 0b49eb1 to 5fc66a4 Compare August 1, 2024 10:50

jpbland1 requested a review from bigbrett August 1, 2024 10:52

bigbrett requested changes Aug 2, 2024

View reviewed changes

jpbland1 added 3 commits August 5, 2024 19:06

refactor the HSM server and keystore to handle a

d4d6976

seperate cache for larger keys so customers with limited memory can still store a limited number of large keys alongside their smaller keys.

fix key commited bug and use descriptive variables

bd38544

for key lengths

remove conditional compile for big key cache

88da7ef

jpbland1 force-pushed the separate-big-key branch from 5fc66a4 to 88da7ef Compare August 5, 2024 23:06

jpbland1 requested a review from bigbrett August 5, 2024 23:09

bigbrett approved these changes Aug 9, 2024

View reviewed changes

billphipps approved these changes Aug 9, 2024

View reviewed changes

billphipps merged commit 79f49b5 into wolfSSL:main Aug 9, 2024
1 check passed

jefferyq2 pushed a commit to jefferyq2/wolfHSM that referenced this pull request Nov 10, 2024

Merge pull request wolfSSL#48 from jpbland1/separate-big-key

48e49fa

refactor the HSM server and keystore to handle a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor the HSM server and keystore to handle a #48

refactor the HSM server and keystore to handle a #48

jpbland1 commented Jun 18, 2024

jpbland1 commented Jun 18, 2024

bigbrett left a comment

bigbrett Jun 28, 2024

jpbland1 Jun 28, 2024

bigbrett Jun 28, 2024

bigbrett Jun 28, 2024

bigbrett Jun 28, 2024

jpbland1 Jun 28, 2024

bigbrett Jun 28, 2024

jpbland1 Jun 28, 2024

bigbrett Jun 28, 2024

jpbland1 Jun 28, 2024

billphipps commented Jul 2, 2024

bigbrett commented Jul 2, 2024

bigbrett commented Jul 15, 2024

billphipps commented Jul 15, 2024

bigbrett left a comment •

edited

Loading

bigbrett left a comment

bigbrett Aug 2, 2024

billphipps left a comment

billphipps Aug 7, 2024

	slotCommited = &server->cache[i].commited;
	slotCommited = &server->bigCache[i].commited;

		(meta->len > WOLFHSM_CFG_SERVER_KEYCACHE_BUFSIZE
		&& meta->len > WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE

refactor the HSM server and keystore to handle a #48

refactor the HSM server and keystore to handle a #48

Conversation

jpbland1 commented Jun 18, 2024

jpbland1 commented Jun 18, 2024

bigbrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billphipps commented Jul 2, 2024

bigbrett commented Jul 2, 2024

bigbrett commented Jul 15, 2024

billphipps commented Jul 15, 2024

bigbrett left a comment • edited Loading

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billphipps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbrett left a comment •

edited

Loading