Dear OAuth WG, and especially the authors of SD-JWT-VC Here is the review I promised during the WG F2F in Montreal. It took more time than I initially thought, but here it is.
Firstly, I would like to state my sincere thanks to the authors and editors. It is a great document. Below are the 28 comments that I came up with. Each comment is numbered as NSxx, and the Location, comment, and proposal are indicated. Some of them might not be relevant, but I have put them just in case. I pasted my markdown to Gmail and am sending it now -- if it does not render well, please let me know. I will think of another way. I hope this is going to help. Best wishes, Nat Sakimura ----- NS01: Location: Throughout the documentComment Just like Figure 1, it would be nice if all examples and figures were numbered and formally labelled to enable clear cross-referencing and hyperlinking to assist the readers. Proposal Assign sequential labels to all code blocks containing non-normative examples. E.g., change the unnumbered JSON block in Section 3.2.1 to Example 3.1: Decoded SD-JWT Header. The appendices examples should be labelled Example B.1, Example B.2, etc. Also, refer to them using the number from the main text, instead of something like “the following … “. Also, put the hyperlink to them. NS02: Location: 1.1. <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#section-1.1>Issuer-Holder-Verifier Model <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#name-issuer-holder-verifier-mode>: Paragraph 1 Figure 1 is not referenced from the main text. Proposal: Add the following just before Figure 1. Figure 1 depicts the Issuer-Holder-Verifier model. NS03: Location: 1.1 Issuer-Holder-Verifier Model <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#name-issuer-holder-verifier-mode>: Paragraph 2Comments Long sentences (46 words in this case), often with nested structure, are hard for non-native speakers to read. It should be broken down for clarity. Currently, it goes: Verifiers can check the authenticity of the data in the Verifiable Credentials and optionally enforce Key Binding, i.e., ask the Holder to prove that they are the intended Holder of the Verifiable Credential, for example, by proving possession of a cryptographic key referenced in the credential. Proposal Modify to: Verifiers can check the authenticity of the data in Verifiable Credentials. Verifiers can also optionally enforce Key Binding, which requires the Holder to prove they are the intended Holder of the Verifiable Credential. This proof is typically done by demonstrating possession of a cryptographic key referenced in the credential NS04: Location: 1.2 SD-JWT as a Credential Format <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#name-sd-jwt-as-a-credential-form>: Paragraph 3Comments The long sentence (35 words) was hard to understand at the first reading. Currently, it goes: SD-JWT is a superset of JWT as it can also be used when there are no selectively disclosable claims and also supports JWS JSON serialization, which is useful for long term archiving and multi signatures. Proposal Modify to: SD-JWT is a superset of JWT. It can also be used when there are no selectively disclosable claims. Furthermore, SD-JWT supports JWS JSON serialization, which is useful for long-term archiving and multi-signatures. NS05: Location: New 1.5 AbbreviationsComments It would be nicer if the document had an abbreviation section so that the reader can refer back to it. Proposal Add an abbreviation section. NS06: Location: 3.2 Data Format: Paragraph 1Comments Passive voice often makes it difficult to understand who should do what. In 3.2, it currently goes: An SD-JWT VC MUST be encoded using the SD-JWT format defined in Section 4 or Section 8 of [I-D.ietf-oauth-selective-disclosure-jwt], where support for the JWS JSON Serialization is OPTIONAL. Proposal This can be modified as follows so that it will be clearer. Issuers MUST encode an SD-JWT VC using the SD-JWT format defined in Section 4 or Section 8 of [[I-D.ietf-oauth-selective-disclosure-jwt]], where support for the JWS JSON Serialization is OPTIONAL. NS07: Location: 3.2.1 JOSE Header: Paragraph 2Comments Passive voice often makes it difficult to understand who should do what. It currently goes: The typ header parameter of the SD-JWT MUST be present. Proposal Proposes to modify to: The Issuer MUST include the typ header parameter in the SD-JWT. NS08: Location: 3.2.2 JWT Claims Set: At the endComments I am not sure if this is the correct place, but I feel that clearly stating the condition for including the cnf claim in the VC before describing its use in the presentation (Section 4) would help the reader. Proposal Perhaps, Add the following requirement to 3.2.2. If the Issuer intends the SD-JWT VC to be Holder-Bound, the JWT Claims Set MUST contain a cnf claim as defined in [[RFC7800]]. The Issuer SHOULD NOT issue a presentation-bound credential (one with a cnf claim) without appropriate key confirmation from the Holder. NS09: Location: 3.4 Verification and Processing: Paragraph 1 Comments Very long sentence (56 words). It currently goes: The check in point 2.3 of Section 7.1 of [I-D.ietf-oauth-selective-disclosure-jwt], which validates the Issuer and ensures that the signing key belongs to the Issuer, MUST be satisfied by determining and validating the public verification key used to verify the Issuer-signed JWT, employing an Issuer Signature Mechanism (defined in Section 3.5) that is permitted for the Issuer according to policy. Proposal Proposed to be amended as: In point 2.3 of Section 7.1 of [I-D.ietf-oauth-selective-disclosure-jwt], the Holder or a Verifier validates the Issuer and ensures that the signing key belongs to the Issuer. This check MUST be satisfied as follows: 1. Determine the public verification key used to verify the Issuer-signed JWT 2. Validate this public verification key 3. Use an Issuer Signature Mechanism (defined in Section 3.5) that is permitted for the Issuer according to policy NS10: Location: 4.2 Key Binding JWT: Paragraph 1Comments Passive voice is currently used, but it can be converted to active voice. Currently, it goes: MUST be ignored by the Verifier. Proposal It can be simplified as follows. Verifier MUST ignore. NS11: Location: 3.2.2.1 Verifiable Credential Type - vct Claim: Paragraph 2 Comments It says: A type is associated with rules defining which claims may or must appear in the Unsecured Payload of the SD-JWT VC and whether they may, must, or must not be selectively disclosable. It is using small case may, must, etc. Is it intentional? If it is intentional, then I would like to point out that it makes it very difficult to translate to the languages that have no notion of “case (capital/small, etc.)”. If it is a mistake, capitalize them. Proposal If it is not intentional, capitalize them. If it is intentional, then perhaps rewrite as: A type is associated with rules defining which claims are permitted or required to appear in the Unsecured Payload of the SD-JWT VC and whether selective disclosure is permitted, required, or prohibited for those claims. NS12: Location: 6.3.2 From a Registry: Paragraph 1Comments It says: The registry MUST be a trusted registry, i.e., the Consumer MUST trust the registry to provide correct Type Metadata for the type. “The registry MUST be a trusted registry” is a little unclear. Is it enough that the Consumer trusts it? What is the quality that needs to be satisfied by the registry to be trusted? Proposal Clarify who needs to trust the registry. NS13: Location: 7. Document integrity: Last paragraphComments It says: Section 3.3.5 of [W3C.SRI <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#W3C.SRI>], but 3.3.5 does not exist in [W3C.SRI <https://www.ietf.org/archive/id/draft-ietf-oauth-sd-jwt-vc-13.html#W3C.SRI> ]. Proposal Point to the correct section. NS14: Location: 8.1.2.2 SVG RenderingComments It states: If the svg_id is not present in the claim metadata, the consuming application SHOULD reject not render the SVG template. Perhaps “and” is missing between “reject” and “not”. Proposal Insert “and”. NS15: Location: 9.2 Claim Display MetadataComment An example showing locale would be very useful here to assist understanding. Since it is given in B.2, perhaps refer to it. Proposal At the end, add “See B.2” for example. NS16: Location: 9.4 Claim Selective Disclosure Metadata: Comments Having a pointer to an example would be good. Proposal At the end, add “See B.2” for example. NS17: Location: 9.5.2 Example for Extending Type Metadata: Comment A multi-locale example would be appreciated, especially mixing full locale ones like ja-Han-JP and ja-Kana-JP with en-Latn-GB, etc. Since the example in B.2 comes close to it, expand it and refer to it. Proposal Expand the example in B.2 to include a full three-segment locale and refer to it from this section. NS18: Location: 10.2 Ecosystem-specific Public Key Verification Methods: Paragraph 2Comments It states: “It MUST be ensured “ but it is not clear who ensures it. If it can be clarified, it would be nice. Proposal Clarify. NS19: Location: 10.5 Risks Associated with Textual Information: Comments Just a thought: Could a Unicode similar character or composition, rtl, etc., combination attack be possible here? If so, mentioning it might be a good idea. Proposal If it is determined that it would constitute a risk, then mention it. NS20: Location: 10.6 Credential Type Extension and Issuer Authorization: Last paragraphComments It goes: “Verifiers and wallets SHOULD implement explicit checks for issuer authorization and SHOULD NOT rely on type extension as a proxy for trust or legitimacy.” Is SHOULD and SHOULD NOT enough? Proposal If yes, do nothing. If no, then upgrade them to MUST and MUST NOT. NS21: Location: New 10.9 Metadata Time-Based RiskComments I feel that there could be an attack leveraging on the difference on the validity time of the data and metadata. What is stated in Proposal is what I felt like a potential risk. (Note: I am writing this at 3 AM so I might be completely off…) Proposal If the following risk is real, then add something in line of the following: The Metadata Time-Based Risk occurs because a Verifiable Credential (VC) is composed of two parts with separate expiration dates: - The Credential (SD-JWT VC): Has a strict expiration date set by the exp claim. After this time, the VC is invalid. - The Type Metadata: This is a separate file (e.g., a JSON document) that tells the Verifier how to display or process the VC. It is retrieved from a URL and relies on web caching rules (like Cache-Control headers) to determine how long it's fresh. The risk is that the metadata's cache time is longer than the Issuer intended for the VC's security rules, or longer than the VC's own lifespan. This decoupling creates a security risk. The risk manifests when the Issuer updates the rules, but the Verifier relies on an old, cached copy: - Security Policy Bypass: The Issuer might update the metadata to change a claim from optional to mandatory for security reasons. If the Verifier uses an old, cached copy, it allows a weaker (less secure) presentation to be accepted. - Outdated Rendering: The Issuer might update display metadata (e.g., to add a security warning or correct a regulatory label). A Verifier using an old cache will display incorrect or misleading information to the user. - Tracking Vector: If the Issuer uses very short cache times, it could force Verifiers to constantly contact their server, which allows the Issuer to track where and how often the VC is used (the "Issuer Phone-Home" issue). (See 11.3 as well.) To mitigate: - When relying on cached Type Metadata, the Verifier MUST check the metadata's age against the credential's exp claim. If the VC expires sooner than the cache, the Verifier must treat the cache entry as expired. - The Issuer SHOULD use cryptographic integrity checks (like the #integrity hash, defined in Section 7) for all external metadata references. This ensures that even if the cache is old, the data hasn't been maliciously altered. NS22: Location: New 11.4 Holder Disclosure RiskComments Something about over-disclosure probably should be mentioned. Proposal Add the following as New 11.4 Holder Disclosure Risk Holders SHOULD provide clear interfaces to mitigate the risk of accidental over-disclosure. The interface SHOULD articulate which claims are being revealed versus hidden during presentation setup and to whom. NS23: Location: New 11.5 Display/Rendering Metadata LeakageComments Display metadata can be used to track the Holder. Proposal Add new subsection 11.5 Display/Rendering Metadata Leakage with the following text. Display metadata (Section 8) SHOULD NOT contain unique image or URL identifiers that could be used for Holder Fingerprinting when resources are fetched. Holder implementations SHOULD mitigate this risk by caching common resources or normalizing retrieval requests NS24: Location: New 12 Equity considerationsComments While it is not common in an IETF document, having equity considerations might be a good idea since this technology is also envisioned to be deployed in national infrastructures. Proposal Add a new section on Equity Considerations to address potential barriers and biases: - Language and Localization: Issuers MUST prioritize supporting relevant languages using the locale property (Section 9.2) to ensure equitable access and understanding of claims for all Holders. - Accessibility: Rendering mechanisms, particularly complex ones like svg_templates, SHOULD adhere to recognized accessibility standards (e.g., WCAG) to ensure compatibility with assistive technologies. - Technological Inclusion: Implementations SHOULD ensure cryptographic requirements are compatible with a broad range of hardware to prevent the exclusion of users with older or low-end devices. NS25: Location: 12.1 Privacy-Preserving Retrieval of Type MetadataComments This subsection does not seem to belong to “12. Relationships to Other Documents”. It should perhaps be moved to privacy considerations (which, by the way, I have already commented on before reaching here). Proposal Move to the privacy considerations section. NS26: Location: 13.1 Normative referenceComments RFC2397 is only referenced with SHOULD as “Binary data in claims SHOULD be encoded as data URIs as defined in [RFC2397].” in 3.2.2.3. Proposal [RFC2397] should be moved to Informative References (Section 13.2). NS27: Location: A.1 JSON Web Token Claims RegistrationComments There is a formatting error. There should be a line break before “Claim Name: "vct#integrity” Proposal Insert a line break NS28: Location: B.2 Example 2: Type MetadataComments If SD-JWT VC payload could be turned into multi-lingual, it would be great. (The metadata example already is, so it will show the mapping more clearly.) Proposal Make the example multi-lingual.
_______________________________________________ OAuth mailing list -- [email protected] To unsubscribe send an email to [email protected]
