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

Unchecked revocation interval between Presentation and PresRequest #41

Closed
whalelephant opened this issue Jan 3, 2023 · 11 comments
Closed

Comments

@whalelephant
Copy link
Contributor

The NonRevocationInterval in the PresentationRequest on the request or the attribute/predicate level are not compared with the timestamp of which the prover has updated the RevocationState to in creating the Presentation.

Given the verifier only provides RevocationRegistry (aka Accumulator value) for the timestamps in the NonRevocationInterval, the required RevocationRegistry for the timestamp given in the Presentation will be missing and would not cause a revoked credential during that interval to be verified.
However, if the verifier provides RevocationRegistry mapped to timestamps outside of the interval,
a revoked credential can be verified.

This might also be related to the issued in #36, i.e. if the RevocationState is for a timestamp that is outside of the one defined in the PresentationRequest, then regardless if it has been revoked, the verifier might not supply the Accumulator value for such a timestamp.

@swcurran
Copy link
Member

swcurran commented Jan 3, 2023

The NonRevocationInterval has been a mess from the beginning.

  • It is not well understood what it is for
  • It's difficult for a holder to use,
  • It's difficult for a verifier to use, and
  • There is confusion with the interval used in the presentation request, and the interval that the holder uses in the request to the Indy ledger to get deltas.

AFAIK, the intention is for the verifier to say to the holder "I'll accept a recent Non Revocation Proof in this range", so that a holder with a cache of RevRegEntries might be able to generate the proof without going to the ledger. In practice, that's not particularly useful -- how often does a holder repeatedly present a credential such that the cache is sufficiently up to date for a verifier? Further, the interval is also used for point-in-time revocation checks so that the verifier can ask both "Is the credential revoked now?" (common case) and "Was the credential revoked on June 23, 2020 when the car accident occurred?"

If the verifier puts the "interval" as a point in time, what does the verification mean? It should be that the RevRegEntry (timestamp) used is the one that was active at the point in time. But if a later one is used, is that a bad thing? I guess it could be if it is the practice of the issuer to revoke and unrevoke credentials over time.

My suggestion is that we leave it as is for now, documenting the issues, and in a future version of AnonCreds, we look for a better solution.

@whalelephant
Copy link
Contributor Author

whalelephant commented Jan 4, 2023

This was helpful thank you @swcurran. It was confusing but examples here also cleared things up.

Regarding a few things you mentioned:

if it is the practice of the issuer to revoke and unrevoke credentials over time.

This is actually something discussed with @egidiocasati @TimoGlastra regarding a transitional revocation (more like a suspension) and a permanent revocation state. There are good reasons and real business needs to support both. We will be happy to discuss and outline some potential requirements and share for feedback.

the intention is for the verifier to say to the holder "I'll accept a recent Non Revocation Proof in this range"

With this being the intention, we should at least make the current code do as intended and document it, i.e. makes sure that the provided timestamp for the RevocationState in the Presentation is in between, after or before the given interval / timestamp. This might solve the ACA-PY issue if we update libindy as well. (see #42)
I will update specs and also the timestamp compare function and see what reviewers think. wdyt?

@swcurran
Copy link
Member

swcurran commented Jan 4, 2023

Sounds good.

Agree that revoking and unrevoking credentials is a viable use case and should be supported.

Idea: Perhaps we add a new output to the verification that leaves "verified" as True (if it is...), but notes if the revocation status is before, between or after.

What you propose is right. Note that I have a PR in for updating the presentation request section of the spec, so any change in the spec should be on top of that.

@whalelephant
Copy link
Contributor Author

Great! That PR on presentation request clearer mentioned your concerns here.

Re the addition status: I do not think the code should proceed to do cryptographic verification at all if the timestamp interval condition in not satisfied. It is likely that the unexpected timestamp does not map to a revocation registry provided by the verifier so it will fail anyway.

@swcurran
Copy link
Member

swcurran commented Jan 5, 2023

I'm not sure of that -- or I'm not understanding what you mean. Suppose the Verifier says "I will accept an interval between 5 days ago and now()", and the last RevRegEntry RevStatusList happened 7 days ago. Then the NRP is more than likely verifiable, the timestamp is not in the interval, but it is correct in that it is the one applicable to the interval. The timestamp in the presentation must be a RevStatusList entry. To verify the interval, the Verifier needs to know the history of the RevStatusList entries -- it must find the one that the holder used, discover it is before the range, and then query (with no query tools, in the case of Indy, at least) to see if there was a more recent RevStatusList that the holder should have used. The verifier may do that at a business level, but I don't think that the AnonCreds code should do that.

@whalelephant
Copy link
Contributor Author

Let me see if I can make this clearer:

Verification option:

  1. If the provided timestamp should be within the interval in the request, otherwise it should fail.
  2. The provided timestamp might not need to be in the interval in the request, whether to accept it or not should be the verifier's business logic (i.e. registry has not been updated so accepting latest) --> Example above

If case 2 is what should be supported, then the verifier should explicitly provide such instruction to the verification method to ignore the request interval. Otherwise it may verify due to unexpected presentation timestamps.

@swcurran
Copy link
Member

swcurran commented Jan 6, 2023

I don't think AnonCreds should fail verification if there is a verified NRP, even if the timestamp is outside the interval. To know that it is outside the interval, one must have a full RevReg history, and that is at best "difficult" to maintain by both the holder and verifier. For example, who would maintain the RevReg history and query the AnonCreds method to maintain it -- AnonCreds code or the business logic?

I think a warning would be good -- hence the suggestion of the additional data element. But for both sides to be precise is I think, very difficult.

If we can give guidance on how the verifier and holder can make sure the NRP is based on an in-interval timestamp, perhaps it can work, but I'm not seeing how it can be done.

This is why I think a better choice is for the verifier not to use an interval, but rather a point in time. But that's for the v2.0 discussions.

@whalelephant
Copy link
Contributor Author

OK, I understand that it is hard for sure to maintain it. Agree with v2, @egidiocasati had some thoughts on using some standard practise already exist.

I will keep this opened for now and return a warning to address this issue.

@egidiocasati
Copy link

Yep, thanks @whalelephant.

If we compare the RevocationList to a Certificate Revocation List (CRL), the Certificate Authority (CA) is required to publish the CRL at a predetermined frequency (usually every 24 hours) as specified in RFC5280, section 5.1.2.5.
The CRL includes a "nextUpdate" field which indicates when the next CRL will be published. If no certificates are revoked for a certain period of time, the CRL will contain the same list of revoked serial numbers for that duration. This is done to demonstrate that the CA revocation service is active.
In the case of a VC RevocationList, if no credentials are issued or revoked for a certain period of time, it is unclear whether everything is functioning properly or if the issuer is unavailable. This is an issue that primarily affects "institutional" issuers in a root of trust context, rather than web of trust, but it is still worth considering in v2. Other related topics I would suggest to include are transitional states like "on hold" and final states like "revoked" in CRLs, which allow for the management of various business scenarios with a high level of detail.

@whalelephant
Copy link
Contributor Author

As all following probably aware but just for future contributors, @swcurran has created another issue where the outcome for this version of nonrevoked interval check will be carried out.

whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 9, 2023
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 13, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 15, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 17, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
whalelephant added a commit to whalelephant/anoncreds-rs that referenced this issue Feb 17, 2023
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
@berendsliedrecht
Copy link
Contributor

@whalelephant can we close this or is it not resolved, yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants