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

KeyPairResourcePublicKeyResolver.resolveKey does not properly parse fully qualified key id #399

Closed
thomasrutger opened this issue Jul 16, 2024 · 2 comments
Labels
triage all new issues awaiting classification

Comments

@thomasrutger
Copy link
Contributor

Bug Report

Describe the Bug

KeyPairResourcePublicKeyResolver.resolveKey should parse the fully qualified key id, as is documented in the method description.

Expected Behavior

when a fully qualified key as DID URL fragment is passed (e.g. did:web:someparticipant#key-123) as the argument to KeyPairResourcePublicKeyResolver.resolveKey, it should correctly parse the key.

Observed Behavior

The key is not correctly parsed, it takes the key id as is.

Steps to Reproduce

In my case, running the identityhub with STS and control plane with remote STS client triggered the issue, as it is unable to find the local public key associated with the access token.

Context Information

EDC 0.8.1-SNAPSHOT

Detailed Description

If applicable, add screenshots and logs to help explain your problem.

Possible Implementation

The logic to parse the key (did url fragment) actually already exists in DidPublicKeyResolverImpl. But this code cannot be easily used in this context. I think it would be wise to have a generic method for parsing DIDs that can be re-used.
I see 2 solutions:

  1. maven/mavencentral/com.apicatalog/carbon-did/0.3.0 is an approved dependency which contains an implementation for a Did URL representation. Currently, this dependency is transitively included through com.apicatalog:iron-verifiable-credentials:1.4.0. This can be used to parse the did and then simply retrieve the fragment. After inspection, it turns out this version (0.3.0) suffers from some issues. For instance it does not recognize Did url paths, among some other things (e.g. Did.from cannot handle more than 2 colons in method specific id. filip26/carbon-decentralized-identifiers#75). If this is the desired approach, then it should be considered to bump to 0.6.0.
  2. Provide own did representation and use that to parse the fq keyid and extract the fragment.

I think solution 1 should should be preferred as the com.apicatalog:carbon-did dependency has already been approved, but bumping the version should be considered.

The tests should also be updated.

Once the solution direction has been selected, I would like to have the opportunity to contribute a solution, if possible. Thanks.

@github-actions github-actions bot added the triage all new issues awaiting classification label Jul 16, 2024
@paullatzelsperger
Copy link
Member

paullatzelsperger commented Jul 16, 2024

@thomasrutger the resolver just takes the publicKeyId that you pass in and performs a lookup against the database, specifically against KeyPairResource.keyId, so there shouldn't be any parsing involved, just a normal DB lookup.

That KeyPairResource.keyId property is set during participant creation, specifically by the ParticipantManifest.key.keyId.

Obviously, when ParticipantManifest.key.keyId is "key-123", and you pass in "did:web:someparticipant#key-123" to the resolver, it will fail. This is different from when we resolve public keys from a DID document.

This has nothing to do with resolving or parsing DID documents, but with using key IDs consistently.

Maybe it would help if you could attach a test case (as gist) that clearly outlines your problem?

@thomasrutger
Copy link
Contributor Author

thomasrutger commented Jul 16, 2024

You are right, I thought about that before but apparently I messed it up which lead me to wrong conclusions. Thanks for clearing that up. I am closing this issue.

@thomasrutger thomasrutger reopened this Jul 16, 2024
@thomasrutger thomasrutger closed this as not planned Won't fix, can't repro, duplicate, stale Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage all new issues awaiting classification
Projects
None yet
Development

No branches or pull requests

2 participants