add key management commands for caching, exporting #16

jpbland1 · 2024-04-02T23:51:11Z

and evicting keys for ram cache

bigbrett

Great work! No big issues that I see but a largenumber of little things that should be fixed before merging.

wolfhsm/wh_server_keystore.h

src/wh_server_keystore.c

src/wh_client.c

src/wh_server_keystore.c

bigbrett · 2024-04-04T19:18:57Z

@jpbland1 this needs a rebase and some additional changes now that crypto and NVM contexts are no longer stored in the server context (see #11)

and evicting keys for ram cache

don't need to read the entire label

billphipps

A number of recommendations to add to the backlog, but let's get this going. Only 1 real bug related to the uniqueId loop end math.

src/wh_client.c

billphipps · 2024-04-08T19:49:39Z

src/wh_client.c

+    whPacket* packet = (whPacket*)rawPacket;
+    uint8_t* packOut = (uint8_t*)(&packet->keyExportRes + 1);
+    if (c == NULL || labelSz > WOLFHSM_NVM_LABEL_LEN || outSz == NULL)


Not sure why any labelSz could cause an error. If the user provides a smaller label storage (even 0), then I think that should be valid. Simply labelSz = min(sizeof(metadata.label), labelSz);

outSz == NULL is valid as long as out == NULL. Probably just a test case, but still consistent.
Should outSz be set to the size of out before the function is called?

labelSz == 1000000000000 would cause out of bounds read, we can't accept any labelSz and outSz can't be NULL because when out is NULL it means the user is expecting outSz to be populated so they know how big their buffer needs to be for a subsequent call

I've thought more about this, we definitely need to validate labelSz because an incorrect value of 25 doesn't guarantee the buffer can actually handle sizeof(metadata.label) which i think is 24. if we have an unknown length we should bail out, going to modify it to only err if label is non-NULL

Scratch that, actually makes total sense for the buffer to be larger than max if they're reusing some generic buffer, my bad

src/wh_client.c

billphipps · 2024-04-08T20:01:26Z

src/wh_client.c

+    uint16_t size;
+    int ret;
+    whPacket packet[1] = {0};


I'm a fan of unions, but some of these messages are really small. Consider using the smaller structs when possible to avoid a big allocations.

Yeah it really saved a lot of space in the old design to only have 1 packet for everything huh?

billphipps · 2024-04-08T20:06:50Z

src/wh_client.c

+    uint16_t keyId)
+{
+    uint8_t rawPacket[WH_COMM_MTU] = {0};


The CommClient context already has an MTU-sized packet precisely for construction. The SendRequest helper was put together to simplify calls, but you are welcome to directly use this buffer (at the data offset) and update the SendRequest to not perform the memcpy when the src of the data is the same as the destination. We'll add this to the todo list so we can provide an accessor and avoid extra copies and allocations.

Oh, and this size should be WH_COMM_DATA_LEN, not MTU.

So change it back to using a single packet instead of taking data as a parameter? The way I had it in the old design?

src/wh_server_keystore.c

billphipps · 2024-04-08T20:52:07Z

src/wh_server_keystore.c

+    }
+    /* if the key wasn't found return an error */
+    if (i >= WOLFHSM_NUM_RAMKEYS)
+        ret = WH_ERROR_BADARGS;


WH_ERROR_NOTFOUND

billphipps · 2024-04-08T20:53:06Z

src/wh_server_keystore.c

+    /* find key in cache */
+    for (i = 0; i < WOLFHSM_NUM_RAMKEYS; i++) {
+        if (server->cache[i].meta->id == keyId) {
+            cacheSlot = &server->cache[i];
+            break;
+        }
+    }
+    if (i >= WOLFHSM_NUM_RAMKEYS)
+        return WH_ERROR_NOTFOUND;


Refactor into _FindCacheIndexByKeyId() or something suitably named.

billphipps · 2024-04-08T20:55:23Z

src/wh_server_keystore.c

+int hsmEraseKey(whServerContext* server, whNvmId keyId)
+{
+    int i;
+    if (keyId == WOLFHSM_ID_ERASED)


Always check for non-null server. Multiple instances.

billphipps · 2024-04-08T20:59:20Z

src/wh_server_keystore.c

+    whPacket* packet = (whPacket*)data;
+    whNvmMetadata meta[1] = {0};
+    switch (action)
+    {


The endianness of the server may not be the same as the client. Even if you don't handle it now, at least perform a quick check using the magic number to return an error on mismatch.

if (WH_COMM_FLAGS_SWAPTEST(magic)) return ABORTED or something.

adding this made it error out every time, don't know why but I've commented it out

Yeah, that's because I made that macro to compare the magics. Should have been !SWAPTEST. My bad.

billphipps

Updated backlog. Still have some API changes I think are necessary but lets get this in and work down the backlog.

src/wh_client.c

src/wh_server_keystore.c

billphipps · 2024-04-09T13:50:43Z

src/wh_server_keystore.c

+    return 0;
+}
+
+int hsmReadKey(whServerContext* server, whNvmMetadata* meta, uint8_t* out,


This API conflates the NvmId and KeyId, which should be distinct. Recommend to change this API to:
int hsmReadKey(whServerContext* server, whKeyId key_id, whNvmMetadata *out_meta, whNvmSize inout_DataSize, uint8_t data);

out_meta == NULL means don't copy the metadata. inout_DataSize should be set to the length of data and will be set to the actual length if non-null. data == NULL means don't copy the data.

I updated the key_id backlog item to capture the need for distinct key_id functions.

I'm confused, are KeyIds not a subset of NvmId? It's just an NvmId but with the context of being a key and with either WOLFHSM_KEYID_CRYPTO, WOLFHSM_KEYID_SHE or WOLFHSM_KEYID_SHE_RAM bits set? Or is there something I'm missing

billphipps · 2024-04-09T13:56:01Z

src/wh_server_keystore.c

+    whPacket* packet = (whPacket*)data;
+    whNvmMetadata meta[1] = {0};
+    switch (action)
+    {


Yeah, that's because I made that macro to compare the magics. Should have been !SWAPTEST. My bad.

bigbrett

@jpbland1 looks like ASAN is failing due to overlapping memcpy in the crypto tests. Good to merge as soon as that is fixed

jpbland1 · 2024-04-09T23:16:32Z

working now, please merge

bigbrett · 2024-04-10T14:04:43Z

src/wh_client.c

+                if (labelSz > sizeof(packet->keyExportRes.label)) {
+                    XMEMCPY(label, packet->keyExportRes.label,
+                        WOLFHSM_NVM_LABEL_LEN);


we should probably have the memcpy size argument correspond to the size check we just did

yeah that's a little messy, still correct but it would be better to use sizeof the actual buffer

jpbland1 self-assigned this Apr 2, 2024

jpbland1 requested review from billphipps and bigbrett April 2, 2024 23:51

jpbland1 assigned billphipps and bigbrett Apr 2, 2024

bigbrett requested changes Apr 3, 2024

View reviewed changes

jpbland1 added 4 commits April 5, 2024 16:26

add key management commands for caching, exporting

0497302

and evicting keys for ram cache

add nvm key functions commit and erase

e016abe

add set key function

90ee2b2

update based on PR comments

5482af4

jpbland1 force-pushed the key-cache-export-evict branch from d00d0c6 to 5482af4 Compare April 5, 2024 20:26

jpbland1 requested a review from bigbrett April 5, 2024 20:27

jpbland1 added 4 commits April 5, 2024 16:28

remove old header

afa643d

stop using wolfSSL errors

e183ad3

add read size to wh_Client_KeyExportResponse so clients

2cfc28b

don't need to read the entire label

add labelSz check

d4525d0

bigbrett previously approved these changes Apr 8, 2024

View reviewed changes

billphipps previously approved these changes Apr 8, 2024

View reviewed changes

update based on PR comments

c3261d3

jpbland1 dismissed stale reviews from billphipps and bigbrett via c3261d3 April 8, 2024 23:41

jpbland1 requested review from bigbrett and billphipps April 8, 2024 23:41

fix label size check

2749ab9

billphipps previously approved these changes Apr 9, 2024

View reviewed changes

bigbrett previously approved these changes Apr 9, 2024

View reviewed changes

jpbland1 added 2 commits April 9, 2024 13:51

update hsmReadKey to support NULL metadata

6d423d1

update hsmReadKey to make meta and data optional

6e60871

jpbland1 dismissed bigbrett’s stale review via 6e60871 April 9, 2024 18:15

jpbland1 dismissed billphipps’s stale review via 6e60871 April 9, 2024 18:15

jpbland1 force-pushed the key-cache-export-evict branch from 072ac77 to 6e60871 Compare April 9, 2024 18:15

jpbland1 requested review from bigbrett and billphipps April 9, 2024 18:23

fix meta read out and commit test

f2a6785

bigbrett approved these changes Apr 10, 2024

View reviewed changes

bigbrett merged commit 2d2db1a into wolfSSL:main Apr 10, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add key management commands for caching, exporting #16

add key management commands for caching, exporting #16

jpbland1 commented Apr 2, 2024

bigbrett left a comment

bigbrett commented Apr 4, 2024

billphipps left a comment

billphipps Apr 8, 2024

jpbland1 Apr 8, 2024 •

edited

Loading

jpbland1 Apr 9, 2024 •

edited

Loading

jpbland1 Apr 9, 2024

billphipps Apr 8, 2024

jpbland1 Apr 8, 2024

billphipps Apr 8, 2024

jpbland1 Apr 8, 2024

billphipps Apr 8, 2024

billphipps Apr 8, 2024

billphipps Apr 8, 2024

jpbland1 Apr 8, 2024

billphipps Apr 8, 2024

jpbland1 Apr 8, 2024

jpbland1 Apr 8, 2024

billphipps Apr 9, 2024

billphipps left a comment

billphipps Apr 9, 2024

jpbland1 Apr 9, 2024

billphipps Apr 9, 2024

bigbrett left a comment

jpbland1 commented Apr 9, 2024

bigbrett Apr 10, 2024

jpbland1 Apr 10, 2024

add key management commands for caching, exporting #16

add key management commands for caching, exporting #16

Conversation

jpbland1 commented Apr 2, 2024

bigbrett left a comment

Choose a reason for hiding this comment

bigbrett commented Apr 4, 2024

billphipps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

jpbland1 Apr 9, 2024 • edited Loading

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

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

jpbland1 commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 Apr 8, 2024 •

edited

Loading

jpbland1 Apr 9, 2024 •

edited

Loading