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

Incorporate revocation best practices from Aries RFCs #132

Closed
Tracked by #175
TimoGlastra opened this issue Jan 10, 2023 · 11 comments
Closed
Tracked by #175

Incorporate revocation best practices from Aries RFCs #132

TimoGlastra opened this issue Jan 10, 2023 · 11 comments

Comments

@TimoGlastra
Copy link
Member

Not sure if this has been discussed, but should we incorporate some of the best practices from the Aries RFC (https://github.com/hyperledger/aries-rfcs/blob/main/concepts/0441-present-proof-best-practices/README.md) into the AnonCreds spec?

Specifically about revocation intervals, date encodings, etc?

E.g. In AFJ we don't support requests where the from key is not 0 or equal to the to value (and to MUST always be defined in specifying a revocation interval)

@swcurran
Copy link
Member

Having a "from" of "0" makes little sense as it implies the holder can use the revocation entry active at the time of issuance and that would be "OK". That sounds like a big disconnect between ACA-Py and AFJ.

The "encoding" issue is talked about in the spec and the REQUIRED encoding algorithm is defined. An implementation of AnonCreds could ignore the encoded data from the issuer or error if the encoded data from the issuer doesn't match the required.

@TimoGlastra
Copy link
Member Author

Having a "from" of "0" makes little sense as it implies the holder can use the revocation entry active at the time of issuance

It could mean "Prove you ever had a non-revoked cred for this cred def". But now you that say it, it may not make too much sense.

@JamesKEbert Any ideas on why we support the from value of 0 explicitly (while not any other values if it's not equal to the to value)?

@JamesKEbert
Copy link

JamesKEbert commented Jan 13, 2023

This took a second to figure out the logic, but the AFJ implementation is actually conformant with the best practices. The following code block is what is used to determine if we should throw an error or not:

if (
    (requestRevocationInterval.from || requestRevocationInterval.from === 0) &&
    requestRevocationInterval.to !== requestRevocationInterval.from
) {

What this code does is that if from exists and if from does not equal to then throw an error. If from does not exist, then don't throw the error. What happened though while testing (which prompted the need for the 0 check), if from is 0 and to is 1000, then the check to see if from exists would be falsy, meaning no error would be thrown even though from did not equal to.

So, it doesn't actually explicitly support the from value of 0. A better way to express this would be:

if (
    typeof requestRevocationInterval.from !== 'undefined' &&
    requestRevocationInterval.to !== requestRevocationInterval.from
) {

It's also worth noting that when querying Indy, from must be 0 (which is different from the proof request from), in order to validate whether or not at time to the credential is revoked (since if the credential is revoked at 5 and the proof request's revocation interval is from: 6, to: 6, it will still be revoked in that interval). Can be somewhat confusing while implementing the behavior though.

I wonder if we can just re-name the proof request revocation interval to be a single value (if it's going to be combined anyways), such as notRevokedAt: number or something along those lines?

@swcurran
Copy link
Member

@JamesKEbert -- definitely agree that the fact that there is an "interval" in the presentation request that means one thing ("I will accept a non-revocation proof from between from and to") and another "interval" in the request sent to Indy that means something completely different ("Please send me the revocation deltas from from to to) adds to the confusion.

The goal of the 1.0 spec. is to define what exists today, with only changes being to support ledger-agnostic AnonCreds. As such, I'd say we can't make a change about interval in this version of the spec, but we should in the future.

@JamesKEbert
Copy link

The goal of the 1.0 spec. is to define what exists today, with only changes being to support ledger-agnostic AnonCreds. As such, I'd say we can't make a change about interval in this version of the spec, but we should in the future.

@swcurran I'd very much so agree with that--I didn't have the context to know what stage the spec is at (whether it be 1.0 or 2.0).

Followup question--does the current scope of the spec take into account the ledger read/write calls, or is that outside the scope of AnonCreds (meaning, read/write calls are more specific to the ledger being utilized, Indy being one such example)?

@swcurran
Copy link
Member

Re:

Followup question--does the current scope of the spec take into account the ledger read/write calls, or is that outside the scope of AnonCreds (meaning, read/write calls are more specific to the ledger being utilized, Indy being one such example)?

The spec. defines the inputs to and outputs from the AnonCreds operations. The AnonCreds Methods specs define how the specific implementations provide the inputs. So the AnonCreds Method for Indy describes how it will provide the inputs.

For example, the spec. says what must be provided to the Presentation generation process from a revocation registry — including the current state of all of the credentials (revoked or not). It is up to the AnonCreds Method to interact with the ledger as needed to get the required information and format as expected. So for Indy, the AnonCreds method must merge the deltas and construct the full state. For Cheqd (where the full state is on the ledger), the method just needs to get the state via a ledger read and get it into the correct state.

BTW — In order to support both full state and delta implementations, issuers will need to track the full bit array so that the AnonCreds method writing the Revocation Transactions can support either type of implementation. For example, the “last write” bit array, plus a pending revocations list. When a revocation registry write is done — the needed transaction can be created and written to the ledger.

@swcurran
Copy link
Member

I've got a presentation to use for the meeting Monday to discuss this.

@whalelephant @TimoGlastra -- could either of you check with what CredX (and/or Indy SDK) did about interval checking so we know the state before we started this round of refactoring?

@swcurran
Copy link
Member

Thought through what is needed, and I think interval could be accurately checked with only one VDR request.

Assumptions/Prerequisites

  • call the timestamp of a given RevRegEntry rr_ts - the ledger timestamp when the RevRegEntry is written.
  • A RevReg is ACTIVE for the period rr_ts to the the rr_ts of the next RevRegEntry written
  • An rr_ts is WITHIN a revocation interval (from, to) if it is ACTIVE at from OR if rr_ts is between from and to
  • We'll call the timestamp of the RevRegEntry used by the holder for its non-revocation proof holder_rr_ts
  • We'll call the timestamp of the RevRegEntry that is active at time from from_rr_ts

Calculation

With that, we know that to meet the interval requirement the RevRegEntry used by the holder must be in the range, or it is the one active at time from. Assuming that I think the following pseudo code would work without an extra call to the ledger.

if (`holder_rr_ts` is inclusively between `from` and `to`) then
      # Interval is correct
      Retrieve from VDR the RevRegEntry at `holder_rs_ts`
else
      Retrieve from VDR the RevRegEntry at time `from` calling the timestamp for the result `from_rr_ts`
      if ( `holder_rr_ts` <> `from_rr_ts` ) then
           INVALID and done
endif
# If we get here, we now know we have a valid interval
if ( non-revocation proof is valid) then
    VALID and done
else 
    INVALID and done

@TimoGlastra @whalelephant @blu3beri --- does that work for you?

I assume that the AnonCreds method must make the right call to the VDR using the right method.

@swcurran
Copy link
Member

Assuming I have the logic right in the previous comment, the next question is who does what in the flow? There is AFAIK, two (or three) pieces of code involved -- the verifier (which I would include for Aries to be both the Aries framework and the business logic), AnonCreds, and perhaps, the AnonCreds Method.

Of those participants -- who makes the call to the VDR (via the AnonCreds Method) to get the RevRegEntry needed?

  • If it is AnonCreds, it would be able to do the "In Interval?" calculation as outlined above.
  • However, if it is the verifier, then either it needs to pass into AnonCreds enough information for AnonCreds to determine "In Interval?", or the verifier makes the determination itself (and perhaps never calling AnonCreds).

In that last case, we are back to AnonCreds not having an opinion on whether or not the Holder provided timestamp is within the Interval. If the verifier wants a verification after retrieving the right RevReg, it must think that the "In Interval?" question has already been decided.

@whalelephant
Copy link
Contributor

To summarise the discussion on the call and follow ups with @TimoGlastra , on the Anoncreds side, the verifier will optionally be able to provide override interval from value during the verification process - NonRevokedOverride to inform the verification process to accept timestamps that are before the interval timestamp non_revoked.from.

NonRevokedOverride: [
  <revRegId>: {
    <non_revoked.from>: <override_timestamp>
    104: 90,
    100: 90
  }
]

@swcurran
Copy link
Member

I believe this has been incorporated sufficiently in the spec. Closing.

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

No branches or pull requests

4 participants