refactor: direct ovi_live_* auth (drop JWT exchange) - #2
Merged
Conversation
keysync was originally designed around a JWT-exchange flow: POST the ovi_live_* to optimum-auth, decode the resulting JWT for operator_id, present that JWT as the Bearer on every billing call, re-exchange before expiry, handle `scope_changed` mid-reconcile. Reviewing the design against keysync's actual workload — one client per operator, hourly cron, ~4 billing calls per reconcile — the exchange pattern's wins (stateless-verify-at-scale, OAuth2-shaped ecosystem fit, scoped capability tokens) don't apply. Meanwhile its costs are real: multi-issuer JWT verification in optimum-billing, operator-key dispatch in optimum-auth, scope_version bump on revoke to invalidate outstanding JWTs, ~150 lines of CLI plumbing here. Billing's `combinedAuth` already accepts `Authorization: Bearer ovi_live_…` directly: HMAC-hashes per request, looks up `internal.operator_api_keys`, stamps role=operator. Revoking the key at the dashboard takes effect on the next request (no 1h cache window). So the direct path is materially simpler AND has faster revocation. What changes here: - `AuthClient` collapses to a credential holder. No httpx, no tenacity, no jwt.decode, no cache window, no scope_changed retry. Just (api_key, operator_id) returned on every token() call. - `KEYSYNC_OPERATOR_ID` is back as a required env var. Operators see it next to the generated key in the dashboard. The JWT model derived it from the claim; without the JWT, customers paste it alongside the key. - `KEYSYNC_AUTH_URL` is gone — no optimum-auth involvement. - `billing_client._do` drops the ScopeChangedError branch. A 401 from billing is now terminal (key was revoked / unknown / scoped to another operator). - `cli._compute_plan` / `_apply_plan` lose their re-exchange closures. The bearer is static, threaded directly. - pyjwt dropped from runtime deps; httpx + tenacity stay (still used by beacon_client + billing_client). - README rewritten to describe the direct flow. Companion changes: optimum-auth#16 and optimum-billing#63 are now unused; both will be closed. Tests: 43/43 green (was 44; one scope_changed-specific test removed, others trivialised to the new shape). ruff + mypy --strict clean. _Written with Claude Code_
📝 WalkthroughWalkthroughThis PR eliminates the JWT-exchange authorization model in favor of direct-bearer authentication. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/optimum_keysync/cli.py (1)
136-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch config build failures before command execution.
Each command calls
_build_config()before entering any error-handling path, so a missingKEYSYNC_OPERATOR_ID, bad billing URL, or any other config validation error will currently escape as an uncaught exception instead of logging a config failure and exiting withEXIT_CONFIG. That breaks the CLI contract on the new fail-fast path. Also at:src/optimum_keysync/cli.py:174-175,src/optimum_keysync/cli.py:214-215.Suggested shape
+def _build_config_or_exit(kwargs: dict[str, object]) -> KeysyncConfig: + try: + return _build_config(kwargs) + except ValueError as e: + _LOG.error("config setup failed", error=str(e)) + sys.exit(EXIT_CONFIG) + `@main.command`("show") `@_common_options` def cmd_show(**kwargs: object) -> None: """Print currently-assigned validators. Read-only.""" - cfg = _build_config(kwargs) + cfg = _build_config_or_exit(kwargs) configure_logging(fmt=cfg.log_format) # type: ignore[arg-type] @@ def cmd_diff(**kwargs: object) -> None: """Print the planned add/remove delta. No writes.""" - cfg = _build_config(kwargs) + cfg = _build_config_or_exit(kwargs) configure_logging(fmt=cfg.log_format) # type: ignore[arg-type] @@ def cmd_sync( @@ ) -> None: """Reconcile current → desired. --dry-run by default.""" - cfg = _build_config(kwargs) + cfg = _build_config_or_exit(kwargs) configure_logging(fmt=cfg.log_format) # type: ignore[arg-type]🤖 Prompt for 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. In `@src/optimum_keysync/cli.py` around lines 136 - 145, The call to _build_config() happens outside the try/except so config-validation exceptions escape; update cmd_show (and the other command entrypoints noted: the functions around lines 174-175 and 214-215) to wrap _build_config(kwargs) in a try/except that catches config errors and logs a clear message via _LOG.error and exits with EXIT_CONFIG; specifically, move or duplicate the _build_config call into the same error-handling flow used for auth/billing, catch the config validation exception type(s) thrown by _build_config, log the failure (include the exception message) and call sys.exit(EXIT_CONFIG).
🤖 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/kubernetes/cronjob.yaml`:
- Line 26: Update the ConfigMap comment for the resource named
"optimum-keysync-config" to correct the env var name: replace the incorrect
"OPERATOR_ID" with the required "KEYSYNC_OPERATOR_ID" so the comment accurately
lists KEYSYNC_BILLING_URL, KEYSYNC_OPERATOR_ID, NETWORK, CHAIN_ID, OWNER_NAME,
and BEACON_URL.
In `@src/optimum_keysync/config.py`:
- Around line 69-75: The operator_id field currently only requires non-empty
strings; change it to enforce the exact direct-auth contract by requiring the
prefix "ovi_live_" followed by a UUID (e.g. regex:
^ovi_live_[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)
— update the Annotated Field for operator_id in the Pydantic model(s) (the
operator_id declaration in src/optimum_keysync/config.py and the other
operator_id occurrence around the later config block) to include this regex and
keep min_length if desired, and update the validation in auth_client (the code
around the operator handling in src/optimum_keysync/auth_client.py, e.g., the
function/method that accepts or parses operator_id) to reject or raise a clear
validation error when the value does not match the new pattern so startup fails
fast.
---
Outside diff comments:
In `@src/optimum_keysync/cli.py`:
- Around line 136-145: The call to _build_config() happens outside the
try/except so config-validation exceptions escape; update cmd_show (and the
other command entrypoints noted: the functions around lines 174-175 and 214-215)
to wrap _build_config(kwargs) in a try/except that catches config errors and
logs a clear message via _LOG.error and exits with EXIT_CONFIG; specifically,
move or duplicate the _build_config call into the same error-handling flow used
for auth/billing, catch the config validation exception type(s) thrown by
_build_config, log the failure (include the exception message) and call
sys.exit(EXIT_CONFIG).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Lite
Run ID: c3dfd854-4960-4c50-94f9-8bf10e11d5bc
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by none and included by none
📒 Files selected for processing (11)
README.mdexamples/kubernetes/cronjob.yamlrequirements.txtsrc/optimum_keysync/auth_client.pysrc/optimum_keysync/billing_client.pysrc/optimum_keysync/cli.pysrc/optimum_keysync/config.pytests/test_auth_client.pytests/test_billing_client.pytests/test_cli_apply.pytests/test_config.py
💤 Files with no reviewable changes (1)
- requirements.txt
| envFrom: | ||
| - configMapRef: | ||
| name: optimum-keysync-config # KEYSYNC_AUTH_URL, BILLING_URL, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL | ||
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, OPERATOR_ID, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL |
There was a problem hiding this comment.
Fix incorrect env var name in ConfigMap comment.
The comment says OPERATOR_ID, but the required variable is KEYSYNC_OPERATOR_ID. This mismatch can mislead deployment setup.
Suggested patch
- name: optimum-keysync-config # KEYSYNC_BILLING_URL, OPERATOR_ID, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL
+ name: optimum-keysync-config # KEYSYNC_BILLING_URL, KEYSYNC_OPERATOR_ID, KEYSYNC_NETWORK, KEYSYNC_CHAIN_ID, KEYSYNC_OWNER_NAME, KEYSYNC_BEACON_URL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, OPERATOR_ID, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL | |
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, KEYSYNC_OPERATOR_ID, KEYSYNC_NETWORK, KEYSYNC_CHAIN_ID, KEYSYNC_OWNER_NAME, KEYSYNC_BEACON_URL |
🤖 Prompt for 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.
In `@examples/kubernetes/cronjob.yaml` at line 26, Update the ConfigMap comment
for the resource named "optimum-keysync-config" to correct the env var name:
replace the incorrect "OPERATOR_ID" with the required "KEYSYNC_OPERATOR_ID" so
the comment accurately lists KEYSYNC_BILLING_URL, KEYSYNC_OPERATOR_ID, NETWORK,
CHAIN_ID, OWNER_NAME, and BEACON_URL.
Comment on lines
69
to
+75
| billing_url: Annotated[str, Field(min_length=1)] | ||
| api_key: Annotated[str, Field(min_length=1)] | ||
| # operator_id is shown next to the freshly-generated key in the | ||
| # partners dashboard; customers paste both into their env. We don't | ||
| # derive it server-side (a /api/v1/me endpoint would let us, but | ||
| # that's a billing change we don't need yet). | ||
| operator_id: Annotated[str, Field(min_length=1)] |
There was a problem hiding this comment.
Tighten validation to the actual direct-auth contract.
operator_id is documented as a UUID and the direct path is documented as ovi_live_*, but this model still accepts any non-empty operator id and any ovi_* prefix. In the new terminal-401 flow, that pushes obvious config typos all the way to billing instead of failing fast at startup. Please validate the UUID shape here and require ovi_live_ specifically. Also at: src/optimum_keysync/config.py:94-99, src/optimum_keysync/auth_client.py:64-69.
As per coding guidelines, validate inputs at boundaries.
🤖 Prompt for 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.
In `@src/optimum_keysync/config.py` around lines 69 - 75, The operator_id field
currently only requires non-empty strings; change it to enforce the exact
direct-auth contract by requiring the prefix "ovi_live_" followed by a UUID
(e.g. regex:
^ovi_live_[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)
— update the Annotated Field for operator_id in the Pydantic model(s) (the
operator_id declaration in src/optimum_keysync/config.py and the other
operator_id occurrence around the later config block) to include this regex and
keep min_length if desired, and update the validation in auth_client (the code
around the operator handling in src/optimum_keysync/auth_client.py, e.g., the
function/method that accepts or parses operator_id) to reject or raise a clear
validation error when the value does not match the new pattern so startup fails
fast.
CryptoFewka
added a commit
that referenced
this pull request
Jun 25, 2026
#1 desired_state._read_indices: reject a bool/float/negative/string validator_index instead of letting int() silently coerce it to a wrong index (the JSON path is the no-beacon input we steer customers to). #2 api_client.batch_register_keys: wrap int(added/skipped) so a null or non-numeric count surfaces as ApiError, not a raw exception escaping the public boundary. #3/#4 chain_id cross-check: BeaconClient.get_chain_id() reads /eth/v1/config/deposit_contract; the CLI verifies it matches the configured chain_id before computing a plan. Catches a testnet beacon paired with the mainnet default, and a wrong beacon_url (which would otherwise resolve an empty set and look like a mass exit). Keeps the convenience defaults. #5 partial-progress: batch_register_keys takes an on_progress callback invoked per committed chunk, so a mid-register failure logs what actually landed instead of 0. Adds tests for each. 107 passed, ruff and mypy clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drops the optimum-auth JWT-exchange flow from keysync; presents the
ovi_live_*directly to billing on every API call.The original design exchanged the API key at optimum-auth for a 1h JWT and threaded that JWT through the billing client (with a
scope_changedre-exchange branch, an 80%-TTL cache window, and JWT-decode foroperator_id). Reviewing it against the actual workload — one client per operator, hourly cron, ~4 billing calls per reconcile — the exchange pattern's wins (stateless-verify-at-scale, OAuth2 ecosystem fit, scoped capability tokens) don't apply. Costs were real: multi-issuer JWT verification in billing, operator-prefix dispatch in optimum-auth, scope_version bump on revoke, ~150 lines of plumbing here. Billing's existingcombinedAuthacceptsovi_live_*Bearers directly with faster revocation (next request vs ≤1h cache).What changes
AuthClientcollapses to a credential holder: just(api_key, operator_id)returned on everytoken(). No httpx, no tenacity, nojwt.decode, no cache window, noscope_changedretry.KEYSYNC_OPERATOR_IDis back as a required env var (operators see it next to the generated key in the partners dashboard).KEYSYNC_AUTH_URLis gone.billing_client._dodrops theScopeChangedErrorbranch — a 401 from billing is now terminal.cli._compute_plan/_apply_planlose their re-exchange closures; the bearer is static, threaded directly.pyjwtremoved from runtime deps.httpx+tenacitystay (still used by beacon + billing clients).Companion PRs
Test plan
Written with Claude Code
Summary by CodeRabbit
Documentation
KEYSYNC_OPERATOR_IDfor operator identificationChores