Skip to content

fix(admin): stop rewriting config.yaml when admin_key is empty#13719

Draft
AlinsRan wants to merge 1 commit into
apache:masterfrom
AlinsRan:fix/no-admin-key-write-back
Draft

fix(admin): stop rewriting config.yaml when admin_key is empty#13719
AlinsRan wants to merge 1 commit into
apache:masterfrom
AlinsRan:fix/no-admin-key-write-back

Conversation

@AlinsRan

Copy link
Copy Markdown
Contributor

Fixes #12170

What was wrong

When deployment.admin.admin_key contains an entry with an empty key, core/id.lua generated a random 32-char key, flipped a changed flag, and rewrote conf/config.yaml via lyaml.dump() of the parsed config. Rewriting a hand-written config file from a parsed table is lossy:

  • every comment in the file is dropped
  • lyaml emits document markers, and a trailing ... makes the next startup fail to parse the config — the restart error in the issue
  • generate_yaml() needed a <PLACEHOLDER> string-substitution hack to keep nulls from turning into []

It also mutates a file the user owns, which is wrong on its own: config directories are frequently read-only in containers, and the generated key is unpredictable and rotates on every restart.

What this changes

Remove the generation and the write-back entirely, and reject the empty key at startup instead:

  • core/id.lua: drop autogenerate_admin_key(), generate_yaml(), and the write_file() call on conf/config.yaml. The module is back to only managing conf/apisix.uid.
  • cli/ops.lua: an admin entry with an empty (or null) key is now a hard error instead of a warning that announced the auto-generation.

The empty-key check no longer sits behind the allow_admin == ["127.0.0.0/24"] exemption. That exemption has to stay for the missing key case (admin_key: null) since it is existing, relied-on behavior, but it cannot cover the empty-key case anymore: with generation removed, an empty key would otherwise survive into runtime, where check_token() compares it against the request token — and an X-API-KEY: header sent with an empty value would match it. Failing fast is the only safe outcome.

Behavior change

This is a breaking change and should be called out in the release notes.

Before: empty admin_key → APISIX silently invented a key, rewrote your config.yaml, and started.
After: empty admin_key → APISIX refuses to start.

Upgrade path, either:

deployment:
  admin:
    admin_key:
      - name: admin
        key: <your key>   # e.g. openssl rand -hex 32
        role: admin

or, to run the Admin API without authentication (trials, local dev — the case admin_key_required already exists for):

deployment:
  admin:
    admin_key_required: false

The shipped conf/config.yaml still has key: '', so a fresh install now has to set one of the two. The comment there and the docs (FAQ.md, admin-api.md, dashboard.md, en + zh) were updated to describe the new behavior instead of the write-back.

Direction

Earlier attempts (#12484, #12535, #13091) tried to keep auto-generation and make the persistence non-destructive, most recently via a sidecar key file. That direction was argued against in the issue thread; the discussion converged on dropping generation altogether and using the existing admin_key_required switch, per #12170 (comment) and the maintainer comments preceding it. This PR implements that.

Test status

Marked as draft — the remaining work is a test-suite decision I would like confirmed before churning it.

Done here: t/cli/test_admin.sh — the case asserting WARNING: using empty Admin API. now asserts the new error, and additionally checks that conf/config.yaml was left untouched (a direct regression test for this issue). Two cases in that file that relied on reading the auto-generated key back out of config.yaml now declare an explicit key.

Not done: the built-in default in apisix/cli/config.lua is admin_key = {{name = "admin", key = "", role = "admin"}}, so any test config that does not mention admin_key inherits an empty key and will now fail to start. That is 33 of 51 files under t/cli, spanning ~147 > conf/config.yaml sites. The main t/ suite is unaffected: t/APISIX.pm sets admin_key: null, which takes the missing-key path, not the empty-key path.

Adding an explicit key at those sites restores the pre-change effective behavior, so the migration is semantically safe, but it is a large mechanical diff and some of those files (test_main.sh, test_validate_config.sh) assert specific startup failure messages. Before doing it I would like a call on the preferred form — explicit admin_key everywhere, or admin_key_required: false for the tests that never touch the Admin API. This is the same question raised in #12535 that was never settled, and it is what stalled the previous attempts.

I have not run t/cli or the t/ suite for this change; verification so far is luacheck plus a syntax check on the two modified Lua files. CI results on the points above are what should be trusted.

APISIX generated a random admin key when deployment.admin.admin_key held an
empty key and wrote the result back to conf/config.yaml. The write-back
re-serialized the parsed config with lyaml, dropping all comments and emitting
document markers that could break the next startup.

Remove the generation and the write-back, and reject an explicitly configured
empty admin key at startup instead. Users who do not want Admin API
authentication can set admin_key_required: false.
@AlinsRan

Copy link
Copy Markdown
Contributor Author

A review pass turned up that the bypass this PR closes is not just a latent risk — a variant is live on master today, which strengthens the case for this change and is worth adding to the description.

autogenerate_admin_key() only rewrites entries where role == "admin" (core/id.lua). And the admin_key schema is written as properties.items rather than items (cli/schema.lua), so per-entry fields are never actually validated. So on current master:

  • admin_key: [{name: viewer, key: '', role: viewer}] — not rewritten by autogeneration → empty key reaches runtime → an empty X-API-KEY: matches that row → unauthenticated read access to the whole Admin API. The shipped conf/config.yaml.example even has this viewer entry sitting commented-out for users to uncomment.
  • role: ops (any non-viewer string) with an empty key — check_token's viewer-method restriction only fires for role == "viewer", so this gives unauthenticated full read/write.

I verified this against upstream/master directly (check_token empty-string match, autogeneration's admin-only loop, viewer_methods = {get=true}, and the properties.items schema slip). This PR's loop util.dies on an empty key for any role, so it closes this too — worth calling out in the description, and arguably a reason to move it out of draft sooner rather than later. The properties.items schema bug is separate and pre-existing; I can file it on its own.

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.

bug: APISIX rewrites yaml configuration file to cause restarting errors

1 participant