hyperledger / anoncreds-spec

The specification for AnonCreds verifiable credential exchange.
https://hyperledger.github.io/anoncreds-spec/
Apache License 2.0
45 stars 24 forks source link

Incorporate revocation best practices from Aries RFCs #132

Closed TimoGlastra closed 8 months ago

TimoGlastra commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Assumptions/Prerequisites

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 commented 1 year ago

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?

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 commented 1 year ago

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 commented 8 months ago

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