fix(auth): fail closed without write-capable admin on public bind - #94
fix(auth): fail closed without write-capable admin on public bind#94seonghobae wants to merge 27 commits into
Conversation
Non-loopback BIND_ADDR refuses readiness unless ADMIN_TOKEN, ADMIN_TOKENS, or WAF_IDS_CREDENTIALS_PATH supplies a write-capable principal. Loopback development still starts and reports auth_mode=development. Management writes return 401 vs 403; presented secrets compare in constant time. Blank WAF_IDS_STATE_PATH is in-memory state, not a post-ready crash. Fixes #78.
The fail-closed credentials change opened as #94; leftover merge blockers remain the org 2-approval ruleset and unread strix 404s.
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough관리자 인증을 RBAC 기반으로 확장했습니다. 비루프백 바인딩은 쓰기 권한 인증 없이는 시작되지 않습니다. CISA KEV 가져오기와 피드 소유권 처리를 추가하고, health 상태·테스트·운영 문서를 갱신했습니다. Changes관리 인증 및 위협 피드 관리
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The change improves the standard startup path, but current validation can still accept whitespace-only credentials or misclassify a localhost name before the actual listener address is checked, allowing management writes to become reachable without a valid write-capable administrator. Alternate application launch paths can also bypass the listener safeguard, so the PR is not safe to merge until these authentication-boundary issues are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Operator
participant run_from_env
participant CredentialRegistry
participant ManagementAPI
participant KEVSource
participant ThreatStore
Operator->>run_from_env: BIND_ADDR 및 관리자 자격 증명 제공
run_from_env->>CredentialRegistry: 토큰과 credentials 파일 로드
run_from_env->>ManagementAPI: 인증된 서버 시작
Operator->>ManagementAPI: KEV import 요청과 관리자 토큰 전달
ManagementAPI->>KEVSource: 서버 설정 URL에서 카탈로그 가져오기
KEVSource-->>ManagementAPI: KEV 카탈로그 반환
ManagementAPI->>ThreatStore: CVE 위협 지표와 피드 소유권 적용
ThreatStore-->>Operator: 가져오기 결과 반환
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 70.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 4 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
One DestinationPolicy mediates gateway upstreams, threat-intel fetches, Clearfolio, and SOC LLM. Private, loopback, link-local, CGNAT, and metadata classes are denied unless DESTINATION_ALLOWLIST (or loopback development) permits them; DESTINATION_DENYLIST wins. Clients ignore ambient HTTP proxies and do not follow redirects. Refs #79.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Coraza/Suricata ingest already fed IP and path hints. Also persist the audit URI query as an engine_payload indicator so the same CRS payload is blocked on /gateway for a different client, keeping route-scoped block mode. Refs #86.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
RED: preserve the Devin findings as executable regressions before changing production behavior. The constant-time helper must reject length deltas that alias through u8, and blank credential paths must be normalized explicitly rather than relying on platform-specific read errors.
GREEN: normalize blank credential bootstrap paths before filesystem access and retain a full length-inequality bit in constant-time secret comparison. This closes the current auth-scope Devin findings without restoring unrelated destination/WAF drift.
RED: a corrupt persisted-state file currently binds and prints the listening readiness line before AppState::load rejects the file. Preserve the review finding as an executable binary regression before changing startup ordering.
Strix failed PR #94 as CWE-798 on the hardcoded smoke.sh secret. The process still sets ADMIN_TOKEN from that value before listen; generating it at runtime removes the shared source-tree credential without weakening the scanner.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Corrupt WAF_IDS_STATE_PATH must fail closed before TcpListener bind and the "waf-ids-ai-soc listening on" line, so a supervisor cannot treat a dying process as ready.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Adopt protected main@b2bcee3 (CISA KEV catalog) without dropping the issue #78 gate: non-loopback binds still refuse readiness without a write-capable admin principal. Credential-file bootstrap keeps parse_credentials_json fail-closed on blank/null keys while recording admin_from_file provenance. Hostname localhost is not a loopback exception; only numeric 127.0.0.0/8 and ::1 skip the write-capable requirement. Signed-off-by: Seongho Bae <me@seonghobae.me>
|
@opencode-agent review | Independent exact-current-head formal review requested for |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Hour-20 exact-head review request. Current head Issue #78 fail-closed public bind. Destination #79/#96 and engine-payload #86 stay out. Localhost is not a loopback exception. Do not self-approve. Do not --admin merge. Checks/reviews are not a reason to weaken fail-closed gates. |
|
Hour-21 exact-head review request. Current head @opencode-agent review |
|
Hour-22 exact-head review request. Current head Issue #78 fail-closed public bind. Blank/null credentials fail closed. Localhost is not a loopback exception. Destination #79/#96 and engine-payload #86 stay out. Devin/CodeRabbit/author COMMENTED is not independent APPROVE. Copilot review requests are not APPROVE. Ruleset 18156473 needs two independent approvals on this exact SHA. Do not self-approve. Do not --admin merge. @opencode-agent review |
|
Hour-23 exact-head review request. Current head Issue #78 fail-closed public bind. Destination #79/#96 and engine-payload #86 stay out. Localhost is not a loopback exception. @opencode-agent review |
|
Superseded by the clean current-main replacement #138. Fresh comparison shows both branches share the issue #78 runtime-auth scope, while #138 carries the current focused auth implementation plus the newer |
2 participants
Summary
Closes the operator-visible half of issue #78: a non-loopback listener no longer becomes ready with open management writes when no write-capable admin principal is configured.
require_write_auth_for_bindrefuses0.0.0.0/::/ LAN binds withoutADMIN_TOKEN,ADMIN_TOKENS(write role), orWAF_IDS_CREDENTIALS_PATH.auth_mode=developmenton/healthz.401(unauthenticated) vs403(authenticated, not permitted to write) without naming the expected role.ADMIN_TOKENSrejects blank, duplicate, and unknown roles at startup; readonly-only maps fail closed.WAF_IDS_STATE_PATHis in-memory state. BlankWAF_IDS_CREDENTIALS_PATHis unset.scripts/smoke.shgenerates a per-process admin token (no shared source-tree secret) and sendsX-Admin-Tokenon audit-log reads. Strix CWE-798 ondev-secretis closed without weakening the scanner.Doctoring (APA 7th):
docs/doctoring/fail-closed-management-auth.md.Merge note
Org ruleset
CWL Central required workflows(id 18156473) requires two approving reviews. Do not--adminmerge. Do not self-approve.Fixes #78.
Summary by CodeRabbit
보안
403 Forbidden이 반환됩니다.새 기능
문서
테스트