fix(standalone): harden the declarative configuration paths - #13886
fix(standalone): harden the declarative configuration paths#13886AlinsRan wants to merge 10 commits into
Conversation
dosubot
Bot
added
size:L
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
Three P1 issues must be fixed before merge.
[P1] Validate resource lists as arrays before applying the configuration
validate_configuration() only checks that resource sections are Lua tables. A top-level array, an object-shaped resource section, or scalar/null array elements can therefore bypass validation or raise outside the protected checker path. standalone.update() then advances resource versions without retaining the malformed section, which can clear existing routes, consumers, or other resources.
Please require a top-level object, require every resource section to be an array, and require every array element to be an object. Add PUT and validation regressions for a top-level array, an object used as a resource list, and scalar/null elements; each case should return 400 and leave both the configuration and its versions unchanged.
[P1] Do not log the full configuration during worker recovery
config_yaml._automatic_fetch() still logs the complete serialized configuration when a new worker restores it from the standalone-config shared dictionary. A reload or worker replacement can therefore write passwords, authentication secrets, and TLS private keys to the error log.
Please log only non-sensitive metadata such as the payload size or digest. Add a regression that pushes a sentinel secret, reloads the workers, verifies successful recovery, and asserts that the sentinel never appears in the logs.
[P1] Make the lifecycle regression prove missed-event reconciliation
The shell test exercises key-auth enabled -> unloaded -> enabled, but every asynchronous transition uses a fixed two-second sleep. It neither forces a worker to miss the reload event nor asserts the reconciled plugin registry or module state, so the pre-fix implementation can still pass through the normal event path.
Please deterministically create a missed-event or version-behind worker, use deadline-bounded polling, and assert both externally observable behavior and the relevant plugin state. Keep the reverse unload/load transition and process/configuration cleanup.
Five defects found while exercising the standalone Admin API, each
reproduced before it was fixed.
`validate_configuration()` assumes the request body is a table and that
every resource list is an array. Neither holds for client-supplied input:
`{"routes": "not-an-array"}` reaches `ipairs` and raises, and
`core.json.decode("123")` returns a scalar that is then indexed. The
validate endpoint turns that into a confusing 400 through its `pcall`,
but `admin/standalone.lua` calls the same function without one, so a PUT
answers 500.
The same handler logs the whole request body when parsing fails. That body
is a full declarative configuration and can carry `key-auth.key`,
`jwt-auth.secret`, `basic-auth.password` and TLS private keys, so a
malformed push writes them to the error log in plaintext. It now logs the
parser error only.
`admin/stream_routes.lua` gates the `superior_id == id` self-reference
check behind `skip_references_check`, so standalone validation accepts a
stream route that names itself as its own superior. That check needs no
etcd lookup; only the fetch below it does.
`cli/file.lua` and `cli/ops.lua` index `deployment.role_traditional` and
`deployment.admin` without a guard. Writing either as YAML null makes
merge_conf drop the default table, and `apisix init` dies with a Lua stack
trace where it should print a configuration error — the neighbouring lines
in ops.lua already guard the same table.
Tests: three cases in t/admin/config-validate.t for the validation paths,
and t/cli/test_deployment_null_sections.sh for the two null sections. Both
fail on master and pass with this change.
The unexpected-error path answers with `tostring(err)` from the pcall around validate_configuration. That string always carries the source path of the file that raised, and carries whatever the failing code put in the message, which is not known to exclude configuration values. The detail stays in the warn log; the response gets a fixed entry.
…nciliation on `admin/standalone.lua` writes the whole configuration to the log twice: the encoded payload at INFO on every successful update, and the raw body at ERROR when parsing fails. That payload carries TLS private keys and plugin credentials, so every push copies them into the error log and into anything downstream of it. It now logs the payload size and the parser error. `admin/init.lua` skips the plugins-reload reconciliation timer whenever the configuration comes from the yaml provider. `/v1/plugins/reload` stays reachable in that mode and bumps the same shared version (apache#13714), and the events broadcast has no delivery guarantee, so a worker that missed it has no way left to converge. The timer is no longer gated on the provider. Tests: t/admin/standalone.t TEST 18 and TEST 19 push a sentinel credential and an unparsable body and assert neither reaches the log; both fail without the logging change. t/cli/test_standalone_plugin_reload.sh drives a real gateway in `traditional + yaml + enable_admin`, pushes a key-auth route through the config API, then unloads and reloads key-auth through /v1/plugins/reload and asserts the external behavior after each transition — the upstream is deliberately absent, so the status separates the two states: 401 means key-auth ran, 502 means the request got past the plugins.
Review found three more holes in the same area. `validate_configuration()` only checked that a resource section was a Lua table, which a JSON array, an object-shaped section and scalar or null elements all satisfy. None of those were validated, and `update()` then advanced every resource version while retaining nothing, so a malformed push answered 202 and cleared the routes and consumers that were there. The shape is now checked with a schema generated from the resource list, which also puts this on the same footing as the rest of the Admin API. `config_yaml._automatic_fetch()` logged the whole serialized configuration when a worker restored it from the shared dict, so a reload wrote credentials and TLS private keys to the error log. It logs the payload size. Tests: t/admin/config-validate.t covers a top-level array, an object used as a resource list, and scalar and null elements against the validate endpoint; t/admin/standalone.t covers the same shapes against PUT and asserts the stored configuration and its versions are untouched afterwards.
09f0b8f to
5da8214
Compare
The plugin reload transitions waited two seconds each, which both slows the test down and hides a reload that is merely late. They now poll the observable status to a deadline.
The envelope schema also constrained the *_conf_version fields, so a negative or non-numeric value was answered with the schema's wording instead of the messages update() and validate_configuration() already produce, which t/admin/standalone.spec.ts asserts. The schema now covers only the resource sections.
- sync_local_conf_to_etcd() returns early under a yaml config provider. The reconciliation timer reaches the admin reload path, which wrote /plugins to an etcd that standalone does not have. - yaml.load() raises on a malformed document rather than returning an error, so PUT /apisix/admin/configs answered 500 for a body the validate endpoint already answered 400 for. - A rejected item is still a resource and can carry TLS private keys or plugin credentials; config_yaml logs its key instead of its value. - read_yaml_conf() rejects a scalar config.yaml document, which lyaml returns as a string and resolve_conf_var then iterates. - deployment.admin written as YAML null is read through try_read_attr, matching the guard the CLI already has.
There was a problem hiding this comment.
Pull request overview
Hardens standalone declarative configuration parsing, validation, logging, deployment initialization, and plugin reload handling.
Changes:
- Validates configuration shapes and stream-route self-references.
- Prevents sensitive configuration bodies from reaching logs.
- Guards nullable deployment sections and adds regression coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
apisix/admin/config_validate.lua |
Adds request-shape validation and safer errors. |
apisix/admin/init.lua |
Guards admin configuration and extends reload reconciliation. |
apisix/admin/standalone.lua |
Handles YAML errors and redacts body logs. |
apisix/admin/stream_routes.lua |
Enforces self-reference validation. |
apisix/cli/file.lua |
Guards scalar YAML and null deployment sections. |
apisix/cli/ops.lua |
Handles null admin configuration. |
apisix/core/config_yaml.lua |
Redacts configuration payloads from logs. |
t/admin/config-validate.t |
Tests malformed configuration shapes and references. |
t/admin/standalone.t |
Tests rejection and log redaction behavior. |
t/cli/test_deployment_null_sections.sh |
Tests null deployment sections. |
t/cli/test_standalone_plugin_reload.sh |
Tests standalone plugin reloads. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- lyaml loads a null document (`~`) as a sentinel table, which passed the object schema and then cleared every resource with a 202. JSON null was already rejected; the two are consistent now. - /v1/plugins/reload is handled by control/router.lua, which loads the plugins but cannot record the version in this module, so the reconciliation timer loaded them a second time. Acknowledge the version on that event. - Cover the yaml parse failures and the scalar config.yaml guard, neither of which had a test.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P1] Add lifecycle coverage for missed-event reconciliation
The new t/cli/test_standalone_plugin_reload.sh only exercises the normal /v1/plugins/reload broadcast path. It never forces a worker to miss the event while plugin-conf-version advances, so the timer-based reconciliation branch in apisix/admin/init.lua remains untested.
This blocks merge because the purpose of the new reconciliation logic is to recover from the event system's lack of delivery guarantees. A passing normal reload path cannot prove that a version-behind worker converges or that stale plugin timers and module state are removed.
Please add a required-CI, real-process t/cli/test_*.sh case that deterministically creates a missed-event or version-behind worker, uses deadline-bounded polling, verifies external behavior and the plugin registry/module state, checks that no stale timer remains after unload, and covers the reverse load/unload transition.
The reload path was only exercised through /v1/plugins/reload, whose broadcast is delivered, so the reconciliation timer never ran. GET /bump advances plugins_conf_version without posting the event, which is the state a worker that missed the broadcast is left in, and only the timer can converge it from there. Asserted in both directions, with deadline-bounded polling: the proxied status (key-auth loaded or not), the plugin api registry rebuilt behind public-api, and error-log-logger's background timer stopping and starting again. Restoring the pre-PR gate makes the new section fail.
Added in 67f214c, in |
Description
Five defects in the standalone declarative-configuration paths, found while exercising the Admin API added by #13483. Each was reproduced before it was fixed, and each has a test that fails on master.
validate_configuration()trusts the shape of the request body. It assumes the body is a table and that every resource list is an array. Neither holds for client-supplied input:{"routes": "not-an-array"}gives#items == 3, thenipairsraises.core.json.decode("123")returns a scalar without an error, which is then indexed.POST /apisix/admin/configs/validateturns that into a confusing 400 through itspcall, butapisix/admin/standalone.luacalls the same function without one, soPUT /apisix/admin/configsanswers 500 instead of rejecting the input.The same handler logs the whole request body when parsing fails. That body is a full declarative configuration and can carry
key-auth.key,jwt-auth.secret,basic-auth.passwordand TLS private keys, so one malformed push writes them to the error log in plaintext. It now logs the parser error only.admin/stream_routes.luaskips the self-reference check during standalone validation. Thesuperior_id == idcheck sits inside theskip_references_checkgate, so a stream route naming itself as its own superior passes validation. That check needs no etcd lookup; only the fetch below it does, so it moves outside the gate.cli/file.luaandcli/ops.luaindex deployment sections without a guard. Writingrole_traditional:oradmin:as YAML null makesmerge_confdrop the default table, andapisix initthen dies with a Lua stack trace where it should print a configuration error. The lines immediately around the one inops.luaalready guard the same table:Which issue(s) this PR fixes
No open issue; found while working on the standalone Admin API.
Checklist