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

feat: disable consumer pull transfers if key settings are missing #3820

Merged
merged 2 commits into from Feb 1, 2024

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Feb 1, 2024

What this PR changes/adds

Add settings check to TransferDataPlaneCoreExtension, if the ones related to the private/public keys are missing, then the "consumer-pull" transfer won't be enabled.

Why it does that

avoid weird exceptions when settings are missing

Further notes

  • added a check also in the JwtGenerationService to avoid null pointer exceptions there in the case the key is not available.

Linked Issue(s)

Closes #3810

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@ndr-brt ndr-brt added the enhancement New feature or request label Feb 1, 2024
@ndr-brt ndr-brt requested a review from wolf4ood February 1, 2024 09:02
private JWSVerifier createVerifier(JWSHeader header, Key publicKey) throws JOSEException {
return new DefaultJWSVerifierFactory().createJWSVerifier(header, publicKey);
}

private RSAKey testKey() throws JOSEException {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I would use Ed25519 keys wherever possible, as they seem to emerge as the new quasi-standard. also, they are much faster to generate than RSA keys:
with Nimbus (this is the one we'd need here):

JWK ed25519Key = new OctetKeyPairGenerator(Curve.Ed25519).generate();

with plain Java:

KeyPair kp = KeyPairGenerator.getInstance("Ed25519").generateKeyPair();

using the BouncyCastle provider:

KeyPair kp = KeyPairGenerator.getInstance("Ed25519", new BouncyCastleProvider()).generateKeyPair();

Copy link
Member Author

Choose a reason for hiding this comment

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

with the first one I get a Export to java.security.PrivateKey not supported, the second one tells me that the algorithm is not supported...
to be honest I didn't change the code here, just moved the method at the bottom, if you're ok with it I would leave it as it is

Copy link
Member

Choose a reason for hiding this comment

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

yeah it's fine. we can do that in a housekeeping task somewhere down the road.

Just FYI: the Nimbus one probably fails, because the module doesn't have the Tink library on the (runtime) classpath.

Copy link
Contributor

@wolf4ood wolf4ood left a comment

Choose a reason for hiding this comment

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

LGTM just a minor nit on logging

Comment on lines 118 to 119
context.getMonitor().info("One of these settings is not configured, so the connector won't be able to provide 'consumer-pull' transfers: [%s, %s]"
.formatted(TOKEN_VERIFIER_PUBLIC_KEY_ALIAS, TOKEN_SIGNER_PRIVATE_KEY_ALIAS));
Copy link
Contributor

Choose a reason for hiding this comment

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

The logging i think it should be on the else branch

Copy link
Member Author

Choose a reason for hiding this comment

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

woa, you're right 😅

@ndr-brt ndr-brt merged commit 695e83e into eclipse-edc:main Feb 1, 2024
17 checks passed
@ndr-brt ndr-brt deleted the 3810-consumer-pull branch February 1, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: transfer-data-plane - Initiate Transfer Process Fails
3 participants