Fix unbounded subscription leak in auth ACL tracking#671
Merged
Conversation
track_targets used mergeMap over the principal stream, which keeps every inner acl_for subscription alive. track_kerberos re-emits the principal UUID on every identities refetch (one per identity write), so long-lived notify sessions accumulated inner subscriptions without bound. Every subsequent emission then fanned out through all of them, pegging the event loop and logging thousands of ACL dumps per second; with the service unresponsive, MQTT logins and the Keycloak federation SPI timed out cluster-wide. Use switchMap so re-resolution of the principal replaces the inner subscription, dedupe the resolved UUID in track_kerberos, and dedupe the computed target set so downstream combineLatest pipelines only re-fire when the ACL actually changes.
This was referenced Jul 9, 2026
AlexGodbehere
added a commit
that referenced
this pull request
Jul 9, 2026
## Summary Keycloak hard-kills user-storage lookups after 3 seconds (`ServicesUtils.timeBoundOne`). The Factory+ federation SPI defaulted its own per-request timeout to 5 seconds, and service-setup provisioned the same value into the federation component, so the SPI's timeout could never fire: Keycloak always interrupted the thread first. The functional consequence is diagnostic, not behavioural. When the F+ auth service is slow (as during the ACL storm fixed by #671), login fails either way, but the log shows an opaque `InterruptedException` inside `HttpClientImpl.send` instead of a clean `HttpTimeoutException` naming the F+ auth call. That cost real time when diagnosing the rc.1 login failure. ## Changes - `FactoryPlusUserStorageProviderFactory.DEFAULT_TIMEOUT_SECONDS`: 5 → 2, with a comment explaining the 3s ceiling. - The `auth.timeout.seconds` help text now warns that values of 3s or more never take effect. - `acs-service-setup/lib/openid.js`: provisioned federation config 5 → 2 to match. ## How to test 1. `mvn test` in `acs-keycloak-spi`: 80 tests, all passing. 2. On a deployed cluster, block the auth service (e.g. scale it to 0) and attempt a Grafana login. The openid pod log should show a timeout exception naming `http://auth.factory-plus...` after ~2s rather than an `InterruptedException` at 3s. 3. Re-run service-setup; the federation component in Keycloak (Admin console → User federation → factoryplus) should show Request timeout = 2.
AlexGodbehere
added a commit
that referenced
this pull request
Jul 9, 2026
…#674) ## Summary Every Factory+ JS service floods the KDC with refused `TGT NOT FORWARDABLE` requests - one per HTTP token fetch and MQTT connection. This PR removes the cause: the `gssapi.js` npm module hardcodes `GSS_C_DELEG_FLAG` in `gss_init_sec_context`, so libkrb5 asks the KDC for a forwarded TGT on every client context. ACS service principals hold non-forwardable TGTs (straight from keytabs), so the KDC refuses every time: ``` TGS_REQ (1 etypes {aes256-cts-hmac-sha1-96(18)}) 10.42.0.42: TGT NOT FORWARDABLE: authtime ..., sv1configdb@... for krbtgt/..., KDC can't fulfill requested option ``` libkrb5 silently drops the flag and carries on, so nothing breaks - but every token costs an extra KDC round trip, and under reconnect storms (see #671) this multiplies into thousands of KDC log lines per minute. Nothing in Factory+ consumes delegated credentials; the server side discards them. ## Changes - **`lib/js-gssapi`**: vendored fork of gssapi.js 2.0.1 (MIT, provenance in README) published as `@amrc-factoryplus/gssapi` 2.1.0. One change: no `GSS_C_DELEG_FLAG`. Publishable via the existing js-lib release workflow with a release tagged `js/gssapi/v2.1.0`. - **`js-service-client`**: `deps.js` prefers the fork and falls back to `gssapi.js`; the fork is added to `optionalDependencies` alongside the existing entries. - **`js-service-api`**: takes `GSS` from the service-client re-export instead of its own `gssapi.js` import, so the native dependency has one owner. service-api now loads on GSS-less platforms and fails at auth time rather than import time. ## Rollout Safe to merge before the npm publish: npm skips unpublished optional dependencies (verified with npm 10.8.2), so installs fall back to upstream `gssapi.js` and behave exactly as today. Once `js/gssapi/v2.1.0` is released to npm, rebuilt images pick the fork up automatically. Services consuming `@amrc-factoryplus/service-client` from the registry (historian-uns, acs-mcp) get the fix when service-client is next published and bumped. ## Verification A/B tested both modules against a live ACS KDC (fpd-ago, via port-forward) from a Linux container (node:22-bookworm, MIT krb5, cmake - same toolchain as the Docker builds): | module | SPNEGO token | KDC log | |---|---|---| | gssapi.js 2.0.1 | minted, 812 bytes | `TGS_REQ ... ISSUE` **plus** `TGS_REQ (1 etypes ...) TGT NOT FORWARDABLE` | | fork | minted, 812 bytes | single `TGS_REQ ... ISSUE`, nothing refused | The refused request's signature (single-etype TGS_REQ for krbtgt) exactly matches the storm seen from every sv1 principal in production KDC logs. Also verified: the fork compiles on Linux and macOS; with the fork installed, `service-client`'s `GSS` export resolves to it; without it, the fallback to `gssapi.js` loads and `service-api`'s auth module imports cleanly. ## Decisions - Left the direct `gssapi.js` deps in the handful of services that declare one (acs-i3x, uns-ingester-sparkplug, historian-uns): they are the fallback path and become vestigial once service-client ships the fork; removing them is trivial follow-up cleanup. - Delegation is removed globally rather than made an option. If a future service genuinely needs delegated credentials it should be an explicit per-context option in the fork.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an unbounded rxjs subscription leak in the auth service's ACL tracking that can peg the event loop and take the whole cluster down with it.
DataFlow.track_targetsusedmergeMapover the principal stream.track_kerberosre-emits the resolved principal UUID on every identities refetch, and_track_modelrefetches the whole identity table on every identity write.mergeMapkeeps each inneracl_forsubscription alive forever, so every long-lived notify session (configdb, cmdesc, git, cluster-manager all hold one) accumulated one more permanent inner subscription per identity write. Each subsequent emission then fanned out through all of them.Observed on a fresh v6.0.0-rc.1 install (where bootstrap writes many identities): auth pinned at a full CPU core writing ~5,600 log lines/second of ACL dumps, fast enough to roll the container log over in about 20 seconds. While it spun, HTTP requests queued: the HiveMQ Kerberos plugin timed out (5s) so every service got "Bad User Name or Password" from MQTT and hammered reconnects, and the Keycloak federation SPI hit its 3s hard limit, making Grafana/Keycloak admin login impossible.
The fix
track_targets:mergeMap→switchMap, so re-resolution of the principal replaces the inner subscription instead of adding one.track_kerberos:distinctUntilChanged()on the resolved UUID, so equal-content identity refetches don't propagate at all.track_targets:distinctUntilChanged(imm.is)on the computed target set, so downstreamcombineLatestpipelines (notify_get_acl,permitted) only re-fire when the ACL genuinely changes. This also moves the "Permitted" log line onto real changes only.Verification
Drove the real
DataFlow.track_targets/track_kerberosmethods with a stubbed identities stream and a countingacl_for:How to test
kubectl top podon auth should stay in single-digit millicores and the log should not storm withACL update/Permitteddumps.Notes
The
XXXfull-refetch in_track_modelis untouched: refetches are now harmless to downstream subscribers, but each write still costs one full-table read. Deduping the notifyResponsebodies forgrantschurn is a possible follow-up; grant writes still produce one notify update per subscriber.