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

feat: support w3c revocation #2024

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

KulkarniShashank
Copy link

What

This PR introduces support for W3C credential revocation. The following changes have been made:

Refactor functionality for Verify Credential and Verify Presentation.
Integrated with the Bit String Status List for checking and updating credential status.

How

Revocation Flow: Implemented the revocation flow by checking a credential status and updating the credential issuance process to handle revocable credentials.
Integration: Integrated the new revocation features with the existing system, ensuring compatibility and proper handling of revoked credentials.

Signed-off-by: KulkarniShashank <[email protected]>
Copy link

changeset-bot bot commented Sep 2, 2024

⚠️ No Changeset found

Latest commit: 95d558c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@KulkarniShashank KulkarniShashank marked this pull request as draft September 2, 2024 07:00
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Left some comments

@@ -265,6 +304,52 @@ export class W3cJsonLdCredentialService {
challenge: options.challenge,
domain: options.domain,
documentLoader: this.w3cCredentialsModuleConfig.documentLoader(agentContext),
checkStatus: async ({ credential }: { credential: W3cJsonCredential }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extract this into a method and reuse that for both checkStatus methods?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimoGlastra I have developed a common function for this purpose. However, in the checkStatus method, there are specific conditions related to credential status that are dependent on the payload received from the verifyCredential and verifyPresentation functions. Therefore, the checkStatus method invokes the common function, as it contains the identical logic.

Comment on lines 334 to 341
const encodedBitString = bitStringCredential.credential.credentialSubject.encodedList
const compressedBuffer = Uint8Array.from(atob(encodedBitString), (c) => c.charCodeAt(0))

// Decompress using pako
const decodedBitString = pako.ungzip(compressedBuffer, { to: 'string' })
const statusListIndex = Number(credential.credentialStatus.statusListIndex)

if (statusListIndex < 0 || statusListIndex >= decodedBitString.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should validate the bitstring status list credential as well

credentialStatus?: SingleOrArray<CredentialStatus>
}

type CredentialStatusType = 'BitstringStatusListEntry'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move all the status list logic related to bit string status list to a separate file?

throw new CredoError('Invalid credential status format')
}

const credentialStatusURL = credential.credentialStatus.statusListCredential
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should first check the type fiets to make sure this is a bit string status type. There could be others and those are not supported at the moment.

@KulkarniShashank
Copy link
Author

@TimoGlastra As per your suggestions, I have made the changes. Please review them and let me know your thoughts.

@Tweeddalex
Copy link

Out of interest @KulkarniShashank will this work with cheqd DIDs and on-ledger Bitstring Status Lists?

@KulkarniShashank
Copy link
Author

Out of interest @KulkarniShashank will this work with cheqd DIDs and on-ledger Bitstring Status Lists?

Its can work for all the DIDs that support JSON-LD credential format

@TimoGlastra
Copy link
Contributor

However @Tweeddalex this PR does not support cheqd yet. The current implementation only suppors https and uses fetch to retrieve the status list. For cheqd support we'd have to fetch the status list differently.

Comment on lines +269 to +271
if ('credentialStatus' in credential) {
await verifyBitStringCredentialStatus(credential, agentContext)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verifyPresentation also has options.verifyCredentialStatus, can you add that check here as well?

Comment on lines +33 to +42
@IsEnum(['BitstringStatusListEntry'])
@IsString()
public type!: CredentialStatusType

@IsEnum(CredentialStatusPurpose)
@IsString()
public statusPurpose!: CredentialStatusPurpose

@IsString()
public type!: string
public statusListIndex!: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the VC Data model specification, these properties are not required fro the credentialStatus property. Only id and type are.

We should have a generic base class and allow multiple extension points. You can look at e.g. how we solved this with the DidDocument service property, where there's a base service class and multiple possible extensions that are matched based on the type property

Comment on lines +48 to 61
// Function to validate the status using the updated method
export const validateStatus = async (status: CredentialStatus, agentContext: AgentContext): Promise<boolean> => {
const entry = plainToInstance(W3cCredentialStatus, status)

try {
await validateOrReject(entry)
return true
} catch (errors) {
agentContext.config.logger.debug(`Credential status validation failed: ${errors}`, {
stack: errors,
})
throw new CredoError(`Invalid credential status type: ${errors}`)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is automatically done when transforming into a specific class instance type, so I don't think it's needed if you follow the did document service approach

Comment on lines +3 to +55
export type CredentialStatusType = 'BitstringStatusListEntry'
// The purpose can be anything apart from this as well
export enum CredentialStatusPurpose {
'revocation' = 'revocation',
'suspension' = 'suspension',
'message' = 'message',
}

export interface StatusMessage {
// a string representing the hexadecimal value of the status prefixed with 0x
status: string
// a string used by software developers to assist with debugging which SHOULD NOT be displayed to end users
message?: string
// We can have some key value pairs as well
[key: string]: unknown
}

export interface CredentialStatus {
id: string
// Since currenlty we are only trying to support 'BitStringStatusListEntry'
type: CredentialStatusType
statusPurpose: CredentialStatusPurpose
// Unique identifier for the specific credential
statusListIndex: string
// Must be url referencing to a VC of type 'BitstringStatusListCredential'
statusListCredential: string
// The statusSize indicates the size of the status entry in bits
statusSize?: number
// Must be preset if statusPurpose is message
/**
* the length of which MUST equal the number of possible status messages indicated by statusSize
* (e.g., statusMessage array MUST have 2 elements if statusSize has 1 bit,
* 4 elements if statusSize has 2 bits, 8 elements if statusSize has 3 bits, etc.).
*/
statusMessage?: StatusMessage[]
// An implementer MAY include the statusReference property. If present, its value MUST be a URL or an array of URLs [URL] which dereference to material related to the status
statusReference?: SingleOrArray<string>
}

// Define an interface for `credentialSubject`
export interface CredentialSubject {
encodedList: string
}

// Define an interface for the `credential` object that uses `CredentialSubject`
export interface Credential {
credentialSubject: CredentialSubject
}

// Use the `Credential` interface within `BitStringStatusListCredential`
export interface BitStringStatusListCredential {
credential: Credential
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these interface names should be bitstring status list specific. It is important to make a distinction between bitstring status list, and possible other credentialStatus methods that will be added in the future.

}
}

export const verifyBitStringCredentialStatus = async (credential: W3cJsonCredential, agentContext: AgentContext) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't validate the signature of the status list credential yet

* @returns Revoke credential notification message
*
*/
public async revokeJsonLdCredential(options: RevokeCredentialOption): Promise<{ message: string }> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a lot of dupcliation in this file with the already present methods. Some suggestions:

  • move all the credential status logic into a specific folder in the w3c module
  • add the revocation related methods to the w3c credentails api, not the DIDcomm credentials api
  • the only thing we should add it here is a sendRevocationNotification method (which we can extend to support non-anoncreds revocadion notification messages as well)
  • making a POST to the credential status list url to update the status list seems implementation specific and may not work for all implementations

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 this pull request may close these issues.

3 participants