Add CWT support for C++ #4804

ipetr0v · 2024-02-15T13:28:16Z

This PR:

Adds support for COSE and CWT
Adds libcppbor as a WORKSPACE dependency
Adds the logic for parsing CWT certificates
- Only extracts SUBJECT_PUBLIC_KEY for now and doesn't provide custom claims

This logic is required to implement C++ integration tests that use DICE attestation evidence.

ipetr0v · 2024-02-15T17:19:34Z

I think the only problem that remains in this PR is using the libcppbor get function with a template (where I have to explicitly add get<int, int>(...)):

template <typename Key,
          typename = std::enable_if_t<std::is_integral_v<Key> || std::is_enum_v<Key> ||
                                      details::is_text_type_v<Key>::value>>
const std::unique_ptr<Item>& Map::get(Key key) const {

This is what happens if I don't have explicit get<int, int>(...):

cc/attestation/cose.cc: In static member function 'static absl::lts_20230125::StatusOr<oak::attestation::CoseKey> oak::attestation::CoseKey::Deserialize(const std::vector<unsigned char>&)':
cc/attestation/cose.cc:79:58: error: no matching function for call to 'cppbor::Map::get(oak::attestation::CoseKey::Parameter) const'
   79 |   const std::unique_ptr<cppbor::Item>& kty = map->get(KTY);
      |                                                          ^
In file included from ./cc/attestation/cose.h:26,
                 from cc/attestation/cose.cc:17:
bazel-out/k8-fastbuild/bin/external/libcppbor/_virtual_includes/libcppbor/libcppbor/include/cppbor/cppbor.h:728:34: note: candidate: 'template<class Key, class Enable> const std::unique_ptr<cppbor::Item>& cppbor::Map::get(Key) const'
  728 |     const std::unique_ptr<Item>& get(Key key) const;
      |                                  ^~~
bazel-out/k8-fastbuild/bin/external/libcppbor/_virtual_includes/libcppbor/libcppbor/include/cppbor/cppbor.h:728:34: note:   template argument deduction/substitution failed:
cc/attestation/cose.cc:79:58: note:   couldn't deduce template parameter 'Enable'
   79 |   const std::unique_ptr<cppbor::Item>& kty = map->get(KTY);

k-naliuka · 2024-02-15T21:26:36Z

I think the only problem that remains in this PR is using the libcppbor get function with a template (where I have to explicitly add get<int, int>(...)):

template <typename Key,
          typename = std::enable_if_t<std::is_integral_v<Key> || std::is_enum_v<Key> ||
                                      details::is_text_type_v<Key>::value>>
const std::unique_ptr<Item>& Map::get(Key key) const {

This is what happens if I don't have explicit get<int, int>(...):

cc/attestation/cose.cc: In static member function 'static absl::lts_20230125::StatusOr<oak::attestation::CoseKey> oak::attestation::CoseKey::Deserialize(const std::vector<unsigned char>&)':
cc/attestation/cose.cc:79:58: error: no matching function for call to 'cppbor::Map::get(oak::attestation::CoseKey::Parameter) const'
   79 |   const std::unique_ptr<cppbor::Item>& kty = map->get(KTY);
      |                                                          ^
In file included from ./cc/attestation/cose.h:26,
                 from cc/attestation/cose.cc:17:
bazel-out/k8-fastbuild/bin/external/libcppbor/_virtual_includes/libcppbor/libcppbor/include/cppbor/cppbor.h:728:34: note: candidate: 'template<class Key, class Enable> const std::unique_ptr<cppbor::Item>& cppbor::Map::get(Key) const'
  728 |     const std::unique_ptr<Item>& get(Key key) const;
      |                                  ^~~
bazel-out/k8-fastbuild/bin/external/libcppbor/_virtual_includes/libcppbor/libcppbor/include/cppbor/cppbor.h:728:34: note:   template argument deduction/substitution failed:
cc/attestation/cose.cc:79:58: note:   couldn't deduce template parameter 'Enable'
   79 |   const std::unique_ptr<cppbor::Item>& kty = map->get(KTY);

I think you're hitting C++ 14/17 compatibilityissue (see b/314141962)

cc/attestation/BUILD

cc/attestation/cose.cc

cc/attestation/cwt.h

cc/attestation/cose.h

thmsbinder · 2024-02-16T07:41:20Z

cc/attestation/cose.h

+ }
+}
+
+absl::Status UnexpectedCborTypeError(std::string_view name, cppbor::MajorType expected,


Can this go to cose.cc ? It is not public.

This function is currently used in cwt.cc as well

thmsbinder · 2024-02-16T07:42:45Z

cc/attestation/cwt.cc

+ absl::StatusOr<CoseSign1> cose_sign1 = CoseSign1::Deserialize(data);
+ if (!cose_sign1.ok()) {
+ return cose_sign1.status();
+ }


Are the usual macros available externally?

ASSIGN_OR_RETURN(absl::StatusOr cose_sign1, CoseSign1::Deserialize(data));

Macros are not available externally

cc/attestation/cwt_test.cc

cc/utils/cose/cose.cc

andrisaar · 2024-02-16T15:28:35Z

cc/utils/cose/cose.cc

+
+namespace oak::utils::cose {
+
+absl::StatusOr<CoseSign1> CoseSign1::Deserialize(const std::vector<uint8_t>& data) {


Prefer absl::Span, unless the cppbor code insists this be a reference to a vector.

libcppbor provides the following parse functions:

ParseResult parse(const uint8_t* begin, const uint8_t* end); ParseResult parse(const std::vector<uint8_t>& encoding); ParseResult parse(const uint8_t* begin, size_t size); ParseResult parse(const Bstr* bstr);

link

We can potentially use (const uint8_t* begin, const uint8_t* end), but those are not iterators. Not sure whether it's ok to get raw pointers from an absl::Span.

It's fine -- Span is a contiguous region of memory -- but we'll most likely deal with vectors anyway, so it's not a big deal. (Otherwise, you could call parse(span.data(), span.size())')

Parsing protobufs will produce an std::string, is there a conversion between those? Or it's better to use absl::string_view instead of span? (I was hitting zero-byte related problems when trying to use string view)

Changed input arguments to absl::string_view

cc/utils/cose/cose.cc

andrisaar · 2024-02-16T15:31:22Z

cc/utils/cose/cose.h

+ // Array of signatures. Each signature is represented as a COSE_Signature structure.
+ const cppbor::Bstr* signature;
+
+ CoseSign1(const cppbor::Bstr* protected_headers, const cppbor::Map* unprotected_headers,


What do these pointers point to? Who owns the memory? How do you guarantee that the memory they point to does not go out of scope while sthis struct is alive?

They point to fields from the item_ which is a unique pointer owned by this struct

andrisaar · 2024-02-16T15:32:53Z

cc/utils/cose/cose.h

+
+ CoseSign1(const cppbor::Bstr* protected_headers, const cppbor::Map* unprotected_headers,
+ const cppbor::Bstr* payload, const cppbor::Bstr* signature,
+ std::unique_ptr<cppbor::Item>&& item)


I it fine for this unique_ptr to be a nullptr?

It cannot be nullptr, because the other pointers should point to the fields from this item

andrisaar · 2024-02-16T15:36:39Z

cc/utils/cose/cose.h

+ // Public key.
+ const cppbor::Bstr* x;
+
+ CoseKey(const cppbor::Uint* kty, const cppbor::Bstr* kid, const cppbor::Nint* alg,


Thinking API-wise, and this is in line with my comments above...

I'd change these constructors to just take cppbor::Item directly. Once you've assigned it to item_, then, in the constructor you can build the pointers to inside the item.

Or: move the constructors to the private section so that the only way how to construct these structs is via the factory method, which enforces the invariants.

Parsing an item could produce errors (i.e. fields not have corret types), so moving the constructor to the private section. And the logic that produces errors stays in the static method.

cc/utils/cose/cose.h

ipetr0v added 3 commits February 15, 2024 13:26

Add CWT support for C++

a0b1a51

Update

e97699d

Update

9e13d1e

ipetr0v marked this pull request as ready for review February 15, 2024 17:13

ipetr0v requested review from conradgrobler, andrisaar, thmsbinder and k-naliuka February 15, 2024 17:14

ipetr0v added 2 commits February 15, 2024 17:32

Update

f591095

Update

16f56ff

k-naliuka approved these changes Feb 15, 2024

View reviewed changes

cc/attestation/BUILD Outdated Show resolved Hide resolved

cc/attestation/cose.cc Outdated Show resolved Hide resolved

cc/attestation/cose.cc Outdated Show resolved Hide resolved

cc/attestation/cwt.h Outdated Show resolved Hide resolved

thmsbinder approved these changes Feb 16, 2024

View reviewed changes

Update

63734cc

conradgrobler approved these changes Feb 16, 2024

View reviewed changes

cc/utils/cose/cose.cc Outdated Show resolved Hide resolved

ipetr0v added 2 commits February 16, 2024 14:55

Update

0169a2b

Update

2245a91

andrisaar reviewed Feb 16, 2024

View reviewed changes

cc/utils/cose/cose.cc Outdated Show resolved Hide resolved

andrisaar reviewed Feb 16, 2024

View reviewed changes

Update

4b169e9

ipetr0v requested a review from andrisaar February 16, 2024 16:20

andrisaar reviewed Feb 16, 2024

View reviewed changes

cc/utils/cose/cose.h Outdated Show resolved Hide resolved

andrisaar approved these changes Feb 16, 2024

View reviewed changes

ipetr0v added 3 commits February 16, 2024 16:28

Update

007753d

Update

324ba57

Update

53478bf

ipetr0v added 2 commits February 19, 2024 14:34

Update

65b1733

Update

2147cff

ipetr0v merged commit aa0e264 into project-oak:main Feb 19, 2024
17 checks passed

ipetr0v deleted the cpp_cwt branch February 19, 2024 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CWT support for C++ #4804

Add CWT support for C++ #4804

ipetr0v commented Feb 15, 2024 •

edited

Loading

ipetr0v commented Feb 15, 2024 •

edited

Loading

k-naliuka commented Feb 15, 2024

thmsbinder Feb 16, 2024

ipetr0v Feb 16, 2024

thmsbinder Feb 16, 2024

ipetr0v Feb 16, 2024

andrisaar Feb 16, 2024

ipetr0v Feb 16, 2024

andrisaar Feb 16, 2024

ipetr0v Feb 16, 2024

ipetr0v Feb 19, 2024

andrisaar Feb 16, 2024

ipetr0v Feb 16, 2024

andrisaar Feb 16, 2024 •

edited

Loading

ipetr0v Feb 16, 2024

andrisaar Feb 16, 2024

ipetr0v Feb 16, 2024


		namespace oak::utils::cose {

		absl::StatusOr<CoseSign1> CoseSign1::Deserialize(const std::vector<uint8_t>& data) {

Add CWT support for C++ #4804

Add CWT support for C++ #4804

Conversation

ipetr0v commented Feb 15, 2024 • edited Loading

ipetr0v commented Feb 15, 2024 • edited Loading

k-naliuka commented Feb 15, 2024

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

andrisaar Feb 16, 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

ipetr0v commented Feb 15, 2024 •

edited

Loading

ipetr0v commented Feb 15, 2024 •

edited

Loading

andrisaar Feb 16, 2024 •

edited

Loading