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

implement vc verification #182

Merged
merged 8 commits into from
May 16, 2024
Merged

implement vc verification #182

merged 8 commits into from
May 16, 2024

Conversation

nitro-neal
Copy link
Contributor

@nitro-neal nitro-neal commented May 8, 2024

This pr does a change on how we process and verify VCs from a VcJwt:

This looks at registered claims fields and does a few checks:

  • If a required register claim does not exist, throw an error
  • If they don't exist in the underlying claims.vc field, then use the registers claims as the new value for the vc
  • If the VC had a value, make sure it matches. If there is a missmatch, throw an error

Here is the spec specific text:

   * Once the verifications are successful, when recreating the VC data model object, this function will:
   * - If exp is present, the UNIX timestamp MUST be converted to an [XMLSCHEMA11-2] date-time, and MUST be used to set the value of the expirationDate property of credentialSubject of the new JSON object.
   * - If iss is present, the value MUST be used to set the issuer property of the new credential JSON object or the holder property of the new presentation JSON object.
   * - If nbf is present, the UNIX timestamp MUST be converted to an [XMLSCHEMA11-2] date-time, and MUST be used to set the value of the issuanceDate property of the new JSON object.
   * - If sub is present, the value MUST be used to set the value of the id property of credentialSubject of the new credential JSON object.
   * - If jti is present, the value MUST be used to set the value of the id property of the new JSON object.

for reference:

This "minimum" vc object is valid according to the spec

{
  "vc": {
    "@context": [
      "https://www.w3.org/2018/credentials/v1"
    ],
    "type": [
      "VerifiableCredential"
    ],
    "credential_subject": {
    }
  },
  "iss": "did:jwk:eyJhbGciOiJFZERTQSIsImNydiI6IkVkMjU1MTkiLCJrdHkiOiJPS1AiLCJ4IjoiRE5NdDhrYUNPanlSVnk4QWhHbVVldm53RFFNRXNUdzlPck53NC0yNXRyVSJ9",
  "sub": "did:jwk:eyJhbGciOiJFZERTQSIsImNydiI6IkVkMjU1MTkiLCJrdHkiOiJPS1AiLCJ4IjoiRE5NdDhrYUNPanlSVnk4QWhHbVVldm53RFFNRXNUdzlPck53NC0yNXRyVSJ9",
  "exp": 1715370592,
  "nbf": 1715368792,
  "jti": "urn:vc:uuid:1302309c-0729-4a00-8f40-93df77d41889"
}

Notice how every field in the vc is missing like id and issuer, these are populated from the claims like jti, iss
We actually dont support "empty" and only support Default like empty string and 0, so we will need to fix this issuer tracked here #202

@nitro-neal nitro-neal marked this pull request as ready for review May 13, 2024 16:51
pub async fn verify_vcjwt(jwt: &str) -> Result<Arc<VerifiableCredential>, CredentialError> {
let jwt_decoded = Jwt::verify::<VcJwtClaims>(jwt).await?;
Ok(Arc::new(jwt_decoded.claims.vc))
fn validate_vc_data_model(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should be an instance method on the impl VerifiableCredential implementation, so maybe like

impl VerifiableCredential {
    // ...
    pub fn validate(&self) -> Result<(), CredentialDataModelValidationError> {
        // ...
    }
}

Which then brings into question, perhaps we should add *_vcjwt to the sign(), verify() and decode() functions. So that would be sign_vcjwt(), verify_vcjwt() and decode_vcjwt(). Rust doesn't support function overloading FWIW.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we've been down this path before

The thought was that the end user would get confused for
vc.validate()
vc.verify()

so the thought was to have a verifyOptions object, which you can have to only do validatePayload vs check specific verification vs all:
decentralized-identity/web5-js#425

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay cool, I may make a follow up GitHub Issue for this matter -- specifically, for the endeavor of maximizing modularity here at the rust core layer I think it's fitting to offer every single one of these little verification/validation checks as its own function. my thought process is a barbell design, wherein verify() does everything, but then we expose public facing methods for each individual one, so if developers want to create their own set of verification rules then they string the atomic ones together

@KendallWeihe
Copy link
Contributor

this is really great work! I appreciate how you created follow-up ticket items. I've added a handful of comments probably worth addressing

Copy link
Contributor

@KendallWeihe KendallWeihe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆

@KendallWeihe KendallWeihe merged commit 1bbeb4a into main May 16, 2024
6 checks passed
@KendallWeihe KendallWeihe deleted the semantic-vc-verification branch May 16, 2024 17:11
diehuxx pushed a commit that referenced this pull request May 21, 2024
* main:
  implement vc verification (#182)
  Remove unused credentials/tests, rename vc.rs to verifiable_credential.rs (#214)
  Refactor KeyManager (#204)
  Rename 'method' module to 'methods' (plural) (#210)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants