Skip to content

Add did:indy transaction version 2 support#3253

Merged
jamshale merged 6 commits intoopenwallet-foundation:mainfrom
jamshale:feat/3224
Feb 21, 2025
Merged

Add did:indy transaction version 2 support#3253
jamshale merged 6 commits intoopenwallet-foundation:mainfrom
jamshale:feat/3224

Conversation

@jamshale
Copy link
Contributor

@jamshale jamshale commented Sep 24, 2024

This adds the ability to create a did:indy with transaction version 2 algorithm. https://hyperledger.github.io/indy-did-method/#nym-transaction-version.

  • It's created in a new /did/indy/create endpoint. If the controller tries to create it in the /wallet/did/create endpoint it will get a indy method not supported (same as before) and tell them to use the new endpoint.
  • Adds seed support with allow-insecure-seed configuration
  • When starting up an agent wither a did:indy or did:sov seed can be used.
  • Does a refactor on wallet startup function to reduce complexity.

@jamshale jamshale changed the title [WIP] Add did:indy transaction version 2 support Add did:indy transaction version 2 support Sep 25, 2024
@jamshale jamshale marked this pull request as ready for review September 25, 2024 23:45
@jamshale
Copy link
Contributor Author

The security alerts are nothing. http used in the scenario tests. Not sure how to ignore it yet.

@dbluhm
Copy link
Contributor

dbluhm commented Sep 26, 2024

I like what you're doing in this PR -- really appreciate the wallet startup cleanup as well. Question: how do we get the DID onto the ledger? Are we saying this is handled out of band?

@jamshale
Copy link
Contributor Author

jamshale commented Sep 26, 2024

It's the same way as a did:sov. You use the /did/indy/create and then post it to /wallet/did/public. There's no way to start up a fresh agent with a seed and a did:indy currently. Doing that with a seed still creates a did:sov.

Edit: oh, to get it on the ledger you just post the did:indy:12345 and the verkey. So, yes I think that would be out of band.

dbluhm
dbluhm previously approved these changes Sep 30, 2024
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Couple of quick comments but otherwise looks good!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarCloud

@jamshale jamshale force-pushed the feat/3224 branch 2 times, most recently from a226b5f to 0ff3979 Compare October 10, 2024 19:31
@swcurran
Copy link
Contributor

Given the 5 errors found (examples with an “http” protocol instead of “https” — would it be easiest to just add an “s” in the indicated places, even though it is irrelevant?

@jamshale
Copy link
Contributor Author

Given the 5 errors found (examples with an “http” protocol instead of “https” — would it be easiest to just add an “s” in the indicated places, even though it is irrelevant?

I think we should be able to disable this rule in the sonarcloud account or sonar-project.properties file, or the workflows. I'll try and look into this. I don't think using https should be required here.

@jamshale
Copy link
Contributor Author

I think the sonarcloud should just be safe to ignore. It's annoying, because we should be able to mark things a safe in sonarcloud but it requires an admin configuring things. Might try and figure it out on a personal account and then ping Ry.

@zoblazo
Copy link
Contributor

zoblazo commented Oct 29, 2024

  • seed parameter should not make any reference to did:indy as it only supports did:sov. It should be documented explicitely as such.

  • /wallet/did/create 'method' parameter is somewhat useless as did method indy has its own endpoint (and, from what I understand, so will any other new did method). so /wallet/did/create is purely for the did:sov method (and probably should become /did/sov/create to stay coherent with other did/*/create)

  • /did/indy/create cannot specify either the did or the encryption key type (as we can with /wallet/did/create). We should be able to fill out the wanted did as parameter.

  • /did/indy/create needs --wallet-allow-insecure-seed or else it won't work. That should be documented OR avoided.

  • seed_to_did: hexdigest vs digest Currently, the generated NYM does not seem to follow the specification (and does not generate same NYM as askar lib generates for a given seed). Again, we should have a way to provide the wanted did (and thus wanted NYM) to aca-py rather than have aca-py generate it for us. Please see my comments on issue 3240

@jamshale
Copy link
Contributor Author

jamshale commented Oct 29, 2024

I agree with pretty much all the points here, except a couple small points:

  • the seed parameter will work for a did:indy type did that was created with the /did/indy/create endpoint. It checks if if the seed works for a did:sov and then tries again with did:indy. If either passes it will use that did to startup.
  • Your right about /did/indy/create not having a did or encryption key type param. My thinking was to get a minimal create endpoint complete and the approach verified first. I can try adding the additional parameters in this PR.
  • There is likely changes to the /did/indy/create payload when we decide what the common design for all did methods is. I was somewhat waiting for the to get decided and agreed upon before adding did creation options/features.
  • I agree about the wallet/did/create endpoint being non intuitive and more like a /did/sov/create endpoint. I think this endpoint should be deprecated after we define the new did management endpoints fully.

I'll do a bit of work here and try and add the extra create options as minimally as possible so they can be changed without too much trouble.

@zoblazo
Copy link
Contributor

zoblazo commented Oct 29, 2024

the seed parameter will work for a did:indy type did that was created with the /did/indy/create endpoint. It checks if if the seed works for a did:sov and then tries again with did:indy. If either passes it will use that did to startup.

What's the use-case for this ? As we already feed the seed when calling /did/indy/create, and, thus, the key pair already generated, it seems redundant to feed it again at startup after the did has been provisionned post-deployment via the API. Or am I missing something here ?

@jamshale
Copy link
Contributor Author

jamshale commented Oct 29, 2024

If you have multiple local dids, you can tell it which one to use as the public did with the seed when starting up. So you could create a did:sov and a did:indy and tell it which one should be the public (active) did on startup.

Do you want to start a brand new agent with the seed option and have it create the did locally at the same time? This is so you can avoid doing it with the endpoints when you initialize an agent?

If that's the case I think it will need to be addressed with #3240.

@zoblazo
Copy link
Contributor

zoblazo commented Oct 29, 2024

If you have multiple dids, you can tell it which one to use as the public did with the seed when starting up.

Do you want to start a brand new agent with the seed option and have it create the did locally at the same time? This is so you can avoid doing it with the endpoints when you initialize an agent?

That's how we use --seed and how I interpreted its intent. I thought the public did status was stored and persisted and once a did had been promoted to public, it would stay public even after agent was restarted.

Then again, what SHOULD the --seed parameter be used for. As you'll read in my comments on issue 3240, for pretty much any did methods other than sov or indy, the association between seed and did/NYM makes no sens at all. seed, at its core, is a notion purely related to the generation of key pairs. ONLY did:indy (and to some extent did:sov) can manage to mix seed and did (via the NYM beeing generated from the public key part of the key pair)

@zoblazo
Copy link
Contributor

zoblazo commented Oct 29, 2024

If you have multiple local dids, you can tell it which one to use as the public did with the seed when starting up. So you could create a did:sov and a did:indy and tell it which one should be the public (active) did on startup.

Even there, if I have multiple local did, and I want to specify the one I want to use as public, then I should specify the did to promote public rather than the seed.

You're right that this is probably better suited in 3240. In the scope of this PR, I'd just exclude anything related with --seed (and indy) and consider --seed to be a pure did:sov parameter for now. And then see what happens in 3240. Otherwise it easily gets confusing for those who do use --seed to provision a new (public) did and expect it to work for a did:indy.

@jamshale
Copy link
Contributor Author

I don't really think the --seed parameter should be used, but I'm not really sure of the history of it. I think that's what that ticket #3240 is trying to figure out. I think the agent controller should create or ensure the did's are correct on startup. That's what the demo and integration tests do.

The public did is persisted, but it can be changed from one did to another. If you wanted to ensure a particular did:indy was the public did on startup you could use the --seed parameter. But yes, if you want to startup and create the did using the --seed parameter on a brand new agent then it would still create a did:sov.

This PR was mostly focused on adding the ability to create and use did:indy dids and allowing the seed parameter was a bit of an aside. So maybe that could get removed.

@zoblazo
Copy link
Contributor

zoblazo commented Nov 4, 2024

When I create a NYM transaction on an indy ledger with NYM transaction version == 2
When I call the /did/indy/create endpoint with a 'did' argument including the NYM previously created but no 'seed' argument
Then aca-py creates the requested did with a randomly generated seed and corresponding keypair
Then the generated verkey does not match the validation algorythm as documented in the specification ( did = Base58(Truncate_msb(16(SHA256(publicKey))))) )

To keep it simple for now, a suggestion would be for the 'seed' parameter to be mandatory when the 'did' parameter is included in a given /did/indy/create request.

@zoblazo
Copy link
Contributor

zoblazo commented Nov 5, 2024

Most probably this is out of scope for this PR but here's my thoughts. Please let me know if I should document this elsewhere.

Given I start acapy not providing the --wallet-allow-insecure-seed parameter
Given I then call the api endpoint /did/indy/create providing a seed
Then I get an error response 400: Insecure seed is not allowed
Expected : DID is created, no error response.

A did:indy is necessarely a public DID. Unless I'm mistaken, there is no use-case (and technically no logic) in a private did:indy. Since now /did/indy/create has its own endpoint, it is to be assumed that the intent is to create a public did. So this --wallet-allow-insecure-seed doesn't really make sens since specifically intended for custom seed used to create a local (private) DID (when POSTING on /wallet/did/create).

One could even argue that having to POST on /wallet/did/public afterwards is redundant but some use case might require to 'switch' between multiple public dids at some point in time (and it really does seem out of scope of this PR) so ... I'll keep this part out of the discussion

@jamshale
Copy link
Contributor Author

Sounds good. I reverted the other error responses other than the one I added in this PR. Will create separate issue to review the Forbidden responses.

Copy link
Member

@ff137 ff137 left a comment

Choose a reason for hiding this comment

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

Added some pedantic notes ...
The one about the "StartupError" -- Not sure if there's anything wrong there, was kinda just thinking out loud.

@jamshale
Copy link
Contributor Author

I'll go over this wallet startup stuff again. I was intending to refactor it without changing any behavior. But from the comments there is still some improvements that should be made.

Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
@jamshale jamshale requested a review from ff137 February 4, 2025 16:37
@dbluhm
Copy link
Contributor

dbluhm commented Feb 11, 2025

Approved but there's BDD tests failing...

@sonarqubecloud
Copy link

@jamshale jamshale merged commit 7c73d98 into openwallet-foundation:main Feb 21, 2025
11 checks passed
ff137 pushed a commit to didx-xyz/acapy that referenced this pull request Feb 27, 2025
* Add did:indy support

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Update from review comments

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Some refactoring and cleanup from comments

Signed-off-by: jamshale <jamiehalebc@gmail.com>

---------

Signed-off-by: jamshale <jamiehalebc@gmail.com>
Co-authored-by: Stephen Curran <swcurran@gmail.com>
Signed-off-by: ff137 <ff137@proton.me>
ff137 added a commit to didx-xyz/acapy that referenced this pull request Feb 27, 2025
ff137 pushed a commit to didx-xyz/acapy that referenced this pull request Feb 28, 2025
* Add did:indy support

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Update from review comments

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Some refactoring and cleanup from comments

Signed-off-by: jamshale <jamiehalebc@gmail.com>

---------

Signed-off-by: jamshale <jamiehalebc@gmail.com>
Co-authored-by: Stephen Curran <swcurran@gmail.com>
Signed-off-by: ff137 <ff137@proton.me>
ff137 added a commit to didx-xyz/acapy that referenced this pull request Apr 14, 2025
…tion#3253)"

This reverts commit 7c73d98.

Signed-off-by: ff137 <ff137@proton.me>
@ff137
Copy link
Member

ff137 commented Apr 14, 2025

This PR introduces the following exception in our existing e2e tests, when creating a connection between endorser and mt-agent

2025-04-11 14:14:32,470 aiohttp.server ERROR Error handling request from 127.0.0.6
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/aiohttp/web_protocol.py", line 480, in _handle_request
    resp = await request_handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aiohttp/web_app.py", line 569, in _handle
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aiohttp/web_middlewares.py", line 117, in impl
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 146, in ready_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 238, in debug_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 360, in setup_context
    return await task
           ^^^^^^^^^^
  File "/usr/local/lib/python3.12/asyncio/futures.py", line 289, in __await__
    yield self  # This tells Task to wait for completion.
    ^^^^^^^^^^
  File "/usr/local/lib/python3.12/asyncio/tasks.py", line 385, in __wakeup
    future.result()
  File "/usr/local/lib/python3.12/asyncio/futures.py", line 202, in result
    raise self._exception.with_traceback(self._exception_tb)
  File "/usr/local/lib/python3.12/asyncio/tasks.py", line 314, in __step_run_and_handle_result
    result = coro.send(None)
             ^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 222, in upgrade_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aiohttp_apispec/middlewares.py", line 51, in validation_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/decorators/auth.py", line 84, in tenant_auth
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/out_of_band/v1_0/routes.py", line 312, in invitation_create
    invi_rec = await oob_mgr.create_invitation(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/out_of_band/v1_0/manager.py", line 651, in create_invitation
    return await creator.create()
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/out_of_band/v1_0/manager.py", line 251, in create
    result = await self.handle_public(attachments, mediation_record)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/out_of_band/v1_0/manager.py", line 414, in handle_public
    return await self.handle_did(public_did, attachments, mediation_record)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/protocols/out_of_band/v1_0/manager.py", line 357, in handle_did
    endpoint, recipient_keys, routing_keys = await self.oob.resolve_invitation(
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/connections/base_manager.py", line 501, in resolve_invitation
    doc, didcomm_services = await self.resolve_didcomm_services(did, service_accept)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/connections/base_manager.py", line 450, in resolve_didcomm_services
    raise BaseConnectionManagerError(
acapy_agent.connections.base_manager.BaseConnectionManagerError: Cannot connect via DID that has no associated services
2025-04-11 14:14:32,474 aiohttp.access INFO 127.0.0.6 [11/Apr/2025:14:14:32 +0000] "POST /out-of-band/create-invitation?auto_accept=true HTTP/1.1" 500 247 "-" "OpenAPI-Generator/1.2.1-20250327/python"

Failing tests: https://github.com/didx-xyz/acapy-cloud/actions/runs/14404985029/job/40399536477?pr=1462
(Above log from "Governance Agent Logs")

All tests pass using our acapy fork image, with this PR commit dropped:
https://github.com/didx-xyz/acapy-cloud/actions/runs/14444169894/job/40501056475?pr=1462

At a glance, I can't see exactly what change in this PR would cause the issue

@ff137
Copy link
Member

ff137 commented Apr 14, 2025

@jamshale @swcurran

@jamshale
Copy link
Contributor Author

I'm still on vacation until Wednesday.

I thought this error was when the did doesn't have an endpoint associated with it. I don't know why the did:indy stuff would have an effect on it.

I thought this was expected with the removal of connection v1 protocols. I changed some tests in the interop tests to create a use of a did with an associated endpoint.

I can try to look at it when I get back.

@ff137
Copy link
Member

ff137 commented Apr 14, 2025

All good. I'll try isolate what the change was that causes the issue. Could be bad config on our side. But we're just calling out-of-band/create-invitation, with use_public_did=True, and the agent does have a valid did:sov on the ledger.

If I had to take a shot in the dark, it's maybe related to did-sov prefix changes, where qualified did is stripped to be unqualified. Perhaps something there.

@ff137
Copy link
Member

ff137 commented Apr 14, 2025

The difference in the logs for the successful scenario and the failure scenario is:

2025-04-14 11:17:34,649 acapy_agent.config.wallet INFO Created new public DID: 2WkBvUzBd1iFSHXhJNBwXi, Verkey: ppnGy93o7ZVstz7KqiWac8SecrCsrDfcveYCHiDVtqx
2025-04-14 11:17:34,737 acapy_agent.config.ledger INFO TAA acceptance needed - performing acceptance
2025-04-14 11:17:34,741 acapy_agent.config.ledger INFO TAA acceptance completed
2025-04-14 11:17:34,766 acapy_agent.ledger.indy_vdr INFO Endpoint or routing keys have changed, updating endpoint
2025-04-14 11:17:34,870 acapy_agent.config.ledger INFO Ledger configuration complete

(success)
vs
(failure)

2025-04-11 14:13:26,626 acapy_agent.config.wallet INFO Created new publicDID: 2WkBvUzBd1iFSHXhJNBwXi, Verkey: ppnGy93o7ZVstz7KqiWac8SecrCsrDfcveYCHiDVtqx
2025-04-11 14:13:26,626 acapy_agent.config.wallet INFO No public DID
2025-04-11 14:13:26,733 acapy_agent.config.ledger INFO Ledger configuration complete

This indicates the following log is triggered:

    if provision and not wallet_local_did and not public_did:
        LOGGER.info("No public DID")

Presumably public_did isn't being set.

And indeed:

        public_did = await _initialize_with_public_did(
            public_did_info, wallet, settings, wallet_seed
        )

But:

async def _initialize_with_public_did(
    public_did_info: DIDInfo,
    wallet: BaseWallet,
    settings: dict,
    wallet_seed: str,
) -> str:
    public_did = public_did_info.did
    # Check did:sov seed matches public DID
    if wallet_seed and (seed_to_did(wallet_seed) != public_did):
        if not settings.get("wallet.replace_public_did"):
            raise ConfigError(
                "New seed provided which doesn't match the registered"
                + f" public did {public_did}"
            )

        LOGGER.info("Replacing public DID due to --replace-public-did flag")
        replace_did_info = await wallet.create_local_did(
            method=SOV, key_type=ED25519, seed=wallet_seed
        )
        public_did = replace_did_info.did
        await wallet.set_public_did(public_did)
        LOGGER.info(
            f"Created new public DID: {public_did}, "
            f"with verkey: {replace_did_info.verkey}"
        )

The -> str is a lie :-) the function doesn't return anything.

I'll post a fix

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.

6 participants