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

Add an integration test for mixed proof with a revocable cred and a n… #1672

Merged
merged 10 commits into from
Apr 5, 2022

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Mar 17, 2022

…on-revocable cred

Signed-off-by: Ian Costanzo ian@anon-solutions.ca

(the branch is incorrectly named against the wrong issue oops)

…on-revocable cred

Signed-off-by: Ian Costanzo <ian@anon-solutions.ca>
@ianco
Copy link
Contributor Author

ianco commented Mar 17, 2022

Good news and bad news - I've updated the aca-py proof validations to account for whether a credential is revocable or not.

BUT now the test fails in Indy or Askar.

Indy: ./run_bdd -t @T003-RFC0454

Aries      | 2022-03-17 19:00:39,110 aries_cloudagent.indy.sdk.verifier ERROR Validation of presentation on nonce=721429308203552123237572 failed with error
Aries      | Traceback (most recent call last):
Aries      |   File "/home/indy/aries_cloudagent/indy/sdk/verifier.py", line 72, in verify_presentation
Aries      |     json.dumps(rev_reg_entries),
Aries      |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/indy/anoncreds.py", line 1756, in verifier_verify_proof
Aries      |     verifier_verify_proof.cb)
Aries      | indy.error.CommonInvalidStructure

Askar: BDD_EXTRA_AGENT_ARGS="{\"wallet-type\":\"askar\"}" ./run_bdd -t @T003-RFC0454

Aries      |   File "/home/indy/aries_cloudagent/protocols/present_proof/v2_0/formats/indy/handler.py", line 160, in create_pres
Aries      |     requested_credentials=requested_credentials,
Aries      |   File "/home/indy/aries_cloudagent/protocols/present_proof/indy/pres_exch_handler.py", line 204, in return_presentation
Aries      |     revocation_states,
Aries      |   File "/home/indy/aries_cloudagent/indy/credx/holder.py", line 522, in create_presentation
Aries      |     rev_state=get_rev_state(cred_id, timestamp) if timestamp else None,
Aries      |   File "/home/indy/aries_cloudagent/indy/credx/holder.py", line 485, in get_rev_state

Signed-off-by: Ian Costanzo <ian@anon-solutions.ca>
@swcurran
Copy link
Contributor

Isn't that good news? We can fix it. We have a failed test -- the right place to start :-)

At least I hope we can fix it.

@ianco
Copy link
Contributor Author

ianco commented Mar 17, 2022

Isn't that good news? We can fix it. We have a failed test -- the right place to start :-)

At least I hope we can fix it.

If Indy/Askar is rejecting the proof then I'm not sure it will be so easy to fix :-(

@ianco
Copy link
Contributor Author

ianco commented Mar 18, 2022

I double-checked indy-sdk and I didn't find any tests with mixed proofs (including revocable and non-revocable creds). It may be worth adding a test to Indy (and/or Askar) to verify whether this is supported or not.
From the error messages it looks like the proof is being rejected by the underlying library.

@swcurran
Copy link
Contributor

I'm wondering if we can do some shenanigans in ACA-Py based on knowledge of the credential being revokable or not. What we want is for the business layer (above ACA-Py) to care if the credential is revokable or not as the verifier may not know when constructing the proof request. For example, a request for a university credential where two universities use the same schema, but one has a revokable credential, the other doesn't. To me, the verifier should always assume a revokable credential and still have things work if the holder presents a proof from a non-revokable credential.

@ianco
Copy link
Contributor Author

ianco commented Mar 18, 2022

I was digging into this a bit more and Indy is definitely rejecting the proof. I found this comment in the code so I suspect that this is a non-Indy supported use case:

        Irrevocable credentials constitute proof of non-revocation, but
        indy rejects proof requests with non-revocation intervals lining up
        with non-revocable credentials in proof: seek and remove.

@swcurran
Copy link
Contributor

Arrggh...

@andrewwhitehead -- thoughts on this? Can/should CredX change that?

I don't like this...

@ianco ianco added the 0.7.4 label Mar 24, 2022
@ianco ianco marked this pull request as ready for review March 24, 2022 22:39
@ianco
Copy link
Contributor Author

ianco commented Mar 24, 2022

Updated the integration test per my comment on Issue #1651 - when the non-revoked interval is specified per attribute (revealed or predicate) then the proof can contain mixed revocable and non-revocable credentials. (I think aca-py is cleverly "fixing" the proof before asking indy-sdk to validate).

If the non-revoked interval is specified globally (for the entire proof request) then all presented credentials must be revocable and non-revoked.

Signed-off-by: Ian Costanzo <ian@anon-solutions.ca>
Signed-off-by: Ian Costanzo <ian@anon-solutions.ca>
@ianco ianco requested a review from shaangill025 April 4, 2022 22:18
@codecov-commenter
Copy link

Codecov Report

Merging #1672 (71e5fdd) into main (9a69a59) will decrease coverage by 0.00%.
The diff coverage is 94.40%.

@@            Coverage Diff             @@
##             main    #1672      +/-   ##
==========================================
- Coverage   95.29%   95.29%   -0.01%     
==========================================
  Files         528      528              
  Lines       32933    33028      +95     
==========================================
+ Hits        31385    31474      +89     
- Misses       1548     1554       +6     

@ianco ianco enabled auto-merge April 5, 2022 16:30
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.

4 participants