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

wip: working on credentials/verify conformance #400

Merged
merged 11 commits into from
Sep 20, 2022

Conversation

brownoxford
Copy link
Collaborator

@brownoxford brownoxford commented Sep 13, 2022

This PR is the beginning of the work to add conformance testing for the /credentials/verify endpoint.

Right now, the problem I'm trying to solve is that mutating the credential properties in the VC results in the signature being invalid, causing negative tests to fail for one of two reasons - either the input fails validation before the signature is checked, or the signature checks fail.

We need for there to be a single reason for test failure, and have to eliminate the bad signature by coming up with a way to sign invalid credentials. We definitely don't want verification to succeed if a 3rd party presents a malformed credential with a valid signature.

Currently, I am able to do that by using the @digitalbazaar/vc package internals, bypassing much of the input validation code. I'm running into trouble with id and type fields, for which the JSON-LD parser definitely does not like invalid values.

If anyone has any suggestions for how to proceed, I would love to hear them.

My current plan is to pre-generate VCs for the test suite using the generate-testdata.js script. The commented-out test cases in that file represent the items with which I am having trouble.

@brownoxford
Copy link
Collaborator Author

This is related to w3c-ccg/vc-api-verifier-test-suite#26

}

{
const invalidValues = {
Copy link

@aljones15 aljones15 Sep 14, 2022

Choose a reason for hiding this comment

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

BTW this looks like a good case for a map:

const invaludValues = new Map([
  ['issuer:array', ['did:example:123']],
 ['issuer:boolean', false]
])

then you get an iterable by default

Also you might want to change the VC context here to allow for the invalid values

  "issuer": {"@id": "cred:issuer", "@type": "@id"},
   "issuer": {"@id": "cred:issuer", "@type": "xsd:boolean"}

Basically create an invalid VC context to allow for invalid values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aljones15

When I modify issuer in the context in this way, and try to set issuer: {"id": false} in the credential, I still get the same error I get without modifying the context:

JsonLdError [jsonld.SyntaxError]: Invalid JSON-LD syntax; "@id" value must a string.

Note that setting issuer: false does not cause any errors, nor does issuer: {"id": "did:web:example.com"}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This appears to be working:

        context['@context'].VerifiableCredential['@context'].issuer = {
          '@id': 'cred:issuer',
          '@type': '@id',
          '@context': {
            id: 'xsd:boolean',
          },
        };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mutating any type property or the @context to anything invalid continues to prove difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mutating any type property or the @context to anything invalid continues to prove difficult.

I just have to say ... that's great! :P ... the above is a sign of a good implementation / reasonably safe APIs. We want it to be hard to create invalid stuff -- since the only use case is a negative test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dlongley -- Your turn to have an un-fenced @context in the quoted content that starts #400 (comment). Please edit to add code fences.

@brownoxford brownoxford marked this pull request as ready for review September 16, 2022 20:20
@brownoxford
Copy link
Collaborator Author

brownoxford commented Sep 16, 2022

I'm moving this out of draft and making it ready for review. The current implementation provides a script that will regenerate a number of malformed (but validly signed) verifiable credentials for use with various bad request tests of the /credentials/verify endpoint.

VC generation is idempotent, and generated VCs are injected into the Postman testing suite to allow for easy regeneration of test request bodies. Note that updated postman JSON is output to stdout and must be captured and saved to the conformance_suite.postman_collection.json conformance suite JSON file.

Most mutations are able to be tested, but a number of mutations that change the @context or any type properties to bad values are not currently possible, largely because the RDF canonicalization algorithms simply will not work, and no signature can be generated.

Copy link
Collaborator

@mkhraisha mkhraisha left a comment

Choose a reason for hiding this comment

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

There are a number of commented-out lines, which will make it difficult going forward for anyone to delete/modify because they will be scared to remove those lines. I think a better path forward is to remove those comments.
Unless you believe a forthcoming PR that is reasonably soon will tackle those test issues.

"id": "did:example:123"
},
"issuanceDate": "2006-01-02T15:04:05Z",
"issuer": "did:web:example.com",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This won't resolve (obviously), do we want to maybe use an issuer that would resolve as a valid example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm. The goal of this request is to fail in the right way. If we think that an implementation might fail because the issuer cannot be resolved, then I would say yes, we should change this to something that will resolve. Suggestions welcome.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree 100% - we should use did:key for the issuer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue #404 has been created to track this change.

@brownoxford brownoxford merged commit 7fb9e9f into w3c-ccg:main Sep 20, 2022
@brownoxford brownoxford deleted the 398-credentials-verify-conformance branch September 20, 2022 18:00
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.

6 participants