feat: add /check_quote endpoint and revocation list#4
Open
azaidelson wants to merge 1 commit intomainfrom
Open
feat: add /check_quote endpoint and revocation list#4azaidelson wants to merge 1 commit intomainfrom
azaidelson wants to merge 1 commit intomainfrom
Conversation
Add a lightweight POST /check_quote endpoint that verifies an attestation
quote and returns {whitelisted, machine_id} without issuing a JWT. Useful
for monitoring, gating, and ops tooling.
Add a baked-in revocation list (src/revoked.txt) that takes precedence
over the whitelist. Revoked machines get a distinct error on /get_jwt
(403 with revocation timestamp) and a distinct response shape on
/check_quote ({revoked: true, revoked_at}).
Other changes:
- Extract verifyAndLookupMachine helper from processQuote
- Switch whitelist matching from prefix (startsWith) to strict equality
- Lowercase whitelist and revocation IDs at load time
- Surface Intel attester stderr when verification fails
- Add check_quote.sh client script with mktemp+trap cleanup
- Gitignore src/whitelist.csv and src/revoked.txt (injected by CI)
- Update CI workflow to inject revoked.txt with empty-file fallback
- Update Dockerfile to COPY revoked.txt
- Update README with /check_quote and revocation documentation
📝 WalkthroughWalkthroughThis PR introduces a machine revocation feature that complements the existing whitelist. Changes include adding a Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server as Server<br/>/check_quote
participant Service as checkQuote<br/>Service
participant Attestation as Attestation<br/>Engine
participant Revoked as Revoked<br/>List
participant Whitelist as Whitelist
Client->>Server: POST /check_quote<br/>{quote}
Server->>Service: checkQuote(hexQuote)
Service->>Attestation: runDcapCheck()<br/>or runSevSnpCheck()
Attestation-->>Service: {stdout, stderr}<br/>with machine-id
Service->>Revoked: Check revocation<br/>list first
alt Revoked
Revoked-->>Service: Entry found<br/>(revocation data)
Service-->>Server: {whitelisted: false,<br/>revoked: true,<br/>revoked_at: timestamp}
else Not Revoked
Revoked-->>Service: Not found
Service->>Whitelist: Check whitelist
alt Whitelisted
Whitelist-->>Service: Entry found
Service-->>Server: {whitelisted: true,<br/>machine_id}
else Not Whitelisted
Whitelist-->>Service: Not found
Service-->>Server: {whitelisted: false,<br/>machine_id}
end
end
Server-->>Client: HTTP 200/403<br/>JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services.js (1)
242-249: ConsiderMapfor O(1) revocation lookup if list grows.Currently using
Array.find()for revocation checks is O(n). If the revocation list remains small this is fine, but if it grows significantly, consider using aMapkeyed by ID for O(1) lookup.Example refactor using Map
const state = { // ... - revoked: [], + revoked: new Map(), // Map<machineId, { revokedAt }> }; // In loadSecrets(): - state.revoked = revokedRaw - .split(/\r?\n/) - // ... - .map((line) => { /* ... */ }); + state.revoked = new Map( + revokedRaw + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith("#")) + .map((line) => { + const [id, revokedAt] = line.split(","); + return [id.trim().toLowerCase(), { revokedAt: revokedAt?.trim() || "unknown" }]; + }) + ); // In verifyAndLookupMachine(): - const revocationEntry = state.revoked.find((e) => e.id === machineId) || null; - const revocation = revocationEntry - ? { revokedAt: revocationEntry.revokedAt } - : null; + const revocationEntry = state.revoked.get(machineId); + const revocation = revocationEntry ? { revokedAt: revocationEntry.revokedAt } : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services.js` around lines 242 - 249, Replace the O(n) Array.find lookups for revocations and whitelist with O(1) Map lookups: change how state.revoked and state.whitelist are populated (or add cached Maps) keyed by id, then replace the revocationEntry computation (currently using state.revoked.find(...) producing revocationEntry) with a Map.get/has lookup for machineId and similarly replace the whitelist lookup used to compute entry; preserve the existing null semantics (revocation = { revokedAt: ... } or null, entry = whitelist entry or null) and ensure any code constructing state.revoked/state.whitelist also keeps the Maps in sync or initializes them when state is loaded/updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server.js`:
- Around line 25-32: The /check_quote route currently lets validation errors
from checkQuote (which calls verifyAndLookupMachine) bubble to the global error
handler and become HTTP 500; change the route handler (app.post("/check_quote",
asyncHandler(...))) to explicitly validate input and/or catch validation
failures from checkQuote/verifyAndLookupMachine and respond with HTTP 400.
Implement this by detecting known validation error types/messages (or having
verifyAndLookupMachine throw a dedicated ValidationError) inside the async
handler for checkQuote, and return res.status(400).json({ error: <message> })
for those cases while rethrowing other errors to the global handler.
---
Nitpick comments:
In `@src/services.js`:
- Around line 242-249: Replace the O(n) Array.find lookups for revocations and
whitelist with O(1) Map lookups: change how state.revoked and state.whitelist
are populated (or add cached Maps) keyed by id, then replace the revocationEntry
computation (currently using state.revoked.find(...) producing revocationEntry)
with a Map.get/has lookup for machineId and similarly replace the whitelist
lookup used to compute entry; preserve the existing null semantics (revocation =
{ revokedAt: ... } or null, entry = whitelist entry or null) and ensure any code
constructing state.revoked/state.whitelist also keeps the Maps in sync or
initializes them when state is loaded/updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbb7f887-3113-4276-b364-0a704c9f31b1
⛔ Files ignored due to path filters (1)
src/whitelist.csvis excluded by!**/*.csv
📒 Files selected for processing (8)
.github/workflows/build-and-publish.yml.gitignoreDockerfileREADME.mdcheck_quote.shsrc/config.jssrc/server.jssrc/services.js
Comment on lines
+25
to
+32
| app.post( | ||
| "/check_quote", | ||
| asyncHandler(async (req, res) => { | ||
| const { quote } = req.body; | ||
| const result = await checkQuote(quote); | ||
| res.json(result); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Validation errors from checkQuote will return HTTP 500 instead of 400.
The checkQuote function calls verifyAndLookupMachine, which throws errors for invalid input (e.g., "Missing quote", "Invalid quote format"). These will bubble up to the global error handler and return HTTP 500. For a "lightweight check" endpoint, clients would reasonably expect HTTP 400 for malformed requests.
Consider adding input validation here or updating the error handler to detect validation errors:
Proposed fix: Add input validation
app.post(
"/check_quote",
asyncHandler(async (req, res) => {
const { quote } = req.body;
+ if (!quote) {
+ return res.status(400).json({ error: "Missing quote" });
+ }
const result = await checkQuote(quote);
res.json(result);
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.post( | |
| "/check_quote", | |
| asyncHandler(async (req, res) => { | |
| const { quote } = req.body; | |
| const result = await checkQuote(quote); | |
| res.json(result); | |
| }), | |
| ); | |
| app.post( | |
| "/check_quote", | |
| asyncHandler(async (req, res) => { | |
| const { quote } = req.body; | |
| if (!quote) { | |
| return res.status(400).json({ error: "Missing quote" }); | |
| } | |
| const result = await checkQuote(quote); | |
| res.json(result); | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.js` around lines 25 - 32, The /check_quote route currently lets
validation errors from checkQuote (which calls verifyAndLookupMachine) bubble to
the global error handler and become HTTP 500; change the route handler
(app.post("/check_quote", asyncHandler(...))) to explicitly validate input
and/or catch validation failures from checkQuote/verifyAndLookupMachine and
respond with HTTP 400. Implement this by detecting known validation error
types/messages (or having verifyAndLookupMachine throw a dedicated
ValidationError) inside the async handler for checkQuote, and return
res.status(400).json({ error: <message> }) for those cases while rethrowing
other errors to the global handler.
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+74
to
+77
| const [id, revokedAt] = line.split(","); | ||
| return { | ||
| id: id.trim().toLowerCase(), | ||
| revokedAt: revokedAt ? revokedAt.trim() : "unknown", |
There was a problem hiding this comment.
If a revocation-list entry omits the timestamp, the loader sets revokedAt: "unknown" and /check_quote//get_jwt surface it as-is, so should we reject malformed revocations or require a parseable ISO-8601 timestamp instead of defaulting to "unknown"?
Finding type: Type Inconsistency | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In src/services.js
around lines 47-83 inside the async loadSecrets() revocation parsing block, the code
currently does `return { id: id.trim().toLowerCase(), revokedAt: revokedAt ?
revokedAt.trim() : "unknown" }`. Refactor this to reject or skip malformed revocation
entries: if a line has no timestamp (or an empty/whitespace-only timestamp), or if the
timestamp is not a parseable ISO-8601 value, log a warning and ignore that line (or
throw to fail fast, depending on your preference). Then verify that checkQuote() (around
lines 406-414) and processQuote() (around lines 276-306) always expose a valid
revoked_at/revocation.revokedAt and never return the literal string "unknown" in
responses or error messages.
Comment on lines
171
to
+175
| function runDcapCheck(hexQuote) { | ||
| return execCommand(config.ATTESTER_BIN, ["check", hexQuote]); | ||
| // The Intel attester exits 0 even when verification fails — the real error | ||
| // is printed to stderr. We need both streams to surface useful errors. | ||
| return new Promise((resolve, reject) => { | ||
| const child = spawn(config.ATTESTER_BIN, ["check", hexQuote]); |
There was a problem hiding this comment.
runDcapCheck duplicates the spawn + stdout/stderr capture wiring from execCommand, so should we reuse execCommand (or factor a shared spawnWithStreams helper) instead of keeping two copies?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Comment on lines
+25
to
+32
| app.post( | ||
| "/check_quote", | ||
| asyncHandler(async (req, res) => { | ||
| const { quote } = req.body; | ||
| const result = await checkQuote(quote); | ||
| res.json(result); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
POST /check_quote in src/server.js uses an action-style, verb-ish path that conflicts with the REST rule about noun/resource URLs—should we move it to a resource-style route like POST /quotes/check or POST /quotes/status?
Finding type: REST API Best Practices | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In src/server.js
around lines 25-32, the Express route `app.post('/check_quote', ...)` uses a
verb/action-style URL, which conflicts with REST best practices that prefer
resource/noun-based paths. Refactor this endpoint to a resource-oriented path such as
`POST /quotes/check` or `POST /quotes/status`, keeping the handler logic (`const { quote
} = req.body; const result = await checkQuote(quote); res.json(result);`) unchanged.
Also update any related client calls/tests or docs that reference `/check_quote` so they
use the new path.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/server.js">
<violation number="1" location="src/server.js:28">
P2: Handle missing quote input in /check_quote so client errors return 400 instead of a 500 from the global error handler.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| app.post( | ||
| "/check_quote", | ||
| asyncHandler(async (req, res) => { | ||
| const { quote } = req.body; |
There was a problem hiding this comment.
P2: Handle missing quote input in /check_quote so client errors return 400 instead of a 500 from the global error handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/server.js, line 28:
<comment>Handle missing quote input in /check_quote so client errors return 400 instead of a 500 from the global error handler.</comment>
<file context>
@@ -22,6 +22,15 @@ app.post(
+app.post(
+ "/check_quote",
+ asyncHandler(async (req, res) => {
+ const { quote } = req.body;
+ const result = await checkQuote(quote);
+ res.json(result);
</file context>
Suggested change
| const { quote } = req.body; | |
| const { quote } = req.body || {}; | |
| if (!quote) { | |
| return res.status(400).json({ error: "Missing quote" }); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Add a lightweight POST /check_quote endpoint that verifies an attestation quote and returns {whitelisted, machine_id} without issuing a JWT. Useful for monitoring, gating, and ops tooling.
Add a baked-in revocation list (src/revoked.txt) that takes precedence over the whitelist. Revoked machines get a distinct error on /get_jwt (403 with revocation timestamp) and a distinct response shape on /check_quote ({revoked: true, revoked_at}).
Other changes:
Summary by cubic
Adds
POST /check_quotefor fast whitelist checks without issuing a JWT, and adds a baked-in revocation list that overrides the whitelist and returns clear 403 errors for revoked machines. Also tightens whitelist matching and surfaces better verifier errors.New Features
POST /check_quote: verifies a quote and returns{ whitelisted, machine_id }; includes{ revoked, revoked_at }when applicable.src/revoked.txt(CI-injected) takes precedence over the whitelist; JWT endpoint denies revoked machines with 403 and timestamp.check_quote.shhelper script for quick local checks.revoked.txtwith empty-file fallback.Refactors
verifyAndLookupMachinefor shared verification and lookups.Written for commit 62a27f7. Summary will update on new commits.
Summary by CodeRabbit
New Features
/check_quotePOST endpoint to verify hardware ID status (whitelisted, unknown, or revoked) without issuing authentication tokensDocumentation
/check_quoteendpoint and revocation list behaviorGenerated description
Below is a concise technical summary of the changes proposed in this PR:
Describe how quote verification now loads whitelist and revocation lists, enforces deny-list precedence, and surfaces attester stderr so
/get_jwtresponds with specific 403 messages for revoked hardware. Introduce the/check_quotehandler plus helper and client tooling so callers can poll whitelist status without requesting JWTs while documenting the new lightweight flow.POST /check_quotebacked by the newcheckQuotehelper, document the lightweight endpoint and its responses, and provide thecheck_quote.shclient so monitoring and ops tooling can poll whitelist status without JWT issuance.Modified files (4)
Latest Contributors(2)
revoked.txt(with fallback), copying it into the container, and keeping repository artifacts out with.gitignoreso the build output matches runtime expectations.Modified files (3)
Latest Contributors(2)
verifyAndLookupMachineby loading the baked-in revocation list, normalizing hardware IDs, surfacing Intel attester stderr, guarding/get_jwtwith revocation-aware 403s, and removing the oldsrc/whitelist.csvstub while explaining the deny-list in the README.Modified files (5)
Latest Contributors(2)