fix(mdoc): correct COSE_Sign1 and MSO structural bugs

[leifj]

Summary

Three real, spec-conformance bugs in mdoc issuance/verification, all caught
by cross-checking our issued mdocs against Google's own reference verifier
(https://digital-credentials.dev/, backed by the isomdoc Python library)
while validating a native Digital Credentials API (DC API) presentation
flow. Our own verifier never caught these, since it happened to agree with
itself on the same (incorrect) conventions on both the issuing and
verifying side.

• issuerAuth's x5chain was in the COSE_Sign1 protected header. Per RFC
  9360 and the mdoc convention, it belongs in the unprotected header - the
  certificate chain isn't itself integrity-critical (trust comes from the
  signature verifying against the chain's leaf key, not from where the
  chain is placed), and a strict verifier rejects any protected header with
  more than the algorithm element (isomdoc's exact error: "issuerAuth COSE_Sign1 protected contains too many elements").
  GetCertificateChainFromSign1 now checks the unprotected header first
  (handling the uint64-vs-int64 CBOR key-decode ambiguity for maps decoded
  generically from the wire), falling back to protected for mdocs issued
  before this fix.

• Sign1's external_aad was left as Go's nil []byte.
  cbor.Marshal encodes a nil slice as CBOR null (0xf6), but a compliant
  verifier reconstructs the Sig_structure with an empty byte string (0x40,
  h'') - so the bytes actually signed diverged from what any verifier
  reconstructs to check the signature, invalidating every issuerAuth
  signature produced by this code (isomdoc's error:
  cryptography.exceptions.InvalidSignature). Both Sign1 and Verify1
  now normalize nil to an explicit empty slice.

• MSOBuilder.Build() pre-serialized DeviceKeyInfo.deviceKey to bytes
  and wrapped those bytes in a byte string, when ISO 18013-5 embeds the
  COSE_Key directly as a CBOR map (isomdoc's error: 'bytes' object has no attribute 'get' trying to read it as a dict). DeviceKeyInfo.DeviceKey
  is now typed COSEKey (not []byte), so the struct's own cbor tags
  produce the correct encoding when the whole MSO is marshaled.

All three were reproducible independent of DC API specifically - any mdoc
presentation (redirect flow included) built with this code would fail
against a spec-strict verifier.

Test plan

• go build ./...
• go test ./pkg/mdoc/... (all pass, including updated fixtures for the
  DeviceKeyInfo.DeviceKey type change)
• go test ./internal/verifier/..., ./internal/issuer/...
• End-to-end on-device verification: issued a real mDL, presented it via
  the W3C Digital Credentials API, and confirmed successful verification
  against both Google's reference verifier (digital-credentials.dev)
  and this fork's own verifier

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhWrEC7D4b3vxZ4wm3gDML

[Copilot]

Pull request overview

Fixes three interoperability/spec-conformance bugs in the mdoc (ISO 18013-5) COSE/MSO encoding and verification logic that were surfaced by strict external verifiers (notably Google’s reference verifier).

Changes:

• Move x5chain from COSE_Sign1 protected headers to unprotected headers, and update extraction logic to prefer unprotected (with backwards-compatible fallback).
• Normalize COSE_Sign1 external_aad so nil is treated as an explicit empty byte string during both signing and verification.
• Fix MSO DeviceKeyInfo.deviceKey encoding to embed COSE_Key as a CBOR map (not double-encoded bytes), updating types and related tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
pkg/mdoc/mso.go	Stops double-encoding deviceKey; embeds COSE_Key directly in the MSO CBOR structure.
pkg/mdoc/mdoc.go	Changes DeviceKeyInfo.DeviceKey from []byte to COSEKey to match ISO 18013-5 encoding requirements.
pkg/mdoc/mdoc_test.go	Updates fixtures/tests for the DeviceKeyInfo.DeviceKey type change (one fixture still needs correction).
pkg/mdoc/device_auth.go	Simplifies device key extraction by using the embedded COSEKey directly.
pkg/mdoc/device_auth_test.go	Updates tests to construct MSOs with COSEKey values instead of serialized bytes.
pkg/mdoc/cose.go	Fixes COSE_Sign1 header placement (x5chain unprotected) and external_aad nil-vs-empty normalization; improves x5chain lookup robustness.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[leifj]

Rebased onto latest `main` (only conflict: `pkg/mdoc/mdoc_test.go` — main's own unrelated test changes still used the old `[]byte` `DeviceKey` shape this PR is replacing; took this PR's `COSEKey` version). While resolving, also addressed both Copilot review comments:

• `TestMobileSecurityObject`'s `COSEKey` fixture was missing the required `Y` coordinate for an EC2 key — added it, and removed the now-unused `testDeviceKeyBytes` helper (only referenced by the pre-merge code this PR replaces).
• Reworded the `DeviceKeyInfo.DeviceKey` comment's reference from "MSOBuilder.Build's doc comment" (misleading — the explanation is an inline comment inside `Build`'s body, not an actual doc comment) to point at it accurately.

Build, vet, and make test all green. (Also double-checked: none of masv3971's recent merges to main touched `cose.go`/`mso.go`/`device_auth.go` since this branched, so all three fixes here are still needed — `main` still places `x5chain` in the protected header.)

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/mdoc/device_auth.go:359

• ExtractDeviceKeyFromMSO now treats a device key as “present” if X is non-empty, but for EC2 keys a missing/empty Y will currently slip through and produce an ECDSA public key with Y=0 (since big.Int.SetBytes(nil) => 0). That can lead to confusing downstream failures (e.g., signature verification failing instead of a clear “device key missing/invalid”) and makes the presence check asymmetric across key types. Consider validating required coordinates based on Kty before calling ToPublicKey().

	if len(mso.DeviceKeyInfo.DeviceKey.X) == 0 {
		return nil, errors.New("device key not present in MSO")
	}

	return mso.DeviceKeyInfo.DeviceKey.ToPublicKey()

pkg/mdoc/cose.go:309

• Sign1/Verify1 now correctly normalize a nil externalAAD to an empty byte string so Sig_structure uses h'' instead of CBOR null. However, Mac0/VerifyCOSEMac0 still accept nil externalAAD and will CBOR-encode it as null, which appears to have the same spec-conformance/interop problem for MAC0 structures. If device-auth MACs are used externally, this PR likely needs the same normalization in the MAC0 helpers for consistency.

	// A nil Go []byte and an empty (len-0, non-nil) one both mean "no AAD",
	// but cbor.Marshal encodes them differently - nil as CBOR null (0xf6),
	// empty as an empty byte string (0x40, i.e. h''). Sig_structure's
	// external_aad element MUST be h'' per RFC 8152/9052 - a verifier
	// reconstructing it as an empty byte string (as every real one does,
	// including Google's isomdoc, which is what caught this) computes
	// different bytes-to-be-signed than what a nil-produced signature was
	// actually computed over, so the signature fails to verify even though
	// nothing else about it is wrong. Normalizing here means no caller has
	// to remember this footgun.
	if externalAAD == nil {
		externalAAD = []byte{}
	}

[leifj]

Fixed both new suppressed findings (commit e250e69):

• `ExtractDeviceKeyFromMSO` didn't validate the Y coordinate for EC2 keys: only checked X was non-empty, so a device key missing Y would silently produce an ECDSA public key with Y=0 (`big.Int.SetBytes(nil) == 0`) instead of a clear error — the failure would only surface later as a confusing signature-verification error. Now checks Y is present when `Kty == KeyTypeEC2`. Added a test.
• `Mac0`/`VerifyCOSEMac0` had the identical nil-vs-empty `external_aad` bug that `Sign1`/`Verify1` got fixed for in this PR, just left unfixed. Both current call sites (device_auth.go's session-key MAC construction/verification) pass `nil` on both sides, so it was self-consistent internally and passed all existing tests either way — but it's the same non-interoperability risk against any external verifier reconstructing `MAC_structure` per spec, which is exactly the class of bug this PR exists to fix. Applied the identical normalization to both.

Build, vet, and make test all green — every existing Mac0/DeviceAuth test still passes unchanged, confirming the fix only changes the encoding to the spec-compliant form without breaking the internal round-trip.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud