search

hyperledger / aries-cloudagent-python

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
404 stars 511 forks source link

Prover cannot know verification outcome from PRESENTATION_ACKED #1754

Closed tdiesler closed 2 years ago

tdiesler commented 2 years ago

Other than with the [role=VERIFIER, state=VERIFIED] event that the Verifier receives, there is no verified flag on the event that the Prover receives. Hence, the Prover cannot know (from the event) whether verification was successful or not.

For quite some time I assumed that events [role=VERIFIER, state=VERIFIED] followed by [role=PROVER, state=PRESENTATION_ACKED] indicates successful verification. That is not the case. Instead, the Verifier needs to check the verified flag that comes with the VERIFIED event. The Prover always receives PRESENTATION_ACKED, but verified is not part of the event payload.

swcurran commented 2 years ago

If this is an addition (which it seems to be) to the data we have today, I agree that we should do this. Unfortunately, we likely have a bit of a semantic issue to deal with. AFAIK, the current "state=VERIFIED" means that the verification process has been run, and not the result of the verification process. So if we are adding a new item, it should be something like "verification_result=" and I assume a boolean -- true or false, based on the AnonCreds verification outcome. AFAIK (again), AnonCreds does not provide any details about why the verification passed/failed, just the true / false result.

swcurran commented 2 years ago

@tdiesler -- can you please review the PR that has been implemented for this? Does it solve the problem?

tdiesler commented 2 years ago

Events for topic present_proof get unmarshalled to a presentation exchange record. In the Java client this is BasePresExRecord and variations of this for V1 and V2.

the current "state=VERIFIED" means that the verification process has been run, and not the result of the verification process.

Yes, and with state=VERIFIED, we already have an attribute called verified that indicates the result of the verification process.

Is it not possible to use that attribute for state=PRESENTATION_ACKED as well?

The Java client (and possibly others) would not have to implement different logic for state=PRESENTATION_ACKED as they already have for state=VERIFIED

Application code would see a boolean for state=PRESENTATION_ACKED instead of 

prooverExchangeRecord.isVerified() == null

as it is now. If you prefer to stick to verification_result for state=PRESENTATION_ACKED we will have to change the Java client as well. The result could be awkward, because appcode would either see both isVerified() and isVerifiedResult() with the same meaning but initialized dependent on event state or just isVerified() with verified and verification_result on the wire respectively

tdiesler commented 2 years ago

Is this really fixed?

With 0.7.4-rc4 the prover can still not know whether the presentation was verified or not.

[@Alice] PROVER PRESENTATION_ACKED PresentationExchangeRecord(... verified=null, ...)
tdiesler commented 2 years ago

More context ...

I forced a credential that fails to verify (e.g. Alice graduated average=3, Acme wants average>=4)

The verifier sees the VERIFIED event with verified=false, which indicates an unsuccessful outcome. The prover (i.e. Alice) still sees the PRESENTATION_ACKED event with verified=null - she cannot know the outcome

[@Acme] VERIFIER VERIFIED PresentationExchangeRecord(... verified=false, ...)
...
[@Alice] PROVER PRESENTATION_ACKED PresentationExchangeRecord(... verified=null, ...)

Please reopen or explain why it is ok as it is