-
Notifications
You must be signed in to change notification settings - Fork 516
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
Unrevealed attributes cause the verification to fail #1893
Comments
I think we should just remove the code from aca-py (that checks for unrevealed attribute values) and just use the anoncreds verification to set the "verified" flag. If there are unrevealed attributes we should set the verification to "true" (assuming the anoncreds verification verifies) and then let the controller decide whether to accept the proof (based on whether attributes are revealed or not or whatever other logic they decide). |
Note that AnonCreds behaviour, in this scenario, is to include the unrevealed attribute in "unrevealed_attributes" and provide neither the encoded nor raw value, for example:
In the above example |
Aca-py does the following pre-validation on received proofs:
The revocation interval validation has had quite a bit of testing, and a few bugs have been fixed. I'm reasonably confident that this logic is correct (and prevents potential malicious proofs). There are some scenarios that AnonCreds will accept that aca-py will reject (for example if a prover leaves out a non-revocation interval in a proof I believe AnonCreds will accept it - aca-py rejects this scenario as a proof of non-revocation was requested). The "overall validity of the proof" hasn't had as many eyes on it. The "revealed" vs "unrevealed" attributes is an error in aca-py (I believe), the question of whether aca-py should hi-light this scenario is an outstanding question. |
Ah -- I get it. Thanks for the clarification. I figured my previous thinking on that had to be flawed, since the "encoded" value was not always encoded in normal use -- e.g. in the case of integers. OK -- that is even better for some scenarios, such as what we were trying to do for the case where we (the verifier) is willing to accept credentials from one of N credentials that the person might have. If the prover is able to reveal the attributes from the credential they have, and put into the "unrevealed_attrs" the attributes from credentials they don't have, then we have a working solution for that 1 of N problem. My suggestion for the proper answer to this is:
|
I have a couple of questions/remarks here:
Generally I agree that the exchange should be valid if an attribute is not revealed, but I also think that the protocol needs to be extended so that the verifier has a way of communicating to the holder what attributes must be revealed. |
I agree with your general opinion about extending "the protocol" to allow a verifier to be more clear about what it means. The question is what protocol should be extended? At minimum, it should be at the Aries RFC level, I think it would be best done at the AnonCreds level (however, that will take a while). I do not think it should be done unilaterally by ACA-Py, Looking at RFC 0347, I think that if we want to go there, we should switch to the DIF Presentation Exchange (at a quick glance, 0347 look quite similar). However, that is a pretty heavy weight change for a relatively subtle change. There may be other reasons for going to DIF PE, but I don't think this is the driver. Agree that the big question is 2. We've been marking these proofs as "not verified", and we are now proposing a change to that. Further, the AnonCreds behaviour is not well known. It also means that verifiers should begin the practice of checking what they are getting from a proof. Any verifier that does not know what Issuer to might get a proof from has to do that, to decide if they trust the issuer (e.g. an education credential). I don't agree that 3 is the right answer for a couple of reasons. I think that not revealing something (perhaps because you don't have a credential to populate the attributes) is a viable response, and doing so saves having to extend the in-flight credential and lots of tricky logic in negotiating. I think verifiers would be easier to implement if they send a problem report to holders if the don't accept the data was provided (for whatever reason...), and then allowing for retrying. I can't imagine what a verifier would do on receipt of a message from the holder saying "Hey, how about this instead?". I'm sure most would throw up their hands and send back a problem report. I don't agree that 4 is the right way as in many cases predicates can't be used. They only work with sortable integers so that the expressions are meaningful with the encoded values. My guess is that this has not been happening much because (a) most wallets don't support letting the user not reveal attributes (I recall one does), and (b) no wallets support responding to a proof for which you don't have all the attributes (which could/should be possible, I think). I also don't think really bad things would happen if we removed this restriction as existing controllers that need required data would error off when the attribute does not have an associated value. Not ideal, but we could publish the change as breaking. |
Discussed on the Aries WG call 2022.08.17. @ianco -- please go ahead with the change to NOT alter the AnonCreds verification when unrevealed attributes are included, and please add a new boolean and string (reason code) to indicate to the controller that the proof was cryptographically correct, but not complete. Thanks! |
I tested my previous use case and now it works correctly thanks a lot for this. |
What is currently happening?
If the verifier is ok in having a flexible proof presentation and would allow the prover to proove only some attributes, currently there isn't a great method to either indicate attributes as optional or to negotiate which attributes to include in a proof.
If the prover simply doesn't reveal some of the requested attributes the verification of the proof will automatically fail.
Example case
If the prover doesn't want to reveal a specific attribute (the "surname" attribute in this example) but wants to follow through with the rest and using the Open API calls the send-presentation endpoint with the following data:
send-presentation call
This message has some attributes set as revealed True and the "surname" to False.
The message send from the prover to the verifier is the following:
Presentation Message
This message contains information about the unrevealed attribute but not the encoded or raw value but rather the encripted value necessary to verify the proof. You can find this following the path:
presentation.proof.proofs.primary_proof.m.surname
When ACA-Py tries to verify the proof it fails while doing some checks both in the "check_timestamps" and "pre_verify" functions found here.
Nevertheless if I remove these 2 checks from ACA-Py then the verifier function "indy.anoncreds.verifier_verify_proof" found here returns true.
From what I understand even though the indy sdk considers this a correctly validated proof ACA-Py considers this as not valid.
Question
I have not ben able to find any definitive information on how not revealing attributes affects the verification of the proof in the Aries RFCs or in the indy sdk documentation. Should a proof be considered valid if the information to verify the signature is present and available but the verifier doesn't get all the encoded/raw values he requested?
If this functionality is open to interpretation for the verifier and he should choose what to do when this edge case arises can I just ammend the pre-check functions to fulfill my needs or can there be other unknown issues when adapting the check to my needs. i e, can I just deploy the cloudagent with the code modified so that it accepts these proofs?
Proposal
If its decided that the proof should be valid even if there are unrevealed attributes, the pre-check funcions should be ammended so that they take into consideration that some attributes might be in the "unrevealed_attrs" section.
If its not clear if the proof should be considered valid or not maybe it would be possible to add more values than just "verified", there could be another value like "validated" or something else that would only check the correctness of the mathematical signatures of the proof.
The text was updated successfully, but these errors were encountered: