Add COSE_Key output options, exact size queries, and CBOR parsing helpers - #66
Add COSE_Key output options, exact size queries, and CBOR parsing helpers#66danielinux wants to merge 14 commits into
Conversation
…g range - WOLFCOSE_KEY_PUBLIC_ONLY on Ed25519, Ed448, RSA and ML-DSA: the public encoding must be shorter than the full one and decode with no private half. Comparing only against the size query cannot catch a no-op flag. - wc_CoseKey_EncodeEccRaw on P-384 and P-521, including rejection of another curve's coordinate size. - RSA d shorter than the modulus, from a fixed key, so the left-padding branch and the size/encode agreement that depends on it are exercised. - wc_CoseKey_PeekInfo with a negative alg, and with alg and crv outside the int32 range.
There was a problem hiding this comment.
Pull request overview
This PR adds new COSE_Key and CBOR helper APIs to better support CTAP2/WebAuthn and embedded credential workflows—specifically: public-only key publication, exact encoded-size queries, non-importing COSE_Key metadata inspection, and CBOR parsing conveniences for mixed-label maps and deferred/nested parsing.
Changes:
- Add
wc_CoseKey_Encode_ex()withWOLFCOSE_KEY_PUBLIC_ONLY, pluswc_CoseKey_EncodeSize()/_ex()for exact encoded-size computation. - Add
wc_CoseKey_EncodeEccRaw()for EC2 COSE_Key encoding directly from raw affine coordinates, andwc_CoseKey_PeekInfo()to read COSE_Key metadata without importing. - Add CBOR helpers:
wc_CBOR_EncoderInit()/wc_CBOR_DecoderInit(),wc_CBOR_SkipItem(), andwc_CBOR_DecodeLabel()(+ label match helpers), plus documentation and tests for strict decoding expectations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cose.c | Adds coverage for public-only encoding, exact size queries, ECC-raw encoding parity, RSA short-d padding branch, and PeekInfo behavior/range checks. |
| tests/test_cbor.c | Adds tests for CBOR context initializers, SkipItem capture/reparse, and int-or-text label decoding/matching. |
| src/wolfcose.c | Implements new key encoding options, exact-size query logic with overflow-checked arithmetic, ECC raw encoding helper, and COSE_Key metadata peeking. |
| src/wolfcose_cbor.c | Implements wc_CBOR_SkipItem(), wc_CBOR_DecodeLabel(), and label comparator helpers. |
| include/wolfcose/wolfcose.h | Exposes new APIs/flags, adds inline CBOR ctx initializers, and documents strict decoding + private-key serialization warning. |
| docs/Getting-Started.md | Documents strict decoding requirements and common interop surprises (preferred serialization, no indefinite lengths, trailing-byte rejection, etc.). |
| docs/API-Reference.md | Documents the new APIs and clarifies wc_CoseKey_Encode() private-material serialization behavior and strict decode expectations. |
| ChangeLog.md | Records the newly added APIs and feature additions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| ## New Feature Additions | ||
|
|
||
| * `wc_CoseKey_Encode_ex()` takes a flags argument, with |
There was a problem hiding this comment.
Can we remove this changelog entry?
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 6 total — 6 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Low] [review+review-security] RSA EncodeSize can fail on very large RSA keys where Encode succeeds (scratch-buffer fallback) —
src/wolfcose.c:1509-1550 - [Low] [review] Exact-size claim depends on key-alg matching the attached key for ML-DSA (and e leading zeros for RSA) —
src/wolfcose.c:2515-2560 - [Low] [review] wc_CBOR_DecodeLabel integer-overflow branches are untested —
src/wolfcose_cbor.c:711-745 - [Info] [review] Weak/near-tautological assertion in public-only ECC test —
tests/test_cose.c - [Info] [review] Test locates RSA d via WOLFCOSE_KEY_LABEL_Y without explanation —
tests/test_cose.c - [Info] [review-security] wc_CoseKey_Encode_ex leaves ctx.cbuf uninitialized (harmless, inconsistent with sibling) —
src/wolfcose.c:1689-1691
Review generated by Skoll
| * declared MP_API, which wolfSSL exports only when built with | ||
| * WOLFSSL_PUBLIC_MP, so calling it here fails to link (undefined reference | ||
| * to sp_unsigned_bin_size) against a stock library. */ | ||
| static int wolfCose_RsaExponentSize(RsaKey* rsa, size_t* eLen) |
There was a problem hiding this comment.
🔵 [Low] RSA EncodeSize can fail on very large RSA keys where Encode succeeds (scratch-buffer fallback) · Logic
In the #if !defined(HAVE_ECC) && !defined(WOLFSSL_EXPORT_INT) fallback branch the exponent is measured with wc_RsaFlattenPublicKey(rsa, eBuf, &len, nBuf, &nLen) where nBuf[WOLFCOSE_MAX_SCRATCH_SZ] (defaults to 512, holding a modulus up to RSA-4096 exactly). For an RSA modulus larger than WOLFCOSE_MAX_SCRATCH_SZ (e.g. RSA-6144/8192 = 768/1024 bytes with the default 512 scratch), wc_RsaFlattenPublicKey returns a buffer error, so wc_CoseKey_EncodeSize()/EncodeSize_ex() returns WOLFCOSE_E_CRYPTO. The encoder path (wc_CoseKey_Encode) flattens n straight into the caller's output buffer (handling moduli up to 65535 bytes), so it succeeds on the same key. The size query and the encoder therefore disagree for very large RSA keys in this narrow no-ECC / no-WOLFSSL_EXPORT_INT build. Only e is needed here, so n should not be size-bounded by scratch. The failure is a clean error return, not memory unsafety; the common build (ECC on, or WOLFSSL_EXPORT_INT) uses only eBuf[8] and is unaffected. Severity: the review mode rated this Low/SUGGEST while review-security rated it Info; the stricter Low is kept.
Fix: Read e without also materializing n in that fallback (e.g. size n from wc_RsaEncryptSize() as the caller already does and export only e), making EncodeSize succeed wherever Encode does. If left as-is, document that the exact-size query supports RSA moduli only up to WOLFCOSE_MAX_SCRATCH_SZ in the no-ECC/no-EXPORT_INT configuration so it stays consistent with the encoder. Low priority given the exotic config and key size.
| else | ||
| #endif /* WOLFCOSE_HAVE_RSAPSS */ | ||
| #ifdef WOLFCOSE_HAVE_MLDSA | ||
| if (key->kty == WOLFCOSE_KTY_AKP) { |
There was a problem hiding this comment.
🔵 [Low] Exact-size claim depends on key-alg matching the attached key for ML-DSA (and e leading zeros for RSA) · API contract
For AKP the size query derives the public-key length from key->alg (1312/1952/2592), whereas the encoder writes the actual wc_MlDsaKey_ExportPubRaw length (src/wolfcose.c:2010). If key->alg disagrees with the attached ML-DSA parameter set, wc_CoseKey_EncodeSize() and wc_CoseKey_Encode() return different lengths, contradicting the documented 'exact, not an upper bound' contract. The RSA e path is similar: EncodeSize strips leading zeros via wolfCose_RsaExponentSize while the encoder emits whatever wc_RsaFlattenPublicKey returns. Neither is a memory-safety issue (an undersized query just yields BUFFER_TOO_SMALL later), but the 'exact' guarantee only holds when alg and key agree.
Fix: Either document that EncodeSize assumes key->alg matches the attached key material, or size the AKP public component from the key object rather than from alg, so 'exact' holds unconditionally.
| ret = WOLFCOSE_E_INVALID_ARG; | ||
| } | ||
| else { | ||
| ret = wolfCose_CBOR_DecodeHead(ctx, &item); |
There was a problem hiding this comment.
🔵 [Low] wc_CBOR_DecodeLabel integer-overflow branches are untested · Test coverage
wc_CBOR_DecodeLabel returns WOLFCOSE_E_CBOR_OVERFLOW when a uint or negint label argument exceeds INT64_MAX. test_cbor_decode_label covers int/negint/tstr/bstr and NULL cases but never exercises the overflow branches, so a regression in the > (uint64_t)INT64_MAX guards or the -1 - (int64_t)item.val arithmetic would go unnoticed. This is the same overflow contract as wc_CBOR_DecodeInt.
Fix: Extend test_cbor_decode_label with a uint label > INT64_MAX (0x1B FF..FF) and a negint at the INT64_MIN boundary, asserting WOLFCOSE_E_CBOR_OVERFLOW and INT64_MIN respectively.
| /* {1,3,-1,-2,-3} = map(5) = 77 bytes */ | ||
| TEST_ASSERT(pubLen == 77u && pub[0] == 0xA5u, | ||
| "pubonly ex encode omits d"); | ||
| TEST_ASSERT(memcmp(pub, full, 2u) == 0 || pub[0] != full[0], |
There was a problem hiding this comment.
⚪ [Info] Weak/near-tautological assertion in public-only ECC test · Test quality
The assertion memcmp(pub, full, 2u) == 0 || pub[0] != full[0] is false only in the narrow case where pub[0]==full[0] but pub[1]!=full[1], so it does not meaningfully verify that the public-only map head differs from the full one. The intended property (map(5) 0xA5 vs map(6) 0xA6) is already asserted directly on the lines above, making this line redundant and confusing.
Fix: Replace with a direct check of the two map heads (TEST_ASSERT(pub[0] == 0xA5u && full[0] == 0xA6u, ...)), or drop the redundant assertion.
| for (i = 0; (ret == 0) && (i < mapCount); i++) { | ||
| ret = wc_CBOR_DecodeInt(&dec, &label); | ||
| if (ret == 0) { | ||
| if (label == (int64_t)WOLFCOSE_KEY_LABEL_Y) { |
There was a problem hiding this comment.
⚪ [Info] Test locates RSA d via WOLFCOSE_KEY_LABEL_Y without explanation · Style
The RSA short-d test scans for label == WOLFCOSE_KEY_LABEL_Y (-3) to find the RSA private exponent d. This is correct because RFC 8230 places d at label -3, numerically identical to EC2's y label constant, and the encoder reuses WOLFCOSE_KEY_LABEL_Y for RSA d. But reading KEY_LABEL_Y in an RSA test is misleading. A one-line comment (or an aliased WOLFCOSE_KEY_LABEL_RSA_D constant) would make the intent obvious.
Fix: Add a clarifying comment (/* RFC 8230: RSA d shares label -3 with EC2 y */) or introduce an RSA-specific -3 alias used by both encoder and test.
| ret = WOLFCOSE_E_INVALID_ARG; | ||
| } | ||
| else { | ||
| ctx.buf = out; |
There was a problem hiding this comment.
⚪ [Info] wc_CoseKey_Encode_ex leaves ctx.cbuf uninitialized (harmless, inconsistent with sibling) · Logic
The refactored wc_CoseKey_Encode_ex sets ctx.buf, ctx.bufSz, and ctx.idx but not ctx.cbuf, leaving the const decode pointer indeterminate. This is harmless because every encode path reads only ctx.buf, and it matches the pre-PR pattern of wc_CoseKey_Encode, so it is not a newly introduced defect. Worth noting only because the sibling raw-coordinate encoder added in the same PR (wc_CoseKey_EncodeEccRaw) explicitly sets ctx.cbuf = NULL, and the new wc_CBOR_EncoderInit() helper clears it — so the direct field assignment here is the odd one out.
Fix: Optionally set ctx.cbuf = NULL; here (or use the new wc_CBOR_EncoderInit()) for consistency and defense-in-depth. No functional change.
Summary
This PR fils a set of API gaps that show up when wolfCOSE is used from a CTAP2/WebAuthn
or embedded-credential context: publishing a public key without leaking the private
half, sizing a buffer before encoding into it, and parsing maps that wolfCOSE itself
does not define. No existing API changes behaviour; every addition is a new symbol or
an
_exvariant whose zero-flags form is the old function.COSE_Key encoding
wc_CoseKey_Encode_ex()adds a flags argument;WOLFCOSE_KEY_PUBLIC_ONLYemitsthe public half only — no
-4: dfor EC2/OKP, no-3: dor CRT factors for RSA, no-2: privseed for an RFC 9964 AKP key. A symmetric key has no public half, so thatcombination returns
WOLFCOSE_E_COSE_KEY_TYPE.This matters because
wc_CoseKey_Encode()serialises the private key whenever theattached key carries one, and an
ecc_keyattached withwc_CoseKey_SetEcc()hashasPrivateset whenever it is a keypair. Encoding "the public key" of a live P-256keypair therefore discloses the private scalar, visible only as one extra map entry
(112 bytes /
map(6)instead of 77 /map(5)). The old function is unchanged butnow documents that; anything that publishes a public key should use the new flag.
wc_CoseKey_EncodeSize()/wc_CoseKey_EncodeSize_ex()report the exact encodedsize without writing anything or exporting key material. The result is exact rather
than an upper bound, so it can size a buffer or reject an oversized key up front.
wc_CoseKey_EncodeEccRaw()encodes an EC2COSE_Keystraight from raw affinecoordinates, with no
ecc_keyand none of the point-import cost, for callers thathold only the coordinates. It does not validate that the point is on the curve —
encoding an unvalidated point is safe, using one is not.
COSE_Key decoding
wc_CoseKey_PeekInfo()readskty/alg/crv/kidout of a buffer withoutimporting anything, so a parser that accepts more than one key type no longer has to
guess and retry against
wc_CoseKey_Decode()'s required pre-attached key. It appliesthe same structural checks as the decoder, and
kidis zero-copy.CBOR helpers
wc_CBOR_EncoderInit()/wc_CBOR_DecoderInit()— static inline, set onedirection of
WOLFCOSE_CBOR_CTXand clear the other, so a context can't behalf-initialised from the wrong side.
wc_CBOR_SkipItem()— likewc_CBOR_Skip(), but also reports the skipped item'sstart and length, zero-copy, which is what deferred or nested parsing needs (a CTAP2
allowListentry, an embeddedCOSE_Key). The captured range feeds straight intowc_CBOR_DecoderInit()orwc_CoseKey_Decode().wc_CBOR_DecodeLabel()withwc_CBOR_LabelIsInt()/wc_CBOR_LabelIsText()—RFC 9052 allows
label = int / tstr, and real COSE and CTAP2 maps use both spellingsfor the same field (
3vs"alg"). Text comparison is byte-exact, matching how CTAP2and COSE compare labels.
Documentation
A new "Strict Decoding" section in
docs/Getting-Started.mdstates that every decodeentry point requires RFC 8949 §4.2.1 preferred serialization and rejects indefinite
lengths, tabulating the rejected forms, their error codes, and the neighbouring
surprises (trailing bytes, exact-size EC2 coordinates, duplicate and text labels). It
closes by explaining why this is not configurable: relaxing it would let a signature or
MAC be recomputed over a re-encoding of the same data. The header carries a matching
note and
ChangeLog.mdlists the new APIs.Testing
Eleven new test cases.
tests/test_cbor.ccovers context init,SkipItemcapture andreparse, and int-or-text labels;
tests/test_cose.ccovers public-only encoding acrossEC2, Ed25519, Ed448, RSA and ML-DSA (asserting the public output is strictly shorter and
re-decodes with no private half, not merely that the size query agrees), exact size
queries per key type,
EncodeEccRawon P-256/P-384/P-521 against theecc_keypath,the RSA short-
dleft-padding branch via a fixed key, andPeekInfoincluding negativeand out-of-int32-range
alg.