Missing critical header checking when using `did:x509` issuers?

[achamayou]

See #326 (comment), there is currently no checking of COSE crit when the issuer is a did:x509.

According to RFC9052:

The array MUST have at least one value in it.

Not all header-parameter labels need to be included in the "crit" header parameter. The rules for deciding which header parameters are placed in the array are:

Integer labels in the range of 0 to 7 SHOULD be omitted.
Integer labels in the range -1 to -128 can be omitted. Algorithms can assign labels in this range where the ability to process the content of the label is considered to be core to implementing the algorithm. Algorithms can assign labels outside of this range and include them in the "crit" header parameter when the ability to process the content of the label is not considered to be core functionality of the algorithm but does need to be understood to correctly process this instance. Integer labels in the range -129 to -65536 SHOULD be included, as these would be less common header parameters that might not be generally supported.
Labels for header parameters required for an application MAY be omitted. Applications should have a statement declaring whether or not the label can be omitted.

It's arguable that CWT Claims(15) and x5chain(33) could be included in crit but that's also not obviously mandated.
The only example that sets crit is in Appendix C.1.3, for a tstr label, similar to "attestedsvc" (where we do it #326).

[achamayou]

I am looking for evidence of use of crit with parameters in the 1-255 range. All other things being equal, it could make sense to require (crit)2: [15(CWT Claims), 34(x5chain)] when using did:x509, but this has two implications:

1. did:attestedsvc would need to require (crit)2: [15(CWT Claims), "attestedsvc"] for consistency.
2. this would be a breaking change, existing test payloads (cts-hashv-cwtclaims-b64url.cose) do not set a crit at the moment.

A backwards-compatible solution may be to enforce (crit)2: [15(CWT Claims), 34(x5chain)] when (crit)2 is present with did:x509 at first, and only require it at all times when all new signatures carry it.

Another solution is to only require crit for non-IANA parameters. Once #326 merges, that will be the case consistently across both supported issuer types.

Regardless of the choice made, we ought to write a CDDL schema for each issuer type, and publish it in the documentation for clarity.

[achamayou]

One data point is RFC9338 (Countersignatures for COSE), which assigns parameters 11 and 12 (above 7), and does not set a crit parameter in any of its examples.

[achamayou]

RFC9360 which defines x5* fields unfortunately never mentions crit, and the same is true for RFC9597. The only datapoint either way seems to be RFC9338.

Considering this, the consistent choice would seem to be not to expect (crit)2: [15(CWT Claims), 34(x5chain)] for did:x505 issuers. If we go that way, two implementations are possible:

1. Allow crit, but check that all values are in the phdr (that's the minimum baseline set by RFC9052).
2. Disallow crit altogether (empty crit is illegal in the spec), since we expect no non-IANA parameters, and our stance is that IANA-registered parameters are typically not included in crit.

Again, either way, this needs documenting and a schema.

[achamayou]

Consensus is to not include standard IANA parameters in crit, but allow them so long as they're values the verifier understand and uses for authentication. Definitely reject unknown crits in any case.