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

aca-py agent can duplicated cred-defs #313

Closed
WadeBarnes opened this issue Dec 20, 2019 · 24 comments
Closed

aca-py agent can duplicated cred-defs #313

WadeBarnes opened this issue Dec 20, 2019 · 24 comments
Assignees
Labels
bug Something isn't working

Comments

@WadeBarnes
Copy link
Member

WadeBarnes commented Dec 20, 2019

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), and ICIA (bc-registries-agent).

  • Start von-network
  • Register an Endorser DID
  • Start ICOB
  • Start the ICIA wallet
  • Use the von-network indy-cli scripts to write schemas and cred-defs to the wallet and ledger, in the same format the ICIA controller would have the ICIA agent write them.
  • When complete, fully restart (stop/start), ICIA.

Observed results:

  • There is a single set of schemas on the ledger, the ones written by the von-network indy-cli scripts.
  • There are two sets of cred-defs, the ones written by the von-network indy-cli scripts, and another set written by the aca-py agent.
  • There are a few (13) additional item records in the wallet, when compared to a regular startup, but none of them appear to be duplicate cred-defs.

Expected behaviour:

  • A single set of schemas and cred-defs to exist on the ledger.
@WadeBarnes
Copy link
Member Author

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 WadeBarnes added the bug Something isn't working label Dec 20, 2019
@WadeBarnes
Copy link
Member Author

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 ianco self-assigned this Dec 20, 2019
@ianco
Copy link
Member

ianco commented Dec 22, 2019

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
Copy link
Member Author

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
Copy link
Contributor

sklump commented Jan 2, 2020

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:

  • not in the wallet, not in the ledger: issuer can create and send it
  • not in the wallet, on the ledger: issuer cannot create it (but can read it from the ledger)
  • in the wallet, not on the ledger: issuer is working against the wrong ledger (raise exception)
  • in the wallet, on the ledger (same cred def): issuer cannot create it (but can read it from the ledger)
  • in the wallet, on the ledger (different cred def): puzzling evidence (currently aca-py re-sends and warns).

I may be missing something obvious here.

@ianco
Copy link
Member

ianco commented Jan 2, 2020

@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
Copy link
Member

ianco commented Jan 3, 2020

... 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
Copy link
Contributor

sklump commented Jan 3, 2020

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
Copy link
Member

ianco commented Jan 3, 2020

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
Copy link
Contributor

sklump commented Jan 3, 2020

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

schema_id_parts = schema_id.split(":")

and subsequent wallet write?

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

@ianco
Copy link
Member

ianco commented Jan 3, 2020

@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
Copy link
Contributor

sklump commented Jan 3, 2020

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
Copy link
Contributor

sklump commented Jan 3, 2020

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
Copy link
Member

ianco commented Jan 4, 2020

Tested with the attached script
BCReg-reg.txt

@sklump
Copy link
Contributor

sklump commented Jan 6, 2020

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
Copy link
Member

swcurran commented Jan 6, 2020

@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
Copy link
Member Author

@sklump tagged on the fix here; bcgov/von-network#85

@sklump
Copy link
Contributor

sklump commented Jan 7, 2020

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
Copy link
Member

ianco commented Jan 8, 2020

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
Copy link
Contributor

sklump commented Jan 9, 2020

I will work on this and propose something tomorrow morning.

@sklump
Copy link
Contributor

sklump commented Jan 9, 2020

Please have a look at #323. I think it covers all the bases.

@ianco ianco assigned WadeBarnes and unassigned ianco Jan 12, 2020
@WadeBarnes
Copy link
Member Author

#323 was re-submitted as #333

@WadeBarnes
Copy link
Member Author

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
Copy link
Member

swcurran commented Jan 24, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants