Feat/oav3 dnsdid did revocation #158
Conversation
Co-authored-by: Raymond Yeh <ray@geek.sg>
* chore: refactored verifier into skip, test & verify * feat: document store verification & generating revoked docs * chore: update comment Co-authored-by: Raymond Yeh <ray@geek.sg>
Co-authored-by: Raymond Yeh <ray@geek.sg>
* feat: verification for dns-did * chore: refactored constraint checks * fix: lint * fix: test description Co-authored-by: Nebulis <nebounet@gmail.com> Co-authored-by: Raymond Yeh <ray@geek.sg> Co-authored-by: Nebulis <nebounet@gmail.com>
* chore: refactored into test verify skip * chore: refactoring test [wip] * chore: refractor test * chores: empty-test-for-v3 * chore: removed unnecessary tests * chore: refractor-test-case * chore: added-v3-test * chore: added-v3-verify * test: added v3-verify-test * chore: fix-lint * chore: refactored code Co-authored-by: Raymond Yeh <ray@geek.sg> Co-authored-by: Chen Ren <siachenren@gmail.com>
* chore: refactored previous verifier * chore: refactored test * chore: rename test & added v3 tests * feat: verify for v3 documents * fix: lint * fix: token registry's test & oav3 integration test (#157) * chore: integration test for all OAv3 documents * chore: better var naming * fix: lint * chore: added getFailingFragments to tests * fix: reasons for dnsTxt moved out Co-authored-by: Raymond Yeh <ray@geek.sg> Co-authored-by: Raymond Yeh <ray@geek.sg>
| @@ -119,12 +140,19 @@ const verifyV2 = async (document: SignedWrappedDocument<v2.OpenAttestationDocume | |||
| issuance, | |||
There was a problem hiding this comment.
we should probably add the details about revocation here
| const revocationStatus = | ||
| metaData.proof.revocation.type === v3.RevocationType.RevocationStore | ||
| ? await isRevokedOnDocumentStore({ | ||
| documentStore: metaData.proof.revocation?.address, |
There was a problem hiding this comment.
that's weird to use optional param here and not above (metaData.proof.revocation.type)
There was a problem hiding this comment.
metaData.proof.revocation.type for v3 is not optional though
its inconsistent with v2, for v2, issuers[x].revocation.type is optional.
There was a problem hiding this comment.
I mean
metaData.proof.revocation.typeL176metaData.proof.revocation?.addressL178
same object (metaData.proof.revocation), but once used with optional and not the other
There was a problem hiding this comment.
yea sorry am not reading correctly
| const validSignatureWithoutDnsTxt = { | ||
| ...baseDidDocument, | ||
| openAttestationMetadata: { | ||
| ...baseDidDocument.openAttestationMetadata, | ||
| identityProof: { | ||
| type: v3.IdentityProofType.DNSDid, | ||
| identifier: "notinuse.tradetrust.io", | ||
| }, | ||
| }, | ||
| }; | ||
| const wrappedInvalidDnsDidDocument = await __unsafe__use__it__at__your__own__risks__wrapDocument( | ||
| validSignatureWithoutDnsTxt | ||
| ); | ||
| const signatureForInvalidDocument = await wallet.signMessage( | ||
| utils.arrayify(`0x${wrappedInvalidDnsDidDocument.proof.merkleRoot}`) | ||
| ); | ||
| const signedInvalidDnsDidDocument: v3.SignedWrappedDocument = { | ||
| ...wrappedInvalidDnsDidDocument, | ||
| proof: { | ||
| ...wrappedInvalidDnsDidDocument.proof, | ||
| key: "did:ethr:0xE712878f6E8d5d4F9e87E10DA604F9cB564C9a89#controller", | ||
| signature: signatureForInvalidDocument, | ||
| }, | ||
| }; | ||
| writeFileSync( | ||
| "./test/fixtures/v3/did-revocation-store-invalid-signed.json", | ||
| JSON.stringify(signedInvalidDnsDidDocument, null, 2) | ||
| ); |
There was a problem hiding this comment.
Probably can leave these out. You probably want two files only:
- DID document with revocation store that is not revoked
- DID document with revocation store that is revoked
| return r?.type === v2.RevocationType.RevocationStore | ||
| ? isRevokedOnDocumentStore({ | ||
| documentStore: r?.address, | ||
| merkleRoot, | ||
| provider: options.provider, | ||
| targetHash, | ||
| }) | ||
| : Promise.resolve({ | ||
| revoked: false, | ||
| }); |
There was a problem hiding this comment.
This will result in a way to "unrevoke" my document. If I'm a recipient of document and I found out that the issuer revoked my document, I simply obfuscate my revocation type to skip the revocation check.
Should check that its NONE, else throw.
Might want to have a separate function to do this on top or something.
There was a problem hiding this comment.
but wont the previous error (lines 57-61) cover this vulnerability?
the code from 64-74 will only run if r.type is well defined.
separate function as in extract it out as a callback back function to make it neater?
There was a problem hiding this comment.
got it after discussion with @yehjxraymond
| proofs, | ||
| provider: options.provider, | ||
| }) | ||
| : { |
There was a problem hiding this comment.
Same vulnerability introduced
|
closing this, as I am going to open another PR due to the master moving ahead and I don't want to deal with the merge-conflicts. |
rough draft for logic and generate script want to have review early to see if correct implementation or not:
no test yet
commits: