Skip to content

fix(auth): remove redundant Cloudflare Access login#28

Open
jesse-merhi wants to merge 1 commit into
mainfrom
jesse/krillswitch-acl-investigation
Open

fix(auth): remove redundant Cloudflare Access login#28
jesse-merhi wants to merge 1 commit into
mainfrom
jesse/krillswitch-acl-investigation

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jul 20, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where people and automation had to pass a separate Cloudflare Access identity gate before Krillswitch could apply its own GitHub-backed roles or role-scoped access tokens. This produced two human login steps and two automation credentials without using the Cloudflare identity inside the application.

Why This Change Was Made

The admin hostname now relies directly on Better Auth GitHub sessions for people and ksat_ tokens for automation. The deployment removes the legacy Cloudflare Access application only after the updated Worker deploy succeeds, verifying its exact application id, name, and hostname before deletion.

The obsolete Access policy provisioning, workflow inputs, CLI headers, environment variables, tests, and documentation are removed. Public flag evaluation and Krillswitch role enforcement are unchanged.

User Impact

People visit the dashboard, sign in once with GitHub, and receive the role resolved by Krillswitch. OpenClaw organization members receive viewer access; explicitly granted users receive their assigned role; all other GitHub users see the no-access screen.

CLI and agent automation now need only a role-scoped Krillswitch access token.

Migration

The krillswitch-admin GitHub environment must retain KRILLSWITCH_ACCESS_APP_ID for the first admin deployment. That deployment uses the existing Cloudflare token to delete the matching legacy Access application. After successful removal, the variable can be removed and later deployments skip the migration step.

Evidence

  • Full suite: 176 tests passed
  • Migration tests cover exact-app deletion, repeat-deploy no-op behavior, and mismatched-host refusal
  • Existing API authentication tests prove unauthenticated management requests return 401 and ungranted sessions return 403
  • Full typecheck passed
  • Full Biome lint passed across 137 files
  • actionlint passed for the deployment workflow
  • Documentation site built successfully

@jesse-merhi
jesse-merhi requested review from a team as code owners July 20, 2026 05:22
Copilot AI review requested due to automatic review settings July 20, 2026 05:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 11:57 PM ET / July 21, 2026, 03:57 UTC.

Summary
The branch removes Cloudflare Access provisioning and CLI service-token headers, deploys the Worker before deleting the verified legacy Access application, and updates tests and documentation for GitHub-session and ksat_-token access.

Reproducibility: yes. at source level: the workflow and removal-script tests define the exact migration paths, but the supplied evidence does not include a real deployed before/after authorization transcript.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 22 files affected; 240 added, 767 removed. The authorization-boundary change spans deployment automation, CLI behavior, migration tooling, tests, and public documentation.
  • Migration safeguards: 3 focused removal scenarios. The new tests cover exact application deletion, repeat-deploy no-op behavior, and refusal for a hostname mismatch.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Provide redacted staging or production evidence for an authorized session, an ungranted session denial, token automation, and guarded legacy-App deletion.
  • Obtain explicit approval for retiring the independent Cloudflare Access boundary.

Risk before merge

  • [P1] Merging deliberately removes Cloudflare Access’s independent identity and service-token gate for switch.openclaw.ai; users who previously relied on its allowlist or service credentials must instead be governed solely by GitHub session role resolution or ksat_ tokens.
  • [P1] The one-time deletion step needs the existing KRILLSWITCH_ACCESS_APP_ID and a Cloudflare credential able to manage Zero Trust Access applications; a production deployment must prove the Worker is serving the intended application authorization before that deletion is accepted.

Maintainer options:

  1. Require boundary approval and live migration proof (recommended)
    Before merge, approve the application-only authorization model and review redacted proof that authorized, ungranted, and token-based production paths behave as documented before Access deletion.
  2. Retain the edge gate
    Keep Cloudflare Access when its independent identity policy or service-token boundary remains required for the admin hostname.
  3. Pause the removal
    Pause this PR if no maintainer can own the migration credential, rollback plan, and revised security boundary.

Next step before merge

  • [P2] A maintainer must choose and explicitly approve the permanent production security boundary; no narrow automated repair can make that policy choice.

Maintainer decision needed

  • Question: Should production switch.openclaw.ai retire Cloudflare Access and rely exclusively on Better Auth GitHub sessions plus Krillswitch role-scoped tokens for authorization?
  • Rationale: The patch intentionally removes an independent edge identity and automation credential boundary; code review can assess safeguards, but only maintainers can choose the desired permanent defense-in-depth posture and migration policy.
  • Likely owner: jesse-merhi — They authored the proposed boundary and migration, so they are the clearest person to supply deployment evidence and clarify intended operational ownership.
  • Options:
    • Approve the application-only boundary (recommended): Accept GitHub-session and Krillswitch-token authorization as the complete production access boundary after reviewing redacted deployment evidence.
    • Keep Cloudflare Access: Retain the existing edge gate and narrow the PR to any non-boundary improvements that do not remove it.
    • Stage the transition: Deploy and observe the application-only path in a controlled environment before deleting the production Access application.

Security
Needs attention: The diff is purpose-built and includes guarded deletion checks, but it intentionally removes a production security boundary and therefore needs explicit maintainer approval.

Review details

Best possible solution:

Approve the boundary only if maintainers want Krillswitch’s GitHub-role and token checks to be the sole production authorization layer, then capture a redacted production or staging transcript showing authorized access, ungranted-session denial, token automation, and the guarded Access-removal migration.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: the workflow and removal-script tests define the exact migration paths, but the supplied evidence does not include a real deployed before/after authorization transcript.

Is this the best way to solve the issue?

Unclear: the implementation is a focused way to remove duplicate login, but whether replacing defense in depth with application-only authorization is the best solution depends on maintainer security policy.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against 5fa9dca504c2.

Label changes

Label justifications:

  • P1: This changes the production authentication path for dashboard users and automation, so an incorrect boundary decision can immediately block or expose a core operator workflow.
  • merge-risk: 🚨 compatibility: Existing deployments and automation using Cloudflare Access allowlists or service-token credentials lose that path after the migration.
  • merge-risk: 🚨 security-boundary: The PR deliberately removes an independent Cloudflare Access gate from the production admin hostname.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a repository member, so the external-contributor proof gate does not apply; the PR body supplies automated validation claims but no inspectable live deployment transcript.
Evidence reviewed

Security concerns:

  • [medium] Confirm the production authorization boundary — .github/workflows/deploy.yml:68
    Removing Cloudflare Access makes application-level GitHub sessions, role resolution, and ksat_ token validation the only authorization controls for the admin hostname. This is an intentional policy change, not a line-level implementation defect, and needs maintainer approval plus live migration evidence.
    Confidence: 0.96

What I checked:

  • Prior review decision: The previous completed ClawSweeper cycle found no discrete patch defect but required maintainer approval for the Access-versus-application authorization boundary; the new head retains that central design while changing the migration implementation. (9bf120eeb878)
  • Deployment migration ordering: The workflow deploys the updated Worker before invoking the legacy Access removal step, reducing the chance of removing the edge gate before application authorization is deployed. (.github/workflows/deploy.yml:68, 847907307799)
  • Scoped deletion guard: The removal script requires the configured application ID, then verifies the Cloudflare application name and root hostname before issuing DELETE; its dedicated tests cover exact deletion, an already-absent application, and hostname mismatch refusal. (scripts/remove-cloudflare-access.mjs:1, 847907307799)
  • Application authorization remains documented: The updated documentation states that management routes require a GitHub-backed session or role-scoped ksat_ token, and that a valid GitHub session alone does not grant a role. (docs/auth-and-roles.md:7, 847907307799)
  • Current review scope: The PR is open, mergeable but blocked, and changes 22 files with 240 additions and 767 deletions; it is not already implemented on current main. (5fa9dca504c2)

Likely related people:

  • jesse-merhi: Authored the current branch and its single commit implementing the Cloudflare Access removal, deploy migration, CLI changes, tests, and documentation. (role: authorization-boundary proposer and recent area contributor; confidence: medium; commits: 847907307799; files: .github/workflows/deploy.yml, scripts/remove-cloudflare-access.mjs, packages/cli/src/client.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-20T05:24:53.182Z sha 9bf120e :: needs maintainer review before merge. :: none

@jesse-merhi
jesse-merhi force-pushed the jesse/krillswitch-acl-investigation branch from 9bf120e to 8479073 Compare July 21, 2026 03:54
@jesse-merhi jesse-merhi changed the title fix(auth): allow GitHub-authorized users through Access fix(auth): remove redundant Cloudflare Access login Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants