Implement Revocation Notification v1.0

[dbluhm]

This PR implements revocation notifications following the RFC: https://github.com/hyperledger/aries-rfcs/blob/main/features/0183-revocation-notification/README.md

Currently, ~please_ack support is not implemented and there is no way to propagate a comment from issuer to recipient. Hopefully this serves as a good starting point to collect feedback.

I've done some testing using the revocation demo I put together a while ago and everything worked as expected but I plan to implement some tests (at a minimum unit tests, perhaps integration test with some direction from @ianco) before we merge this one.

[swcurran]

Daniel -- I see that there are startup parameters related to this that say "send revocation notification" for an issuer. Does the Admin API have to change at all when a revocation is made? How does ACA-Py know what connection to use to send the message. Notably -- does this assume that the connection ID is in ACA-Py storage, or can be passed from the controller as part of the revocation action.

@shaangill025 -- remember the idea of you doing this protocol? Nevermind :-).

Thanks @dbluhm -- good to have this one done.

[dbluhm]

As currently implemented, the revocation notification is automatically sent if the --notify-revocation flag is set. I added thread ID and connection ID to the IssuerCredRevRecord (which, as I understand it, is the primary source of info for issued credentials with revocation support after exchange records have been deleted). So, with this automated flow, connection ID is considered to be in storage and is not passed from the controller. This flow requires that data is passed down the stack a few layers which I'm not fond of. I can't say I'm familiar enough with the rest of the revocation stack to know clean points to refactor from though. Open to suggestions 🙂

[ianco]

I don't think you can assume that aca-py has stored any information about previously issued credentials

So the controller is going to have to keep track of the connection_id associated with the credential and provide this parameter as part of the revocation request

From my recollections aca-py can optionally keep track of issued credentials but we expect this is not going to be a typical production deploy/configuration

[dbluhm]

Perhaps this is a misunderstanding on my part but I believe ACA-Py must store at a minimum the tails file identifiers of the issued credentials to actually revoke them. This is one of the pieces of information that the IssuerCredRevRecord held. However, I acknowledge that including more info (thread ID and connection ID) in that record may be at odds with the overall intent of ACA-Py's persistence model (keep as little data as possible to complete the exchange, delegate to the controller for the rest). So shall I pivot to a "controller knows all" model?

[ianco]

Perhaps this is a misunderstanding on my part but I believe ACA-Py must store at a minimum the tails file identifiers of the issued credentials to actually revoke them...

No we recommend that the controller store this information

[dbluhm]

No we recommend that the controller store this information

Okay, thanks for the info! I think making revocation notification purely a controller responsibility simplifies changes within ACA-Py and provides a clearer path for including things like comments. I'll revert changes to the IssuerCredRevRecord and focus on triggering sending of notifications through the Admin API.

[swcurran]

It shouldn't be a new API call though - the controller shouldn't have to make a second call to send the notification. It should just be part of the existing revoke credential call, with the connectionID added to the call to trigger the notification.

However -- an interesting issue. If ACA-Py is doing batch revocations, should the notification be sent when the "revoke credential" call is made, or when the "publish revocations" call is made? I think it should be the latter, and we'd need to store the connectionID with the data for the publish action.

[ianco]

However -- an interesting issue. If ACA-Py is doing batch revocations, should the notification be sent when the "revoke credential" call is made, or when the "publish revocations" call is made? I think it should be the latter, and we'd need to store the connectionID with the data for the publish action.

I think the notification should happen when the revocation is published to the ledger (because that's when the credential is officially "revoked")

[dbluhm]

I think the notification should happen when the revocation is published to the ledger (because that's when the credential is officially "revoked")

Agreed. So what I'm thinking is:

• Revoke endpoint accepts the following new attributes in the body:
  • notify boolean defaulting to false that controls whether a revocation notification is sent. I think it probably also makes sense to keep the flag I added that controls the default. This would work similar to how auto-accept works in connections.
  • connection_id that must be present if notify is true
  • thread_id representing the thread ID of the exchange that resulted in the issued credential; also must be present if notify is true
  • comment which is an optional string that will be included in the revocation notification
• To capture the above data at revoke call but to hold until revocation is actually published:
  • Create a new RevocationNotificationRecord. Create and store these on /revocation/revoke.
  • On revocation publish (RevocationManager.revoke_credential when publish is true, RevocationManager.publish_pending_revocations), emit event with enough detail that the RevocationNotificationRecord can be recalled.
  • From Revocation Notification protocol, register handler for the above event, creating and sending a notification from the record described above. Once the message has been sent, delete the record.
  • On pending revocation clear, delete RevocationNotificationRecords associated with the cleared pending revocations.

[ianco]

Hi @dbluhm that looks good, with one comment: I'm not sure what you need the thread_id for, I think you only need the connection_id to send the notification. Aca-py typically won't have a history of the thread_id because we delete credential exchange records when the credential exchange is completed. (With the exception that you can override this with a startup parameter, however this is intended only for testing/dev.)

Are you thinking we need to include the thread_id in the notification? It's likely the holder agent won't retain this info either ...

[dbluhm]

Very good points and I agree that the holder agent likely doesn't hold on to this information; however, this is required to be included in the notification as part of the protocol by the RFC. I believe the reasoning is that across agent implementations and credentialing systems, the thread that issued the credential is the only identifier guaranteed to be consistent for both parties.

[ianco]

Ok got it, thanks

[ianco]

Another consideration - we should identify whether the credential is being revoked to replace it (e.g. updated drivers license for new address) or is being revoked because it's not valid any more (too many driving infractions!)

A related issue is credential expiry (i.e. a credential, like a business license or drivers license has an expiry date) - there's no standard approach to credential expiry but maybe there should be? Sometimes we may want to auto-revoke the credential after it expires, sometimes we may rely on the verifier to "know" that there is an expiry date that they need to look at. It would be nice if there was a standard that the wallet (holder) could use to present this status to the user.

[dbluhm]

Untested and missing the pending revocations clear implementation but most of the pieces I noted in my comment above are now implemented.

[dbluhm]

Everything should now be implemented. Test coverage for my additions should be > 95%. I have not implemented any new integration tests (still unfamiliar with behave 😬 but happy to implement some integration tests if we'd like that before merging this one).

[ianco]

Just taking a look at this now. It could use some documentation hint hint @dbluhm ... I'll figure out where it makes sense to add into the integration tests

[ianco]

I think the validation for thread_id may be over aggressive. I added support for revocation notifications to alice/faber (see https://github.com/ianco/aries-cloudagent-python/commit/4c468e07e44d2fa1414bd3de89309288a299599f) and I get the following error when revoking a credential:

Faber      | 2021-11-16 19:40:17,607 aries_cloudagent.core.event_bus ERROR Error occurred while processing event
Faber      | Traceback (most recent call last):
Faber      |   File "/home/indy/aries_cloudagent/core/event_bus.py", line 120, in notify
Faber      |     await processor()
Faber      |   File "/home/indy/aries_cloudagent/protocols/revocation_notification/v1_0/routes.py", line 51, in on_revocation_published
Faber      |     record.to_message(), connection_id=record.connection_id
Faber      |   File "/home/indy/aries_cloudagent/protocols/revocation_notification/v1_0/models/rev_notification_record.py", line 113, in to_message
Faber      |     "No thread ID set on revocation notification record, "
Faber      | ValueError: No thread ID set on revocation notification record, cannot create message

[ianco]

See error from faber demo

[dbluhm]

Added some docs and hopefully addressed the errors seen in your testing with the alice/faber demo. My logic was off when checking whether notify was set.

[ianco]

I suggest that we make the thread_id optional when we're revoking (with notifications). The cred_exchange records are transitory and the controller may not record the thread_id. Likewise the holder may not know the thread_id.

[dbluhm]

The RFC requires the thread ID. So, while I agree with your assessment that it's likely the thread_id is not known by one side or the other or both, this would be a spec divergent change. Happy to code it up and make it happen but wanted to call that out at least first.

[swcurran]

I think we probably should have reviewed this more carefully when we accepted the RFC. :-(.

Obviously, for an Indy Credential, we would use a different ID to communicate what it is that we are revoking. For Indy we need a combination of RevRegID+IndexOfCredInRevReg. For any other credential format, there is likely some sort of ID available. I think the use of thread_id was picked because without knowing the details of a specific implementation, you can't guess what the ID should be. Worse, with the suggestion that we can issue multiple credentials using a single protocol instance (e.g. a bank sends to the holder a credential for each bank account they have, or a university sends a credential to the holder each degree they have been granted), the "thread_id" is not going to work.

I suggest we propose a change to the Aries RFC(s). Ideal would be that the "Issue Credential" protocol define a "revocation_id" that is VC format specific and understood by the holder and issuer, and the Revocation Notification V2.0 message use that ID in the notification message. Since I'd rather not change the "Issue Credential" protocol, I suggest that we change "thread_id" to "revocation_id", and define it to be VC format specific, and for indy have a concatenated key for the credential. We could even say as a 1.0 clarification that the "thread_id" can be EITHER the thread id or the VC-format specific "revocation_id" that is defined in the V2.0 message.

Thoughts?

[ianco]

We could define the format of what is to be included in thread_id, for example:

"thread_id": "12345678-1234-1234-1234-123456789012" = an actual thread id
"thread_id": "indy::<reg_rev_id>::<cred_index_in_rev_reg>" = an indy-specific format

This would at least respect the format of the notification message, if not (necessarily) the content.

However I agree with @swcurran that it's better to add a new "revocation_id" (could use the above format for indy) and push a clarification to the RFC (if "revocation_id" is specified then "thread_id" is not required)

[TelegramSam]

Agree with the addition of an opaque revocation_id used for this purpose. I have less of an opinion about how we bridge between old and new.

[swcurran]

I personally like Ian's suggestion, with the holder checking first to see if they have thread_id that matches what is passed, and then falling back to an implementation specific identifier. AFAIK, this is a first implementation so I don't think it would be a huge issue.

I vote we add a clarification to the RFP, and document this in v1.0, and also add either a v1.1 (keep thread_id and add revocation_id and how to process both) or v2.0 version (replace thread_id with revocation_id).

@smithbk -- thoughts on this?

[dbluhm]

Here's my proposal for this PR:

• Make thread_id optional in ACA-Py's revoke endpoint. When absent and notify is true, ACA-Py will fill in and send a thread_id value in the revocation notification with the format indy::<rev_reg_id>::<cred_index>. When present, ACA-Py will use the given value.
• Loosen restrictions on format of thread_id to accommodate the above.
• Address revocation_id vs thread_id in a future revision once conversation has coalesced.

@ianco @swcurran does this seem like a good compromise as we drive towards better language in the RFC?

[ianco]

Yes that makes sense to me

[ianco]

I suggest we add the revocation registry id and rev index to this notification. The holder may not know the thread_id (if they don't store it when they receive the credential) but they will know the rev reg id and index because it will be part of the credential.

[dbluhm]

Similar feelings here. I agree that these values would be much more useful than the thread ID but I hesitate to deliberately diverge from the spec without calling out that we are at least.

[ianco]

@swcurran what are your thoughts on this?

[swcurran]

what are your thoughts on this?

I add them to the thread above, when I meant add them to this one :-). See above ^^^.

[ianco]

One more question - what if I have revocation notifications enabled via command-line params, but I have to revoke a credential where I no longer have the connection? Can I just say "notify": False in the revocation call, and this will override the global setting?

[dbluhm]

One more question - what if I have revocation notifications enabled via command-line params, but I have to revoke a credential where I no longer have the connection? Can I just say "notify": False in the revocation call, and this will override the global setting?

Yes, that's correct or is the intended behavior at least. I'll add that to the documentation, thanks for calling that out.

[ianco]

@dbluhm this looks good, can you re-sync with the main branch and then we can merge