[PR] fix: preserve multiline SSL environment values in standalone YAML [apisix]

["ruanweihong-aaa (via GitHub)" <gi...@apache.org>]

ruanweihong-aaa opened a new pull request, #14074:
URL: https://github.com/apache/apisix/pull/14074

   ## Description
   
   Standalone YAML expands environment variables before parsing. Raw multiline PEM values can therefore cause YAML parse errors or have their newlines folded, preventing SSL resources from loading.
   
   This change preserves multiline values when a complete environment reference is used in an SSL resource's `cert`, `key`, `certs`/`keys` array item, or `client.ca`.
   
   For example, `SERVER_CERT` and `SERVER_KEY` can contain original multiline PEM:
   
   ```yaml
   ssls:
     - id: example
       snis: ["example.com"]
       cert: "${{SERVER_CERT}}"
       key: "${{SERVER_KEY}}"
   #END
   ```
   
   The loader builds a temporary discovery copy, identifies supported fields, and protects the corresponding variable occurrences with placeholders during the final YAML parse. It checks scalar source locations and final field paths before restoring the original values, preventing accidental replacement of user content.
   
   CLI validation and runtime file reload share the same helper. No new dependency or configuration switch is introduced.
   
   ### Compatibility and supported scope
   
   - Supports complete quoted or unquoted references in block-style and flow-style YAML.
   - Supports anchors and merges whose protected values are used only in supported SSL fields.
   - Preserves existing single-line substitution, YAML type inference, and literal `\n` escape handling.
   - Preserves existing folding and structure-expansion behavior for non-SSL multiline fields.
   - Reuses existing variable lookup, defaults, missing-variable errors, and exported-variable tracking.
   - Falls back to the existing loading behavior when discovery is ambiguous, protected references are shared with non-SSL fields, or final checks fail. These fallback cases do not receive the new multiline protection.
   - Does not add protection for embedded references or references inside literal/folded block scalars.
   - Preserves legacy expansion when an entire configuration is supplied through one variable; values introduced by another variable are not recursively expanded.
   
   JSON and bootstrap `config.yaml` processing are unchanged. `$ENV://` remains an existing alternative. This change does not bypass SSL schema or certificate validation.
   
   ### Related issue
   
   Related to #14038.
   
   ### Validation
   
   Tested using `apache/apisix:3.19.0-ubuntu` with the working-tree candidate mounted:
   
   - 24/24 focused regressions passed under plain LuaJIT.
   - 24/24 focused regressions passed under OpenResty.
   - Confirmed that stock CLI initialization rejects block-style unquoted multiline PEM.
   - Verified candidate startup with quoted PEM using the served certificate's SHA-256 fingerprint.
   - Verified that live YAML reload with unquoted flow references serves a different certificate.
   - Verified reload using the existing literal-backslash-n PEM format.
   - Verified explicit candidate `apisix init`.
   - Covered marker collisions, YAML escapes, structural substitution, shared anchors, and cyclic aliases.
   - Changed-file Lua lint passed with 0 warnings and 0 errors across four Lua files.
   - `git diff --check` and patch reverse-apply checks passed.
   
   The full project test suite remains unverified. The release image lacks `make` and the complete project test environment; full-suite validation requires CI.
   
   ### Checklist
   
   - [x] I have explained the need for this PR and the problem it solves.
   - [x] I have explained the changes.
   - [x] I have added tests corresponding to this change.
   - [x] I have updated the documentation.
   - [ ] I have verified that this change is backward compatible.
   
   The compatibility checkbox remains open pending full CI validation and maintainer review of the documented fallback behavior.
   
   


-- 
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: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: preserve multiline SSL environment values in standalone YAML [apisix]

["ruanweihong-aaa (via GitHub)" <gi...@apache.org>]

ruanweihong-aaa commented on PR #14074:
URL: https://github.com/apache/apisix/pull/14074#issuecomment-6424123838

   Thanks @jjansenvr! Added a once-per-load warning when an identified multiline SSL reference cannot be safely protected, without logging its value. Ordinary multiline configuration stays quiet; cases where SSL fields cannot be identified still retain legacy behavior.
   
   Added CRLF and warning/fallback tests; the existing tests cover the other cases you listed. All 28 tests pass under LuaJIT and OpenResty, plus real TLS startup/reload checks. The `ssls`-only scope is intentional, and `render()` now documents its ordering assumption. No additional parsing passes were added (the existing path can perform up to three YAML loads and two event scans on fallback). Also merged the latest master.


-- 
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: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: preserve multiline SSL environment values in standalone YAML [apisix]

["jjansenvr (via GitHub)" <gi...@apache.org>]

jjansenvr commented on PR #14074:
URL: https://github.com/apache/apisix/pull/14074#issuecomment-6415508670

   Thanks for working on this. The approach looks right to me, and I like that it is careful about backward compatibility.
   
   **What I like**
   
   - **Nothing changes for existing setups.** If none of the environment values contain a line break, configuration loads exactly as it does today. Quoted placeholders such as `"${{API_KEY}}"` stay strings, unquoted ones such as `${{STATUS}}` are still read as numbers or booleans, and lists and `null` from env vars keep working.
   - **Multiline certificates and keys now work.** A PEM stored in an environment variable can be used as-is in `cert`, `key`, `certs`, `keys` and `client.ca`, both quoted and unquoted, without extra escaping.
   - **PEM content is never altered.** The original value ends up in the config exactly as it was provided.
   - **Predictable failure mode.** When the loader cannot be sure which field a value belongs to, it falls back to the existing behavior instead of guessing.
   - **Same behavior in the CLI and at runtime.** Validation and reload use the same logic, so a config that passes the check also loads when it is reloaded.
   - **Clear documentation** of what is and is not supported, including the `$ENV://` alternative for the unsupported cases.
   
   **Questions / suggestions**
   
   1. **Silent fallback.** Every ambiguous case (alias cycle, shared anchor, marker outside an SSL field, parse failure) falls back to `yaml.load(expanded)`. For a multiline value this usually means a YAML error or folded newlines, with no hint why the protection did not apply. Could we log a warning, or return a clearer error, when multiline values were detected but no field was protected?
   2. **Scope.** Only `ssls[].cert/key/certs/keys/client.ca` are covered. Multiline PEMs also appear in other places, e.g. `upstreams[].tls.client_cert/client_key` and plugin configs holding public or private keys. Is the SSL-only scope intentional for this PR? If so, a sentence in the docs, or a follow-up issue, would help. It looks like extending `is_ssl_field` would be enough for the path-based cases.
   3. **Assumption in `render()`.** It indexes `values[index]` and relies on `substitute` calling the callback the same number of times and in the same order on every run. That holds for `var_sub` today, but a short comment (or an assertion that `index == #values` after rendering) would protect against future changes to the substitution logic.
   4. **Cost.** With multiline values there are up to three YAML parses plus one event-parser pass per load. This only happens at startup and reload, so it is probably fine, but it may be worth noting.
   
   **Tests**
   
   The current tests cover the main case. I would also add:
   
   - multiline PEM in a quoted and an unquoted scalar;
   - flow-style sequences containing placeholders;
   - CRLF line endings inside the PEM;
   - a multiline value in a non-SSL field (behavior should be unchanged, and other fields must not be affected);
   - a variable shared through an anchor with a non-SSL field (must fall back);
   - a regression test showing that single-line values behave exactly as before, including `"${{N}}"` with a numeric value staying a string.


-- 
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: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org