Skip to content

feat: accept OPR_EXECUTOR_SECRET_PREVIOUS during secret rotation - #267

Merged
loks0n merged 1 commit into
mainfrom
fix/accept-previous-executor-secret
Nov 2, 2026
Merged

loks0n merged 1 commit into
mainfrom
fix/accept-previous-executor-secret

Conversation

@loks0n

@loks0n loks0n commented Nov 2, 2026

Copy link
Copy Markdown
Member

What

The /v1/* auth hook compared the bearer against OPR_EXECUTOR_SECRET alone. Rotating that secret meant the executor and every caller had to switch at the same moment, or calls failed in between.

The executor now also accepts an optional OPR_EXECUTOR_SECRET_PREVIOUS. A rotation becomes three steps:

  1. Set OPR_EXECUTOR_SECRET_PREVIOUS to the new value, so the executor accepts both.
  2. Move callers to the new value. Then swap: OPR_EXECUTOR_SECRET becomes the new value, and _PREVIOUS holds the old one until every caller has moved.
  3. Unset _PREVIOUS.

The comparison uses hash_equals instead of !==. Empty and "0" secrets are still refused.

Tests

  • testPreviousSecretAuthorizes (e2e) checks that a request with the previous secret gets 200 on /v1/runtimes, and that an unknown secret gets 401. docker-compose.yml and .env set OPR_EXECUTOR_SECRET_PREVIOUS for it.
  • Ran locally against the compose stack together with testGetRuntimesUnauthorized and testCommandsUnauthorized: 3 tests, 7 assertions, OK.
  • I have not run it against the unchanged hook to see it fail.

🤖 Generated with Claude Code

The executor compared the bearer against one secret, so rotating it meant
every caller and the executor had to switch at the same instant. It now
also accepts an optional previous secret, and compares with hash_equals.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hansi-codes

hansi-codes Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

🔵 Tier A · Mergeable after minor fixes

The auth change is sound, but the README setup example needs to forward the documented variable for the feature to work there.

The executor auth hook now accepts either the primary secret or an optional previous secret, using hash_equals for comparisons. The compose configuration and end-to-end tests exercise the additional credential, and the README lists the new setting.

Verdict New comments Fixed Still open
✅ Approved 1 0 0
Finding Where
🟡 Forward the new variable in the README compose example README.md:210
Fix with agent prompt
### Issue 1
README.md:210
**Forward the new variable in the README compose example**

The Getting Started `docker-compose.yml` example at lines 50–64 explicitly forwards environment variables but omits `OPR_EXECUTOR_SECRET_PREVIOUS`. Setting this documented option in the example's `.env` therefore won't reach the executor, so the second key is not accepted for users following that setup.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
📂 Walkthrough · 5
File Change
.env Adds a previous secret for the local compose test setup.
README.md Documents the optional previous-secret setting.
app/controllers.php Checks the supplied bearer against both configured secrets.
docker-compose.yml Forwards the previous-secret environment variable to the executor.
tests/e2e/ExecutorTest.php Tests access with the previous secret and rejection of an unknown secret.

Reviewed 220c425 · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

@hansi-codes hansi-codes Bot 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.

🔵 Tier A · Looks good to merge. Summary

Comment thread README.md
| OPR_EXECUTOR_MAINTENANCE_INTERVAL| Interval (in seconds) at which the Executor performs maintenance tasks, ex: `60` |
| OPR_EXECUTOR_NETWORK | Network used by the executor for runtimes, ex: `openruntimes-runtimes` |
| OPR_EXECUTOR_SECRET | Secret key used by the executor for authentication |
| OPR_EXECUTOR_SECRET_PREVIOUS | Optional second key the executor also accepts, so callers can move to a new `OPR_EXECUTOR_SECRET` without downtime |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Forward the new variable in the README compose example

The Getting Started docker-compose.yml example at lines 50–64 explicitly forwards environment variables but omits OPR_EXECUTOR_SECRET_PREVIOUS. Setting this documented option in the example's .env therefore won't reach the executor, so the second key is not accepted for users following that setup.

Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 210

Comment:
**Forward the new variable in the README compose example**

The Getting Started `docker-compose.yml` example at lines 50–64 explicitly forwards environment variables but omits `OPR_EXECUTOR_SECRET_PREVIOUS`. Setting this documented option in the example's `.env` therefore won't reach the executor, so the second key is not accepted for users following that setup.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

🟡 Minor · documentation · Reply if this doesn't apply.

@loks0n
loks0n merged commit e29c2ca into main Nov 2, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant