Skip to content

move things to an SPI pattern#372

Closed
mkleene wants to merge 1 commit into
DSPX-3309-hybrid-pq-key-wrappingfrom
DSPX-3309-hybrid-pq-key-wrapping-provider
Closed

move things to an SPI pattern#372
mkleene wants to merge 1 commit into
DSPX-3309-hybrid-pq-key-wrappingfrom
DSPX-3309-hybrid-pq-key-wrapping-provider

Conversation

@mkleene
Copy link
Copy Markdown
Contributor

@mkleene mkleene commented May 29, 2026

No description provided.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 97b96756-2ae9-4969-9989-d88bf00602dd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-3309-hybrid-pq-key-wrapping-provider

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the hybrid post-quantum key wrapping implementation by extracting it into a separate module (sdk-hybrid-bouncycastle) and introducing the HybridKeyWrapProvider Service Provider Interface (SPI). This decouples the core sdk module from a compile-time dependency on BouncyCastle, resolving implementations at runtime via ServiceLoader. The review feedback recommends adding defensive null checks for keyType and other parameters across several methods (such as wrapDEK, unwrapDEK, HybridKeyWrapResolver.get, and HybridTestKeys.generate) to prevent potential NullPointerExceptions when switching on enum values or processing null arguments.

Comment on lines +30 to +43
public byte[] wrapDEK(KeyType keyType, String publicKeyPem, byte[] dek) {
switch (keyType) {
case HybridXWingKey:
return XWingKeyPair.wrapDEK(XWingKeyPair.pubKeyFromPem(publicKeyPem), dek);
case HybridSecp256r1MLKEM768Key:
return HybridNISTKeyPair.P256_MLKEM768.wrapDEK(
HybridNISTKeyPair.P256_MLKEM768.pubKeyFromPem(publicKeyPem), dek);
case HybridSecp384r1MLKEM1024Key:
return HybridNISTKeyPair.P384_MLKEM1024.wrapDEK(
HybridNISTKeyPair.P384_MLKEM1024.pubKeyFromPem(publicKeyPem), dek);
default:
throw new SDKException("unsupported hybrid key type: " + keyType);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Java, switching on a null reference throws a NullPointerException. To prevent runtime failures, add defensive null checks for keyType, publicKeyPem, and dek before executing the switch statement.

    public byte[] wrapDEK(KeyType keyType, String publicKeyPem, byte[] dek) {
        if (keyType == null) {
            throw new SDKException("keyType cannot be null");
        }
        if (publicKeyPem == null) {
            throw new SDKException("publicKeyPem cannot be null");
        }
        if (dek == null) {
            throw new SDKException("dek cannot be null");
        }
        switch (keyType) {
            case HybridXWingKey:
                return XWingKeyPair.wrapDEK(XWingKeyPair.pubKeyFromPem(publicKeyPem), dek);
            case HybridSecp256r1MLKEM768Key:
                return HybridNISTKeyPair.P256_MLKEM768.wrapDEK(
                        HybridNISTKeyPair.P256_MLKEM768.pubKeyFromPem(publicKeyPem), dek);
            case HybridSecp384r1MLKEM1024Key:
                return HybridNISTKeyPair.P384_MLKEM1024.wrapDEK(
                        HybridNISTKeyPair.P384_MLKEM1024.pubKeyFromPem(publicKeyPem), dek);
            default:
                throw new SDKException("unsupported hybrid key type: " + keyType);
        }
    }

Comment on lines +46 to +59
public byte[] unwrapDEK(KeyType keyType, String privateKeyPem, byte[] wrappedDek) {
switch (keyType) {
case HybridXWingKey:
return XWingKeyPair.unwrapDEK(XWingKeyPair.privateKeyFromPem(privateKeyPem), wrappedDek);
case HybridSecp256r1MLKEM768Key:
return HybridNISTKeyPair.P256_MLKEM768.unwrapDEK(
HybridNISTKeyPair.P256_MLKEM768.privateKeyFromPem(privateKeyPem), wrappedDek);
case HybridSecp384r1MLKEM1024Key:
return HybridNISTKeyPair.P384_MLKEM1024.unwrapDEK(
HybridNISTKeyPair.P384_MLKEM1024.privateKeyFromPem(privateKeyPem), wrappedDek);
default:
throw new SDKException("unsupported hybrid key type: " + keyType);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Java, switching on a null reference throws a NullPointerException. To prevent runtime failures, add defensive null checks for keyType, privateKeyPem, and wrappedDek before executing the switch statement.

    public byte[] unwrapDEK(KeyType keyType, String privateKeyPem, byte[] wrappedDek) {
        if (keyType == null) {
            throw new SDKException("keyType cannot be null");
        }
        if (privateKeyPem == null) {
            throw new SDKException("privateKeyPem cannot be null");
        }
        if (wrappedDek == null) {
            throw new SDKException("wrappedDek cannot be null");
        }
        switch (keyType) {
            case HybridXWingKey:
                return XWingKeyPair.unwrapDEK(XWingKeyPair.privateKeyFromPem(privateKeyPem), wrappedDek);
            case HybridSecp256r1MLKEM768Key:
                return HybridNISTKeyPair.P256_MLKEM768.unwrapDEK(
                        HybridNISTKeyPair.P256_MLKEM768.privateKeyFromPem(privateKeyPem), wrappedDek);
            case HybridSecp384r1MLKEM1024Key:
                return HybridNISTKeyPair.P384_MLKEM1024.unwrapDEK(
                        HybridNISTKeyPair.P384_MLKEM1024.privateKeyFromPem(privateKeyPem), wrappedDek);
            default:
                throw new SDKException("unsupported hybrid key type: " + keyType);
        }
    }

Comment on lines +36 to +44
static HybridKeyWrapProvider get(KeyType keyType) {
for (HybridKeyWrapProvider p : Holder.PROVIDERS) {
if (p.supports(keyType)) {
return p;
}
}
throw new SDKException("No HybridKeyWrapProvider registered for " + keyType
+ ". Add io.opentdf.platform:sdk-hybrid-bouncycastle to your classpath.");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Add a defensive null check for keyType at the beginning of the get method to prevent potential NullPointerException or unexpected behavior during resolution.

    static HybridKeyWrapProvider get(KeyType keyType) {
        if (keyType == null) {
            throw new SDKException("keyType cannot be null");
        }
        for (HybridKeyWrapProvider p : Holder.PROVIDERS) {
            if (p.supports(keyType)) {
                return p;
            }
        }
        throw new SDKException("No HybridKeyWrapProvider registered for " + keyType
                + ". Add io.opentdf.platform:sdk-hybrid-bouncycastle to your classpath.");
    }

Comment on lines +31 to +32
public static PemPair generate(KeyType keyType) {
switch (keyType) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Java, switching on a null reference throws a NullPointerException. Add a defensive null check for keyType before executing the switch statement.

    public static PemPair generate(KeyType keyType) {
        if (keyType == null) {
            throw new SDKException("keyType cannot be null");
        }
        switch (keyType) {

@mkleene mkleene changed the base branch from DSPX-3309-hybrid-pq-key-wrapping to main May 29, 2026 15:49
@mkleene mkleene changed the base branch from main to DSPX-3309-hybrid-pq-key-wrapping May 29, 2026 15:58
@mkleene mkleene closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant