use key type and key user as part of the internal keyId

[jpbland1]

splitting the key handle up into 3 parts internally, while presenting a one byte key id to the user, allows the same external or user facing key to be identical for each user value and between SHE and crypto keys. algorithm types were added as flags instead of being a part of the key handle and the user param was added to the client so a user can split operations between 16 clients. may not fully implement multiple client handling but seems good for keys

[bigbrett]

A few things I'd like to see (here or in next PR):

• Helper functions/macros for keyId bit manipulation/masking, as they are used lots of places in the code and easy to screw up
• Expanded testing (check all return values and invariants for one - I already pointed out the bitmasking bug with the keyIds earlier that wasn't caught by tests). Perhaps using other keyIds and different keys, ensuring they changes are propagated through

[billphipps]

This is definitely getting close to what I was hoping. Instead of hitting each file with repeated comments, I'm going to identify a few design misses that should be pretty quick to correct and some items that need to be made more clear before we move too much farther down the road.

Stuff that is close but must be corrected:

1. The client_id (called user in your code) is set in whClientConfig as a value between 0-0xF. client_id==0 means anonymous/public/not set. This will replace the client_id in whCommClient and whNvm, which will now need these to be passed down from the upper level.
2. The client_id is intended to be used as part of the whNvmId and server-side key Id to identify the user that owns the resource. The client was expected to send the WH_MESSAGE_COMM_ACTION_INIT action after connecting so that the server context would store this as the client_id to be passed/masked with every request. I see that I did not implement the client/server functions for this. My bad.
3. The header of each message MUST remain at 8 bytes, so the "user" field will need to be removed and the server will simply keep the id for each of the components as a member like server->client_id, again set by the client after connecting.
4. The client's perspective of the key identifier should simply be an identifier from 0x1-0xFE, where we reserve 0x00 and 0xFF as Invalid. These id's are NOT unique per key type, so you can't have AES key 0x1 and RSA private key 0x1. These are fully user-defined and are unique (to the server) per client_id.
5. The client's perspective of an NVM object is similar, in that each NVM object has a unique ID per client_id. This means that a client's key id of 0x1 is distinct from a client's NvmId of 0x1. The client will be able to see the NVM objects that store the key values when invoking the List command.
6. The NVM metadata design expected the client_id to be masked/passed/used as part of the whNvmAccess field (think of user/group/global Unix permissions), the object-type specific flags to be used in the whNvmFlags field, and the whNvmId to uniquely identify the owner, object number, and object type (key, counter, cert, SHE NVM data, ordinary data).

Here's some examples:

1. Client 0x4 wants to add an owned NVM object data[67] as id 0x55, with label "My Object".
   whClient_Init();
   whClient_SetClientId(0x4); // Doesn't exist yet. Use WH_MESSAGE_COMM_ACTION_INIT
   char* label ="My Object";
   int remote_rc = 0;
   whClient_NvmAddObject(c, 0x55, WH_ACCESS_ANY, WH_FLAGS_ANY,
   strlen(label), label, 67, data, &remote_rc);
   The server gets this request and constructs a whNvmId = 0x4055, which is the client_id==0x4, object_type==0x0 (ordinary data), and num==0x55. Note the server will deny any id's with the top nibble set as WH_ERROR_BADARGS. The second nibble is allowed to be set here as long as it is a valid object type. We should add explicit whClient_AddDataObject() that the user should be using to avoid any object-type snafus.

2. Client 0x5 wants to add an owned AES key at key[32] as key id 0x33 to NVM, with label "My Key".
   whClient_Init();
   whClient_SetClientId(0x5);
   char* label = "My Key";
   int remote_rc = 0;
   /* Put key into RAM. This function doesn't exist yet /
   whClient_ImportKey(c, 0x33, WH_ACCESS_ANY, WH_FLAGS_KEY_AES,
   strlen(label), label, 32, key, &remote_rc);
   Note: the type of the key is encoded in the flags metadata, rather than the ID. In the key cache, the server will construct a whNvmId as 0x5133 with flags of WH_FLAGS_KEY_AES, as if it were already in the NVM.
   / Store the key into NVM */
   whClient_CommitKey(c, 0x33, &remote_rc);
   The server gets this request and constructs a whNvmId==0x5133, which is the client_id==0x5, object_type==0x1 (NVM key), and num== 0x33. The flags holds the NVM key kind which is set by WH_FLAGS_KEY_AES.

3. Client 0x5 wants to Read an owned AES key with ID 0x33 from NVM back into RAM and use it with an AES context
   whClient_Init();
   whClient_SetClientId(0x5);
   int remote_rc = 0;
   whClient_ReadKey(c, 0x33, WH_ACCESS_ANY, WH_FLAGS_KEY_AES, &remote_rc);
   The server gets this request and constructs a whNvmId==0x5133. It reads the metadata and checks that the flags==WH_FLAGS_KEY_AES. If not, it returns WH_ABORTED. Note the client could have used WH_FLAGS_ANY to match any key type with the matching ID.
   Aes aes = {0};
   wc_AesInit_ex(aes, NULL, WH_CLIENT_DEVID);
   whClient_SetAesKey(c, aes, 0x33);
   The client modifies the aes->devCtx to encode it 0x33. The server will construct the whNvmId and flags during the server side crypto message handling to match the unique id within the key cache of 0x5133. The client is not aware of this encoding.

4. Client 0x3 wants to directly update the unowned SHE NVM key[16] as WH_SHE_KEY_4, rather than using the SHE update key protocol. Note that all SHE keys are 16 bytes.
   whClient_Init();
   whClient_SetClientId(0x0); //Note that this is an unowned key so multiple clients can use it
   int remote_rc = 0;
   /* Put key into RAM. Note WH_SHE_KEY_4 is hard-coded as WH_NVM_TYPE_SHE | WH_SHE_KEYID_4 = 0x0204./
   whClient_ImportKey(c, WH_SHE_KEY_4, WH_ACCESS_ANY, WH_FLAGS_SHE_NVM,
   0, NULL, 16, key, &remote_rc);
   In the key cache, the server will construct a whNvmId as 0x0204 with object type of 0x2 (rather than 0x1) and flags of WH_FLAGS_SHE_NVM.
   / Store the key into NVM */
   whClient_CommitKey(c, WH_SHE_KEY_4, &remote_rc);
   The server gets this request and constructs a whNvmId==0x0204, which is the client_id==0x0, object_type==0x2 (SHE key), and num== 0x04. The flags holds the NVM key kind which is set by WH_FLAGS_SHE_NVM.

Hopefully this will better show how the whNvmId and whNvmFlags were intended to be used. Also, it should point out that there should be a distinct whKeyId type passed in and out of the key functions that don't need the client_id encoded in the top nibble because it is the server that actually handles that.

FLAGS_KEY_* should include: AES, RSA_PRIVATE, RSA_PUBLIC, ECC_PRIVATE, ECC_PUBLIC, HMAC. I haven't worked through how we should tag KDF'ed keys yet, but we'll get there.

Can you update your code to put the key types into the flags field and switch the user to client_id and make it a member of the server context? I can craft a quickie that provides the whClient_SetClientId() functions if you'd like.

[billphipps]

Looks really good! A few nits, but I'd like to merge it as is and we'll fix a few things in the next round. Only bug I see is not using the client_id correctly, but I'll fix as I go.

[billphipps]

This will likely not set the entire devCtx value since whNvmId is probably a smaller size than void*. Recommend to use uintptr_t to let the compiler promote the unsigned integer up to the proper size.

Like: key->devCtx = (void*) ((uintptr_t)keyId);

[billphipps]

Probably should be a macro or function as I suspect we'll need this a lot.

[billphipps]

Either check the parameter for NULL at the beginning and return WH_ERROR_BADARGS or gate it here.

[billphipps]

Recommend to simply use wh_Nvm_GetMetadata() != WH_ERROR_NOTFOUND since you are only looking for an exact match.

[billphipps]

This shouldn't work this way. The server should be applying the client id that it stored after the client invoked wh_Client_CommInit(c); I understand this is for testing only, but I don't see how comm->client_id is being used at all in the code. Are we sure this is working the way you expected?

[billphipps]

I would remove this and simply use the wh_Client_CommInit function to update the server-side state.