membphis commented on PR #13346:
URL: https://github.com/apache/apisix/pull/13346#issuecomment-4561228843
Review notes from merge-risk check:
### Dependency usage check: `lua-resty-saml`
This PR adds `lua-resty-saml = 0.2.5`. The object lifecycle is mostly
aligned with `api7/lua-resty-saml` and API7 EE usage: initialize `resty_saml`
once, create/cache a SAML object per APISIX plugin config, then call
`saml:authenticate()` per request.
One concrete inconsistency still needs attention: upstream `lua-resty-saml`
tests configure a stable `session_config.secret`, and `lua-resty-saml` passes
`opts.secret` to `resty.session`. In this PR, `secret` is optional, which means
`resty.session` can fall back to process-local random keying material. That is
not safe for APISIX multi-worker / reload behavior; see P1 below.
### [P1] `secret` is optional, which can make SAML sessions fail across
workers/reloads
- Problem: `saml-auth` marks `secret` as optional, but the underlying
`lua-resty-saml` session uses it as the `resty.session`
encryption/key-derivation secret.
- Why this blocks merge: when `secret` is absent, `lua-resty-session` may
use random process-local keying material. The worker that handles the ACS
callback may be unable to read the `saml_state` cookie created by the worker
that initiated login; reloads have the same problem.
- Impact: SAML login can fail intermittently in normal multi-worker
deployments, or after worker reload/restart.
- Trigger: enable `saml-auth` without configuring `secret`.
- Evidence: `apisix/plugins/saml-auth.lua` makes `secret` optional and
passes `conf` into `resty_saml.new`; `lua-resty-saml` builds
`session_config.secret = opts.secret`; `lua-resty-session` derives IKM from
`secret` when present, otherwise defaults to random keying material. APISIX
`openid-connect` already requires `session.secret` for non-bearer interactive
login for the same class of reason.
- Suggested fix: make `secret` required for `saml-auth`, require a stronger
minimum length, document that all APISIX nodes must share the same value, and
add schema coverage.
### [P1] Runtime image misses `libxslt`, so default plugin loading can fail
- Problem: `lua-resty-saml` links `saml.so` with `-lxslt`, but the final
Debian runtime image only installs `libxml2`, not the runtime `libxslt` package.
- Why this blocks merge: `saml-auth` is added to the default plugin list and
directly requires `resty.saml` at module load time. If `libxslt` is missing,
the gateway/container can fail during plugin load/startup.
- Impact: official/minimal runtime images may fail to start after this PR,
even when users do not configure the new plugin on any route.
- Trigger: start APISIX with the default plugin list in a runtime image that
lacks `libxslt1.1`.
- Evidence: `lua-resty-saml` Makefile links `-lxslt`; this PR's
`docker/debian-dev/Dockerfile` final stage installs `libxml2` but not
`libxslt1.1`; `apisix/plugins/saml-auth.lua` uses top-level
`require("resty.saml")`.
- Suggested fix: install the runtime `libxslt` package in the final image
and verify all dynamic runtime deps of `saml.so` are present. If graceful
missing-dependency behavior is intended, keep `pcall(require, ...)` and align
the PR description/tests with that 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
