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

IdentityHub should be able to use multiple local public keys #356

Closed
paullatzelsperger opened this issue May 23, 2024 · 5 comments · Fixed by #362
Closed

IdentityHub should be able to use multiple local public keys #356

paullatzelsperger opened this issue May 23, 2024 · 5 comments · Fixed by #362
Assignees
Labels
dcp enhancement New feature or request triage all new issues awaiting classification

Comments

@paullatzelsperger
Copy link
Member

paullatzelsperger commented May 23, 2024

Feature Request

When verifying access tokens on incoming presentation queries, we currently rely on a locally-supplied public key (LocalPublicKeySupplier), which relies either on a vault alias, a path or a raw (PEM) string.

There are two key problems with this:

  • it can only handle one public key
  • when IH generates the key pair when the ParticipantContext is created, the public key is not available locally, only from the DID document

Which Areas Would Be Affected?

AccessTokenVerifierImpl

Why Is the Feature Desired?

being able to handle multiple ParticipantContexts

Solution Proposal

We must parse the kid header from the access token to resolve the public key by ID.

Option 1:

When generating ParticipantContexts, we also store the public key in the vault. Then we can use the LocalPublicKeyService (from the Connector code base), which resolves the key by ID from the vault.

Option 2:

We extend the LocalPublicKeyService so that it also able to resolve public keys from the database. That way, we don't have to store the public key in the vault.

Option 3:

We simply use the DidPublicKeyResolver to resolve "our own" public key.

Caution: simply relying on the access token's kid header is not enough, we always need the participantContextId to avoid impersonation attacks!

@paullatzelsperger paullatzelsperger added enhancement New feature or request dcp labels May 23, 2024
@paullatzelsperger paullatzelsperger self-assigned this May 23, 2024
@github-actions github-actions bot added the triage all new issues awaiting classification label May 23, 2024
@thomasrutger
Copy link
Contributor

thomasrutger commented May 23, 2024

This would be a nice feature in my case. Once it is confirmed that this addition will be implemented and a solution option is chosen, but it is not a priority for you at this moment, I am willing to submit a contribution.

Option 1 seems to make most sense to me at this point. Otherwise option 1 can be tried and if it fails option 3 can be used as a fallback. I am unsure about the specifics of Option 2.

@paullatzelsperger
Copy link
Member Author

paullatzelsperger commented May 23, 2024

Thanks for offering, but we (the EDC technical committee) will need to align the approach (option 1, 2 or 3) with other ongoing developments and discussions. Once an approach is chosen the triage label is removed and implementation can start. Until such time, we ask for patience.

To me, Option 2 with a fallback to Option 3 would be best, because it avoids data duplication: DID documents are stored in the database and served from there, so we could just look in the DB for "our own" public key.

I'm not the biggest fan of keeping non-secret information in the vault, as resolving it usually involves remote calls and that may be costly.

@thomasrutger
Copy link
Contributor

Because I wanted to proceed with my demonstration setup, I meanwhile implemented it in the following way so that I could continue:

  • Add a LocalDatabasePublicKeyResolver that resolves a public key using the DidDocumentService
  • Add this LocalDatabasePublicKeyResolver to AccessTokenVerifierImpl and pass it to tokenValidationService.validate in verify method.
  • Add ParticipantContextService to AccessTokenVerifierImpl such that it can be used to find the DID using a participantId
  • Add issuerMustMatchParticipantIdRule to check if the issuer equals the matched participant DID to avoid abuse.

Alternatively DidDocumentService can be used in AccessTokenVerifierImpl to find DID corresponding to participantId, but I found using ParticipantContextService to find DID directly from a ParticipantContext easier.

Alternatively interface AccessTokenVerifier can be adapted to use ParticpantContext directly instead of participantId (but probably it is not desirable to change the interface).

In addition, I also had to address these issues: #360 and #361 in order to continue.

As said, I am open to submitting a PR, but I understand if patience is required.

@paullatzelsperger
Copy link
Member Author

paullatzelsperger commented Jun 3, 2024

@thomasrutger as coincidence would have it I had time to work on this today, see the linked PR (still draft). I think you got the general idea right (resolving keys from the database), although the devil may be in the details. Especially tightly coupling services may not be ideal.

Also, the verifier must check that resolved public key belongs to the sender (= participant ID) of the request.

as for those other issues, they'll have to go through triage first, and we shouldn't resolve multiple issues with one PR if it can be helped.

Naturally, to bridge the gap on your end, temporarily patching the code base is always a possibility :)

@thomasrutger
Copy link
Contributor

Excellent news! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dcp enhancement New feature or request triage all new issues awaiting classification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants