🐛 fix(conformance): fix negative-probe secret perms and the edge handshake expectation - #202
Conversation
…shake expectation Second live matrix run (31058731379): main auth-handshake green on all rows after #201, three more harness bugs surfaced, all pinned against live published images: - 🐛 fix(conformance): wrong-secret probe's mktemp file is 0600 runner-owned -- the throwaway drydock EACCESes as node (1000) before sending the request that would 401; the garbage value is now chmod 0644 - 🐛 fix(conformance): edge probe's throwaway key gets chowned 65532:65532 so the throwaway portwing can actually read it (same mktemp lockout) - 🐛 fix(conformance): edge rows assert 'Edge agent connected' -- drydock's WS path never emits standard mode's 'Handshake successful' line; the unknown-key probe now reads 'rejected hello|unknown-key' from the throwaway portwing's logs because drydock rejects bad hellos silently (error frame to client, no server-side log; verified in portwing-ws source and against a live rig) - 📝 docs(examples): README edge section rewritten with the verified log lines ('Edge agent connected: portwing-edge-<id>', portwing-side 'controller rejected hello ... (unknown-key)', drydock's world-readable-keys startup warning) - 📝 docs(changelog): record round 2 under [Unreleased]
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe conformance harness now makes temporary authentication credentials readable by the required runtime users. Unknown-key checks use Portwing logs and specific rejection messages. Authentication success checks use Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/compose/tri-tool/README.md`:
- Line 114: Update the permission guidance near the drydock startup warning to
target portwing_authorized_keys, which drydock reads as UID 1000, rather than
the Portwing private key portwing_ed25519.pem. Provide the ownership and
restrictive mode command for portwing_authorized_keys and keep the existing
private-key permissions guidance separate.
In `@scripts/tri-tool-conformance/run-matrix.sh`:
- Around line 495-503: Update all cleanup paths after the chown in the probe
setup around key_file to remove the UID 65532-owned file with sudo, including
the failure cleanup and later cleanup calls. Use the existing key_file variable
and preserve the current cleanup behavior while ensuring set -e cannot fail on
unlinking the chowned file.
- Around line 518-532: Update the unknown-key probe in
scripts/tri-tool-conformance/run-matrix.sh lines 518-532 to require an
unknown-key-specific rejection signal rather than matching bad-signature through
the current OR pattern. Update examples/compose/tri-tool/README.md line 114 to
distinguish remediation: check key registration or revocation for unknown-key,
and verify private/public key matching for bad-signature.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eefabf13-f29c-4b98-8f45-f73139954e68
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (2)
examples/compose/tri-tool/README.mdscripts/tri-tool-conformance/run-matrix.sh
- 🐛 fix: require 'rejected hello.*unknown-key' on one line so a bad-signature rejection can't falsely pass the unknown-key probe - 🐛 fix: sudo rm the chowned throwaway key on both post-chown cleanup paths (sticky /tmp blocks the runner from unlinking a 65532-owned file) - 📝 docs: edge README now distinguishes bad-signature (registered key doesn't match the private key) from unknown-key, and states the authorized_keys chown/chmod command explicitly in the startup warning
1 participant
Round 2 from the live conformance matrix (31058731379): #201 got the main auth handshake green on all three rows; this fixes the three remaining harness bugs it exposed, each verified against the live published images (portwing 0.9.2, drydock latest):
mktempcreates the file 0600 owned by the runner, so the throwaway drydock EACCESed as its node user (UID 1000) before ever sending the request that would 401 — the probe timed out instead of observing the documented failure mode. The deliberately-wrong value is now chmod 0644.sendErrorAndClose, no server-side log — verified inapp/api/portwing-ws.ts), so the assertion now readsrejected hello|unknown-keyfrom the throwaway portwing's own logs (controller rejected hello: Unknown or revoked key (unknown-key), captured from a live rig).Handshake successfulline — the second run's log dump showed the Ed25519 hello succeeding (Edge agent connected: portwing-edge-<id> (version=0.9.2, drydockCompat=1.4.0)) while the assertion still timed out grepping for the standard line. Edge rows now assertEdge agent connected.The bundle README's Edge section documented the same wrong log line and pointed debugging at drydock's (silent) side — rewritten with the verified texts, including drydock's startup
Failed to load DD_PORTWING_AUTHORIZED_KEYSwarning when the keys file is world-readable.Self-test, shellcheck, and the harness npm tests pass. After merge: dispatch run 3 of the matrix from
dev/v1.6againstghcr.io/codeswhat/sockguard:1.6.0-rc.1.Changelog
Edge agent connected.Concerns