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

Add new changes from AnonCreds RS to AFJ #1371

Closed
2 of 3 tasks
TimoGlastra opened this issue Mar 7, 2023 · 12 comments · Fixed by #1427
Closed
2 of 3 tasks

Add new changes from AnonCreds RS to AFJ #1371

TimoGlastra opened this issue Mar 7, 2023 · 12 comments · Fixed by #1427
Assignees
Milestone

Comments

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Mar 7, 2023

  • Revocation override
  • prover_did / entropy changes
    • new field entropy added, interface has changed.
    • Need to extend the AnonCredsHolderService interface to add useLegacyProverDid boolean.
    • In the IndySdkHolderService we need to throw an error when useLegacyProverDid is false/undefined.
    • in the AnonCredsRsHolderService we need generate either the legacy prover did or entropy. (need to think about randomness). optionally generate public key, derive the prover_did based on it (but don't store the private key)
  • fromJson / toJson changes to interface
@TimoGlastra TimoGlastra converted this from a draft issue Mar 7, 2023
@TimoGlastra
Copy link
Contributor Author

@genaris are we missing any changes here? You already had some local code for the fromJson / toJson right? Can you also pick up the prover_did / entropy and revocation override changes or do you want one of us to pick that up?

@TimoGlastra
Copy link
Contributor Author

Otherwise @Vickysomtee could pick up the revocation override and prover_did / entropy changes, but it would be good if you can pick up the fromJson / toJson stuff

@Vickysomtee
Copy link
Contributor

I can pick up the revocation override and prover_did/entropy changes

@genaris
Copy link
Contributor

genaris commented Mar 7, 2023

Don't worry, I'll do both

@TimoGlastra TimoGlastra assigned Vickysomtee and genaris and unassigned Vickysomtee Mar 7, 2023
@genaris
Copy link
Contributor

genaris commented Mar 8, 2023

prover_did / entropy changes

  • new field entropy added, interface has changed.
  • Need to extend the AnonCredsHolderService interface to add useLegacyProverDid boolean.
  • In the IndySdkHolderService we need to throw an error when useLegacyProverDid is false/undefined.
  • in the AnonCredsRsHolderService we need generate either the legacy prover did or entropy. (need to think about randomness). optionally generate public key, derive the prover_did based on it (but don't store the private key)

In regards to this, I'm struggling a bit because anoncreds-rs is always requiring me to provide an entropy, even if i use identifiers like LjgpST2rjsoxYegQDRm7EL or TL1EaPFCZ8Si5aUrqScBDt, but besides this issue I would like to know how this useLegacyProverDid is expected to be used.

I mean, in current LegacyIndyCredentialFormatService should we always set this flag to true when calling holderService.createCredentialRequest, even if the issuerId of the credentialDefinition is a fully qualified did:indy identifier?

If that's the case, according to current anoncreds-rs behaviour (which requires entropy for new identifiers) I think we should check the issuerId in AnonCredsRsHolderService and:

  • If it's a new identifier, generate entropy (always). If useLegacyProverDid is set I think it should throw an error
  • If it's a legacy identifier, generate either entropy or proverDid according to useLegacyProverDid

Am I missing something?

@TimoGlastra
Copy link
Contributor Author

If it's a new identifier, generate entropy (always). If useLegacyProverDid is set I think it should throw an error
makes sense

If it's a legacy identifier, generate either entropy or proverDid according to useLegacyProverDid

also makes sense

I mean, in current LegacyIndyCredentialFormatService should we always set this flag to true when calling holderService.createCredentialRequest, even if the issuerId of the credentialDefinition is a fully qualified did:indy identifier?

We shouldn't support non-legacy identifiers in the legacy indy format services (and should probably add a check for this)

@genaris genaris moved this from Todo to In Progress in Ledger Agnostic AnonCreds Mar 10, 2023
@genaris
Copy link
Contributor

genaris commented Mar 14, 2023

I mean, in current LegacyIndyCredentialFormatService should we always set this flag to true when calling holderService.createCredentialRequest, even if the issuerId of the credentialDefinition is a fully qualified did:indy identifier?

We shouldn't support non-legacy identifiers in the legacy indy format services (and should probably add a check for this)

Makes sense and I'm now adding checks in Legacy Indy Format Services in #1374, I think before (or soon after it's merged) it would be nice to have the new AnonCreds format services in #1168 and #1169. This way, folks using Cardano, Cheqd or other methods can use AnonCreds without any limitation. Are any of you working on this? Otherwise I think I can pick them up (AFAIK they should be nearly the same as Legacy with a few tweaks aligned to the new AnonCreds Aries RFC).

@TimoGlastra
Copy link
Contributor Author

No one from our side working on this currentlt

@genaris
Copy link
Contributor

genaris commented Mar 29, 2023

Revocation override

I'm not sure how to properly check if I need to provide overrides.

My initial understanding is that in AnonCredsProofFormatService.processPresentation() we should query the VDR to get the revocation status list for every involved revocation registry definition listed at proof's identifiers, array, not at the provided timestamp but at proof requests' non_revoked.from. The response from the VDR will return a timestamp, which may or not equal to non_revoked.from: if it does not (it's an earlier date), we add it to the overrides array.

Is this reasoning correct? If it is, how should I deal with local non_revoked fields (e.g. should I map a requested attribute to the rev_reg_id by looking at its restrictions to get its schema_id/cred_def_id?)? Any guidance to understand it better will be appreciated! 🤯

@swcurran
Copy link
Contributor

My suggestion was slightly different from that.

If the provided timestamp is between from and to in the proof request, then use the provided timestamp to query the ledger, and you should get the right accumulator/revreg entry.

If the provided timestamp is outside the from and to range and less than from, you should query using the from time. If you get the same revreg entry timestamp as the provided one — proceed with the verification as the presentation is in the range. If you don’t, then the the presentation does not meet the from and to range requirement.

This is logic outside of AnonCreds and it’s possible AnonCreds is never called, if the range check fails.

It is more interesting if the provided timestamp is after the to time. Strictly, it is a fail, but a verify might be OK with that if the verifier’s intention was to get the revocation status as of “now()”.

@TimoGlastra TimoGlastra added this to the 0.4.0 milestone Mar 30, 2023
@genaris
Copy link
Contributor

genaris commented Mar 30, 2023

My suggestion was slightly different from that.

If the provided timestamp is between from and to in the proof request, then use the provided timestamp to query the ledger, and you should get the right accumulator/revreg entry.

If the provided timestamp is outside the from and to range and less than from, you should query using the from time. If you get the same revreg entry timestamp as the provided one — proceed with the verification as the presentation is in the range. If you don’t, then the the presentation does not meet the from and to range requirement.

This is logic outside of AnonCreds and it’s possible AnonCreds is never called, if the range check fails.

It is more interesting if the provided timestamp is after the to time. Strictly, it is a fail, but a verify might be OK with that if the verifier’s intention was to get the revocation status as of “now()”.

Thanks @swcurran , I think your explanation clarified me the use case from a high-level perspective. Here I was trying to figure out how to use a new parameter that has been added to anoncreds-rs verifier that asks the library to accept some overrides (which will be used when the logic you said determines that it is good to continue the flow regardless of a difference between timestamp and the from in proof request's non_revoked).

Something that came up during today's AFJ call was: why the verifier does not always put an accurate from/to into proof requests? With accurate I mean that it can query the ledger to see what's the exact timestamp corresponding to the valid revocation status list at the from and to. At the end, it will do the same job but at the proof request creation time instead of doing so at the proof processing time.

For example: user wants to have a non_revoked interval between Monday and Wednesday. But it happens that there are revocation entries only Sunday and Tuesday. So why don't just use 'from Monday' and 'to Tuesday' in the generated Proof Request? Following that practice should solve the inconsistency in current libraries like indy-sdk, shouldn't it?

@swcurran
Copy link
Contributor

My $0.02CDN. I think the reason for not doing it is speed. Why make two (relatively expensive) calls to the ledger just to ask for something that the verifier probably doesn’t care about? In most cases, the verifier wants to know if the credential is revoked now(), and doesn’t really care about the interval. So they put from and to the same now() value. Forcing the verifier to use accurate times is likely not very helpful, in most cases.

@genaris genaris linked a pull request Apr 15, 2023 that will close this issue
@github-project-automation github-project-automation bot moved this from In Progress to Done in Ledger Agnostic AnonCreds Nov 22, 2023
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

Successfully merging a pull request may close this issue.

4 participants