hyperledger / aries-cloudagent-python

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
407 stars 511 forks source link

`aca-py` agent can duplicated `cred-defs` #313

Closed WadeBarnes closed 4 years ago

WadeBarnes commented 4 years ago

If cred-defs already exist in the wallet and on the ledger, and they were NOT written there by the aca-py agent, and the DID for the agent has the permission to write to the ledger, the first time the agent starts up it will write duplicate cred-defs to the ledger.

Steps to reproduce: Using local (docker) instances of von-network (von-network), ICOB (indy-catalyst - credential-registry), andICIA (bc-registries-agent).

Observed results:

Expected behaviour:

WadeBarnes commented 4 years ago

von-network indy-cli script process is here; Writing Transactions to a Ledger for an Un-privileged Author

A script following that process, for the described Steps to reproduce is attached; BCReg-reg.txt

WadeBarnes commented 4 years ago

The code referenced here, https://github.com/bcgov/von-network/blob/master/cli-scripts/cred_def.py#L120, creates a cred-def in the wallet only if it does not exist, and returns the cred-def json and id. If the cred-def already exists in the wallet it returns the json and id of the existing cred-def. So something similar could be used to "search" for an existing cref-def. The code only operates on the wallet, no operations are performed on the ledger.

ianco commented 4 years ago

I suggest adding a "--wallet-sync" command that specifically checks for this situation (wallet is pre-initialized with cred defs but there are no non-secrets records). This is something we only need to run once, not something that needs to run every time the agent starts up.

WadeBarnes commented 4 years ago

aca-py behaves as one would expect with the schemas, it does not attempt to write a duplicate to the ledger. One would expect it to behave the same with the cred-defs. The end user of aca-py should not have to make a conscious effort to ensure the desired (expected) behavior whenever a new cred-def is written to it's wallet and the ledger by another process, since for many agent's this will be the norm as they will not have permission to write schemas and cred-defs to the ledger without third party endorsement.

sklump commented 4 years ago

I don't see how a cred def ends up in the wallet of an agent who did not issue it?

For completeness, if a cred def is:

I may be missing something obvious here.

ianco commented 4 years ago

@sklump the cred def is created through an "endorser" process, not through aca-py. (See the links in @WadeBarnes 's comments above.) The aca-py DID does not have privileges to write to the ledger.

The "endorser" process creates the cred defs on the ledger and in the wallet, but it does not create the "non secrets" records that aca-py uses to look up the cred defs, so aca-py does not know the cred defs are there.

It's like your bullet 4 scenario above ("in the wallet, on the ledger (same cred def)") but aca-py doesn't know it, tries to re-create it, and gets an error.

ianco commented 4 years ago

... actually to amend the above, aca-py tries to create the cred def in the wallet using Indy Anoncreds, however if the cred def already exists then it appears that Anoncreds updates the existing wallet record, and then the ledger needs to be updated to match, so this results in the second cred def entry on the ledger. Even for a non-privileged DID.

sklump commented 4 years ago

Can the endorser process not check the ledger first via indy-sdk (e.g., https://github.com/hyperledger/indy-sdk/blob/f708f496dd23ada341303db7a4e6a0a7e29a549f/wrappers/python/tests/interation/test_interaction.py#L78) ? Or is a race condition with multiple competing endorsers trying to send the same cred def a reasonable expectation?

ianco commented 4 years ago

It's not an issue with the endorser process, aca-py needs to check on startup (or on registration) for any existing cred defs.

sklump commented 4 years ago

Humour me here, because I know on some level I am missing something fundamental.

If the endorser can create the cred def and write it to the wallet of the issuer, then surely it can write corresponding non-secret records as per https://github.com/hyperledger/aries-cloudagent-python/blob/6c669cf1d0ddb7701fe82a6becd1c884c485d51e/aries_cloudagent/ledger/indy.py#L563 and subsequent wallet write?

Then on startup, aca-py would find its cred defs and not try to re-create them?

ianco commented 4 years ago

@sklump correct, that is one option. The endorser process is currently using indy-cli (augmented with some scripts by @WadeBarnes ) but could be updated to write the aca-py records. It doesn't handle the case where we're upgrading a von-x agent to aca-py, so that's why we're considering an alternate approach to update aca-py to handle this situation.

sklump commented 4 years ago

Just a note that the non-secrets records are highly useful, since they allow aca-py clients to ask what cred defs are available from an issuer without having to trawl the whole ledger and filter for issuer-id. Whatever we come up with, I hope we can keep these in.

sklump commented 4 years ago

The trouble is that without those records, an agent would have to do just that (on startup): trawl the ledger, get every transaction, keep cred defs it authored. As the ledger grows, this becomes worse and worse. There is no way to query the ledger intelligently.

ianco commented 4 years ago

Tested with the attached script BCReg-reg.txt

sklump commented 4 years ago

I can't export the wallet:

./scripts/manage: line 83: ./cli-scripts/export-wallet.run: Permission denied

Similarly for any indy-cli command.

It looks like it can't write to the .run file in the cli-scripts directory on evaluating the envsubst and redirect. I don't know why.

swcurran commented 4 years ago

@sklump - I think the basic problem is likely that the volume mounted folder that is being used is owned/writeable being created by Docker as root on Linux, and being accessed in the Docker scripts by another user. Covered here: https://stackoverflow.com/questions/47197493/docker-mounting-volume-permission-denied

Suggestion for the short term is to pre-create the folder and make it's access open, including the "s" permission so that items created under the top of the folder inherits the same permission. e.g. run chmod uga+rwxs foo for that folder.

Wade has info to make the change more restrictive for the long term by adding stuff to the Dockerfile to work around that.

Test - remove the folder and then run Docker to see how it gets created, and then play with the permissions on it and try again.

WadeBarnes commented 4 years ago

@sklump tagged on the fix here; https://github.com/bcgov/von-network/pull/85

sklump commented 4 years ago

There is more to this fix to come. It appears (as Andrew noticed) that indy-sdk doesn't raise AnoncredsCredDefAlreadyExistsError anymore for cred def repeats, for starters. And checking the ledger first should change some logic after the wallet check (i.e., never check twice). So I'm refraining from approving this at the moment until I figure out what's up with indy-sdk, then I ought to take a deeper dive into the corner cases.

ianco commented 4 years ago

I think the fix is:

  1. Check if the cred def is on the ledger, if it's not we're good and we can create it
  2. Try to create a credential offer, if successful then the cred def is in the wallet

If the cred def is in the ledger and not in the wallet, bail with a fatal error.

If the cred def is in the wallet and not on the ledger, also fatal bail (?)

If the cred def exists in both ledger and wallet, we're good and we can create any tags we need to create (tags will not exist if the cred def was created by an old von-x agent or through the endorser process, which uses indy cli)

sklump commented 4 years ago

I will work on this and propose something tomorrow morning.

sklump commented 4 years ago

Please have a look at https://github.com/hyperledger/aries-cloudagent-python/pull/323. I think it covers all the bases.

WadeBarnes commented 4 years ago

323 was re-submitted as #333

WadeBarnes commented 4 years ago

I've finished testing on the RC image, bcgovimages/aries-cloudagent:py36-1.11-1_0.4.1-rc1, containing the changes in #333. The updates address the described issues.

swcurran commented 4 years ago

w00t!!! Go forth and LOAD!

On Fri, Jan 24, 2020 at 2:45 PM Wade Barnes notifications@github.com wrote:

I've finished testing on the RC image, bcgovimages/aries-cloudagent:py36-1.11-1_0.4.1-rc1, containing the changes in #333 https://github.com/hyperledger/aries-cloudagent-python/pull/333. The updates address the described issues.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hyperledger/aries-cloudagent-python/issues/313?email_source=notifications&email_token=AAHYRQX5KXDRVXFH5WTGYQLQ7NVRPA5CNFSM4J6DSM4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ4KIWA#issuecomment-578331736, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHYRQXGRBB3Z5YFMGGOZFDQ7NVRPANCNFSM4J6DSM4A .

--

Stephen Curran Principal, Cloud Compass Computing, Inc. (C3I) Technical Governance Board Member - Sovrin Foundation (sovrin.org)

*Schedule a Meeting: *https://calendly.com/swcurran https://calendly.com/swcurran