Service messages #73
Service messages #73
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR implements CBOR marshalling and unmarshalling for DICE service messages, establishing the message format and serialization layer for NAT20 service communication. The implementation adds support for various DICE operations including CDI promotion, certificate issuance, and cryptographic signing through a well-defined API.
- Defines comprehensive message structures for all supported DICE operations
- Implements CBOR encoding/decoding with proper error handling and validation
- Provides extensive test coverage for round-trip serialization scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
include/nat20/service/messages.h |
Defines message structures and API for DICE service operations |
src/service/messages.c |
Implements CBOR marshalling/unmarshalling with validation logic |
src/service/messages.cddl |
CDDL specification documenting the CBOR message format |
src/service/test/messages.cpp |
Comprehensive test suite covering serialization round-trips and edge cases |
src/service/test/test.cpp |
Basic test runner for the service module |
include/nat20/error.h |
Adds new error codes for message parsing and validation |
CMakeLists.txt |
Integrates service library and tests into build system |
.github/license-check/license-config.json |
Adds license checking for CDDL files |
.github/license-check/header-apache2-semicolon.txt |
License header template for CDDL files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/service/test/test.cpp
Outdated
|
|
||
| int main(int argc, char *argv[]) { | ||
| ::testing::InitGoogleTest(&argc, argv); | ||
| std::cout << "Testing libnat20 service..." << std::endl; |
There was a problem hiding this comment.
The string 'libnat20' should be 'Libnat20' to match the project naming convention seen elsewhere in the codebase.
| std::cout << "Testing libnat20 service..." << std::endl; | |
| std::cout << "Testing Libnat20 service..." << std::endl; |
Copilot uses AI. Check for mistakes.
src/service/messages.c
Outdated
| n20_msg_read_map_with_int_key(istream, n20_msg_open_dice_input_read_cb, input); | ||
|
|
There was a problem hiding this comment.
The return value of n20_msg_read_map_with_int_key is ignored. This function can return errors that should be propagated up to the caller.
| n20_msg_read_map_with_int_key(istream, n20_msg_open_dice_input_read_cb, input); | |
| n20_error_t error = n20_msg_read_map_with_int_key(istream, n20_msg_open_dice_input_read_cb, input); | |
| if (error != n20_error_ok_e) { | |
| return error; | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
agree with copilot here. why not just return n20_msg_read_map_with_int_key like n20_msg_promote_request_read does above?
src/service/messages.c
Outdated
| break; | ||
| default: | ||
| // Skip unknown keys. | ||
| n20_cbor_read_skip_item(istream); |
There was a problem hiding this comment.
The return value of n20_cbor_read_skip_item is ignored, but other similar cases check this return value and return an error if it fails. This should be consistent with the error handling pattern used elsewhere.
| n20_cbor_read_skip_item(istream); | |
| if (!n20_cbor_read_skip_item(istream)) { | |
| return n20_error_unexpected_message_structure_e; | |
| } |
Copilot uses AI. Check for mistakes.
| /** | ||
| * @brief Parent path size exceeds maximum allowed. | ||
| * | ||
| * When deriving the effective CDI on behalf of a proxy DCIE service |
There was a problem hiding this comment.
| * When deriving the effective CDI on behalf of a proxy DCIE service | |
| * When deriving the effective CDI on behalf of a proxy DICE service |
| * The implementation does not recognize the request type | ||
| * as a valid request type. | ||
| */ | ||
| n20_error_request_type_unknown_e = 18, |
There was a problem hiding this comment.
any reason for skipping error numbers?
src/service/messages.c
Outdated
| uint64_t cbor_value; | ||
|
|
||
| switch (key) { | ||
| case 1: // compressed_context |
There was a problem hiding this comment.
did we want to add #defines or enums for the cbor map keys?
src/service/messages.c
Outdated
| n20_msg_read_map_with_int_key(istream, n20_msg_open_dice_input_read_cb, input); | ||
|
|
There was a problem hiding this comment.
agree with copilot here. why not just return n20_msg_read_map_with_int_key like n20_msg_promote_request_read does above?
| EXPECT_EQ(0, memcmp("testkey", read_request.payload.issue_eca_ee_cert.name.buffer, 7)); | ||
| EXPECT_EQ(2, read_request.payload.issue_eca_ee_cert.key_usage.size); | ||
| EXPECT_EQ(0x01, read_request.payload.issue_eca_ee_cert.key_usage.buffer[0]); | ||
| EXPECT_EQ(0x02, read_request.payload.issue_eca_ee_cert.key_usage.buffer[1]); |
There was a problem hiding this comment.
missing check for challenge
src/service/test/messages.cpp
Outdated
| n20_msg_request_type_issue_cdi_cert_e, | ||
| n20_msg_request_type_issue_eca_cert_e, | ||
| n20_msg_request_type_issue_eca_ee_cert_e, | ||
| n20_msg_request_type_eca_ee_sign_e)); |
There was a problem hiding this comment.
nit: missing newline at end of file.
| @@ -0,0 +1,32 @@ | |||
| /* | |||
| * Copyright 2024 Aurora Operations, Inc. | |||
There was a problem hiding this comment.
| * Copyright 2024 Aurora Operations, Inc. | |
| * Copyright 2025 Aurora Operations, Inc. |
Implements marshalling and unmarshalling for DICE service messages using CBOR encoding.
Move certificate format enum to nat20/constants.h
werwurm
force-pushed
the
werwurm/cbor_parser
branch
from
96ca001 to
088e415
Compare
werwurm
force-pushed
the
werwurm/service_messages
branch
from
fb866a7 to
eeb91ba
Compare
Implements marshalling and unmarshalling for DICE service messages using
CBOR encoding.