fix(provider-security): ChefVault Bearer auth for ref probes - #2
fix(provider-security): ChefVault Bearer auth for ref probes#2OnlineChef wants to merge 2 commits into
Conversation
CPM policy ref inspection now uses CHEF_PROVIDER_SECURITY_TOKEN and optional X-Chef-* identity headers on /v1/refs/* while leaving /healthz unauthenticated. Doctor reports chefvaultReachable vs chefvaultAuthenticated separately. Co-authored-by: Cursor <cursoragent@cursor.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChefVault now supports bearer-token and workload-identity headers. Protected reference inspection and authentication probing distinguish missing, rejected, and accepted credentials. Policy doctor and CLI output report ChefVault reachability and authentication separately. ChangesChefVault authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyDoctor
participant ChefVaultProviderSecurityClient
participant ChefVault
PolicyDoctor->>ChefVaultProviderSecurityClient: create client
PolicyDoctor->>ChefVaultProviderSecurityClient: health()
ChefVaultProviderSecurityClient->>ChefVault: request /healthz
ChefVault-->>ChefVaultProviderSecurityClient: health status
PolicyDoctor->>ChefVaultProviderSecurityClient: probeAuthentication()
ChefVaultProviderSecurityClient->>ChefVault: request protected reference
ChefVault-->>ChefVaultProviderSecurityClient: authentication status
PolicyDoctor-->>PolicyDoctor: report reachability and authentication
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/provider-security/chefvault-client.ts`:
- Around line 70-82: Update probeAuthentication to preserve the accepted-token
behavior only for the expected 404 response, while returning ok: false for 500
responses, transport errors, and other unexpected failures; retain the existing
401/403 failure handling. Add regression tests covering a 500 response and a
rejected fetch call, using inspectRef and probeAuthentication behavior as the
relevant symbols.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09aba9e1-68c5-4f30-889c-c48daa460ee2
📒 Files selected for processing (7)
src/cli.tssrc/provider-security/chefvault-client.tssrc/provider-security/config.tssrc/provider-security/policy-ops.tssrc/provider-security/types.tstest/chefvault-client.test.tstest/provider-security.test.ts
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (status.error?.includes("(401)") || status.error?.includes("(403)")) { | ||
| return { ok: false, error: status.error }; | ||
| } | ||
| // 404 and other non-auth failures mean the token was accepted. | ||
| return { ok: true }; |
There was a problem hiding this comment.
probeAuthentication rejects only 401 and 403 responses, then treats every other failed protected-ref probe as proof that the credentials were accepted. A timeout or network error, 429 rate limit, or 500 server error therefore returns ok: true; when /healthz succeeds, policy doctor reports chefvaultAuthenticated=yes and omits the authentication issue even though it cannot inspect the protected ref. Only the intentionally expected non-auth response, such as a 404 if that is the service contract, should count as successful authentication; transport and other HTTP failures must remain failures.
Artifacts
Fail-closed expectation fails for timeout, HTTP 429, and HTTP 500
- Executed the focused Vitest fail-closed expectation; all three cases received `true` authentication instead of `false`, confirming the defect.
Focused runtime reproduction passes for direct probe and policy doctor
- Executed six focused tests against the real client and policy-doctor paths; timeout, 429, and 500 each produced authenticated results.
Fail-closed expectation test source
- Authored Vitest source that invokes the actual authentication probe for each failing protected-ref outcome and expects it to fail closed.
Direct probe and policy-doctor reproduction test source
- Authored Vitest source that exercises actual client and policy-doctor functions with healthy health checks and failing protected-ref probes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/provider-security/chefvault-client.ts
Line: 77-81
Comment:
**Non-auth failures pass authentication**
`probeAuthentication` rejects only 401 and 403 responses, then treats every other failed protected-ref probe as proof that the credentials were accepted. A timeout or network error, 429 rate limit, or 500 server error therefore returns `ok: true`; when `/healthz` succeeds, `policy doctor` reports `chefvaultAuthenticated=yes` and omits the authentication issue even though it cannot inspect the protected ref. Only the intentionally expected non-auth response, such as a 404 if that is the service contract, should count as successful authentication; transport and other HTTP failures must remain failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Summary
ChefVaultProviderSecurityClientnow sendsAuthorization: Bearer $CHEF_PROVIDER_SECURITY_TOKENon protected/v1/refs/*calls;/healthzstays unauthenticated.CHEF_WORKLOAD_ID,CHEF_HOST_ID, andCHEF_ACTOR→ matchingx-chef-*headers when set.cpm policy doctorreportschefvaultReachable(healthz) andchefvaultAuthenticated(Bearer probe) separately so a live health endpoint no longer implies ref access works.Context
Closes the cross-repo auth gap after #1 (CPM provider-security plane) and OnlineChefGroep/chefgroep-vault#86 (ChefVault provider-security HTTP with Bearer on
/v1/refs/*). CPM previously probed refs without a token and could report ChefVault as healthy while every ref inspection would fail at runtime.Operator contract
CHEF_PROVIDER_SECURITY_URLCHEF_PROVIDER_SECURITY_TOKEN/v1/refs/*CHEF_WORKLOAD_ID/CHEF_HOST_ID/CHEF_ACTORTest plan
npm test(79 tests, incl.test/chefvault-client.test.ts)cpm policy doctordistinguishes reachable vs authenticated (mocked in tests)Made with Cursor
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Greptile Summary
This change adds ChefVault credentials and optional workload-identity headers to protected ref probes while keeping health checks unauthenticated, and updates
policy doctorto report reachability and authentication separately.Focused execution confirmed that protected-ref timeouts, rate limits, and server failures are reported as successful authentication. With a healthy
/healthzresponse,policy doctorcan therefore showchefvaultAuthenticated=yeseven though protected ref inspection is unavailable.Confidence Score: 4/5
What T-Rex did
Comments Outside Diff (1)
General comment
inspectRef()returns a timeout/network error or a non-401/403 HTTP error such as 429 or 500,probeAuthentication()returns{ ok: true }. With a successful/healthz, policy doctor consequently reportschefvaultAuthenticated: trueand does not addchefvault-unauthenticated.probeAuthentication()only rejects error strings containing(401)or(403)and unconditionally accepts every other failed protected-ref result as evidence that the token was accepted.{ ok: false, error: status.error }for network failures and all other non-2xx responses.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(provider-security): send Bearer auth..." | Re-trigger Greptile