openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
419 stars 510 forks source link

Update /revocation endpoints to anoncreds #2432

Closed usingtechnology closed 1 year ago

usingtechnology commented 1 year ago

Sub-task for #2383

Update the remaining /revocation/ endpoints to use anoncreds.

Some work has been done while updating /schemas (see #2427) and associated BDD Tests.

usingtechnology commented 1 year ago

I am having an extremely difficult time completing the mappings for this task.

Some are very straightforward but stumbling on something as basic as /revocation/registry/{rev_reg_id}. Attempting tp transform an anoncreds RevRegDef to a IssuerRevRegRecord should be straightforward but I am not seeing where fields line up. For instance, in anoncreds, we do have a RevRegDefState, that I could use to set the IssuerRevRegRecord.state, except I don't see the anoncreds value persisted. Also, no idea what IssuerRevRegRecord.record_id is.

There are other things like is the anoncreds RevRegDef.value.tails_location the tails_local_path or the tails_public_uri?

And very lost on what/where I find values for IssuerRevRegRecord.revoc_reg_entry. Only thing close I could find would be calling registry.get_revocation_list (not sure what timestamp) and getting entries from that list?

I can probably attribute this to never using the revocation api and not knowing that those data calls contain or what / how they are used. But it looks like a lot of data was persisted before that is no longer persisted?

swcurran commented 1 year ago

This is the trickiest part, as revocation has changed between the CredX implementation and AnonCreds. Summarized:

That all said, it means that there are differences between the two implementations, and you are going to need help to determine the mapping between them. I don’t know the details enough to know exactly what changed.

usingtechnology commented 1 year ago

As we talked about this morning, I will be adding context here for discussion and pointers from the experts.

@dbluhm @andrewwhitehead @esune @Jsyro

Showing the current response data structure and then where I think I get data from in the "new" anoncreds code with pointers to the result fields I believe match up. I hope it makes sense.

I will start with the following endpoint:

GET "/revocation/registry/{rev_reg_id}"

Response:

RevRegResultSchema:
    result:
        record_id
        state
        cred_def_id
        error_msg
        issuer_did
        max_cred_num
        revoc_def_type (CL_ACCUM)
        revoc_reg_id
        revoc_reg_def:
            ver
            id_
            revoc_def_type (CL_ACCUM)
            tag
            cred_def_id
            value:
                issuance_type (one of ["ISSUANCE_ON_DEMAND", "ISSUANCE_BY_DEFAULT"])
                max_cred_num
                public_keys:
                    accum_key:
                        z (example="1 120F522F81E6B7 1 09F7A59005C4939854")
                tails_hash
                tails_location
        revoc_reg_entry:
            ver
            value:
                prev_accum
                accum
                revoked: [list of int]
        tag
        tails_hash
        tails_public_uri
        tails_local_path
        pending_pub: [list of strings/numbers]
        created_at
        updated_at

result.revoc_reg_def

Given a rev_reg_id, call revocation.get_created_revocation_registry_definition, return RevRegDef object

RevRegDef:
    issuer_id -> result.issuer_did
    type -> result.revoc_def_type, result.revoc_reg_def.revoc_def_type
    cred_def_id -> result.cred_def_id, result.revoc_reg_def.cred_def_id
    tag -> result.revoc_reg_def.tag
    value:
        public_keys -> result.revoc_reg_def.public_keys
        max_cred_num -> result.revoc_reg_def.max_cred_num
        tails_location -> result.revoc_reg_def.tails_location
        tails_hash -> result.revoc_reg_def.tails_hash, result.tails_hash

Assume that we are hardcoding ver as 1.0, same as what is in the anoncreds revocation classes.

result.revoc_reg_entry

To populate the result.revoc_reg_entry call (legacy_indy).get_revocation_list(rev_reg_def_id=rev_reg_id, timestamp=now) and return GetRevListResult.

Assumption: The timestamp parameter should be the current timestamp.

GetRevListResult:
    revocation_list:
        issuer_id
        rev_reg_def_id
        revocation_list: list of int -> result.revoc_reg_entry.revoked
        current_accumulator -> result.revoc_reg_entry.accum
        timestamp
    resolution_metadata
    revocation_registry_metadata

Assume that we are hardcoding ver as 1.0, same as what is in the anoncreds revocation classes.

Populate remaining fields

Assuming the above is correct, how do we populate the remaining fields?

  1. What is record_id?
  2. What value for result.revoc_reg_def.id_?
  3. Where/how do we get result.revoc_reg_entry.prev_accum?
  4. Do we populate result.pending_pub by calling AnonCredsRevocation.get_revocation_lists_with_pending_revocations?
    1. Do we need to transform the result?
  5. Is RevRefDef.value.tails_location a local path or public uri?
    1. How would we populate the other result field (result.tails_public_uri, result.tails_local_path
swcurran commented 1 year ago

Thanks — this is helpful. The results of these calls need not be identical to what we have had in the past, because of the change in local representation. We will need to selectively alter the results, and the tests to match. We can document the differences.

Have you looked at the data from the call and looked at the data? Or at least to see what the data is from the existing code? I think that would help. What calls are made by the existing implementation? I assume it is calling the secure storage to retrieve data by the RevRegId?

As an outsider, I can’t offer much help. The only one that I’m pretty sure of is the tails_file location, where the local tails file can now be null, as the issuer no longer needs it after generation and publication. The tails_public_uri comes from the RefRegDef.

usingtechnology commented 1 year ago

After posting this, I did find that the tails_public_uri is set by the agent/user and stored, so I believe we can ignore that match.

I will look over the tests to see about data and will check AATH too.

usingtechnology commented 1 year ago

Following up, from AATH (not much in the way of tests against the revocation API) and ACA-Py pytests and BDD, it doesn't appear that anything is using the result.revoc_reg_entry except the accum field. That is not to say that no other parties are using it, but it's not covered by tests. And the tails_public_uri can either be the value explicitly set OR the tails_file_location.

swcurran commented 1 year ago

I would say drop all of the fields that you don’t know how to populate, fix the tests, and document as BREAKING in the PR all of the fields that are dropped.

dbluhm commented 1 year ago
  1. What is record_id?

  2. What value for result.revoc_reg_def.id_?

  3. Where/how do we get result.revoc_reg_entry.prev_accum?

  4. Do we populate result.pending_pub by calling AnonCredsRevocation.get_revocation_lists_with_pending_revocations?

    1. Do we need to transform the result?
  5. Is RevRefDef.value.tails_location a local path or public uri?

    1. How would we populate the other result field (result.tails_public_uri, result.tails_local_path
  1. Record ID is an arbitrary UUID associated with the IssuerRevRegRecord. It is not useful from the Admin API.
  2. This should be the same as the rev_reg_id used to recall the rev reg def
  3. Previous accumulator value is only used when actively revoking a credential so I think this should be safe to omit from this object.
  4. It would be revocation.get_pending_revocations(rev_reg_id). It will already be in a "delta style" so it should look the same as it did before so no transform needed.
  5. This is something that will be changing once we pull in the upstream changes to anoncreds-rs. It should always reference public location (but to use the anoncreds-rs interface as it is, we have to manually set it to a local location for the duration of the library call -- it's really gross but has been fixed in the next version of anoncreds-rs). Local tails path will be irrelevant but at the moment is just a computed property from {{tails bucket directory}}/{{tails_file_hash}} instead of something derived from the rev reg def id. Omission of this value in the result should not be a problem. Controllers don't need to know this value.
dbluhm commented 1 year ago

The revoc_reg_entry is the last entry submitted to the revocation registry. This is another value that I think the controller should never have needed to know, even when revocation was controlled more directly through the Admin API. I think it should be safe to omit.

swcurran commented 1 year ago

Thanks for the guidance, Daniel. I thought it would be something like that, but def didn’t know the details. Big help!