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(anoncreds): issue revocable credentials #1427

Merged

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Apr 11, 2023

This PR includes some initial work done for supporting the issuance of revocable AnonCreds credentials in AFJ.

Brief summary of the changes:

  • Update of AnonCredsRegistry and AnonCredsIssuerService interfaces to add registration of revocation registry definitions and revocation status lists
  • AnonCreds module, through its AnonCredsApi, now allows to register these objects and update revocation status lists
  • AnonCredsCredentialFormatService has been updated to handle accept revocable credential requests, retrieving a Revocation Registry Definition and assigning an index to the credential that is going to be issued. Both the revocation registry definition Id and index are specified by agent controller when creating the credential offer.
  • Credentials module allows to send revocation notification for both indy-anoncreds and a potentially new anoncreds revocation format. This is very basic right now and needs some more work to support future formats and get information directly from the credential record to be more aligned with other modules API.
  • AnonCreds module now has some new configuration items for object creation. In particular it adds the new TailsFileService, which is an interface for downloading and uploading tails files. The default implementation works as right now (only allows to download files), while a custom one can be used to upload files to any chosen tails server. In samples directory there is an example of tails server and service interface (not sure if they belong there but for the moment didn't find a better place). For the tests there is a dummy implementation that allows us to run them without the need of having an actual HTTP server

Flow:

For the moment, as seen in the few tests added in anoncreds-rs package, all VDR objects are managed exclusively from AnonCreds API and created/registered separately in order to have a simpler state management. For instance, if we want to create a revocable credential definition, we'd need to call:

  • registerSchema
  • registerCredentialDefinition
  • registerRevocationRegistryDefinition
  • registerRevocationStatusList

The only step that does two actions atomically is registerRevocationRegistryDefinition, as it will attempt to successfully uploading the tails file before registering the object in the VDR (this is mandatory because we may not know upfront what's the publicly available tails location).

To revoke a credential (or a number of credentials), AnonCredsApi.updateRevocationStatusList() must be called using the revocation registry definition id and credential indexes as an input. If we want to notify the holder/s about this revocation, we must use Credentials API afterwards.

Some notes/missing pieces

  • In this PR there is only a generic implementation using InMemoryAnonCredsRegistry. There is not yet any implementation for Indy VDR or Indy SDK
  • It could feel more 'natural ' if we add a method called revokeCredentials in CredentialsApi that does both the revocation state update and send notification. However, this integration would need us to deal with the different credential formats and, after all, I'm not sure if it will be really useful in real world scenarios, because I think that usually an issuer would prefer to revoke multiple credentials at once (to avoid creating lots of ledger updates)
  • In terms of agent storage, there are no specific changes to mark credentials as revoked and to store more information about credential revocation identification from the issuer side. I think it could be useful to find a good place to do so within AFJ model
  • Sorry for the monster PR!

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
return new Promise<string>((resolve, reject) => {
const shasum = createHash(algorithm)
try {
const s = fs.createReadStream(filePath)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
Comment on lines +49 to +83
app.get('/:tailsFileId', async (req, res) => {
logger.debug(`requested file`)

const tailsFileId = req.params.tailsFileId
if (!tailsFileId) {
res.status(409).end()
return
}

const fileName = tailsIndex[tailsFileId]

if (!fileName) {
logger.debug(`no entry found for tailsFileId: ${tailsFileId}`)
res.status(404).end()
return
}

const path = `${baseFilePath}/${fileName}`
try {
logger.debug(`reading file: ${path}`)

if (!fs.existsSync(path)) {
logger.debug(`file not found: ${path}`)
res.status(404).end()
return
}

const file = fs.createReadStream(path)
res.setHeader('Content-Disposition', `attachment: filename="${fileName}"`)
file.pipe(res)
} catch (error) {
logger.debug(`error reading file: ${path}`)
res.status(500).end()
}
})

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
Comment on lines +85 to +125
app.put('/:tailsFileId', multer({ storage: fileStorage }).single('file'), async (req, res) => {
logger.info(`tails file upload: ${req.params.tailsFileId}`)

const file = req.file

if (!file) {
logger.info(`No file found: ${JSON.stringify(req.headers)}`)
return res.status(400).send('No files were uploaded.')
}

const tailsFileId = req.params.tailsFileId
if (!tailsFileId) {
// Clean up temporary file
fs.rmSync(file.path)
return res.status(409).send('Missing tailsFileId')
}

const item = tailsIndex[tailsFileId]

if (item) {
logger.debug(`there is already an entry for: ${tailsFileId}`)
res.status(409).end()
return
}

const hash = await fileHash(file.path)
const destinationPath = `${baseFilePath}/${hash}`

if (fs.existsSync(destinationPath)) {
logger.warn('tails file already exists')
} else {
fs.copyFileSync(file.path, destinationPath)
fs.rmSync(file.path)
}

// Store filename in index
tailsIndex[tailsFileId] = hash
fs.writeFileSync(indexFilePath, JSON.stringify(tailsIndex))

res.status(200).end()
})

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
This route handler performs
a file system access
, but is not rate-limited.
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

Merging #1427 (61ac750) into main (0865ea5) will increase coverage by 22.39%.
The diff coverage is 44.36%.

❗ Current head 61ac750 differs from pull request most recent head 0096052. Consider uploading reports for the commit 0096052 to get more accurate results

@@             Coverage Diff             @@
##             main    #1427       +/-   ##
===========================================
+ Coverage   62.44%   84.83%   +22.39%     
===========================================
  Files         775      972      +197     
  Lines       17906    23619     +5713     
  Branches     3081     4168     +1087     
===========================================
+ Hits        11181    20038     +8857     
+ Misses       6185     3375     -2810     
+ Partials      540      206      -334     
Files Coverage Δ
packages/anoncreds/src/AnonCredsModule.ts 100.00% <100.00%> (ø)
...RevocationRegistryDefinitionRecordMetadataTypes.ts 100.00% <100.00%> (ø)
packages/anoncreds/src/repository/index.ts 100.00% <100.00%> (ø)
...s/anoncreds/src/services/AnonCredsIssuerService.ts 100.00% <ø> (ø)
packages/anoncreds/src/services/index.ts 100.00% <100.00%> (ø)
packages/anoncreds/src/services/tails/index.ts 100.00% <100.00%> (ø)
packages/anoncreds/src/utils/index.ts 100.00% <100.00%> (ø)
packages/anoncreds/src/utils/timestamp.ts 100.00% <100.00%> (ø)
packages/anoncreds/tests/legacyAnonCredsSetup.ts 76.00% <100.00%> (ø)
...protocol/revocation-notification/services/index.ts 100.00% <100.00%> (ø)
... and 26 more

... and 476 files with indirect coverage changes

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@TimoGlastra
Copy link
Contributor

This is great @genaris, I need some more time to look at this, but really looking forward to merge this 🚀

@genaris genaris linked an issue Apr 15, 2023 that may be closed by this pull request
3 tasks
@genaris genaris marked this pull request as ready for review April 28, 2023 04:57
@genaris genaris requested a review from a team as a code owner April 28, 2023 04:57
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris
Copy link
Contributor Author

genaris commented Jul 22, 2023

@TimoGlastra I did a few updates based on the last feedback from last month. Would be good to have a brief discussion of hyperledger/anoncreds-rs#227 with other AnonCreds maintainers just to be sure that our reasoning is correct.

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@swcurran
Copy link
Contributor

@genaris — wondering about the approach used here. We’ve found in with ACA-Py that it is very difficult for the controller (application code) to manage RevRegs, and have put almost all the work on the Framework. Are you doing that here? It will also make it easier to use other methods (e.g., StatusList2021) since the interface stays much the same.

Notably:

  • Issuer just flags that revocation will be used when creating the CredDef and the size of the RevReg. The library takes case of creating the RevReg(s) needed without further action by the controller.
  • For AnonCreds current Rev approach, two RevRegs are always active, so there is no delay when one fills up, and another is needed. Whenever one fills up, a new one is created. On creation, tails file handling is within Framework.
  • On Issuance, controller receives the “revocation ID" of the issued credential to track. “Revocation ID” is the combination of RevRegId+Index.
  • On Revocation, controller passes in “revocation ID" of the revoked credential with “notify holder” flag. Framework tracks revocations.
  • To publish an update to the revocation state, Issuer passes in CredDefId, and Framework figures out what RevRegs need to be published and does so.

This limits the tracking to be done by controller to a minimum, and relieves it of ever dealing with RevRegs.

From experience, we’ve added to two features to deal with unexpected conditions:

  • If the local copy of a RevReg gets out of sync of the Indy ledger copy (due to a failed ledger write), the Framework can create an update to “fix” the issue. This is needed for a deltas-based solution, but wouldn’t be needed for a full state version.
  • From time to time there may be a need to “rotate” a RevReg — mark existing, still useful RevRegs as “decommissioned” and create new ones in their place. We’ve added an endpoint (api call) for that.

@genaris
Copy link
Contributor Author

genaris commented Jul 24, 2023

@genaris — wondering about the approach used here. We’ve found in with ACA-Py that it is very difficult for the controller (application code) to manage RevRegs, and have put almost all the work on the Framework. Are you doing that here? It will also make it easier to use other methods (e.g., StatusList2021) since the interface stays much the same.

Thank you for the comments @swcurran , this is certainly very valuable! At the beginning we started a similar approach, but as it would require more work (especially in regards to the synchronization and possible race conditions with multiple agent instances) for the moment we kept it more simple at AFJ layer and left the logic for an upper layer like AFJ REST package. Personally, I'd like to create an example and tutorial showing how to use this feature. For sure I'll integrate or at least mention the concepts you've mentioned here.

Once we find out how to better handle multi-instance and horizontal scaling use cases I think it will be good to offer right into AFJ's AnonCreds module an automated way like ACA-Py, especially to make the life simpler to framework users.

@genaris genaris linked an issue Aug 1, 2023 that may be closed by this pull request
1 task
@genaris genaris force-pushed the feat/issue-revocable-credentials branch from adb811e to 8f5d6bf Compare August 17, 2023 18:20
@genaris genaris mentioned this pull request Aug 18, 2023
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@TimoGlastra
Copy link
Contributor

@genaris is this ready to be merged? Any things you're waiting on?

@genaris
Copy link
Contributor Author

genaris commented Nov 17, 2023

@genaris is this ready to be merged? Any things you're waiting on?

Yes, I think so!

@TimoGlastra TimoGlastra merged commit c59ad59 into openwallet-foundation:main Nov 22, 2023
8 checks passed
@TimoGlastra
Copy link
Contributor

Thanks @genaris ❤️

amitpadmani-awts pushed a commit to ayanworks/aries-framework-javascript that referenced this pull request Nov 29, 2023
* fix: listen to incoming messages on agent initialize not constructor (openwallet-foundation#1542)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* fix(cheqd): make cosmos payer seed optional (openwallet-foundation#1547)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix: create message subscription first (openwallet-foundation#1549)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix(transport): Use connection in WebSocket ID (openwallet-foundation#1551)

* fix: priority sorting for didcomm services (openwallet-foundation#1555)

Signed-off-by: Timo Glastra <timo@animo.id>

* fix: bump missing dependencies version (openwallet-foundation#1557)

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

* chore(release): v0.4.1 (openwallet-foundation#1548)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix: do not send package via outdated session (openwallet-foundation#1559)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* build(deps): bump @types/indy-sdk from 1.16.26 to 1.16.27 (openwallet-foundation#1564)

Signed-off-by: dependabot[bot] <support@github.com>

* fix(cheqd): changed the name formatting to a encoded hex value (openwallet-foundation#1574)

Signed-off-by: Tom Lanser <tom@animo.id>

* build(deps): bump @cheqd/ts-proto from 2.2.0 to 2.2.2 (openwallet-foundation#1568)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump @cosmjs/proto-signing from 0.31.0 to 0.31.1 (openwallet-foundation#1566)

Signed-off-by: dependabot[bot] <support@github.com>

* fix(core): remove node-fetch dependency (openwallet-foundation#1578)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: log and throw on WebSocket sending errors (openwallet-foundation#1573)

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

* fix(oob): support oob with connection and messages (openwallet-foundation#1558)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: allow connection invitation encoded in oob url param (openwallet-foundation#1583)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: duplicate service ids in connections protocol (openwallet-foundation#1589)

* fix: implicit invitation to specific service (openwallet-foundation#1592)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(askar): throw error if imported wallet exists (openwallet-foundation#1593)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: service validation in OOB invitation objects (openwallet-foundation#1575)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix: update tsyringe for ts 5 support (openwallet-foundation#1588)

Signed-off-by: Timo Glastra <timo@animo.id>

* chore(release): v0.4.2 (openwallet-foundation#1591)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix: save AnonCredsCredentialRecord createdAt (openwallet-foundation#1603)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: sped up lookup for revocation registries (openwallet-foundation#1605)

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>

* build(deps): bump @types/express from 4.17.17 to 4.17.18 (openwallet-foundation#1596)

Bumps [@types/express](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/express) from 4.17.17 to 4.17.18.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/express)

---
updated-dependencies:
- dependency-name: "@types/express"
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump actions/checkout from 3 to 4 (openwallet-foundation#1599)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump amannn/action-semantic-pull-request from 5.2.0 to 5.3.0 (openwallet-foundation#1598)

build(deps): bump amannn/action-semantic-pull-request

Bumps [amannn/action-semantic-pull-request](https://github.com/amannn/action-semantic-pull-request) from 5.2.0 to 5.3.0.
- [Release notes](https://github.com/amannn/action-semantic-pull-request/releases)
- [Changelog](https://github.com/amannn/action-semantic-pull-request/blob/main/CHANGELOG.md)
- [Commits](amannn/action-semantic-pull-request@v5.2.0...v5.3.0)

---
updated-dependencies:
- dependency-name: amannn/action-semantic-pull-request
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: abandon proof protocol if presentation fails (openwallet-foundation#1610)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat!: upgrade shared components (openwallet-foundation#1606)

* feat: update dockerfile to node 18 and sample mediator to askar (openwallet-foundation#1622)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat(sd-jwt-vc): Module for Issuer, Holder and verifier (openwallet-foundation#1607)

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>

* feat(indy-vdr): ability to refresh the pool manually (openwallet-foundation#1623)

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>

* build(deps): bump @babel/traverse from 7.21.4 to 7.23.2 (openwallet-foundation#1608)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump uuid and @types/uuid (openwallet-foundation#1597)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): use node's built-in fetch (openwallet-foundation#1631)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* build(deps): bump react-devtools-core from 4.27.6 to 4.28.5 (openwallet-foundation#1630)

Signed-off-by: dependabot[bot] <support@github.com>

* build(deps): bump @mattrglobal/bbs-signatures from 1.1.0 to 1.3.1 (openwallet-foundation#1601)

Signed-off-by: dependabot[bot] <support@github.com>

* fix: some log messages (openwallet-foundation#1636)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: bump indy-vdr version (openwallet-foundation#1637)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(core): allow string for did document controller (openwallet-foundation#1644)

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

* feat(anoncreds): issue revocable credentials (openwallet-foundation#1427)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* refactor(anoncreds)!: move supportRevocation to options (openwallet-foundation#1648)

Signed-off-by: Timo Glastra <timo@animo.id>

* refactor: move message-pickup directory (openwallet-foundation#1650)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

---------

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Tom Lanser <tom@animo.id>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Co-authored-by: Niall Shaw <100220424+niall-shaw@users.noreply.github.com>
Co-authored-by: Timo Glastra <timo@animo.id>
Co-authored-by: Alexander Shenshin <93187809+AlexanderShenshin@users.noreply.github.com>
Co-authored-by: ericvergnaud <eric.vergnaud@wanadoo.fr>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tom Lanser <tommy.lanser@gmail.com>
Co-authored-by: Ariel Gentile <gentilester@gmail.com>
Co-authored-by: Wade King <wkingnumber2@gmail.com>
Co-authored-by: Berend Sliedrecht <61358536+berendsliedrecht@users.noreply.github.com>
genaris added a commit to genaris/credo-ts that referenced this pull request Dec 4, 2023
)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
genaris added a commit to genaris/credo-ts that referenced this pull request Jan 29, 2024
)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
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.

Add new changes from AnonCreds RS to AFJ Add support for issuance of revocable anoncreds credentials
4 participants