Credential format profiles: 'claims' definitions a̶r̶e̶ ̶b̶r̶o̶k̶e̶n̶ should be improved

[danielfett]

Right now, the definitions (e.g., this) for claims/credentialSubject in the credential format profiles are broken (as pointed out earlier).

At least the following problems exist:

• There is no way to distinguish between a claim named mandatory and the syntax element mandatory (same for display etc.).
• There is no way to describe, e.g., the address claim and the address/region sub-claim.
• The implementer has to parse the structure in order to figure out whether a certain object defines properties of the claim or contains nested claims. While this is possible, it is hard to implement.
• Handling of arrays of objects is undefined.

This is also related to this issue, both go back to not properly accounting for nested structures.

The problem exists in all three profiles defined in Appendix A.

[danielfett]

Some of the solutions discussed on the last call:

1. Instead of trying to mimic the structure of the credential, use pointers to the claims that are being described. There are multiple options:
   1. Use JSONPointer (this is what the current draft for SD-JWT VC Type Metadata uses).
   2. Use JSONPath (in my opinion this is not a good option due to complexity, potential security issues and JSONPath being a query language, not a pointer)
   3. Use arrays as pointers, e.g., ["address", "region"]. How array elements can be addressed would need to be defined.
2. Define that for each element, there is an object with the properties mandatory, display, but also the new property nested which may contain descriptions about nested objects.
3. At least for SD-JWT VC, an option would be to say that no properties can be defined directly in the issuer metadata, but the vct value can be used to find VC Type Metadata. A new element vctm could contain metadata documents as an array of base64url-encoded documents, similar to the glue documents described in the specification. Integrity protection would need to be figured out.

I personally would prefer 1.3 or 1.1 and provide an option for 3.

[paulbastian]

To help clarify the issue:

• Currently the claims value is a JSON object that mimics the nested structure of the credential
• The proposal is to make it a map, where nested structures are "flattened", hence the new structure could describe leafs but also parents

We favor Option 1.1, but we observe there might be some obscure use cases not covered by JSON Pointer (description for objects within arrays). Early implementation experience would be handy.

We also favor Option 3 and see 1 as the default fallback option if a credential format does not further specify detailed metadata.

[danielfett]

I created a PR on VC Type Metadata to see what complexity solution 1.3 proposed by @bc-pi would bring and I think it is comparable to or easier to implement than the JSONPointer solution considering that the JSONPointer solution would require a workaround for arrays and entail escaping etc..

This is what the metadata could look like: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#section-2

  "claims":[
    {
      "path":["name"],
      "display":{...}
    },
    {
      "path":["address"],
      "display":{...}
    },
    {
      "path":["address", "street_address"],
      "display":{...}
    },
    {
      "path":["degrees", null],
      "display":{...}
    }
  ],

And this is how to parse the path property (Sections 8 and 8.1): https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#section-8

(PR in SD-JWT VC Type Metadata)

[jogu]

I don't think we currently use 'null' JSON values anywhere in the VCI spec. I would argue that JSON nulls are generally problematic and should be avoided as many JSON libraries (at least by default) consider 'null' to be equivalent to 'not present'.

Perhaps we could follow the precedent set in the SD JWT spec with _sd_alg and so on and use something like _oid4vc_all_array_entries? Other possibilities might be -1, true, or {}.

[paulbastian]

Option 1.4:
rename mandatory and display to values with _mandatory and _display
(addresses only parts of the problems)

[selfissued]

For what it's worth, https://www.rfc-editor.org/rfc/rfc7592.html#section-2.2 treats null metadata values as a request to delete the metadata parameter - not as a literal value.

For similar reasons, the OpenID Federation spec states that null metadata parameter values MUST not be used.

This is rooted in Java and Python behaviors where "getting" a value from a structure returns null for values that are not present, rather than throwing an exception.

[bc-pi]

A null value here in the context of @danielfett's 1.3 is an array element, which is a different usage than the reasons we've (sometimes) avoided it in specs. So I kinda think it's fine here. But if we really feel that using null needs to be avoided, I'd argue that we still want to stay away from special names like "_oid4vc_all_array_entries" or similar. I think using -1or any non-positive number would be the way to go.

[danielfett]

I have a really hard time imagining how null could be a problem in this specific context, but -1 would be the second best option.

[awoie]

Some of the solutions discussed on the last call:

1. Instead of trying to mimic the structure of the credential, use pointers to the claims that are being described. There are multiple options:

   1. Use JSONPointer (this is what the current draft for SD-JWT VC Type Metadata uses).
   2. Use JSONPath (in my opinion this is not a good option due to complexity, potential security issues and JSONPath being a query language, not a pointer)
   3. Use arrays as pointers, e.g., ["address", "region"]. How array elements can be addressed would need to be defined.

2. Define that for each element, there is an object with the properties mandatory, display, but also the new property nested which may contain descriptions about nested objects.

3. At least for SD-JWT VC, an option would be to say that no properties can be defined directly in the issuer metadata, but the vct value can be used to find VC Type Metadata. A new element vctm could contain metadata documents as an array of base64url-encoded documents, similar to the glue documents described in the specification. Integrity protection would need to be figured out.

I personally would prefer 1.3 or 1.1 and provide an option for 3.

I would prefer option 2, introduce a nested property. It is the cleanest option and most intuitive option imo.

[bc-pi]

Agree it is a problem that should be addressed and support the proposed option 1.3 (aka 1.iii) "Use arrays as pointers." 

It also seems worthwhile to consider allowing for something along the lines of option 3.  And have SD-JWT VC Type Metadata also use an arrays as pointers approach.

[c2bo]

To help clarify the issue:

• Currently the claims value is a JSON object that mimics the nested structure of the credential
• The proposal is to make it a map, where nested structures are "flattened", hence the new structure could describe leafs but also parents

We favor Option 1.1, but we observe there might be some obscure use cases not covered by JSON Pointer (description for objects within arrays). Early implementation experience would be handy.

We also favor Option 3 and see 1 as the default fallback option if a credential format does not further specify detailed metadata.

Perhaps I am misunderstanding JSON Pointer, but from my understanding you can reference objects within an array?

Wouldn't this work?

{
  "data": [
    {
      "some": "value"
    },
    {
      "more": 42
    }
  ]
}

JSONPointer("/data/0/some") == "value"

I don't fully understand or see the benefit of going with the Option 1.3 over 1.1 but I might very well be missing something.

[danielfett]

The problem is that you might want to say "every element in that array". So "/data/*/some" (which is not valid JSONPointer). More like a query language...
_{So maybe this is an appropriate use of JSONPath (but that is still too complex and still potentially insecure).}

[javereec]

My preference would be

1. Prefix parameters to decrease collisions (e.g. __display) (option 2(-2))
2. Use nested attribute (option 2(-1))

It keeps things simple, closely linked to the attribute and straightforward to implement, improving adoption.

[danielfett]

Consensus in the call was to go ahead with solution 1.3 following the SD-JWT VC Type Metadata mechanism (and then collecting initial implementer's feedback before proceeding).

If everyone could please review this section in VC Type Metadata and provide feedback before I create the PR for VCI on Monday, that would be very helpful: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#name-claim-metadata

PR for commenting here: vcstuff/sd-jwt-vc-types#5

[jogu]

If everyone could please review this section in VC Type Metadata and provide feedback before I create the PR for VCI on Monday, that would be very helpful: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#name-claim-metadata

I presume we'll carry across the 'mandatory' and 'value_types' fields that are in the current VCI draft?

I'm not sure if it's clear whether or not the proposal is to add the 'sd' and 'verification' fields that are defined in the above link to VCI?

(And as discussed in the call, this change would mean we can remove the 'order' parameter that's in the current spec.)

[awoie]

We have been working on a similar problem in the context of a format-specific query syntax. Perhaps that work can help inform thinking and conversation around describing claim structures for different formats (IETF SD-JWT VC, W3C VCDM 2.0, and ISO mdocs) in JSON and vice versa. Here's a the link:

https://docs.google.com/document/d/10JT--pXWsfwC4QVu3XJpXGwcO08M6tnpkZ4PKdtCEWo

[danielfett]

If everyone could please review this section in VC Type Metadata and provide feedback before I create the PR for VCI on Monday, that would be very helpful: https://vcstuff.github.io/sd-jwt-vc-types/danielfett/fix-claims/draft-fett-oauth-sd-jwt-vc-types.html#name-claim-metadata

I presume we'll carry across the 'mandatory' and 'value_types' fields that are in the current VCI draft?

yes

I'm not sure if it's clear whether or not the proposal is to add the 'sd' and 'verification' fields that are defined in the above link to VCI?

I think we wouldn't use those.

(And as discussed in the call, this change would mean we can remove the 'order' parameter that's in the current spec.)

Which has its own problems.

[Sakurann]

we discussed this internally with Microsoft engineers. We do not currently have use-cases with the claims that have nested structure and array-based solution is too big of a breaking change. So our preference is actually approach 2 with defining the new property nested which may contain descriptions about nested objects.

As a compromise, my suggestion would be fixing the text that becomes ID-1 with nested approach. and than after ID-1, incorporating the array approach.

[Sakurann]

suggesting @jogu to do a non-normative PR explaining what does not work in the current structure, before PR #278 gets merged. ideally before March 18th to get into ID-1

[Sakurann]

@danielfett can you please change the issue name from "is broken" to something like "needs improvement"? Implementation feedback showed that the current structure works for majority of the current use-cases, so broken feels too strong and potentially misleading. thank you!