Wrapping EverCBOR #7533

maxtropets · 2025-12-15T19:45:23Z

This's purely reading CBORs, and lacking more exhausting tests, but is a good starting point, maybe?

src/crypto/sobor.h

Copilot

Pull request overview

This PR replaces the direct use of EverCBOR with a new CBOR wrapper abstraction (ccf::cbor). The primary purpose is to simplify CBOR parsing operations throughout the codebase by providing a higher-level, type-safe API.

Key Changes:

Introduces a new CBOR wrapper library (src/crypto/cbor.h and src/crypto/cbor.cpp) that encapsulates EverCBOR operations
Refactors UVM endorsement parsing to use the new wrapper, significantly reducing code complexity (from ~400 lines to ~100 lines in the decode functions)
Updates error handling to use the new CBORDecodeError type consistently

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/crypto/cbor.h	Defines the new CBOR wrapper API with types and parsing functions
src/crypto/cbor.cpp	Implements the CBOR wrapper, including parsing and value extraction methods
src/node/uvm_endorsements.cpp	Refactors protected header decoding to use the new CBOR wrapper instead of raw EverCBOR calls
cmake/crypto.cmake	Adds the new cbor.cpp to the build configuration

Comments suppressed due to low confidence (1)

src/node/uvm_endorsements.cpp:343

Missing closing brace for the namespace ccf block. The namespace opened at line 9 should be closed after line 168.

}
}

src/crypto/cbor.cpp

Co-authored-by: Copilot <[email protected]>

src/crypto/cbor.cpp

src/node/uvm_endorsements.cpp

src/crypto/cbor.h

src/crypto/cbor.cpp

src/node/uvm_endorsements.cpp

src/crypto/cbor.cpp

src/node/uvm_endorsements.cpp

Co-authored-by: Amaury Chamayou <[email protected]>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Copilot · 2025-12-24T21:34:50Z

src/node/uvm_endorsements.cpp

-          }
+          auto bytes = x5chain_value->as_bytes("x5chain");
+          chain.emplace_back(bytes.begin(), bytes.end());
        }


The validation for empty x5chain array has been removed. The old implementation threw an error if the parsed x5chain was empty. This validation should be restored to ensure at least one certificate is present in the chain.

Suggested change

}

}

if (chain.empty())

{

throw ccf::cbor::CBORDecodeError(

"x5chain must contain at least one certificate");

}

Copilot · 2025-12-24T21:34:51Z

src/node/uvm_endorsements.cpp

+        constexpr std::string_view iss_context{"phdr: iss"};
+        const auto& iss =
+          parsed_phdr->map_at(ccf::cbor::make_string("iss"), iss_context);
+        result.iss = iss->as_string(iss_context);


Inconsistent string conversion from string_view. Line 99 and 114 use explicit std::string conversion, while line 109 relies on implicit conversion. For consistency and clarity, all string assignments should use the same approach.

Suggested change

result.iss = iss->as_string(iss_context);

result.iss = std::string(iss->as_string(iss_context));

Copilot · 2025-12-24T21:34:51Z

src/node/uvm_endorsements.cpp

+        const auto ct_context = "phdr: " + std::to_string(259);
+        const auto& content_type =
+          parsed_phdr->map_at(ccf::cbor::make_unsigned(259), ct_context);


Magic number 259 should be replaced with a named constant. Based on the context and comparison with line 96-98 which uses headers::PARAM_CONTENT_TYPE, this appears to be a content type parameter. Consider defining this constant in the headers namespace or using the existing constant if 259 is the correct value for PARAM_CONTENT_TYPE.

Suggested change

const auto ct_context = "phdr: " + std::to_string(259);

const auto& content_type =

parsed_phdr->map_at(ccf::cbor::make_unsigned(259), ct_context);

const auto ct_context =

"phdr: " + std::to_string(headers::PARAM_CONTENT_TYPE);

const auto& content_type = parsed_phdr->map_at(

ccf::cbor::make_unsigned(headers::PARAM_CONTENT_TYPE), ct_context);

Copilot · 2025-12-24T21:34:51Z

src/crypto/cbor.cpp

+    // Return all as one bytes, leave detailed parsing to the user. EverCBOR
+    // does not support more granular parsing, as well as floating point numbers
+    // with extra payload yet.


The comment says "Return all as one bytes" but the function returns a Simple value (uint8_t), not bytes. The comment should be corrected to accurately describe the function's behavior.

Suggested change

// Return all as one bytes, leave detailed parsing to the user. EverCBOR

// does not support more granular parsing, as well as floating point numbers

// with extra payload yet.

// Return the raw simple value (single byte) and leave detailed interpretation

// to the caller. EverCBOR does not yet support more granular parsing, or

// floating point numbers with extra payload.

Copilot · 2025-12-24T21:34:51Z

src/crypto/cbor.cpp

+    std::visit(
+      [&os, indent](const auto& v) {
+        using T = std::decay_t<decltype(v)>;
+        if constexpr (std::is_same_v<T, Unsigned>)
+        {
+          print_indent(os, indent);
+          os << "Unsigned: " << v << std::endl;
+        }
+        else if constexpr (std::is_same_v<T, Signed>)
+        {
+          print_indent(os, indent);
+          os << "Signed: " << v << std::endl;
+        }
+        else if constexpr (std::is_same_v<T, Bytes>)
+        {
+          print_indent(os, indent);
+          os << "Bytes[" << v.size() << "]: ";
+          for (size_t i = 0; i < std::min(v.size(), size_t(16)); ++i)
+          {
+            os << std::hex << std::setw(2) << std::setfill('0')
+               << static_cast<int>(v[i]);
+          }
+          if (v.size() > 16)
+          {
+            os << "...";
+          }
+          os << std::dec << std::endl;
+        }
+        else if constexpr (std::is_same_v<T, String>)
+        {
+          print_indent(os, indent);
+          os << "String: \"" << v << "\"" << std::endl;
+        }
+        else if constexpr (std::is_same_v<T, Array>)
+        {
+          print_indent(os, indent);
+          os << "Array[" << v.items.size() << "]:" << std::endl;
+          for (const auto& item : v.items)
+          {
+            print_value_impl(os, item, indent + 1);
+          }
+        }
+        else if constexpr (std::is_same_v<T, Map>)
+        {
+          print_indent(os, indent);
+          os << "Map[" << v.items.size() << "]:" << std::endl;
+          for (const auto& [key, val] : v.items)
+          {
+            print_indent(os, indent + 1);
+            os << "Key:" << std::endl;
+            print_value_impl(os, key, indent + 2);
+            print_indent(os, indent + 1);
+            os << "Value:" << std::endl;
+            print_value_impl(os, val, indent + 2);
+          }
+        }
+        else if constexpr (std::is_same_v<T, Tagged>)
+        {
+          print_indent(os, indent);
+          os << "Tagged[" << v.tag << "]:" << std::endl;
+          print_value_impl(os, v.item, indent + 1);
+        }
+      },
+      value->value);
+  }


The print_value_impl function is missing a handler for the Simple type in the std::visit. This means Simple values won't be printed, which could cause silent failures or undefined behavior during debugging. Add a handler for Simple values similar to other types.

maxtropets self-assigned this Dec 15, 2025

maxtropets changed the title ~~[DRAFT] [WIP] [DO-NOT-REVIEW-YET] Wrapping EverCBOR~~ [DRAFT:DO-NOT-REVIEW-YET] Wrapping EverCBOR Dec 15, 2025

maxtropets force-pushed the f/cbor-wrappers branch from e80864b to 24b02d3 Compare December 15, 2025 23:34

achamayou reviewed Dec 16, 2025

View reviewed changes

src/crypto/sobor.h Outdated Show resolved Hide resolved

maxtropets force-pushed the f/cbor-wrappers branch 2 times, most recently from 92dc7d0 to f8c95f0 Compare December 16, 2025 16:12

maxtropets changed the title ~~[DRAFT:DO-NOT-REVIEW-YET] Wrapping EverCBOR~~ Wrapping EverCBOR Dec 16, 2025

maxtropets force-pushed the f/cbor-wrappers branch from f8c95f0 to fe60567 Compare December 16, 2025 16:18

maxtropets marked this pull request as ready for review December 16, 2025 16:18

maxtropets requested a review from a team as a code owner December 16, 2025 16:18

Copilot AI review requested due to automatic review settings December 16, 2025 16:18

Copilot AI reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.cpp Outdated Show resolved Hide resolved

Wrap EverCBOR C-style parsers

de8463e

maxtropets force-pushed the f/cbor-wrappers branch from fe60567 to de8463e Compare December 16, 2025 16:20

Update src/crypto/cbor.cpp

7ddf372

Co-authored-by: Copilot <[email protected]>

achamayou reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.cpp Outdated Show resolved Hide resolved

eddyashton reviewed Dec 16, 2025

View reviewed changes

src/node/uvm_endorsements.cpp Outdated Show resolved Hide resolved

eddyashton reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.h Outdated Show resolved Hide resolved

src/crypto/cbor.h Outdated Show resolved Hide resolved

achamayou reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.cpp Show resolved Hide resolved

achamayou reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.cpp Outdated Show resolved Hide resolved

cjen1-msft reviewed Dec 16, 2025

View reviewed changes

src/node/uvm_endorsements.cpp Show resolved Hide resolved

achamayou reviewed Dec 16, 2025

View reviewed changes

src/crypto/cbor.cpp Show resolved Hide resolved

achamayou reviewed Dec 16, 2025

View reviewed changes

src/node/uvm_endorsements.cpp Show resolved Hide resolved

achamayou approved these changes Dec 16, 2025

View reviewed changes

maxtropets and others added 3 commits December 23, 2025 14:51

Update src/crypto/cbor.cpp

5650abb

Co-authored-by: Amaury Chamayou <[email protected]>

Code review, polishing, clang-tidy

68bb71d

More code review

a25ead9

maxtropets added the run-long-test Run Long Test job label Dec 24, 2025

Merge branch 'main' into f/cbor-wrappers

78c9390

maxtropets mentioned this pull request Dec 24, 2025

Complete EverCBOR wrappers interface #7542

Open

maxtropets added 2 commits December 24, 2025 16:02

Cmake fix linkage

a1049f6

Tidy

bc31579

maxtropets requested a review from Copilot December 24, 2025 21:24

Copilot started reviewing on behalf of maxtropets December 24, 2025 21:25 View session

Copilot AI reviewed Dec 24, 2025

View reviewed changes

-        }
+        }
+        if (chain.empty())
+        {
+          throw ccf::cbor::CBORDecodeError(
+            "x5chain must contain at least one certificate");
+        }

	result.iss = iss->as_string(iss_context);
	result.iss = std::string(iss->as_string(iss_context));

Wrapping EverCBOR #7533

Are you sure you want to change the base?

Wrapping EverCBOR #7533

Conversation

maxtropets commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxtropets commented Dec 15, 2025 •

edited

Loading