AlinsRan opened a new pull request, #13719:
URL: https://github.com/apache/apisix/pull/13719
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:
```yaml
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):
```yaml
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
https://github.com/apache/apisix/issues/12170#issuecomment-3597623938 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
