Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

LdpPresentationGenerator fails to add public key to a VP proof node. #330

Closed
ricardas-buc opened this issue Apr 26, 2024 · 7 comments
Closed
Labels
triage all new issues awaiting classification

Comments

@ricardas-buc
Copy link

Bug Report

With version 0.6.2 there seem to be a change of how VP proofs are populated. Missing publicKeyJwk leads to an verification exception in LdpVerifier as it fails to correctly parse verification method.

if (!(verificationMethod instanceof VerificationKey)) { return failure("Proof did not contain a valid VerificationMethod, expected VerificationKey, got: %s".formatted(verificationMethod.getClass())); }

Possible Implementation

LdpPresentationGenerator fails to add publicKeyJwk as it was not passed to proof draft verification method. JsonWebKeyPair uses provided key to derive public key.

https://github.com/eclipse-edc/IdentityHub/blob/main/core/identity-hub-credentials/src/main/java/org/eclipse/edc/identityhub/core/creators/LdpPresentationGenerator.java#L169

Passing jwk to the proof makes VP proof valid again.

        var proofDraft = Jws2020ProofDraft.Builder.newInstance()
                .proofPurpose(ASSERTION_METHOD)
                .verificationMethod(new JsonWebKeyPair(URI.create(controller + "#" + publicKeyId), verificationMethodType, controllerUri, jwk))
                .created(Instant.now())
                .mapper(mapper)
                .build();
@github-actions github-actions bot added the triage all new issues awaiting classification label Apr 26, 2024
Copy link

Thanks for your contribution 🔥 We will take a look asap 🚀

@paullatzelsperger
Copy link
Member

paullatzelsperger commented Apr 26, 2024

I think you don't have the whole picture here. LD proofs can have embedded JWKs, in which case you'd be correct, simply adding the jwk would solve the issue. But they can also be "linked", which means, you have to resolve the JWK during JSON-LD expansion. This basically downloads the JWK (which would have to be publicly available as JSON document) and inserts it into the expanded form of the signed credential.

Generally, LDP verification has to be done on the expanded form of the credential.

As a matter of security, public key material should not be embedded directly, but should be resolved from the credential/presentation issuer's DID to establish provenance.

@ricardas-buc
Copy link
Author

I think you don't have the whole picture here. LD proofs can have embedded JWKs, in which case you'd be correct, simply adding the jwk would solve the issue. But they can also be "linked", which means, you have to resolve the JWK during JSON-LD expansion. This basically downloads the JWK (which would have to be publicly available as JSON document) and inserts it into the expanded form of the signed credential.

Generally, LDP verification has to be done on the expanded form of the credential.

As a matter of security, public key material should not be embedded directly, but should be referenced from the credential/presentation issuer's DID to establish provenance.

Fair point. I will assume that even though it is possible, edc IH is not exepcted to populate it.

I did try using default approach, it fails due to mismatching interfaces in LdpVerifier.
image

org.eclipse.edc.verifiablecredentials.linkeddata.DidMethodResolver is being used, which always create DataIntegrityKeyPair. Another oversight on my end or a bug?

@paullatzelsperger
Copy link
Member

that's not a bug either :) the DidMethodResolver doesn't know anything about the specific verification method, JWK is just one, but there are others like RSA or Ed25519.
it will simply delegate the object down to the verification infrastructure.

@ricardas-buc
Copy link
Author

that's not a bug either :) the DidMethodResolver doesn't know anything about the specific verification method, JWK is just one, but there are others like RSA or Ed25519. it will simply delegate the object down to the verification infrastructure.

Sorry, I am still not fully grasping what should be correct implementation here. Is default MethodResolver not capable of passing LdpVerification checks as it will never provide correct verification method?

To me it looks like that DataIntegrityKeyPair class implements wrong interface.

0.6.1:
This class is part of iron vc lib.
Class inheritance: DataIntegrityKeyPair -> KeyPair -> VerificationKey -> VerificationMethod

0.6.2:
Class was moved to edc:
Class inheritance: DataIntegrityKeyPair -> VerificationMethod

In both version LdpVerifier checks VerificationKey interface:

if (!(verificationMethod instanceof VerificationKey)) {
                return failure("%s: %s".formatted(ErrorType.Unknown, methodProperty.term()));
            }

@paullatzelsperger
Copy link
Member

no, the key aspect is to remember, that all verification checks happen on the expanded JSON-LD of a LDP-Credential. Expansion is the process of dereferencing and resolving all linked data, such as the public key material.

That means, whether you embed the JWK or link it via a URL does not matter in the end, because both "variants" produce the same (ugly) JSON document. You have to expand though.

The LdpVerifier takes a String, which contains the credential/presentation in JSON(-LD) format.

If you have discovered bug, I would ask you to provide a reproducible test case, preferably as Gist, otherwise I would like to convert this issue in a discussion, as that seems more appropriate.

@ricardas-buc
Copy link
Author

Thanks. Since its not directly identity hub issue- lets convert this one to discussion then. I will try using referenced verification and see if it works.

Still, LdpVerifier and DataIntegrityKeyPair code looks a bit misleading or wrong. In any case, if that part will continue blocking me - will report with a gist in connector repo :)

@eclipse-edc eclipse-edc locked and limited conversation to collaborators Apr 28, 2024
@paullatzelsperger paullatzelsperger converted this issue into discussion #332 Apr 28, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
triage all new issues awaiting classification
Projects
None yet
Development

No branches or pull requests

2 participants