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

Use did:key for recipient keys #1886

Merged

Conversation

TheTechmage
Copy link
Contributor

As part of #1859, I have made it so the coordinate-mediation protocol uses did:key representation. Apologies that this took as long as it did. Implementing this impacted more of ACA-Py than I realized it would while keeping it compatible with existing agents.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
This reverts commit d0b19d9.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Also attempt to revert key formats before forwarding messages

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Add the RoutingKey Validator that supports both DIDKey and Raw Indy
Public Keys for backwards compatibility reasons

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage
Copy link
Contributor Author

@dbluhm It looks like I could have gotten away without modifying the MediationRecord. I would just have had to replace every instance of record.routing_keys = grant.routing_keys (for example) with the conversion code.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 5, 2022

@dbluhm It looks like I could have gotten away without modifying the MediationRecord. I would just have had to replace every instance of record.routing_keys = grant.routing_keys (for example) with the conversion code.

In the interest of the principle of least change, would this help us avoid needing to modify connections, didexchange, pack, etc.? Might be a bit of a pain to make that change now but if it results in a cleaner set of changes, that might be worth it.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2022

Codecov Report

Merging #1886 (465a09b) into main (a8101e1) will increase coverage by 0.00%.
The diff coverage is 97.29%.

@@           Coverage Diff           @@
##             main    #1886   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files         539      540    +1     
  Lines       34162    34187   +25     
=======================================
+ Hits        32003    32027   +24     
- Misses       2159     2160    +1     

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage force-pushed the fix/RFC-0211-Compliance branch from c827dd5 to dc85e32 Compare August 16, 2022 17:23
@TheTechmage
Copy link
Contributor Author

@dbluhm I believe that addresses your feedback

@TheTechmage TheTechmage marked this pull request as ready for review August 16, 2022 17:48
dbluhm
dbluhm previously approved these changes Aug 17, 2022
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.

Looks good! I left some minor suggestions but they're not pressing. The only other comment coming to mind is that we should document that we receive did:key values but are storing the keys as base58 encoded strings still.

Comment on lines 37 to 42
if recipient_key.startswith("did:key:"):
self.recipient_key = recipient_key
else:
self.recipient_key = DIDKey.from_public_key_b58(
recipient_key, KeyType.ED25519
).did
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a helper method similar to the normalize_public_key method would be good since we perform this normalization several times throughout the messages.inner package.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage force-pushed the fix/RFC-0211-Compliance branch from 70bb6e2 to 36f1230 Compare August 22, 2022 14:55
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
3.1% 3.1% Duplication

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.

Looks good! Nice work!

@swcurran
Copy link
Contributor

@frostyfrog -- can you please check the failed integration test. It's a "can't find a DID" error which may be related to the PR (or not... :-) ). From the test: "7hAcme.agent | 2022-08-23 22:21:35,515 aries_cloudagent.connections.base_manager WARNING No corresponding DID found for recipient verkey: CcGfdN6DgibjWEU3kShAkHC6f1eyMAghfi8hknrZEZFF"

@TheTechmage
Copy link
Contributor Author

Interesting that it's failing now. I'll take a look!

@TheTechmage
Copy link
Contributor Author

When I pulled down the latest changes (the two merges), I was unable to reproduce the error locally. Opening up the logs from the failed test reveals that my changes may not be a likely cause here. There was an exception in the revocation code where ACA-Py received something that did not match the revocation schema.

Logs below:

7hBob.agent  | 2022-08-23 22:21:54,679 aries_cloudagent.admin.server ERROR Handler error with exception: Unprocessable Entity
7hBob.agent  | Traceback (most recent call last):
7hBob.agent  |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/webargs/asyncparser.py", line 90, in parse
7hBob.agent  |     result = schema.load(parsed)
7hBob.agent  |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/marshmallow/schema.py", line 723, in load
7hBob.agent  |     data, many=many, partial=partial, unknown=unknown, postprocess=True
7hBob.agent  |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/marshmallow/schema.py", line 904, in _do_load
7hBob.agent  |     raise exc
7hBob.agent  | marshmallow.exceptions.ValidationError: {'rev_reg_id': ['Value None is not an indy revocation registry identifier']}
7hBob.agent  | 
7hBob.agent  | During handling of the above exception, another exception occurred:
7hBob.agent  | 
7hBob.agent  | Traceback (most recent call last):
7hBob.agent  |   File "/home/indy/aries_cloudagent/admin/server.py", line 171, in ready_middleware
7hBob.agent  |     return await handler(request)
7hBob.agent  |   File "/home/indy/aries_cloudagent/admin/server.py", line 208, in debug_middleware
7hBob.agent  |     return await handler(request)
7hBob.agent  |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp_apispec/middlewares.py", line 34, in validation_middleware
      Traceback (most recent call last):
        File "/home/indy/demo/runners/support/agent.py", line 848, in admin_request
          resp.raise_for_status()
        File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp/client_reqrep.py", line 1009, in raise_for_status
          headers=self.headers,
      aiohttp.client_exceptions.ClientResponseError: 422, message='Unprocessable Entity', url=URL('http://10.1.0.158:8031/revocation/registry/None')
      
      The above exception was the direct cause of the following exception:
      
      Traceback (most recent call last):
        File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features/steps/0586-sign-transaction.py", line 375, in step_impl
          f"/revocation/registry/{context.rev_reg_id}",
        File "/home/indy/demo/bdd_support/agent_backchannel_client.py", line 200, in agent_container_GET
          params=params,
        File "/home/indy/demo/bdd_support/agent_backchannel_client.py", line 19, in run_coroutine
          return loop.run_until_complete(coroutine(*args, **kwargs))
        File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/asyncio/base_events.py", line 488, in run_until_complete
          return future.result()
        File "/home/indy/demo/runners/agent_container.py", line 1015, in admin_GET
          return await self.agent.admin_GET(path, text=text, params=params)
        File "/home/indy/demo/runners/support/agent.py", line 895, in admin_GET
          "GET", path, None, text, params, headers=headers
        File "/home/indy/demo/runners/support/agent.py", line 851, in admin_request
          raise Exception(f"Error: {resp_text}") from e
      Exception: Error: {"rev_reg_id": ["Value None is not an indy revocation registry identifier"]}

Copy link
Contributor

@andrewwhitehead andrewwhitehead left a comment

Choose a reason for hiding this comment

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

LGTM!

@swcurran swcurran merged commit 2637941 into openwallet-foundation:main Aug 25, 2022
@dbluhm
Copy link
Contributor

dbluhm commented Aug 25, 2022

If you pull these changes and attempt to use a mediator that has not been updated, you'll see problem reports bouncing back from the mediator on keylist update messages:

2022-08-25 09:16:50,493 aries_cloudagent.messaging.base_handler INFO Received problem report from: MGY45PK93sKVJjHRLJxA1p, <ProblemReport(_message_id='7ef74d0d-3801-4573-a6fe-79932a016442', _message_new_id=False, _message_decorators=<DecoratorSet{~thread: <ThreadDecorator(_thid='af21ac95-4492-4e06-b096-c39103909bcd', _pthid=None, _sender_order=None, _received_orders=None)>}>, description={'en': 'Error deserializing message: KeylistUpdate schema validation failed', 'code': 'message-parse-failure'}, problem_items=None, who_retries=None, fix_hint=None, impact=None, where=None, noticed_time=None, tracking_uri=None, escalation_uri=None)>

Mediators can account for clients that continue to use the original raw key format but the inverse is unfortunately more complicated to account for.

@swcurran
Copy link
Contributor

I assume we should update the aries-mediator-service to use the latest? How is it determined what version of ACA-Py to use? Do we need to do an "-rc1" for that upgrade? @dbluhm -- do you have someone that can take that on? I assume that Indicio will be updating their public mediator?

Thanks!

@dbluhm
Copy link
Contributor

dbluhm commented Aug 25, 2022

Sure, I'll add that to the to-do list 🙂

cc @reflectivedevelopment

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.

5 participants