Remove whPacket union

[bigbrett]

Removes use of whPacket union for messaging in the crypto, keystore, counter, and SHE protocol layers, and replaces with standardized messaging structures with endianness translation functions.

Future work:
In creating this PR, I'm realizing we need to build in a generic "server return code" field into the comm layer that is propagated to the client, irrespective of the nature of the message structs. Similar to how "group" and "action" are built into the comm layer, we should do the same for a server return code. This would allow the server to generically inform the client of an error in the response without needing to know exactly which response structure it needs to serialize into. Right now there has to be a return code field baked into EVERY message struct, and it is not possible to inform the client of a server failure without picking the appropriate response structure and serializing it, which could be one of MANY structs depending on the specific protocol ("group/action"). This would also mean the client can always length-check responses for safety once it knows there is no server error and the received data is of the expected type (vs just "error").

For inspiration as to why this is necessary, see the two gross hacks I had to implement: 1) the "subprotocol" headers added to the crypto layer messaging, and 2) this absolutely CURSED function in the SHE layer). For the keystore and counter layers we were able to get away with a simple return code in every message struct, as there weren't any needs for "early" returns.

I'm sure there might be more clever or at least more unified ways to have implemented this given the way we currently do it, but it would be better to bake this into the comm layer itself.

[Copilot]

Copilot reviewed 28 out of 29 changed files in this pull request and generated no comments.

Files not reviewed (1)

• test/Makefile: Language not supported

Comments suppressed due to low confidence (3)

src/wh_server_keystore.c:538

• The return value of the translation function is ignored, which might hide potential errors during message translation. Consider checking and handling this return value.

(void)wh_MessageKeystore_TranslateCacheRequest(magic, (whMessageKeystore_CacheRequest*)req_packet, &req);

src/wh_server_keystore.c:516

• [nitpick] The 'magic' parameter is now part of the function signature without inline documentation describing its purpose. Consider adding a comment to clarify its role in endianness translation.

int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, uint16_t action, uint16_t req_size, const void* req_packet, uint16_t* out_resp_size, void* resp_packet)

src/wh_server_keystore.c:561

• [nitpick] The TODO comment regarding fatal server errors remains unresolved. It would be useful to clarify or document the expected error handling behavior for production use.

/* TODO: Are there any fatal server errors? */

[billphipps]

Need for a generic response code: look at whCommHeader.aux. On the request side, it was intentioned to inform the server which session this request was part of from that user. On the response side, it was intentioned to allow the comm layer to report back failure or to support a limited number of generic returns without an explicit response message. Look at WH_COMM_AUX_ENUM in wh_comm.h. Note this is only an uint16_t so it can't pass a full int return code.

[bigbrett]

@billphipps OK I can look at using comm header.aux, however that would be another relatively large refactor on both server and client side to ensure those values are honored and proper length checking added - definitely food for a subsequent PR, not part of this one.

[billphipps]

Wow! This is a much better solution in general. I approved for now, but added some comments that should be considered. Nothing major at all. Let me know if you want to merge then fix before the next PR or if you want to fix before merging.

[miyazakh]

I see compile failures when rebasing PR's changes and compiling by CC-RH.

1. _Static_assert() cannot be compiled. CC-RH follows c99
2. CC-RH doesn't allow the following line

byte* p = (uint8_t*)(in + sizeof(foo));

instead of this. The line needs to be like:

byte* p = (uint8_t*)(in) + sizeof(foo);

Please check the patch that it works RH850 support.
latest_ccrh.patch

[bigbrett]

@miyazakh addressed your suggestions in my last few commits. Please let me know if all is good with the latest on CC-RH

[billphipps]

Woo hoo! Looks GREAT to me!