JwtPresentationGenerator.generatePresentation adds incorrect public key id to VP

[thomasrutger]

Bug Report

Describe the Bug

It seems that in JwtPresentationGenerator.generatePresentation. The public key id is incorrectly set. This is because the additionalData[CONTROLLER_ADDITIONAL_DATA] that is set in VerifiablePresentationServiceImpl.createPresentation is set to the participantContextId, instead of the participants' DID. JwtPresentationGenerator.generatePresentation uses the controllerId to check if the publicKey id is correctly formatted,but since this is not the DID it will not yield the desired result.

This for me is resulting in incorrectly formatted public key ids. eg when my public key id is did:web:example#example-key it gets formatted as example#did:web:example#example-key`.

in the JwtPresentationGeneratorTest the controller is correctly set to a did and not a participantContextId.

I haven't tested, but LdpPresentationGenerator might have the same problem.

Expected Behavior

I expect the keyId to be correctly formatted.

Observed Behavior

Incorrectly formatted public keyId, verifier cannot verify the VP.

Steps to Reproduce

Context Information

Add any other context about the problem here.

version: 0.8.1-SNAPSHOT

Detailed Description

Possible Implementation

Possible solutions:

• Use the publicKeyId as is and assume it is already encoded as the DID fragment.
• Add the controller DID to additional data map only in PresentationCreatorRegistryImpl.createPresentation, after the DID has been resolved.
• Add the DID as CONTROLLER_ADDITIONAL_DATA instead of the participantContextId. Then pass both the DID and participantContextId to PresentationCreatorRegistryImpl.createPresentation, so it does not have the be resololved twice.
• Use a PresentationParameters class of sorts with a Builder (possibly use decorators (like TokenParameters)).

[paullatzelsperger]

I don't think that is the case: VerifiablePresentationServiceImpl calls out to the PresentationCreatorRegistryImpl:

IdentityHub/core/lib/verifiable-presentation-lib/src/main/java/org/eclipse/edc/identithub/verifiablepresentation/VerifiablePresentationServiceImpl.java

Line 98 in c444f67

String jwtVp = registry.createPresentation(participantContextId, jwtVcs, CredentialFormat.JWT, additionalDataJwt);

which in turn delegates to a PresentationGenerator, but resolves the did beforehand:

IdentityHub/core/lib/verifiable-presentation-lib/src/main/java/org/eclipse/edc/identithub/verifiablepresentation/PresentationCreatorRegistryImpl.java

Lines 72 to 77 in c444f67

	var did = participantContextService.getParticipantContext(participantContextId)
	.map(ParticipantContext::getDid)
	.orElseThrow(f -> new EdcException(f.getFailureDetail()));
	return (T) creator.generatePresentation(credentials, keyPair.getPrivateKeyAlias(), keyPair.getKeyId(), did, additionalData);

in addition, the controller metadata is prepended to the key-id, if it does not start with already:

IdentityHub/core/lib/verifiable-presentation-lib/src/main/java/org/eclipse/edc/identithub/verifiablepresentation/generators/JwtPresentationGenerator.java

Lines 103 to 105 in c444f67

	if (!publicKeyId.startsWith(controller)) {
	composedKeyId = controller + "#" + publicKeyId;
	}

[thomasrutger]

Yes, but additionalData[CONTROLLER_ADDITIONAL_DATA] contains the participantContextId and not the DID, which I think it should be? See:

IdentityHub/core/lib/verifiable-presentation-lib/src/main/java/org/eclipse/edc/identithub/verifiablepresentation/VerifiablePresentationServiceImpl.java

Lines 82 to 84 in c444f67

	var additionalDataJwt = new HashMap<String, Object>();
	ofNullable(audience).ifPresent(aud -> additionalDataJwt.put(AUDIENCE, audience));
	additionalDataJwt.put(CONTROLLER_ADDITIONAL_DATA, participantContextId);

[paullatzelsperger]

ah I see, yes, that seems incorrect. I guess a good way to solve this, would be to add the "controller" entry in the PresentationCreatorRegistryImpl#createPresentation method, since we already resolve the did there anyway.

something along those lines: https://gist.github.com/paullatzelsperger/4f58ce0a39a98360462cd9459ef8b630

[thomasrutger]

That is pretty much exactly my local workaround indeed, seems like the best way to fix it. Would it be alright if I submit a pull request with the changes?

[paullatzelsperger]

That is pretty much exactly my local workaround indeed, seems like the best way to fix it. Would it be alright if I submit a pull request with the changes?

sure go ahead :)