feat(sdk): Implement pluggable assertion binding and validation framework #317
Conversation
…work Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Summary of ChangesHello @cshamrick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Java SDK by introducing a flexible and extensible pluggable framework for handling TDF assertions. This refactoring moves away from a monolithic approach, allowing developers to easily integrate custom assertion logic and verification mechanisms. The changes improve the SDK's adaptability to evolving assertion standards and provide more robust control over the assertion lifecycle, from creation to validation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable pluggable framework for assertion binding and validation, significantly enhancing the SDK's flexibility. The overall implementation is solid, with a good separation of concerns and consideration for backward compatibility. My review includes several suggestions to improve the code further. I've identified a few critical issues, including a potential NullPointerException and a bug in a hash computation method, which should be addressed. Additionally, there are high-severity comments regarding brittle logic that relies on exception messages and areas with code duplication that could be refactored for better maintainability. Other comments focus on improving code clarity, consistency, and removing unused code. Addressing these points will strengthen the robustness and quality of this new framework.
| // SECURITY: Assertions without cryptographic bindings cannot be verified and must fail | ||
| // This prevents unsigned assertions from being tampered with | ||
| // Unsigned assertions represent a security risk and should not be accepted | ||
| if (assertion.binding.signature == null || assertion.binding.signature.isEmpty()) { |
There was a problem hiding this comment.
This check could cause a NullPointerException if assertion.binding is null. You should add a null check for assertion.binding before accessing assertion.binding.signature.
if (assertion.binding == null || assertion.binding.signature == null || assertion.binding.signature.isEmpty()) {| public ByteArrayOutputStream computeAggregateHash() { | ||
| ByteArrayOutputStream aggregateHash = new ByteArrayOutputStream(); | ||
| for (Manifest.Segment segment : this.encryptionInformation.integrityInformation.segments) { | ||
| byte[] decodedHash = Base64.getDecoder().decode(segment.hash); | ||
| try { | ||
| aggregateHash.write(decodedHash); | ||
| } catch (IOException e) { | ||
| throw new SDKException("failed to decode segment hash"); | ||
| } | ||
| } | ||
| return aggregateHash; | ||
| } |
There was a problem hiding this comment.
This instance method computeAggregateHash appears to have a bug. It always Base64-decodes the segment hash, which is only correct when the payload is encrypted. The static computeAggregateHash method correctly handles both encrypted and unencrypted cases. This method should delegate to the static one to ensure correct behavior and avoid code duplication. Additionally, the try-catch block for IOException within the loop is unnecessary as ByteArrayOutputStream.write does not throw IOException.
public ByteArrayOutputStream computeAggregateHash() {
try {
return computeAggregateHash(this.encryptionInformation.integrityInformation.segments, this.payload.isEncrypted);
} catch (IOException e) {
// This should not happen with ByteArrayOutputStream
throw new SDKException("failed to compute aggregate hash", e);
}
}| if (Objects.equals(assertion.binding.signature, "")) { | ||
| throw new SDK.AssertionException("assertion has no cryptographic binding", assertion.id); | ||
| } |
There was a problem hiding this comment.
The check for an empty signature is not robust enough. It should also handle cases where assertion.binding or assertion.binding.signature is null. A more comprehensive check is used elsewhere in the codebase (TDF.java:688) and should be applied here for consistency and to prevent potential NullPointerExceptions.
| if (Objects.equals(assertion.binding.signature, "")) { | |
| throw new SDK.AssertionException("assertion has no cryptographic binding", assertion.id); | |
| } | |
| if (assertion.binding == null || assertion.binding.signature == null || assertion.binding.signature.isEmpty()) { | |
| throw new SDK.AssertionException("assertion has no cryptographic binding", assertion.id); | |
| } |
| dekVerified = true; // DEK verification succeeded | ||
| validator = dekValidator; // Assign dekValidator as the effective validator | ||
| } catch (SDKException e) { | ||
| if (e.getMessage().equals("Unable to verify assertion signature")) { |
There was a problem hiding this comment.
Relying on the exception message string for control flow is brittle and error-prone. A future change to the error message in Manifest.Assertion.verify would break this logic silently. It would be much more robust to introduce a specific exception type (e.g., SignatureVerificationException) for this failure case and catch that specific type here.
| boolean useHex = tdfObject.manifest.tdfVersion == null || tdfObject.manifest.tdfVersion.isEmpty(); | ||
|
|
||
| var encodedHash = Base64.getEncoder().encodeToString(completeHash); | ||
| var assertionHashAsHex = assertion.hashAsHexEncodedString(); | ||
| byte[] assertionHashBytes; | ||
| if (useHex) { | ||
| assertionHashBytes = assertionHashAsHex.getBytes(StandardCharsets.UTF_8); | ||
| } else { | ||
| try { | ||
| assertionHashBytes = Hex.decodeHex(assertionHashAsHex); | ||
| } catch (DecoderException e) { | ||
| throw new SDKException("error decoding assertion hash", e); | ||
| } | ||
| } | ||
| byte[] completeHash = new byte[aggregateHash.size() + assertionHashBytes.length]; | ||
| System.arraycopy(aggregateHash.toByteArray(), 0, completeHash, 0, aggregateHash.size()); | ||
| System.arraycopy(assertionHashBytes, 0, completeHash, aggregateHash.size(), assertionHashBytes.length); | ||
|
|
||
| var assertionSigningKey = new AssertionConfig.AssertionKey(AssertionConfig.AssertionKeyAlg.HS256, | ||
| tdfObject.aesGcm.getKey()); | ||
| if (assertionConfig.signingKey != null && assertionConfig.signingKey.isDefined()) { | ||
| assertionSigningKey = assertionConfig.signingKey; | ||
| } | ||
| var hashValues = new Manifest.Assertion.HashValues( | ||
| assertionHashAsHex, | ||
| encodedHash); | ||
| try { | ||
| assertion.sign(hashValues, assertionSigningKey); | ||
| } catch (KeyLengthException e) { | ||
| throw new SDKException("error signing assertion hash", e); | ||
| var encodedHash = Base64.getEncoder().encodeToString(completeHash); | ||
| var hashValues = new Manifest.Assertion.HashValues(assertionHashAsHex, encodedHash, null); | ||
| try { | ||
| assertion.sign(hashValues, dekKey); | ||
| } catch (KeyLengthException e) { | ||
| throw new SDKException("error signing assertion hash", e); | ||
| } |
There was a problem hiding this comment.
The logic for calculating the assertion hash and signature inside this if block is a duplication of the Manifest.Assertion.calculateAssertionHashValues static method. To reduce code duplication and improve maintainability, you should call that helper method instead.
boolean useHex = tdfObject.manifest.tdfVersion == null || tdfObject.manifest.tdfVersion.isEmpty();
try {
var hashValues = Manifest.Assertion.calculateAssertionHashValues(aggregateHash, assertion, useHex);
assertion.sign(hashValues, dekKey);
} catch (IOException e) {
throw new SDKException("error calculating assertion hash for signing", e);
} catch (KeyLengthException e) {
throw new SDKException("error signing assertion hash", e);
}| import com.connectrpc.Interceptor; | ||
|
|
||
| import com.connectrpc.impl.ProtocolClient; | ||
| import com.nimbusds.jose.JOSEException; |
There was a problem hiding this comment.
The import com.nimbusds.jose.JOSEException is unused in this file and can be removed.
| import java.io.OutputStream; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.channels.SeekableByteChannel; | ||
| import java.text.ParseException; |
There was a problem hiding this comment.
The import java.text.ParseException is unused in this file and can be removed.
| // otherwise no explicit signing key provided - use the payload key (DEK) | ||
| // this is handled by passing the payload key from the TDF creation context | ||
| // for now, return the unsigned assertion - it will be signed by a DEK-based binder |
There was a problem hiding this comment.
These comments describe implementation details of another part of the system. While helpful for the developer, they are better suited for a broader design document or a more central part of the code. In this context, they might become outdated or confusing. Consider removing them to keep the code focused on its direct responsibilities.
| * // Schema returns the schema URI this validator handles. | ||
| * // The schema identifies the assertion format and version. | ||
| * // Examples: "urn:opentdf:system:metadata:v1", "urn:opentdf:key:assertion:v1" | ||
| * | ||
| * @return The schema URI. | ||
| */ | ||
| String getSchema(); | ||
|
|
||
| void setVerificationMode(AssertionVerificationMode verificationMode); | ||
|
|
||
| /** | ||
| * // Verify checks the assertion's cryptographic binding. | ||
| * // | ||
| * // Example: | ||
| * // assertionHash, _ := a.GetHash() | ||
| * // manifest := r.Manifest() | ||
| * // expectedSig, _ := manifest.ComputeAssertionSignature(assertionHash) | ||
| * | ||
| * @param assertion The assertion to verify. | ||
| * @param manifest The manifest. | ||
| * @throws SDK.AssertionException If the verification fails. | ||
| */ | ||
| void verify(Assertion assertion, Manifest manifest) throws SDK.AssertionException; | ||
|
|
||
| /** | ||
| * // Validate checks the assertion's policy and trust requirements |
There was a problem hiding this comment.
The Javadoc comments in this interface contain non-standard formatting (e.g., // prefixes) and examples from a different programming language (Go). Please update the comments for getSchema, verify, and validate to follow standard Javadoc conventions for clarity and consistency within the Java codebase.
| import com.nimbusds.jose.JOSEException; | ||
| import io.opentdf.platform.sdk.Manifest.Assertion; | ||
|
|
||
| import java.io.IOException; | ||
| import java.text.ParseException; |
There was a problem hiding this comment.
The following imports are unused in this interface and should be removed to keep the code clean: com.nimbusds.jose.JOSEException, java.io.IOException, java.text.ParseException.
X-Test Failure Report❌ go-v0.4.34 |
This PR introduces a new pluggable framework for assertion binding and validation within the Java SDK. This feature enhances the flexibility and extensibility of handling TDF assertions.
Key changes include:
Config.javahas been modified to support the configuration of the new assertion framework.