Skip to content

Add option for verifying signed remote attestation #28

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AnthonyRonning
Copy link
Contributor

@AnthonyRonning AnthonyRonning commented Mar 26, 2025

Summary by CodeRabbit

  • New Features

    • Enhanced attestation validation with asynchronous checks that perform comprehensive local and remote verifications.
    • Introduced historical data caching to boost performance and reliability during the validation process.
    • Added new functions for managing and validating PCR history, including remote validation capabilities.
    • Implemented a new interface for structured PCR management.
    • Introduced a new function for resetting global test state to improve test isolation.
  • Tests

    • Added integration tests to confirm robust validation behavior and smooth error handling in various scenarios.
    • Improved test setup to ensure clean state management between tests.

Copy link

coderabbitai bot commented Mar 26, 2025

Walkthrough

The changes introduce asynchronous PCR validation enhancements. A new function validatePcrSet validates a complete PCR set by calling the existing validatePcr0Hash asynchronously. The PCR module now supports remote validations via an optional remoteValidationUrl, and a new PCR history module has been added to cache and verify PCR history data. The module's interface is extended via exports in the index, and integration tests have been added to ensure proper local and remote validation behavior.

Changes

File(s) Summary of Changes
src/lib/attestationForView.ts Introduced new async function validatePcrSet for validating PCR0, PCR1, and PCR2; updated parseAttestationForView to perform asynchronous PCR0 validation.
src/lib/index.ts Added exports for the new PCR functionalities: validatePcrSet (from attestationForView), validatePcr0Hash (from pcr), and PCR history functions/types (PcrEntry, validatePcrAgainstHistory, getPcrHistoryList).
src/lib/pcr.ts Updated validatePcr0Hash to be asynchronous, returning a promise; added an optional remoteValidationUrl property to PcrConfig and implemented logic to perform remote validation via validatePcrAgainstHistory when needed.
src/lib/pcrHistory.ts New module implementing PCR history management; defines the PcrEntry interface and provides functions for fetching, caching (15-minute expiration), signature verification, and history-based validation (getPcrHistoryList, loadPcrPublicKey, verifyPcrSignature, validatePcrAgainstHistory).
src/lib/test/integration/pcr.test.ts Added integration tests for PCR validations, including tests for local validation, remote validation simulation, PCR history validation, and error handling using mocked fetch and localStorage.
src/lib/test/integration/developer.test.ts Enhanced test setup by adding clearPcrHistoryCache and resetTestState to ensure clean state between tests.
src/lib/test/preload.ts Introduced resetTestState function to clear global state between tests, including localStorage, sessionStorage, and invoking clearPcrHistoryCache.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant V as validatePcrSet
    participant H as validatePcr0Hash
    participant P as validatePcrAgainstHistory
    C->>V: Call validatePcrSet(pcr0, pcr1, pcr2, config)
    V->>H: Validate PCR0 hash asynchronously
    alt Local Validation Success
        H-->>V: Return valid result
    else Remote Validation Required
        H->>P: Validate PCR against history
        P-->>H: Return remote validation result
    end
    V-->>C: Return PCR0ValidationResult
Loading

Poem

I hop through lines of code with glee,
New async checks set my worries free.
PCR values dance in a validation spree,
Remote and local join in a merry decree.
With each function call, I feel so light,
Coding by day and hopping by night! 🐰✨

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added remote PCR validation capabilities with signature verification for enhanced security attestation in the OpenSecret SDK.

  • Added pcrHistory.ts with caching and ECDSA signature verification for remote PCR validation
  • Modified validatePcr0Hash in pcr.ts to support async remote validation via remoteValidationUrl config
  • Added integration tests in pcr.test.ts for both local and remote validation paths
  • Potential security concern: Silent error handling in validatePcr0Hash could mask validation failures
  • Misleading implementation: validatePcrSet only validates PCR0 despite suggesting full PCR set validation

5 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🔭 Outside diff range comments (1)
src/lib/pcrHistory.ts (1)

189-203: 🛠️ Refactor suggestion

isDev depends on window.localStorage.
If you ever run these utilities in Node or a non-browser environment, direct references to window may cause errors. Consider adding checks or fallbacks for server-side usage if that’s a future concern.

🧹 Nitpick comments (7)
src/lib/attestationForView.ts (1)

84-89: Consider utilizing pcr1 and pcr2 values
Lines 84–89 highlight that pcr1 and pcr2 remain unused. If comprehensive PCR validation is intended, integrate them into the existing validation flow. Otherwise, as suggested, update the documentation to avoid confusion.

src/lib/pcr.ts (1)

68-77: Remote validation error handling
When an error occurs during remote validation, the function logs it but returns an “unknown” mismatch result instead of propagating the error. Consider returning a distinguishable error or result state informing the caller of the remote validation failure.

} catch (error) {
  console.error("Error with remote PCR validation:", error);
+ return {
+   isMatch: false,
+   text: "Remote PCR validation attempt failed"
+ };
}
src/lib/test/integration/pcr.test.ts (3)

21-32: Consider adding negative test entries.
To bolster coverage, you could add malformatted or invalid signatures to ensure the validation logic in downstream functions handles malformed PCR entries gracefully.


40-55: Mock fetch usage is solid.
Providing a default mock response in beforeEach is concise. As an enhancement, you might track the calls to fetch using an assertion library to confirm it’s invoked with the expected URL.


74-97: Remote validation coverage is adequate.
It might be beneficial to assert that fetch was called with the remoteValidationUrl, validating that the remote path is indeed attempted.

src/lib/pcrHistory.ts (2)

1-16: Interface design is explicit.
Specifying discrete PCR fields is straightforward, but if more registers are introduced in the future, consider a more extensible structure (e.g., a map or array).


24-68: Robust error handling in getPcrHistoryList.
It’s good practice to return an empty array on errors, but consider logging partial data if any subset is retrievable. This could help with debugging partial fetch failures.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad54fca and f6acd85.

📒 Files selected for processing (5)
  • src/lib/attestationForView.ts (2 hunks)
  • src/lib/index.ts (1 hunks)
  • src/lib/pcr.ts (2 hunks)
  • src/lib/pcrHistory.ts (1 hunks)
  • src/lib/test/integration/pcr.test.ts (1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/lib/attestationForView.ts (2)
src/lib/index.ts (4)
  • validatePcrSet (33-33)
  • PcrConfig (34-34)
  • Pcr0ValidationResult (34-34)
  • validatePcr0Hash (35-35)
src/lib/pcr.ts (3)
  • PcrConfig (32-36)
  • Pcr0ValidationResult (27-30)
  • validatePcr0Hash (46-83)
src/lib/test/integration/pcr.test.ts (2)
src/lib/pcrHistory.ts (2)
  • PcrEntry (9-16)
  • validatePcrAgainstHistory (141-187)
src/lib/pcr.ts (1)
  • validatePcr0Hash (46-83)
src/lib/pcrHistory.ts (2)
src/lib/index.ts (5)
  • PcrEntry (36-36)
  • getPcrHistoryList (37-37)
  • validatePcrAgainstHistory (37-37)
  • PcrConfig (34-34)
  • Pcr0ValidationResult (34-34)
src/lib/pcr.ts (2)
  • PcrConfig (32-36)
  • Pcr0ValidationResult (27-30)
🔇 Additional comments (14)
src/lib/index.ts (2)

33-33: Exporting the new validatePcrSet function
This addition cleanly exposes the new PCR set validation functionality to consumers.


35-37: Good addition of PCR-related exports
Exporting validatePcr0Hash, PcrEntry, and the history methods broadens the module’s interface for downstream consumers to flexibly handle PCR validation and history verification.

src/lib/pcr.ts (4)

35-35: Optional remoteValidationUrl property
Good addition for remote PCR validation. Ensure that calling code handles potential timeouts or unreachable endpoints to avoid hanging.


38-38: Import for remote PCR validation
Importing validatePcrAgainstHistory nicely consolidates the validation logic. No issues observed.


53-53: Local validation comment
No functional changes needed here. The comment accurately reflects the intent to match against baseline hashes before attempting remote validation.


46-49:

✅ Verification successful

Asynchronous function signature
Changing validatePcr0Hash to async aligns with remote validations. Please confirm all calling sites await or handle the Promise correctly.


🏁 Script executed:

#!/bin/bash
# Searching for all calls to validatePcr0Hash to verify it’s awaited or handled
rg -A 2 "validatePcr0Hash"

Length of output: 2564


Async Handling Verified

All instances where validatePcr0Hash is called are correctly either awaited or have proper Promise handling. The integration tests in src/lib/test/integration/pcr.test.ts and its usage in src/lib/attestationForView.ts show no issues with the asynchronous implementation.

src/lib/test/integration/pcr.test.ts (4)

3-15: Local mock for localStorage is well-structured.
This approach effectively isolates tests from the actual browser storage. Consider implementing event-related behaviors (e.g., the storage event) if those are needed in testing future features.


59-72: Straightforward test for hardcoded PCR0 values.
The logic here is clear and ensures that known good values pass validation. Good job.


98-121: Validate final response in addition to calling the function.
To further confirm correctness, consider asserting the returned isMatch and text fields to ensure the function’s success path is tested thoroughly.


123-139: Proper handling of network errors.
This test accurately covers failure scenarios, confirming that an empty or invalid result returns a negative match.

src/lib/pcrHistory.ts (4)

18-23: Cache mechanism is clear and direct.
Defining CACHE_EXPIRATION is clear for time-based invalidation. This is a well-structured approach to limit frequent network requests.


70-94: Key import strategy aligns with ECDSA best practices.
Decoding the base64 key and importing in spki format is a standard approach. This code block looks well-implemented for cryptographic usage.


96-133: Signature verification logic is methodical.
Excluding the signature field from the signed message is correct if that matches the backend’s signing format. Good job handling potential exceptions gracefully.


135-187: validatePcrAgainstHistory thoroughly checks and verifies.
The logic to iterate through each entry and verify signatures is correct. If multiple matching entries exist, consider short-circuiting once a valid signature is found, which you already do.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (7)
src/lib/attestationForView.ts (1)

45-65: Function validates PCR0 only, future expansion needed for PCR1/PCR2

While the function name and documentation suggest validating a complete PCR set (PCR0, PCR1, PCR2), the implementation currently only validates PCR0. The parameters pcr1 and pcr2 are accepted but unused.

Consider adding a TODO comment within the function body to make it clearer that PCR1 and PCR2 validation will be implemented in the future:

export async function validatePcrSet(
  pcr0: string,
  pcr1: string,
  pcr2: string,
  pcrConfig?: PcrConfig
): Promise<Pcr0ValidationResult> {
  // First check with local and remote validation
  const validation = await validatePcr0Hash(pcr0, pcrConfig);

+  // TODO: Add validation for PCR1 and PCR2 values in future updates
+
  // If validation passed, return the result
  // This includes both local values and remote history checks
  return validation;
}
src/lib/test/preload.ts (1)

39-39: Consider clarifying usage of global.window = global.
Setting global.window to global is often a test environment hack to emulate a browser context in Node. While it works, this can lead to confusion about the presence of window in non-browser contexts.

src/lib/test/integration/developer.test.ts (2)

6-7: Imports may be redundant.
clearPcrHistoryCache is already called inside resetTestState(), so calling it again may be unnecessary. You might want to unify the approach into a single utility function or remove the direct import if you rely on resetTestState() alone.


93-100: Avoid double-clearing the PCR cache for better clarity.
Since resetTestState() already clears the PCR cache asynchronously, explicitly calling clearPcrHistoryCache() again (line 100) might be redundant. Removing the second call can reduce confusion.

beforeEach(async () => {
  resetTestState();
  cachedLoginResponse = null;
- clearPcrHistoryCache();
});
src/lib/pcrHistory.ts (3)

5-7: Keep the public key secure.
Ensure that this hard-coded public key is suitable for your use case. If it’s ever updated or replaced, you’ll need a process to handle key rotation and distribution.


19-23: Cache constant and structure satisfy typical needs.
Storing timestamps in-memory for caching is acceptable for many client-use or ephemeral scenarios. Just note that concurrency or multi-instance usage may need a more robust shared store in production.


202-211: isDev environment check.
This logic is straightforward for toggling dev vs. prod behavior by reading api_url. Ensure you handle cases where api_url might be set to QA or staging subdomains if that’s relevant.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6acd85 and d32bc63.

📒 Files selected for processing (7)
  • src/lib/attestationForView.ts (2 hunks)
  • src/lib/index.ts (1 hunks)
  • src/lib/pcr.ts (2 hunks)
  • src/lib/pcrHistory.ts (1 hunks)
  • src/lib/test/integration/developer.test.ts (2 hunks)
  • src/lib/test/integration/pcr.test.ts (1 hunks)
  • src/lib/test/preload.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/test/integration/pcr.test.ts
  • src/lib/index.ts
  • src/lib/pcr.ts
🧰 Additional context used
🧬 Code Definitions (3)
src/lib/test/preload.ts (1)
src/lib/pcrHistory.ts (1)
  • clearPcrHistoryCache (27-31)
src/lib/attestationForView.ts (2)
src/lib/index.ts (4)
  • validatePcrSet (33-33)
  • PcrConfig (34-34)
  • Pcr0ValidationResult (34-34)
  • validatePcr0Hash (35-35)
src/lib/pcr.ts (3)
  • PcrConfig (32-36)
  • Pcr0ValidationResult (27-30)
  • validatePcr0Hash (46-83)
src/lib/test/integration/developer.test.ts (2)
src/lib/test/preload.ts (1)
  • resetTestState (45-60)
src/lib/pcrHistory.ts (1)
  • clearPcrHistoryCache (27-31)
🔇 Additional comments (10)
src/lib/attestationForView.ts (1)

84-89: LGTM: Async validation with improved documentation

The updated comments provide good clarity about the validation process, and the implementation correctly uses asynchronous validation for PCR0, which will handle both local and remote validation as needed.

src/lib/test/preload.ts (1)

62-64: Immediate state reset at file load.
Calling resetTestState() at load time ensures a fresh test environment up front. This looks fine for ensuring shared globals are consistently reset.

src/lib/pcrHistory.ts (8)

1-3: Imports look appropriate.
Using @stablelib/base64’s decode is a valid approach for base64 decoding in TypeScript; no issues found here.


9-16: PCR entry interface is well-defined.
All required PCR fields are present; this should minimize confusion when referencing them elsewhere.


27-31: Clear function adheres to its contract.
clearPcrHistoryCache thoroughly deletes all keys in pcrHistoryCache. Good for test resets or forced recaching.


39-77: Caching logic in getPcrHistoryList is well-organized.
The function gracefully handles fetch errors by returning an empty array, which correctly triggers a validation fail in downstream code. Consider logging warnings if the cache becomes stale to help debugging.


83-103: Asynchronous public key import is appropriate.
The loadPcrPublicKey function fully handles decoding and import, throwing an error if something goes wrong. This robust approach is good practice for cryptographic operations.


125-126: JSON order is not guaranteed without deterministic serialization.
As previously mentioned in older reviews, you may need to sort object keys to match the original signing format or confirm the backend’s signing process is order-agnostic.


111-142: Signature verification covers typical ECDSA usage.
You’re using ECDSA with SHA-384 and the expected curve is P-384; this is correct for verifying with the imported public key.


150-196: validatePcrAgainstHistory properly integrates fetching history and verifying.
Your approach cross-checks each entry’s PCRs and signature. Throwing or returning an error fallback if empty or invalid data is present is good for security.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances PCR validation by adding remote attestation support and consolidating related functions under a unified interface.

  • Updated exports in src/lib/index.ts to include validatePcrSet and new PCR history functions.
  • In src/lib/attestationForView.ts, awaiting validatePcr0Hash ensures remote attestation is properly integrated.
  • Modified src/lib/pcr.ts to conditionally invoke remote history checks when a remote URL is provided.
  • Introduced src/lib/pcrHistory.ts for cached PCR history fetching with ECDSA signature verification.
  • Expanded integration tests now cover both successful and error paths in remote attestation validation.

7 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +50 to +57
// Import and clear PCR history cache
import("../pcrHistory")
.then(({ clearPcrHistoryCache }) => {
clearPcrHistoryCache();
})
.catch((err) => {
console.error("Failed to clear PCR history cache:", err);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: clearPcrHistoryCache is invoked asynchronously without awaiting. Consider making resetTestState async to ensure the cache is fully cleared before tests run.

Suggested change
// Import and clear PCR history cache
import("../pcrHistory")
.then(({ clearPcrHistoryCache }) => {
clearPcrHistoryCache();
})
.catch((err) => {
console.error("Failed to clear PCR history cache:", err);
});
// Import and clear PCR history cache
await import("../pcrHistory")
.then(({ clearPcrHistoryCache }) => {
return clearPcrHistoryCache();
})
.catch((err) => {
console.error("Failed to clear PCR history cache:", err);
});

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/lib/test/integration/pcr.test.ts (2)

65-90: Test could be more specific about expected remote validation results

While the test correctly verifies that remote validation is attempted, it only checks that a result is returned with the expected structure, not that the result contains the expected values.

Consider extending this test to verify the actual validation result when possible:

  // Check that we received a result
  expect(result).toBeDefined();
  expect(typeof result.isMatch).toBe("boolean");
  expect(typeof result.text).toBe("string");
+
+  // If the environment supports signature verification, check the actual result
+  // This may depend on Web Crypto API support in the test environment
+  // expect(result.isMatch).toBe(true);
+  // expect(result.text).toBe("PCR matches history with valid signature");

92-118: Similar to above, this test could verify actual validation results

The test for validatePcrAgainstHistory only verifies the result structure but not the actual validation outcome.

Consider extending this test to verify the actual validation result:

  // Validate result structure
  expect(result).toBeDefined();
  expect(typeof result.isMatch).toBe("boolean");
  expect(typeof result.text).toBe("string");
+
+  // If the environment supports signature verification, check the actual result
+  // expect(result.isMatch).toBe(true);
+  // expect(result.text).toBe("PCR matches history with valid signature");
src/lib/pcrHistory.ts (1)

204-213: Environment detection logic is functional but could be more robust

The isDev function relies on checking the API URL from localStorage, which works for this case but might not be the most robust method of environment detection.

Consider adding a more explicit environment configuration option:

function isDev(): boolean {
  // Check if the API URL contains development indicators
  const apiUrl = window.localStorage.getItem("api_url") || "";
+  // Check for explicit environment setting first
+  const explicitEnv = window.localStorage.getItem("env");
+  if (explicitEnv) {
+    return explicitEnv === "dev";
+  }
  return (
    apiUrl.includes("localhost") ||
    apiUrl.includes("127.0.0.1") ||
    apiUrl.includes("dev.") ||
    apiUrl.includes(".dev")
  );
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d32bc63 and 8d4f1fb.

📒 Files selected for processing (7)
  • src/lib/attestationForView.ts (2 hunks)
  • src/lib/index.ts (1 hunks)
  • src/lib/pcr.ts (4 hunks)
  • src/lib/pcrHistory.ts (1 hunks)
  • src/lib/test/integration/developer.test.ts (2 hunks)
  • src/lib/test/integration/pcr.test.ts (1 hunks)
  • src/lib/test/preload.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/test/integration/developer.test.ts
  • src/lib/pcr.ts
  • src/lib/test/preload.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/lib/attestationForView.ts (2)
src/lib/index.ts (4)
  • validatePcrSet (33-33)
  • PcrConfig (34-34)
  • Pcr0ValidationResult (34-34)
  • validatePcr0Hash (35-35)
src/lib/pcr.ts (3)
  • PcrConfig (34-38)
  • Pcr0ValidationResult (29-32)
  • validatePcr0Hash (46-84)
src/lib/test/integration/pcr.test.ts (2)
src/lib/pcrHistory.ts (2)
  • PcrEntry (9-16)
  • validatePcrAgainstHistory (152-198)
src/lib/pcr.ts (1)
  • validatePcr0Hash (46-84)
🔇 Additional comments (11)
src/lib/attestationForView.ts (2)

45-65: The validatePcrSet function serves as a clear entry point for PCR validation

This function provides a well-documented interface for validating the PCR set while properly indicating that PCR1 and PCR2 are currently only used for remote validation. The implementation correctly forwards to validatePcr0Hash and the documentation clearly describes the purpose of each parameter.


84-89: Good enhancement to use async validation for both local and remote checking

The changes to use asynchronous validation with both local and remote checking are well-implemented. The comments clearly explain the purpose of finding PCR0 and indicate that PCR1 and PCR2 are available for full validation if needed.

src/lib/index.ts (2)

33-37: Good addition of PCR validation exports

The exports for PCR validation functions and types provide a complete and coherent API surface for consumers of the library to perform PCR validation tasks.


39-60: Excellent high-level PCR validation interface

The PcrValidator constant provides a well-designed high-level interface that aggregates the validation functions in a logical way. This approach makes the API more consistent and easier to use while hiding implementation details.

src/lib/test/integration/pcr.test.ts (2)

1-63: Good test coverage for PCR validation with hardcoded values

The test setup and assertions for validating known and unknown PCR0 values are comprehensive and well-structured.


120-136: Good error handling test

This test effectively verifies that the function handles fetch errors gracefully and returns the expected result.

src/lib/pcrHistory.ts (5)

1-31: Well-structured PCR history interface and cache implementation

The PCR history cache implementation with a 15-minute expiration is a good approach to reduce network requests. The clearPcrHistoryCache function provides a clean way to reset the cache for testing.


39-77: Robust implementation of PCR history fetching with caching

The getPcrHistoryList function includes good error handling and properly implements caching. It correctly handles custom URLs and different environments.


83-103: Good implementation of public key loading for verification

The loadPcrPublicKey function properly handles decoding the base64-encoded key and importing it for verification. The error handling is comprehensive.


111-144: Secure implementation of signature verification

The signature verification function correctly reconstructs the message that was signed and uses deterministic JSON serialization to ensure consistent key ordering for verification. The error handling is robust, defaulting to false on errors.


152-198: Comprehensive implementation of PCR validation against history

The validatePcrAgainstHistory function effectively combines fetching PCR history, key loading, and signature verification. It handles errors appropriately and provides clear validation results.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/lib/test/integration/pcrHistorySignature.test.ts (2)

5-12: Consider adding additional sample PCR entries.
Having multiple sample entries with variations (e.g., invalid signature, edge-case timestamps) can help ensure broader coverage.


19-29: Test scenario is clear and adequately checks signature verification.
A more comprehensive approach could include negative tests (e.g., corrupted signature) to fully verify the reliability of the signature checker.

src/lib/pcr.ts (1)

38-38: New optional remoteValidationUrl.
Makes sense for enabling remote validation. Consider documenting expected endpoint format.

src/lib/pcrHistory.ts (1)

104-131: Signature verification focuses on PCR0 only.
This is presumably by design. Consider verifying all relevant PCR fields if required by spec.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4f1fb and 1592107.

📒 Files selected for processing (4)
  • src/lib/pcr.ts (3 hunks)
  • src/lib/pcrHistory.ts (1 hunks)
  • src/lib/test/integration/pcrHistorySignature.test.ts (1 hunks)
  • src/lib/test/integration/pcrHistoryValidation.test.ts (1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
src/lib/test/integration/pcrHistoryValidation.test.ts (2)
src/lib/pcrHistory.ts (3)
  • PcrEntry (9-15)
  • clearPcrHistoryCache (26-30)
  • validatePcrAgainstHistory (139-189)
src/lib/test/preload.ts (1)
  • resetTestState (45-60)
src/lib/pcrHistory.ts (2)
src/lib/index.ts (5)
  • PcrEntry (36-36)
  • getPcrHistoryList (37-37)
  • validatePcrAgainstHistory (37-37)
  • PcrConfig (34-34)
  • Pcr0ValidationResult (34-34)
src/lib/pcr.ts (2)
  • PcrConfig (35-39)
  • Pcr0ValidationResult (29-33)
src/lib/test/integration/pcrHistorySignature.test.ts (2)
src/lib/pcrHistory.ts (4)
  • PcrEntry (9-15)
  • clearPcrHistoryCache (26-30)
  • loadPcrPublicKey (82-102)
  • verifyPcrSignature (110-131)
src/lib/test/preload.ts (1)
  • resetTestState (45-60)
src/lib/pcr.ts (1)
src/lib/pcrHistory.ts (1)
  • validatePcrAgainstHistory (139-189)
🔇 Additional comments (23)
src/lib/test/integration/pcrHistorySignature.test.ts (2)

1-4: Imports and test setup look good.
All necessary imports are specified, and using Bun’s test framework is a straightforward choice.


14-17: Clearing state and cache is well-handled.
Resetting test state and clearing the PCR history cache in beforeEach ensures tests run in a predictable, isolated environment.

src/lib/test/integration/pcrHistoryValidation.test.ts (6)

1-3: Imports and references look consistent.
The references to validatePcrAgainstHistory and utility functions align with the integration testing approach.


5-16: Good use of a realistic PCR entry for testing.
Using a real or near-real sample entry improves confidence in the integration test.


17-31: Mocking fetch for local tests is appropriate.
Restoring the original fetch in afterEach ensures minimal side effects outside this test suite.


37-53: Positive workflow test is thorough.
Validating a matching PCR ensures the integration flow from the mocked fetch to the final result is correct.


55-71: Negative test for non-matching PCR is well-structured.
Properly verifies that the function rejects unknown PCR values.


73-91: Fetch error handling test is effective.
Ensures resilience against network issues by expecting a failure result.

src/lib/pcr.ts (5)

27-27: Importing validatePcrAgainstHistory is a logical extension of local checks.
No issues found.


32-32: New optional timestamp property.
This property may facilitate returning enriched validation info. Looks good.


47-50: Async function signature is well-chosen.
Switching to asynchronous operations allows for remote lookups.


54-54: Local validation remains streamlined.
Preserving the local check first helps ensure minimal overhead if known values match.


69-79: Remote validation fallback is logically placed.
If configured, uses remote validation as a secondary check. Logging the error and returning a non-match is consistent with existing patterns.

src/lib/pcrHistory.ts (10)

1-2: Base64 decode import is appropriate.
StableLib’s base64 utilities are straightforward for decoding the DER key.


4-6: Clear naming for the public key constant.
Naming clarifies purpose as a DER-encoded PCR verification key.


8-15: PcrEntry interface is well-defined.
Covers all relevant PCR fields: PCR0, PCR1, PCR2, timestamp, and signature.


17-18: Per-file cache object for pcrHistory.
Straightforward approach for storing fetched entries in memory.


20-21: Cache expiration override is sensible.
15 minutes is typical for ephemeral verification data.


23-30: Cache clearing function is simple and effective.
Supports test resets and ensures repeated network calls can be forced.


32-76: Fetching PCR history with caching is well-structured.
Appropriately handles success, error, and caching logic.


78-102: Loading the public key with Web Crypto.
Matches ECDSA "P-384" usage. Properly catches and throws if import fails.


133-189: validatePcrAgainstHistory thoroughly checks each entry.
Returns an informative message on success or failure. Handles errors gracefully.


191-204: Environment check via localStorage.
Lightweight approach for distinguishing dev vs. prod.

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