kaleido-io / kaleido-iden3-samples

Sample code for using the iden3 protocol to issue verifiable claims
Apache License 2.0
5 stars 4 forks source link

Comment on the GDPR note in Readme #10

Closed OBrezhniev closed 1 year ago

OBrezhniev commented 1 year ago

Hello @jimthematrix, @Chengxuan!

You have a following note in the Readme file:

Note that the state published to the smart contract is the hash of the latest Merkle trees: hash(claims_root, revocation_root, roots_root). This means that care should be taken when designing the claim schemas. No PII should be part of the claim because a hash of PII is still considered PII under some regulations such as GDPR (details available here).

In the protocol we have two ways of issuing credentials: using MerkleTree Proofs (MTP) and using Babyjubjub signature (Sig). Only claims issued using Merkletree proofs are added to Claims Tree and end up in Identity State Hash. Claim issued using BJJ signature are not added to Claims Tree, and do not change Identity State. So there are no GDPR problems with this way of issuing credentials.

Credentials can (and should) include PII. We want users to be able to prove some statements based on the raw data, like date of birth or passort data. Care should be taken not to add such credentials into Claims Tree.

jimthematrix commented 1 year ago

thanks @OBrezhniev for the feedback.

can I validate my understanding on this. I understand the different ways to prove that an issuer gave me a claim, by either showing a merkle proof of the claim in the issuers claims tree, or by showing an issuer's signature on the claim. And if you can present a signature you don't need to present a merkle proof.

I assume the circuit used to validate a claim by signature is credentialAtomicQuerySig? This is what is used in the samples code.

I see in the circuit credentialAtomicQuerySig the following logic:

    // verify issuer state includes issuerClaim
    component verifyClaimIssuanceIdenState = checkIdenStateMatchesRoots();
    verifyClaimIssuanceIdenState.claimsTreeRoot <== issuerClaimNonRevClaimsTreeRoot;
    verifyClaimIssuanceIdenState.revTreeRoot <== issuerClaimNonRevRevTreeRoot;
    verifyClaimIssuanceIdenState.rootsTreeRoot <== issuerClaimNonRevRootsTreeRoot;
    verifyClaimIssuanceIdenState.expectedState <== issuerClaimNonRevState;

and in particular these values represent the merkle tree and state after the issued claim has been added to the claims tree. This seems to run contrary to the idea that signature based proofs do not require the issued claims to be added to the issuer's claims tree. Did I understand it correctly?

Given that the circuit already checked for the claim's signature right before this step:

    // issuerClaim  check signature
    component verifyClaimSig = verifyClaimSignature();
    for (var i=0; i<8; i++) { verifyClaimSig.claim[i] <== issuerClaim[i]; }
    verifyClaimSig.sigR8x <== issuerClaimSignatureR8x;
    verifyClaimSig.sigR8y <== issuerClaimSignatureR8y;
    verifyClaimSig.sigS <== issuerClaimSignatureS;
    verifyClaimSig.pubKeyX <== issuerAuthPubKey.Ax;
    verifyClaimSig.pubKeyY <== issuerAuthPubKey.Ay;

would it make sense to remove the check for the claim being included in the merkle tree?

OBrezhniev commented 1 year ago

@jimthematrix

can I validate my understanding on this. I understand the different ways to prove that an issuer gave me a claim, by either showing a merkle proof of the claim in the issuers claims tree, or by showing an issuer's signature on the claim. And if you can present a signature you don't need to present a merkle proof.

You are right.

I assume the circuit used to validate a claim by signature is credentialAtomicQuerySig?

Yes.

I see in the circuit credentialAtomicQuerySig the following logic:

 // verify issuer state includes issuerClaim
 component verifyClaimIssuanceIdenState = checkIdenStateMatchesRoots();
 verifyClaimIssuanceIdenState.claimsTreeRoot <== issuerClaimNonRevClaimsTreeRoot;
 verifyClaimIssuanceIdenState.revTreeRoot <== issuerClaimNonRevRevTreeRoot;
 verifyClaimIssuanceIdenState.rootsTreeRoot <== issuerClaimNonRevRootsTreeRoot;
 verifyClaimIssuanceIdenState.expectedState <== issuerClaimNonRevState;

and in particular these values represent the merkle tree and state after the issued claim has been added to the claims tree. This seems to run contrary to the idea that signature based proofs do not require the issued claims to be added to the issuer's claims tree. Did I understand it correctly?

Comment on the top of this block is incorrect. Probably copied from the other block that was doing it. This particular code is verifying that provided issuer state matches with the roots of trees provided to prove non-revocation of the claim. It's not for verifications of issuance of the claim.

And you are right that the following code is verifying correctness of issuer's signature of the claim:

  // issuerClaim  check signature
 component verifyClaimSig = verifyClaimSignature();
 for (var i=0; i<8; i++) { verifyClaimSig.claim[i] <== issuerClaim[i]; }
 verifyClaimSig.sigR8x <== issuerClaimSignatureR8x;
 verifyClaimSig.sigR8y <== issuerClaimSignatureR8y;
 verifyClaimSig.sigS <== issuerClaimSignatureS;
 verifyClaimSig.pubKeyX <== issuerAuthPubKey.Ax;
 verifyClaimSig.pubKeyY <== issuerAuthPubKey.Ay;

This might be a bit confusing at first, but we are providing as inputs to this circuit:

Most of these inputs are private. Only a few ones are public and revealed to the verifier to validate: query params, identity states, challenge, timestamp, user id (in the coming release will be substituted by profile id, which user can generate from user id to prevent tracking between issuers & verifiers).

credentialAtomicQuerySig circuit is very similar, but it's not checking bjj signature of the claim and issuer's auth claim validity. Instead it checks mtp of the requested claim's inclusion in issuer's claims tree at the time of issuance.

jimthematrix commented 1 year ago

thanks @OBrezhniev for the detailed response.

For all of the fields related to the issuer's latest state, such as mtp of issuer's auth claim inclusion in issuer's claims tree; or current issuer's identity state, used to prove non-revocation of it's auth claim and requested claim;, all require the latest root of the issuer's claim tree, which is a hash (of hashes) of the auth claim and the requested claim. And to validate the proof, one of the required public inputs is the identity states, which is the hash of the tree roots. Is that understanding correct?

OBrezhniev commented 1 year ago

@jimthematrix Yes, but mtp of issuer's auth claim inclusion in issuer's claims tree could be (but doesn't have to be) generated against the latest issuer state. It is bundled with the claim signature at the time of claim issuance, and user wallet doesn't have to update it. In circuits we are checking that as some point in time it was valid for this issuer and old state is enough for this purpose. But it's non-revocation proof should be recent one.

the issuer's claim tree, which is a hash (of hashes) of the auth claim and the requested claim.

If claims are issued only using BJJ signatures, then issuer's claims tree would have only it's own auth claim(s) there. Revocation tree also doesn't store any PII, because revocation nonce of the claim is not PII. And Roots tree stores all previous claim tree roots, so as long as claims tree has no PII, it has no too.

jimthematrix commented 1 year ago

Thanks for the further clarification Olek. So in the sample code we did add each issued claim to the issuer's claims tree, which was based on the test code for the credentialAtomicQuerySig circuit, https://github.com/iden3/go-circuits/blob/v1.0.1-alpha.2/credentialAtomicQuerySig_test.go#L113. I think it'd be helpful if the test code is updated to not do that, so it properly tests the logic paths for the circuit where the issuer state stays the same after new claims are issued, and avoid confusion for anyone (like myself) who uses the test code as a reference to use the go-circuits library.

In the meanwhile, I'll update the sample code to not add the issued claims to the issuer's claims tree.

Note that I've already removed the snippet in the readme regarding the hashes. I'll add back some content clarifying the fact that the PIIs in the claims are not added to the claims tree when the proof generation and verification are based on the credentialAtomicQuerySig circuit.

OBrezhniev commented 1 year ago

Thanks @jimthematrix !

the test code for the credentialAtomicQuerySig circuit, https://github.com/iden3/go-circuits/blob/v1.0.1-alpha.2/credentialAtomicQuerySig_test.go#L113

Yeah, looks like that is a copy-paste from the mtp test. We will review the tests before the next release.