fix(apisix): send OIDC hosts to a canonical https origin before openid-connect (#5525)

* fix(apisix): send OIDC hosts to a canonical https origin before openid-connect

The shared-plugin defaults already carry APISIX's `redirect` plugin with
http_to_https, but `redirect` has a lower priority than openid-connect (2599),
and APISIX dispatches a phase in descending priority. On every OIDC-protected
route the upgrade therefore never runs: openid-connect has already answered.

That matters because APISIX derives only a relative redirect_uri and
lua-resty-openidc 1.8.0 -- the version APISIX 3.17 pins -- makes it absolute
from ngx.var.scheme and ngx.var.http_host. Both are raw connection values, so
a plain-HTTP request sends Keycloak an http:// redirect_uri and a request
carrying `Host: <host>:443` sends an authority with the port still on it.
Keycloak registers bare-host https URIs only and rejects both with
error="invalid_redirect_uri", killing the login at the authorization endpoint
before any callback exists for the recovery function to rescue. Reproduced
against production on both api.learn and nb.learn, so this is not specific to
the host the Keycloak log happens to show most.

Worse than the failed logins: with the upgrade shadowed, APISIX answers plain
-HTTP requests to these hosts with an OIDC session cookie over cleartext and
without the Secure attribute.

Redirecting is what fixes this rather than setting X-Forwarded-Proto and
X-Forwarded-Host. lua-resty-openidc does prefer those headers over the ngx
vars, but on this deployment none of Forwarded / X-Forwarded-Proto /
X-Forwarded-Host reaches it -- sending each against production leaves the
redirect_uri unchanged -- so pinning them would be a no-op that reads like a
fix.

APISIX keys a plugin config by plugin name, so a route can carry exactly one
serverless-pre-function, and both this and the error-callback recovery have to
run ahead of openid-connect. They are consequently one plugin with two
functions rather than two plugins, which is why the builder is renamed.
serverless/init.lua runs `functions` in array order and stops at the first
returning a code, so the origin is normalised before recovery decides whether
to redirect back into a login flow -- otherwise recovery would target an
http:// origin and fail again.

Leaving port 80 answering with a redirect is only safe because every ACME
ClusterIssuer on the cluster solves via dns01/Route53; an issuer switched to
http-01 would need its challenge path carved out.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G9yBC9UqoVwiPtxqLNgUD9

* fix(apisix): validate the redirect status, correct the 308 rationale

Copilot review on #5525, both points verified against source before acting.

ngx.redirect accepts only 301/302/303/307/308 and raises a Lua error on
anything else (ngx_http_lua_control.c:209-219). The block this setting travels
in is not part of serverless-pre-function's schema, so APISIX will not reject a
bad value either -- it would first surface as a 500 on live traffic. The
`Literal` annotation alone does not prevent that: this repo's mypy hook runs
without the project installed, so a cross-module call resolves to Any and the
bad literal passes. Validating in the builder moves the failure to `pulumi
preview`, with tests over both the accepted and rejected sets.

The 308 rationale was also wrong. APISIX's `redirect` plugin is method-dependent
-- 301 for GET/HEAD, 308 for everything else (redirect.lua:208-215) -- so it
would not have downgraded a POST, and the previous comment claimed a
behavioural fix where there is only a simplification to one uniform status.
Corrected in the Lua comment and the docstring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G9yBC9UqoVwiPtxqLNgUD9

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: blarghmatey

src/ol_infrastructure/applications/jupyterhub/deployment.py

@@ -23,7 +23,7 @@
     OLApisixRouteConfig,
     OLApisixSharedPlugins,
     OLApisixSharedPluginsConfig,
-    oidc_error_callback_recovery_plugin,
+    oidc_gateway_pre_function_plugin,
     stale_session_cookie_cleanup_plugin,
 )
 from ol_infrastructure.components.services.cert_manager import (
@@ -187,11 +187,17 @@ def provision_jupyterhub_deployment(  # noqa: PLR0913
                 # host belongs to mit-learn, which clears it from its own
                 # routes.  Safe to delete once the old cookies have aged out.
                 stale_session_cookie_cleanup_plugin(),
-                # Same expired-authentication-session callbacks that api.learn
-                # and mitxonline see, at this host's much smaller volume (2/day
-                # on nb.learn.mit.edu).  Attached for consistency of behaviour
-                # across the OIDC-protected hosts rather than for the volume.
-                oidc_error_callback_recovery_plugin(),
+                # Two pre-openid-connect fixes, necessarily one plugin (APISIX
+                # allows a single serverless-pre-function per config).  The
+                # expired-authentication-session callbacks are the same ones
+                # api.learn and mitxonline see, at this host's much smaller
+                # volume (2/day).  The canonical-origin redirect matters more
+                # here than anywhere else: nb.learn and authoring.nb.learn are
+                # the two hosts Keycloak actually rejects today, 61 hits over
+                # 7 days between them, because they take direct plain-HTTP
+                # traffic that the `redirect` default plugin never gets to
+                # upgrade.
+                oidc_gateway_pre_function_plugin(),
             ],
         ),
     )

src/ol_infrastructure/applications/mit_learn/__main__.py

@@ -55,7 +55,7 @@
     OLApisixRouteConfig,
     OLApisixSharedPlugins,
     OLApisixSharedPluginsConfig,
-    oidc_error_callback_recovery_plugin,
+    oidc_gateway_pre_function_plugin,
     stale_session_cookie_cleanup_plugin,
 )
 from ol_infrastructure.components.services.cert_manager import (
@@ -1355,8 +1355,11 @@
             # openid-connect plugin serves each one a 500.  Both route groups
             # on this host need it, and the plugin derives its redirect target
             # from the request URI, so the /login and /learn/login prefixes are
-            # handled from this one attachment.
-            oidc_error_callback_recovery_plugin(),
+            # handled from this one attachment.  The same attachment also
+            # canonicalises the origin: `curl http://api.learn.mit.edu/login`
+            # currently sends Keycloak an http:// redirect_uri, and answers with
+            # an OIDC session cookie over cleartext.
+            oidc_gateway_pre_function_plugin(),
         ],
     ),
 )

src/ol_infrastructure/applications/mitxonline/__main__.py

@@ -51,7 +51,7 @@
     OLApisixRouteConfig,
     OLApisixSharedPlugins,
     OLApisixSharedPluginsConfig,
-    oidc_error_callback_recovery_plugin,
+    oidc_gateway_pre_function_plugin,
     stale_session_cookie_cleanup_plugin,
 )
 from ol_infrastructure.components.services.cert_manager import (
@@ -815,8 +815,10 @@
             # cleanup below, this is safe to attach here rather than per route
             # group: it derives its redirect target from the request URI and
             # its guard cookie is host-only, so neither depends on which parent
-            # domain a group's session cookie was scoped to.
-            oidc_error_callback_recovery_plugin(),
+            # domain a group's session cookie was scoped to.  Ditto the
+            # canonical-origin redirect it also carries, which is derived from
+            # the request's own host.
+            oidc_gateway_pre_function_plugin(),
         ],
     ),
 )

src/ol_infrastructure/components/services/apisix.py

@@ -15,13 +15,20 @@
 )
 from ol_infrastructure.lib.pulumi_helper import parse_stack
 
-# Read once at import: the file is shipped verbatim as the serverless function
-# body, with configuration passed separately on the plugin config.
+# Read once at import: the files are shipped verbatim as serverless function
+# bodies, with configuration passed separately on the plugin config.
 OIDC_ERROR_RECOVERY_LUA = (
     Path(__file__)
     .parent.joinpath("files", "oidc_error_callback_recovery.lua")
     .read_text()
 )
+CANONICAL_HTTPS_REDIRECT_LUA = (
+    Path(__file__).parent.joinpath("files", "canonical_https_redirect.lua").read_text()
+)
+
+# The only statuses ngx.redirect accepts; anything else is a Lua error at
+# request time (ngx_http_lua_control.c:209-219).
+NGX_REDIRECT_STATUSES = (301, 302, 303, 307, 308)
 
 
 class OLApisixPluginConfig(BaseModel):
@@ -112,21 +119,60 @@ def stale_session_cookie_cleanup_plugin(
     )
 
 
-def oidc_error_callback_recovery_plugin(
+def oidc_gateway_pre_function_plugin(
     recoverable_errors: list[str] | None = None,
     guard_cookie_name: str = "apisix_oidc_recovery",
     guard_max_age: int = 60,
+    *,
+    canonical_https_redirect: bool = True,
+    canonical_redirect_status: Literal[301, 302, 303, 307, 308] = 308,
 ) -> OLApisixPluginConfig:
-    """Restart the login flow when the IdP redirects back with a recoverable error.
-
-    An authorization request whose Keycloak authentication session has expired
-    -- the user left the login tab open, or followed a stale bookmark -- comes
-    back to the callback with ``error=temporarily_unavailable`` and no ``code``.
+    """Everything that has to happen before openid-connect sees the request.
+
+    APISIX keys a plugin config by plugin name, so a route can carry exactly ONE
+    ``serverless-pre-function``.  Both of the fixes below have to run ahead of
+    openid-connect (priority 2599), and ``serverless-pre-function`` (priority
+    10000) is the only hook that gets there, so they are necessarily one plugin
+    rather than two.  ``serverless/init.lua`` runs ``functions`` in array order
+    and stops at the first one returning a code or body, which is exactly the
+    sequencing wanted here: normalise the origin first, and only then look at
+    whether this is a failed callback.
+
+    **Canonical origin** (``canonical_https_redirect.lua``).  The shared-plugin
+    defaults already include APISIX's ``redirect`` plugin with ``http_to_https``,
+    but its priority is below openid-connect's, so on an OIDC route it is dead
+    code -- openid-connect has already answered.  The consequences are measured,
+    not hypothetical.  APISIX derives only a relative redirect_uri and
+    lua-resty-openidc 1.8.0 (the version APISIX 3.17 pins) makes it absolute from
+    ``ngx.var.scheme`` and ``ngx.var.http_host``, so a plain-HTTP request sends
+    Keycloak ``http://...`` and a request carrying ``Host: <host>:443`` sends
+    ``https://<host>:443/...``.  Keycloak registers bare-host https URIs only and
+    rejects both with ``error="invalid_redirect_uri"``; the login dies at the
+    authorization endpoint, before any callback exists for the recovery function
+    below to rescue.  Worse than the failed logins: because the upgrade never
+    runs, APISIX answers plain-HTTP requests with an OIDC session cookie over
+    cleartext and without the ``Secure`` attribute.
+
+    Redirecting is what fixes this, not header-setting.  lua-resty-openidc does
+    prefer ``Forwarded`` / ``X-Forwarded-Proto`` / ``X-Forwarded-Host`` over those
+    ngx vars, but on this deployment none of the three reaches it -- sending each
+    against production leaves the redirect_uri unchanged -- so pinning them would
+    be a no-op dressed up as a fix.
+
+    Note this leaves port 80 answering with a redirect rather than closing it.
+    That is only safe because every ACME ClusterIssuer on the cluster solves via
+    dns01/Route53; an issuer switched to http-01 would need its challenge path
+    carved out of the redirect.
+
+    **Error-callback recovery** (``oidc_error_callback_recovery.lua``).  An
+    authorization request whose Keycloak authentication session has expired --
+    the user left the login tab open, or followed a stale bookmark -- comes back
+    to the callback with ``error=temporarily_unavailable`` and no ``code``.
     Keycloak's intent there is that the client start over; it even marks the
-    event ``restart_after_timeout="true"``.  ``lua-resty-openidc`` instead
-    treats any ``error`` parameter as fatal and hands the openid-connect plugin
-    a failure, which APISIX serves as a 21KB HTTP 500.  The user sees a
-    stack-trace page where they expected a login form, and nothing retries.
+    event ``restart_after_timeout="true"``.  ``lua-resty-openidc`` instead treats
+    any ``error`` parameter as fatal and hands the openid-connect plugin a
+    failure, which APISIX serves as a 21KB HTTP 500.  The user sees a stack-trace
+    page where they expected a login form, and nothing retries.
 
     This is measured, not hypothetical: 614 such callbacks a day across
     api.learn.mit.edu, mitxonline.mit.edu and nb.learn.mit.edu, from 530
@@ -154,14 +200,15 @@ def oidc_error_callback_recovery_plugin(
     looping: a persistently broken IdP should surface as an error, not as an
     infinite redirect.
 
-    The Lua itself lives in ``files/oidc_error_callback_recovery.lua`` and is
-    shipped verbatim -- nothing is interpolated into it.  Tunables travel as an
-    ``oidc_error_recovery`` block on the plugin config, which the function reads
-    off ``conf``: ``serverless/init.lua`` invokes each function as
+    Both functions live in ``files/`` and are shipped verbatim -- nothing is
+    interpolated into them.  Tunables travel as ``oidc_error_recovery`` and
+    ``canonical_https_redirect`` blocks on the plugin config, which the functions
+    read off ``conf``: ``serverless/init.lua`` invokes each as
     ``func(conf, ctx)``, and its schema does not set ``additionalProperties``,
-    so extra keys validate.  Keeping it a real ``.lua`` file means it is
+    so extra keys validate.  Keeping them real ``.lua`` files means they are
     syntax-highlighted, reviewable, and testable under APISIX's own test-nginx
-    harness (``t/oidc_error_callback_recovery.t``).
+    harness (``t/oidc_error_callback_recovery.t``,
+    ``t/canonical_https_redirect.t``).
 
     :param recoverable_errors: OAuth 2.0 ``error`` codes to restart the flow
         for.  Defaults to ``temporarily_unavailable``, which is 100% of what
@@ -171,21 +218,51 @@ def oidc_error_callback_recovery_plugin(
     :param guard_cookie_name: Name of the loop-breaker cookie.
     :param guard_max_age: Seconds the guard cookie lives, bounding how often one
         browser can be sent back through login.
+    :param canonical_https_redirect: Whether to send non-canonical origins to
+        ``https://<bare host>`` before openid-connect runs.  ``False`` drops the
+        function entirely, for a host that must keep answering on plain HTTP.
+    :param canonical_redirect_status: Status for that redirect, uniform across
+        methods.  APISIX's own ``redirect`` plugin instead picks per method --
+        301 for GET/HEAD, 308 for everything else (``redirect.lua`` 208-215) --
+        so 308 here is a simplification rather than a behavioural fix: both
+        preserve a POST.  Restricted to the codes ``ngx.redirect`` accepts.
 
     :returns: A ``serverless-pre-function`` plugin config to attach to routes.
     :rtype: OLApisixPluginConfig
     """
+    # Checked rather than left to the annotation: the `Literal` above documents
+    # the contract but nothing enforces it at the call sites, since this repo's
+    # mypy hook runs without the project installed and resolves a cross-module
+    # import to Any.  APISIX will not catch it either -- the block this travels
+    # in is not part of serverless-pre-function's schema -- so an unchecked bad
+    # value would first surface as a 500 on live traffic.  Raising here moves
+    # that to `pulumi preview`.
+    if canonical_redirect_status not in NGX_REDIRECT_STATUSES:
+        msg = (
+            f"canonical_redirect_status must be one of {NGX_REDIRECT_STATUSES}, "
+            f"got {canonical_redirect_status}: ngx.redirect rejects anything else."
+        )
+        raise ValueError(msg)
+
+    # Order matters and is load-bearing: serverless/init.lua stops at the first
+    # function returning a code, so the origin has to be canonical before the
+    # recovery function decides whether to redirect back into the login flow.
+    functions = [OIDC_ERROR_RECOVERY_LUA]
+    if canonical_https_redirect:
+        functions.insert(0, CANONICAL_HTTPS_REDIRECT_LUA)
     return OLApisixPluginConfig(
         name="serverless-pre-function",
         secretRef=None,
         # rewrite rather than the plugin's default access phase: openid-connect
         # also runs in rewrite, and serverless-pre-function's priority (10000)
-        # outranks it (2599), so this gets to inspect the callback and bail out
-        # before the plugin turns the error parameter into a 500.  In the access
-        # phase it would run after openid-connect had already failed.
+        # outranks it (2599), so this gets to normalise the origin and inspect
+        # the callback before the plugin reads either.  In the access phase it
+        # would run after openid-connect had already built its redirect_uri and
+        # failed.
         config={
             "phase": "rewrite",
-            "functions": [OIDC_ERROR_RECOVERY_LUA],
+            "functions": functions,
+            "canonical_https_redirect": {"status": canonical_redirect_status},
             "oidc_error_recovery": {
                 # `is None`, not `or`: an explicit empty list means "recover
                 # nothing", and `or` would quietly turn that back into the

src/ol_infrastructure/components/services/files/canonical_https_redirect.lua

@@ -0,0 +1,64 @@
+-- Send the request to the canonical https://<host> origin before the
+-- openid-connect plugin builds a redirect_uri out of it.
+--
+-- Attached as a serverless-pre-function in the `rewrite` phase, where this
+-- plugin's priority (10000) outranks openid-connect's (2599). That ordering is
+-- the whole point: the shared-plugin defaults already carry APISIX's `redirect`
+-- plugin with http_to_https, but `redirect` has a lower priority than
+-- openid-connect, so on an OIDC route it never gets to run.
+--
+-- APISIX only derives a *relative* redirect_uri (`<uri>/.apisix/redirect`), and
+-- lua-resty-openidc 1.8.0 -- the version pinned by APISIX 3.17 -- turns it
+-- absolute from `ngx.var.scheme` and `ngx.var.http_host`. Both are raw
+-- connection values, so a plain-HTTP request produces `http://...` and a
+-- request carrying `Host: example.org:443` produces `https://example.org:443/...`.
+-- Keycloak registers bare-host https URIs only, so it rejects both with
+-- error="invalid_redirect_uri" and the login dies at the authorization endpoint,
+-- before any callback exists to recover.
+--
+-- Normalising the request rather than the header is deliberate. lua-resty-openidc
+-- does consult `Forwarded` / `X-Forwarded-Proto` / `X-Forwarded-Host` ahead of
+-- those ngx vars, but on this deployment those headers demonstrably do not reach
+-- it: sending any of the three against production changes nothing about the
+-- redirect_uri. Setting them here would be a no-op that reads like a fix.
+--
+-- Configuration arrives on the plugin config under `canonical_https_redirect`,
+-- the same mechanism `oidc_error_callback_recovery.lua` uses.
+--
+--   canonical_https_redirect.status  redirect status code
+--
+-- See `oidc_gateway_pre_function_plugin` in ../apisix.py for the deployment
+-- reasoning and t/canonical_https_redirect.t for the behavioural tests.
+return function(conf, ctx)
+    -- $host is the Host header lowercased with any port stripped, falling back
+    -- to server_name; $http_host is the header verbatim. Comparing them catches
+    -- an explicit :443, an uppercased host, and anything else that would reach
+    -- lua-resty-openidc as a non-canonical authority.
+    local raw_host = ngx.var.http_host
+    local host = ngx.var.host
+
+    -- No Host header at all (HTTP/1.0). There is nothing to build a canonical
+    -- origin from -- $host would be the server_name -- so leave it alone and let
+    -- openid-connect's own 400 handle it.
+    if not raw_host or not host then
+        return
+    end
+
+    if ngx.var.scheme == "https" and raw_host == host then
+        return
+    end
+
+    local core = require("apisix.core")
+    local opts = conf.canonical_https_redirect or {}
+
+    core.log.warn("non-canonical origin scheme=", ngx.var.scheme,
+                  " host=", raw_host, " redirecting to https://", host)
+
+    -- One status for every method. The shadowed `redirect` plugin picks per
+    -- method instead -- 301 for GET/HEAD, 308 for the rest (redirect.lua
+    -- 208-215) -- so both preserve a POST and this is a simplification, not a
+    -- behavioural fix. `status` is constrained in ../apisix.py to the codes
+    -- ngx.redirect accepts; anything else raises a Lua error here.
+    return ngx.redirect("https://" .. host .. ngx.var.request_uri,
+                        opts.status or 308)
+end

src/ol_infrastructure/components/services/files/oidc_error_callback_recovery.lua

@@ -15,7 +15,7 @@
 --   oidc_error_recovery.guard_cookie_name   loop-breaker cookie name
 --   oidc_error_recovery.guard_max_age       guard cookie lifetime, seconds
 --
--- See `oidc_error_callback_recovery_plugin` in ../apisix.py for why each branch
+-- See `oidc_gateway_pre_function_plugin` in ../apisix.py for why each branch
 -- is here, and t/oidc_error_callback_recovery.t for the behavioural tests.
 return function(conf, ctx)
     local uri = ngx.var.uri

tests/apisix_integration/conftest.py

@@ -27,7 +27,7 @@
     from collections.abc import Generator
 
 from ol_infrastructure.components.services.apisix import (
-    oidc_error_callback_recovery_plugin,
+    oidc_gateway_pre_function_plugin,
 )
 
 # Must track the APISIX shipped by the chart pinned in bridge.lib.versions
@@ -90,7 +90,14 @@ def apisix_routes() -> dict[str, Any]:
     what the gateway itself does before proxying, and a request that reaches the
     upstream is one the plugin correctly declined to intercept.
     """
-    recovery = oidc_error_callback_recovery_plugin()
+    # APISIX listens on plain HTTP here, so the canonical-origin function would
+    # upgrade every request before the recovery function ever ran -- which is
+    # the production behaviour, and is asserted directly on the routes below.
+    # The recovery routes therefore switch it off so they exercise the callback
+    # handling in isolation, exactly as they did before the two were fused into
+    # the single serverless-pre-function APISIX allows per plugin config.
+    recovery = oidc_gateway_pre_function_plugin(canonical_https_redirect=False)
+    full = oidc_gateway_pre_function_plugin()
     dead_upstream = {"type": "roundrobin", "nodes": {"127.0.0.1:1": 1}}
     return {
         "routes": [
@@ -106,6 +113,15 @@ def apisix_routes() -> dict[str, Any]:
                 "upstream": dead_upstream,
                 "plugins": {recovery.name: recovery.config},
             },
+            # Not a catch-all: `_wait_until_ready` polls `/` and follows
+            # redirects, so a `/*` route carrying this plugin would send the
+            # readiness probe at an https origin that does not exist here.
+            {
+                "id": "canonical-origin",
+                "uri": "/origin/*",
+                "upstream": dead_upstream,
+                "plugins": {full.name: full.config},
+            },
         ]
     }
 
@@ -207,3 +223,27 @@ def _callback(
         return response.status, dict(response.headers)
 
     return _callback
+
+
+@pytest.fixture
+def origin_request(apisix):
+    """Request a canonical-origin route and return (status, headers).
+
+    Sends an explicit Host so the port-stripping and scheme-upgrade branches can
+    be driven independently of the container's own listen address.
+    """
+
+    def _origin_request(
+        path: str = "/origin/",
+        host: str = "nb.learn.mit.edu",
+    ) -> tuple[int, dict[str, str]]:
+        response = urllib3.PoolManager(retries=False).request(
+            "GET",
+            f"{apisix}{path}",
+            headers={"Host": host},
+            redirect=False,
+            timeout=10.0,
+        )
+        return response.status, dict(response.headers)
+
+    return _origin_request