feat(runners): allow to create unregistered runners for terraform usage

[fiftin]

No description provided.

[cursor]

Stale comment

Security review — PR #3904

Outcome: 1 medium-severity finding (runner pre-provisioning hijack).

The unregistered-runner flow is useful for Terraform, but finishing registration with only the global runner_registration_token plus a numeric runner_id does not prove the caller owns that pre-created slot. Anyone with the shared registration secret can claim an unregistered runner before the intended host registers and then receive that runner's bearer token (and downstream job secrets).

Recommendation: Return a one-time registration secret when creating an unregistered runner (admin API) and require it (or a signed claim) in POST /api/internal/runners when runner_id is set.

No prior automation threads were present to resolve.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: abdd1bee84

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Hide the transient runner flag from responses

Because this field is tagged db:"-", it is never loaded from either store after creation, but the json:"unregistered" tag makes every runner GET/list response serialize it as false. For the Terraform flow added here, a runner created with unregistered: true will read back as unregistered: false on the next refresh, causing clients that round-trip this request field to see permanent drift even though the runner is still tokenless; either omit it from response JSON or derive it from Token == "".

Useful? React with 👍 / 👎.

[cursor]

Stale comment

Security review — PR #3904

Outcome: 1 medium-severity finding remains.

This revision adds one-time registration_token generation and hashed storage at admin create time (RunnerService), but POST /api/internal/runners with runner_id still only checks the fleet-wide runner_registration_token. The per-runner secret is never required, so pre-provisioned runner hijacking is still possible for anyone who knows the shared registration secret and the numeric runner ID.

Recommendation: When runner_id is present, require the per-runner registration_token (hash compare + expiry) instead of (or in addition to) the global token; extend RunnerRegistration / semaphore runner register to pass it.

Slack summary: PR #3904 security review — 1 medium finding. Pre-provisioned runner hijack still open: per-runner registration tokens are issued on create but not enforced on POST /api/internal/runners with runner_id; fleet-wide token + ID is still enough to claim the slot and obtain the runner bearer token.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: 1 medium-severity finding remains.

Admin create now issues a per-runner registration_token (hashed + TTL in RunnerService), but POST /api/internal/runners with runner_id still only checks the fleet-wide runner_registration_token. The per-runner secret is never validated, so pre-provisioned runner hijacking is still possible for anyone who knows the shared registration secret and the numeric runner ID.

Recommendation: When runner_id is present, require the per-runner registration_token from the create response (constant-time hash compare, enforce registration_token_expires_at, clear hash on success). Extend RunnerRegistration and semaphore runner register to send it.

Slack summary: PR #3904 security review — 1 medium finding. Pre-provisioned runner hijack still open: per-runner registration tokens are stored at create time but not enforced on POST /api/internal/runners with runner_id; fleet-wide token + ID is still enough to claim the slot and obtain the runner bearer token.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: 1 medium-severity finding remains.

Admin create (RunnerService) now issues a per-runner registration_token (SHA-256 hash + 1h TTL persisted), but POST /api/internal/runners with runner_id still only checks the fleet-wide runner_registration_token. RegisterRunner never compares the presented token to registration_token / registration_token_expires_at on the runner row, so the one-time secret does not bind registration to the admin/Terraform-created slot.

Recommendation: When runner_id is set, require the per-runner registration_token from the create response (constant-time hash compare via HashRunnerRegistrationToken, enforce expiry, clear hash on success). Extend RunnerRegistration and semaphore runner register to send it (distinct from the global token).

Slack summary: PR #3904 security review — 1 medium finding. Pre-provisioned runner hijack still open: per-runner registration tokens are stored at create time but not enforced on POST /api/internal/runners with runner_id; fleet-wide token + numeric ID is still enough to claim the slot and obtain the runner bearer token (and downstream job secrets).

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No medium-or-higher findings on the current revision.

The earlier pre-provisioned runner hijack is addressed: POST /api/internal/runners now branches on the presented registration_token — the fleet-wide runner_registration_token only creates a new runner; any other value is hashed and passed to RegisterRunner, which binds registration to the one-time smrs_ token issued at admin create time (hash + 1h expiry, cleared on success). runner_id is no longer accepted on registration, so global token + numeric ID cannot claim a slot.

Controls verified: admin APIs remain behind adminMiddleware; runner auth tokens stay json:"-" in list/get responses; unregistered runners cannot be activated via update/active endpoints.

Slack summary: PR #3904 security review — no open medium+ findings. Prior runner-hijack issue is fixed via one-time registration tokens enforced in RegisterRunner.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No medium-or-higher findings on the current revision (4184061c).

The earlier pre-provisioned runner hijack is addressed. POST /api/internal/runners now branches on the presented registration_token: the fleet-wide runner_registration_token only creates a new runner; any other value is hashed and passed to store.RegisterRunner, which binds registration to the one-time smrs_ token issued at admin create time (SHA-256 hash, 1h TTL, cleared on success). runner_id is no longer accepted on registration, so global token + numeric ID cannot claim a pre-created slot.

Controls verified: admin runner APIs remain behind adminMiddleware; auth tokens stay json:"-" in list/get responses; unregistered runners cannot be activated via update/active endpoints.

Slack summary: PR #3904 security review — no open medium+ findings. Prior runner-hijack issue is fixed via one-time registration tokens enforced in RegisterRunner.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No medium-or-higher findings on the current revision (b44c431e).

Prior findings validated: The earlier pre-provisioned runner hijack is addressed. POST /api/internal/runners branches on registration_token: the fleet-wide runner_registration_token only creates a new runner; any other value is hashed and passed to store.RegisterRunner, which binds registration to the one-time smrs_ token issued at admin create time (SHA-256 hash, 1h TTL, cleared on success). runner_id is no longer accepted on registration, so global token + numeric ID cannot claim a pre-created slot.

Latest delta (b44c431e): POST …/registration-token (admin-only, behind adminMiddleware + runner middleware) rotates the one-time hash via RunnerService.RegenerateRegistrationToken; this does not weaken registration binding and invalidates the previous hash.

Controls verified: runner auth tokens remain json:"-" in list/get responses; unregistered runners cannot be activated via update/active endpoints; registration plaintext is not persisted (only hash + expiry).

Slack summary: PR #3904 security review — no open medium+ findings. Prior runner-hijack issue remains fixed; token regeneration endpoint reviewed with no new issues.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No open medium/high/critical findings.

Prior finding (pre-provisioned runner hijacking) — resolved

Earlier reviews flagged that an attacker with the deployment-wide runner_registration_token could claim a Terraform-preprovisioned slot by supplying a guessable runner_id on POST /api/internal/runners.

The current head addresses this:

• RunnerRegistration no longer accepts runner_id.
• Unregistered runners receive a one-time smrs_… token; only its SHA-256 hash and a 1-hour expiry are stored.
• The non-global registration path calls RegisterRunner(HashRunnerRegistrationToken(token), …), which binds registration to the matching row and clears the hash on success.
• The global token path only creates a new runner; it cannot finalize an existing unregistered slot.

Possession of the per-runner registration token remains the capability to register that slot (by design). Token entropy (~256 bits) and TTL are appropriate for an unauthenticated internal endpoint.

Other checks

• Admin-only create/regenerate endpoints remain behind adminMiddleware / project runner middleware.
• Activation of unregistered runners is blocked in UpdateRunner and SetRunnerActive.
• Registration token plaintext is not persisted; list/get responses use json:"-" on stored hash fields.
• SQL uses parameterized queries for token hash lookup.

No new inline comments — nothing meets the bar for a current exploitable issue in the added/modified code.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No open medium/high/critical findings on commit 9fc1d9c.

Prior findings validated

The earlier pre-provisioned runner hijacking issue is resolved. An intermediate revision allowed finishing a tokenless runner using only the deployment-wide runner_registration_token plus a guessable runner_id. The current code requires the per-runner one-time smrs_… token: RegisterRunner hashes the presented token and calls store.RegisterRunner(hash, …), which matches the stored hash and expiry. The global config token can only create a new runner, not claim an existing unregistered slot.

What was reviewed

• Admin runner create/regenerate paths (RunnerService, GlobalRunnerController) — admin-gated; registration secrets returned once; only hashes persisted (SQL + Bolt).
• Public runner registration (POST /api/internal/runners) — one-time token flow; empty-token runners cannot authenticate via RunnerMiddleware.
• Credential fields on db.Runner — auth token and registration hash are json:"-"; client cannot bind a token via JSON.

No plausible authz bypass, injection, or secret-leak paths were found in the added/modified code.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Security review — PR #3904

Outcome: No open medium/high/critical findings on commit d858536b.

Prior findings validated

The earlier pre-provisioned runner hijacking issue is resolved. POST /api/internal/runners now branches on registration_token: the fleet-wide runner_registration_token only creates a new runner; any other value is hashed (HashRunnerRegistrationToken) and passed to store.RegisterRunner, which matches the stored hash, enforces expiry, and clears the one-time secret on success. runner_id is no longer accepted on registration.

Controls verified

• Admin create/regenerate paths (RunnerService, GlobalRunnerController) remain behind adminMiddleware; registration secrets are returned once; only hashes are persisted.
• Auth token and registration hash fields use json:"-" on db.Runner.
• SQL/Bolt registration lookups use parameterized queries / safe filters.
• Unregistered runners cannot authenticate via RunnerMiddleware (empty auth token).

No plausible authz bypass, injection, or secret-leak paths were found in the added/modified code on this revision.

Slack summary: PR #3904 security review — no open medium+ findings. Prior runner-hijack issue remains fixed; token regeneration endpoint reviewed with no new issues.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Security review — PR #3904

Outcome: No open medium/high/critical findings on commit 636ced6f.

Prior findings validated

The earlier pre-provisioned runner hijacking issue is resolved. POST /api/internal/runners no longer accepts runner_id. The fleet-wide runner_registration_token only creates a new runner; finishing a Terraform-preprovisioned slot requires the per-runner smrs_… token, which is hashed (HashRunnerRegistrationToken) and validated in store.RegisterRunner (hash match, expiry, single use).

Latest revision (636ced6f)

RegenerateRegistrationToken / ResetRunnerRegistration now reset registered runners (clear auth token and public key, issue a new one-time hash). This endpoint remains behind adminMiddleware on global runners; impact is limited to admins intentionally rotating credentials.

Controls verified

• Registration secrets returned once from admin APIs; only SHA-256 hashes + expiry persisted.
• Auth token and stored registration hash use json:"-" on db.Runner.
• SQL registration lookup is parameterized; Bolt filters by hash in-process.
• Unauthenticated runner API calls require a non-empty X-Runner-Token matching a stored auth token (RunnerMiddleware).

No plausible authz bypass, injection, or secret-leak paths were found in the added/modified code on this revision.

Slack summary: PR #3904 security review — no open medium+ findings. Prior runner-hijack issue remains fixed; registered-runner reset/regenerate path reviewed with no new issues.

  

_{Sent by Cursor Automation: Find vulnerabilities}