Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Conflict between VC Data Model specification and JwtCredentialPayload #136

Closed
3 tasks done
ankurdotb opened this issue Jun 1, 2023 · 13 comments
Closed
3 tasks done
Labels
bug Something isn't working stale

Comments

@ankurdotb
Copy link

ankurdotb commented Jun 1, 2023

Prerequisites

Please answer the following questions for yourself before submitting an issue.

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Current Behavior

The payload section of a JWT VC as defined in JwtCredentialPayload only retains the following fields after normalisation:

export interface JwtCredentialPayload {
  iss?: string
  sub?: string
  vc: Extensible<{
    '@context': string[] | string
    type: string[] | string
    credentialSubject: JwtCredentialSubject
    credentialStatus?: CredentialStatus
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    evidence?: any
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    termsOfUse?: any
  }>
  nbf?: number
  aud?: string | string[]
  exp?: number
  jti?: string

  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  [x: string]: any
}

This will skip values such as issuer, issuanceDate, expirationDate etc - anything not explicitly mentioned above.

Expected Behavior

According to the VC Data Model specification, Section 4.5 Issuer, these fields should be in the vc field and duplicated to native JWT fields such as iss, nbf, etc. Jump to Example 8 and switch to the "Verifiable Credential (as JWT)" tab:

--------------- JWT payload ---------------
// NOTE: The example below uses a valid VC-JWT serialization
// that duplicates the iss, nbf, jti, and sub fields in the
// Verifiable Credential (vc) field.

If you look at the JWT in that example:

eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJ2YyI6eyJAY29udGV4dCI6WyJodHRwczovL3
d3dy53My5vcmcvMjAxOC9jcmVkZW50aWFscy92MSIsImh0dHBzOi8vd3d3LnczLm9yZy8yMDE4L
2NyZWRlbnRpYWxzL2V4YW1wbGVzL3YxIl0sImlkIjoiaHR0cDovL2V4YW1wbGUuZWR1L2NyZWRl
bnRpYWxzLzM3MzIiLCJ0eXBlIjpbIlZlcmlmaWFibGVDcmVkZW50aWFsIiwiVW5pdmVyc2l0eUR
lZ3JlZUNyZWRlbnRpYWwiXSwiaXNzdWVyIjoiaHR0cHM6Ly9leGFtcGxlLmVkdS9pc3N1ZXJzLz
E0IiwiaXNzdWFuY2VEYXRlIjoiMjAxMC0wMS0wMVQxOToyMzoyNFoiLCJjcmVkZW50aWFsU3Via
mVjdCI6eyJpZCI6ImRpZDpleGFtcGxlOmViZmViMWY3MTJlYmM2ZjFjMjc2ZTEyZWMyMSIsImRl
Z3JlZSI6eyJ0eXBlIjoiQmFjaGVsb3JEZWdyZWUiLCJuYW1lIjoiQmFjaGVsb3Igb2YgU2NpZW5
jZSBhbmQgQXJ0cyJ9fX0sImlzcyI6Imh0dHBzOi8vZXhhbXBsZS5lZHUvaXNzdWVycy8xNCIsIm
5iZiI6MTI2MjM3MzgwNCwianRpIjoiaHR0cDovL2V4YW1wbGUuZWR1L2NyZWRlbnRpYWxzLzM3M
zIiLCJzdWIiOiJkaWQ6ZXhhbXBsZTplYmZlYjFmNzEyZWJjNmYxYzI3NmUxMmVjMjEifQ.oOoii
TBKC6zbJ8rx916SSHEKk4Fhc5-gFD8Qh64zRsf5ea7D-bu9zA1ii12hnXLloL7Cz0reXKA9P1nB
ZzQvTw

...the payload decodes to:

{
  "vc": {
    "@context": [
      "https://www.w3.org/2018/credentials/v1",
      "https://www.w3.org/2018/credentials/examples/v1"
    ],
    "id": "http://example.edu/credentials/3732",
    "type": [
      "VerifiableCredential",
      "UniversityDegreeCredential"
    ],
    "issuer": "https://example.edu/issuers/14",
    "issuanceDate": "2010-01-01T19:23:24Z",
    "credentialSubject": {
      "id": "did:example:ebfeb1f712ebc6f1c276e12ec21",
      "degree": {
        "type": "BachelorDegree",
        "name": "Bachelor of Science and Arts"
      }
    }
  },
  "iss": "https://example.edu/issuers/14",
  "nbf": 1262373804,
  "jti": "http://example.edu/credentials/3732",
  "sub": "did:example:ebfeb1f712ebc6f1c276e12ec21"
}

You'll see the iss value is copied in, and issuer is left intact under vc field.

The VC-JWT specification, which is a separate specification, also uses a very similar example where those fields are left intact under vc (Example 6 in the specification):

eyJraWQiOiJ1cm46ZXhhbXBsZTppc3N1ZXIja2V5LTAiLCJhbGciOiJFUzI1NiIsInR5cCI6InZjK2xkK2p3dCIsImN0eSI6InZjK2xkK2pzb24ifQ.eyJAY29udGV4dCI6WyJodHRwczovL3d3dy53My5vcmcvbnMvY3JlZGVudGlhbHMvdjIiXSwiaWQiOiJodHRwOi8vZXhhbXBsZS5lZHUvY3JlZGVudGlhbHMvMzczMiIsInR5cGUiOlsiVmVyaWZpYWJsZUNyZWRlbnRpYWwiLCJVbml2ZXJzaXR5RGVncmVlQ3JlZGVudGlhbCJdLCJpc3N1ZXIiOiJodHRwczovL2V4YW1wbGUuZWR1L2lzc3VlcnMvMTQiLCJpc3N1YW5jZURhdGUiOiIyMDEwLTAxLTAxVDE5OjIzOjI0WiIsImNyZWRlbnRpYWxTdWJqZWN0Ijp7ImlkIjoiZGlkOmV4YW1wbGU6MTIzIiwiZGVncmVlIjp7InR5cGUiOiJCYWNoZWxvckRlZ3JlZSIsIm5hbWUiOiJCYWNoZWxvciBvZiBTY2llbmNlIGFuZCBBcnRzIn19fQ.pfbhgWlTUZA8WmoFbi8WEIUFyC_lSQaAswoW87D1YeimdWZLq4MiJ3o-CmTkvkEQFhffvRiCzmkhxjS_R_RdOw

Which decodes to:

{
  "@context": [
    "https://www.w3.org/ns/credentials/v2"
  ],
  "id": "http://example.edu/credentials/3732",
  "type": [
    "VerifiableCredential",
    "UniversityDegreeCredential"
  ],
  "issuer": "https://example.edu/issuers/14",
  "issuanceDate": "2010-01-01T19:23:24Z",
  "credentialSubject": {
    "id": "did:example:123",
    "degree": {
      "type": "BachelorDegree",
      "name": "Bachelor of Science and Arts"
    }
  }
}

Failure Information

N/A

Steps to Reproduce

As above. This is more of a standards vs implementation point, for anything that does create credential.

Environment Details

N/A - affects all situations and is not environment dependent.

Discussion

  1. Both the VC Data Model and VC JWT specifications say fields like iss, nbf etc should be duplicated, but this implementation deviates from that. Which one is correct?
  2. If there's agreement, we're happy to create a PR to fix this, but IMO we need agreement first on whether the specification is wrong or the implementation.
  3. Prior discussions: expect transformCredentialInput to move top-level props of W3C credentials inside vc #73, [BUG] did-jwt-vc does not appear to handle expirationDate properly #102
@ankurdotb ankurdotb added the bug Something isn't working label Jun 1, 2023
@ankurdotb
Copy link
Author

I think I partially narrowed down where the confusion arises from:

However, the conflict between VC JWT specification and the implementation still exists as evidenced by the second example.

@mirceanis
Copy link
Member

This lib currently implements vc1.1 spec, which describes a mapping between jwt properties and the rest of the VC data model.

The spec does not mandate duplication of data, and for efficiency reasons, this library prefers to avoid duplication where possible. There is still some ambiguity, though. See also #64

@mirceanis
Copy link
Member

It's also worth noting that the JWT example in https://www.w3.org/TR/vc-data-model/#issuer contradicts the JWT spec which requires the iss property to be a string

@mirceanis
Copy link
Member

We added the removeOriginalFields option to methods dealing with creating tokens to allow you to duplicate the data.
It defaults to true, but you can set it to false to duplicate

@ankurdotb
Copy link
Author

@mirceanis Thanks for that look-through. My thoughts:

  1. IMO, the JWT on its own is what actually matters and it should have the full credential. Any of the plaintext VC in response is informational only.
  2. I agree that VC Data Model TR v1.1 is what this library should follow since it's the more stable specification.
  3. The conflict, then, as I see it is between the Section 4.5 Issuer, which suggests that the contents should be duplicated vs the JWT Encoding section which suggests it shouldn't. Section 4.5 is ambiguous in its example whether it's a requirement or a recommendation. Perhaps this was the idea behind dropping those examples from Section 4.5 working draft, since it's in conflict. Still, the current TR's conflict between 4.5 and JWT Encoding causes confusion.

Potential solution

Current behaviour

The behaviour of removeOriginalFields IMO should be slightly different. Right now, with removeOriginalFields = false, it does this:

{
  "@context": [
    "https://www.w3.org/2018/credentials/v1"
  ],
  "credentialSubject": {
  ...
  },
  "iss": "did:cheqd:testnet:553a68d3-e16d-4ea1-8f4b-ee17a317acd3",
  "issuanceDate": "2023-05-31T13:29:40.815Z",
  "issuer": {
    "id": "did:cheqd:testnet:553a68d3-e16d-4ea1-8f4b-ee17a317acd3"
  },
  "nbf": 1685539780,
  "sub": "did:vda:testnet:0x0D0BB3a44A37Da9C9ec6B49938375492c4c36323",
  "type": [
    "VerifiableCredential"
  ],
  "vc": {
    "@context": [
      "https://www.w3.org/2018/credentials/v1",
      ...
    ],
    "credentialSubject": {
      ...
    },
    "type": [
      "VerifiableCredential"
    ]
  }
}

You'll see the encoded JWT itself contains things like @context, credentialSubject etc twice: once at top-level, and again at vc level. The** vc level skips the fields replicate in JWT like issuer.

This kind of encoding in the payload seems to be not referenced in any of the specifications: in VC Data Model TR v1.1, VC Data Model v2.0, or in VC JWT.

Proposed behaviour

Instead, the removeOriginal = false fields could behave like this:

{
  "iss": "did:cheqd:testnet:553a68d3-e16d-4ea1-8f4b-ee17a317acd3",
  "nbf": 1685539780,
  "sub": "did:vda:testnet:0x0D0BB3a44A37Da9C9ec6B49938375492c4c36323",
  "vc": {
    "@context": [
      "https://www.w3.org/2018/credentials/v1",
      ...
    ],
    "issuer": {
       "id": "did:cheqd:testnet:553a68d3-e16d-4ea1-8f4b-ee17a317acd3"
    },
   "issuanceDate": "2023-05-31T13:29:40.815Z",
    "credentialSubject": {
      ...
    },
    "type": [
      "VerifiableCredential"
    ]
  }
}

Instead of duplicating the VC body twice, one at top-context and again under vc field, this would leave the issuer, issuanceDate etc under the vc field. This is exactly the example given in VC Data Model TR v1.1 and in VC JWT. It's taking the interpretation of duplication, vs removal (reconstructed by app from JWT fields).

If the value wasremoveOriginal = true fields would retain its current behaviour:

{
  "iss": "did:cheqd:testnet:553a68d3-e16d-4ea1-8f4b-ee17a317acd3",
  "nbf": 1685539780,
  "sub": "did:vda:testnet:0x0D0BB3a44A37Da9C9ec6B49938375492c4c36323",
  "vc": {
    "@context": [
      "https://www.w3.org/2018/credentials/v1",
      ...
    ],
    "credentialSubject": {
      ...
    },
    "type": [
      "VerifiableCredential"
    ]
  }
}

Here, the issuer field, issuanceDate etc are removed and moved to JWT-native fields.

@nklomp
Copy link
Member

nklomp commented Jun 1, 2023

I agree that the implementation is correct.

The spec example states for the JWT vc this:

NOTE: The example below uses a valid VC-JWT serialization
//       that duplicates the iss, nbf, jti, and sub fields in the
//       Verifiable Credential (vc) field.

That is one of the 2 options you have. Copy the respective JWT fields to the vc object, and depending on the type have it in a different format (dates), or only have them in the JWT fields. If they are in both, obviously they should match, which in reality multiple implementations do not, because for instance they are creating timestamps for the JWT property and the date for the vc object on separate lines.

Also the VCDM talks quite a bit about encoding and decoding, from which it become clear that you sort of should create in internal VCDM compliant representation from the JWT

@ankurdotb
Copy link
Author

@nklomp Sounds like it the current implementation of removeOriginalFields is perhaps then the issue (as in example above)?

@mirceanis
Copy link
Member

mirceanis commented Jun 1, 2023

There's definitely something that can be improved here.

The problem stems from the assumptions made early on by this library, which were that it can be used as a semi-drop-in-replacement for did-jwt, but also do some VC data model validation.
Because of this, methods for creating credentials and presentations also accept payloads that already look like the JWT encoding in the VCDM.

If the accepted payloads were strictly VCDM and not JWT, the logic should be this:

  • copy all properties of the payload to the vc property
  • create JWT specific properties according to the encoding section in the VCDM
  • optionally remove the duplicated properties from the vc

Things get more complicated when accepting JWT payloads as well, but I think we could have a heuristic to determine if the input is already a JWT payload (for example, by looking for a vc object property and a lack of a @context property at the top level) and then treating it by a different logic.

The way the code looks like now, it is treating both these cases in the same logic block, resulting in this weird behavior when removeOriginalFields is set to false.

@ankurdotb
Copy link
Author

Is the use case of being just passed the payload section of JWT common? Your logic might work if JWT payloads always nest the values under vc but I'm not sure whether that's logic everyone expects.

Wouldn't a better way be to always normalise top-level/final output like so regardless of whether removeOriginalFields is true or false):

  • vc or vp (if true, remove issuer etc) <- Don't care whether original payload was JWT or not
  • JWT specific fields (iss, nbf`, etc)
  • Remove anything else at the top level

BTW, a tangible example of where the VCDM fields as well as their replication in vc is left behind is the when doing veramo credential create.

@ankurdotb
Copy link
Author

(On a related note, I raised this issue on the VC JWT specification w3c/vc-jose-cose#100 separately because it relates to the header in VC JWTs, and I didn't want to keep that separate.)

@ankurdotb
Copy link
Author

@OR13 Do you have any thoughts on this one? Would appreciate your advice.

@OR13
Copy link
Contributor

OR13 commented Aug 11, 2023

I don't think v1 was implementable for JWT, at least in a way that was interoperable.

v2 essentially says , don't delete any required fields, don't use vc or vp and you can add any registered claims you like to the claimset.

I'd start heading that direction, but it's possible the target will have moved by the time you get there.

@stale
Copy link

stale bot commented Oct 20, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 20, 2023
@stale stale bot closed this as completed Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

No branches or pull requests

4 participants