[OAUTH-WG] draft-ietf-oauth-sd-jwt-vc-19 ietf last call Secdir review

[Ionuț Mihalcea via Datatracker <[e-mail]>]

Document: draft-ietf-oauth-sd-jwt-vc
Title: SD-JWT-based Verifiable Digital Credentials (SD-JWT VC)
Reviewer: Ionuț Mihalcea
Review result: Has Issues

I have reviewed this document as part of the security directorate's ongoing effort
to review all IETF documents being processed by the IESG. These comments were
written primarily for the benefit of the security area directors. Document editors
and WG chairs should treat these comments just like any other last call comments.

The summary of the review is Has Issues, though this could be predicated on my
misunderstanding.

The draft builds on top of RFC9901 to define a complete, interoperable data model
and ecosystem profile for SD-JWT Verifiable Digital Credentials (VC). It provides
not just the model for the VC, but also for metadata for verifying and displaying
VCs, as well as credential typing and inheritance mechanics, key and metadata
discovery, and more.

The document is thorough and quite broad in scope. The shape and flow of the draft
seems good. The points below cover the issues and nits that stood out to me.

* Section 4 describes the use of a well-known URL to find issuer metadata, and
mandates the shape of the starting URL in iss. However, no requirement is made for
that URL to be parsed and normalised as per sec. 6 of RFC3986 before the well-known
URL is constructed. Is that on purpose?

* In section 5.5.1.1, before the bulletpoints you have "The object contains the
following properties". Should this be normative (i.e., "The object MUST contain at
least one of the following properties"), since all the properties are optional?
Asking because Section 5.5.1.2.1 uses normative language in a similar context.

* In section 5.5.1.2.2 you give a list of characters that must be escaped before
rendering the SVG. However, you should also mandate that invalid XML characters be
stripped, or that the credential should be rejected if they're encountered.

* The processing of path components in Section 5.6.1.2 and of claim mandatory
metadata in 5.6.3 seems prone to issues, when combined. For example, for string
path components, you state "If the key does not exist in an element currently
selected, remove that element from the selection." This does not take into account
whether the element at the end of the path is expected to be mandatory. And
actually, 5.6.3 is not clear on this either: if the claim is within elements of an
array, MUST all elements include that claim or at least one? Does this apply only
at the "leaf element" level, or must all arrays along the path have elements that
allow traversal all the way to this mandatory element? Either way, this should be
stated explicitly, and the path traversal text should also reflect it. If my
description is not clear I can provide an example.

* In Section 7.1 you mandate that "the Consumer MUST validate the URL to ensure
that it is a valid HTTPS URL and that it does not point to internal resources". Two
observations here: this could be subject to a DNS rebinding attack, so it would be
good to hint towards that and ask that the destination address get cached and used
after check, instead of performing another request for it. Second, this requirement
seems to assume that SD-JWT VCs will be used exclusively on the open internet, and
never in internal deployments. Is this on purpose? If closed / private environments
are out of scope, it would be good to make that explicit.

* Section 7.10 mandates that "consuming applications MUST ensure that references to
external resources (images, etc.) from within the SVG cannot be used to track users
or the usage of credentials." It would be helpful to provide some guidance or
references on how to achieve this.

Nits:

* I'm struggling to parse the first paragraph in Section 5.3. Not certain what "the
vct in the reference to the Type Metadata" means. Also, I can't easily tell what
the relationship is between the text before the brackets and the text in the
brackets.

* In Section 5.3.1: "it is at the consumer's discretion whether to attempt retrieve
Type Metadata" - should probably be "... to attempt to retrieve ...".

* There are two ways in which optional and required properties / claims are
indicated throughout the document. In some places (e.g., Section 4.2) it's
"claim_name: OPTIONAL. ...". In others (e.g., Section 5.5) it's "claim_name: ...
This property is OPTIONAL.". In some places (e.g., Section 5.2) it's both. This is
just an observation, I think both work in conveying the intent.