hyperledger / aries-rfcs

Hyperledger Aries is infrastructure for blockchain-rooted, peer-to-peer interactions
https://hyperledger.github.io/aries-rfcs/
Apache License 2.0
326 stars 217 forks source link

RFC 36: Suggested modifications to support revocation #162

Closed smithbk closed 5 years ago

smithbk commented 5 years ago

I suggest two enhancements related to credential revocation:

1) When an issuer revokes a credential, a message type is needed in order to notify the holder that the issuer has revoked the credential. This is preferable to having the issuer find out later, when trying to use the credential, that it has been revoked. This is a common scenario such as when a person is fired or quit their job. Or would it be more appropriate to introduce a new message family?

2) The issue-credential message for indy does not have a slot to pass the revocation registry definition (i.e. the 2nd return value from issuerCreateAndStoreRevocReg) from the issuer to the prover. The prover requires this value when calling proverStoreCredential to store the credential.

swcurran commented 5 years ago

The revocation of a credential is a unilateral step by the Issuer, so in theory there is no need to notify the Holder. However, as you point out, it is likely that the Issuer will attempt to notify the Holder. I think the best way to do that is via the Credential Offer, where the Issuer offers the Holder a replacement credential with the latest update (e.g. end date of employment when a person is fired or quits their job). I'm going to propose that as a v1.1 update to the "Issue-Credential" protocol.

As you pointed out on Rocketchat, there is the case of an Issuer revoking a credential and not offering a new credential that has to be covered. That likely needs to be another protocol with a single message from the Issuer, with an optional "ack" back from the Holder.

On your second point, the intention of the "issue-credential" message is that it is just passing along the Indy generated parameters for an Indy-based credential issuance. If the revo reg. is missing, that is an unintentional omission that does indeed need to be fixed. @TelegramSam, @dbluhm, @andrewwhitehead - could one of you see if that is missing from the issue credential RFC message?

llorllale commented 5 years ago

@swcurran @smithbk there is already a rev_reg_def_id attribute in the Indy-specific payload for issue-credential described in the RFC

smithbk commented 5 years ago

@llorllale The value of rev_reg_def_id described in the RFC is simply an ID string and is the 1st return value from issuerCreateAndStoreRevocReg, but the call to proverStoreCredential requires revRegDef which is a JSON string and is the 2nd return value from issuerCreateAndStoreRevocReg.

kdenhartog commented 5 years ago

Is this blocking PR #158 to update the credentials HIPE to accepted status or would this be optional? From what I gather we may want to define this as a separate RFC and separate message in the family.

smithbk commented 5 years ago

@kdenhartog I assume you are referring to the 1st point about adding a new revocation message. I agree that this can be in a separate message family and therefore would not prevent RFC 36 from being accepted. However, I think the 2nd point should be addressed in RFC 36 simply by adding a rev_reg_def field to the issue-credential message.

swcurran commented 5 years ago

As per the recent update to the credential RFC, the JSON structure with the rev_reg_def_id needs to be removed from the RFC. We are intentionally leaving out of these RFC credential exchange implementation specific data structures for reasons like this - they should be in the implementation specs.

I'll submit a PR now for that.

@smithbk - are you going to propose a revocation notification RFC?

swcurran commented 5 years ago

PR submitted.

smithbk commented 5 years ago

@swcurran Yes, done ... see https://github.com/hyperledger/aries-rfcs/pull/183/files

kdenhartog commented 5 years ago

I see the PR has now been merged. Can we close this issue now?

swcurran commented 5 years ago

Yes - closing this issue based on the following:

On the latter, since it has been taking so long to get the 1.0 version of the protocol moved from proposal that I feel like we should just put it in that version. It's been talked about since the very first HIPE.

Oh well...let's keep this moving.