feat(authenticator): OIDC login + sessions + cookie-to-JWT exchange (nginx+auth step 04)#1686
Merged
cyberantonz merged 12 commits intoJul 9, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (37)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cyberantonz
force-pushed
the
nginx-authenticator-04
branch
from
July 8, 2026 13:49
00078d6 to
b19032a
Compare
aleksdotbar
approved these changes
Jul 8, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/backend/services/authenticator/Dockerfile (1)
66-68: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffVerify watchexec download integrity with a checksum.
The watchexec binary is downloaded from GitHub releases over HTTPS but without SHA256 verification. A compromised release or CDN cache poisoning would inject a malicious binary into the production image. Since the binary persists in the runtime image (even though only used in compose), adding checksum verification closes the supply-chain gap.
🔒️ Suggested checksum verification
ARG WATCHEXEC_VERSION=2.3.2 +ARG WATCHEXEC_SHA256_AMD64=<amd64-sha256> +ARG WATCHEXEC_SHA256_ARM64=<arm64-sha256> RUN set -eux; \ case "${TARGETARCH:-amd64}" in \ - amd64) WX_ARCH=x86_64-unknown-linux-musl ;; \ - arm64) WX_ARCH=aarch64-unknown-linux-musl ;; \ + amd64) WX_ARCH=x86_64-unknown-linux-musl; WX_SHA256="${WATCHEXEC_SHA256_AMD64}" ;; \ + arm64) WX_ARCH=aarch64-unknown-linux-musl; WX_SHA256="${WATCHEXEC_SHA256_ARM64}" ;; \ *) echo "unsupported TARGETARCH=${TARGETARCH}" >&2; exit 1 ;; \ esac; \ apt-get update; \ apt-get install -y --no-install-recommends ca-certificates curl xz-utils; \ - curl -fsSL "https://github.com/watchexec/watchexec/releases/download/v${WATCHEXEC_VERSION}/watchexec-${WATCHEXEC_VERSION}-${WX_ARCH}.tar.xz" \ - | tar -xJ -C /tmp; \ + curl -fsSL -o /tmp/watchexec.tar.xz "https://github.com/watchexec/watchexec/releases/download/v${WATCHEXEC_VERSION}/watchexec-${WATCHEXEC_VERSION}-${WX_ARCH}.tar.xz"; \ + echo "${WX_SHA256} /tmp/watchexec.tar.xz" | sha256sum -c -; \ + tar -xJ -C /tmp -f /tmp/watchexec.tar.xz; \ + rm -f /tmp/watchexec.tar.xz; \ install -m 0755 "/tmp/watchexec-${WATCHEXEC_VERSION}-${WX_ARCH}/watchexec" /usr/local/bin/watchexec; \ rm -rf /tmp/watchexec-*; \ apt-get purge -y curl xz-utils; \ apt-get autoremove -y; \ rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/Dockerfile` around lines 66 - 68, The watchexec download/install step in the Dockerfile does not verify the release artifact integrity, so update the release fetch flow to validate the archive against a published SHA256 checksum before extracting it. Add checksum retrieval and verification alongside the existing curl and tar pipeline in the watchexec install block, and keep the install step gated on a successful match so the image build fails on tampering or corruption.src/backend/services/authenticator/tests/run-e2e.sh (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
''for empty env var assignment to satisfy shellcheck SC1007.
VAR= \with a trailing space is valid bash but triggers shellcheck. UsingVAR='' \is more explicit and avoids the warning.✨ Suggested fix
-APP__gears__authenticator__config__identity_url= \ +APP__gears__authenticator__config__identity_url='' \ APP__gears__authenticator__config__gateway_issuer=http://localhost:8080 \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/tests/run-e2e.sh` around lines 45 - 46, The environment variable assignment for the authenticator test setup uses an empty value with a trailing space, which triggers shellcheck SC1007. Update the assignment in run-e2e.sh to use an explicit empty string for APP__gears__authenticator__config__identity_url, and leave the adjacent APP__gears__authenticator__config__gateway_issuer assignment unchanged.Source: Linters/SAST tools
src/backend/services/authenticator/helm/templates/deployment.yaml (1)
64-65: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider
readOnlyRootFilesystem: trueand aseccompProfilefor defense-in-depth.The container security context is solid (
allowPrivilegeEscalation: false, non-root). AddingreadOnlyRootFilesystem: trueand a RuntimeDefault seccomp profile further hardens the pod. Verify the app doesn't write to the container filesystem (the ConfigMap setshome_dir: "/app/data"— if the app writes there, a writable emptyDir volume would be needed).🛡️ Optional hardening
securityContext: allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + seccompProfile: + type: RuntimeDefault🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/helm/templates/deployment.yaml` around lines 64 - 65, The container securityContext in the deployment template should be hardened further beyond allowPrivilegeEscalation and non-root. Update the pod/container spec to add readOnlyRootFilesystem and a RuntimeDefault seccompProfile, and verify the authenticator app does not need writable paths under the image filesystem. If it writes to the configured home_dir (/app/data), adjust the deployment to mount a writable volume such as emptyDir for that path while keeping the root filesystem read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/authenticator/helm/templates/deployment.yaml`:
- Around line 80-82: The signing-keys Secret volume in the deployment template
is using Kubernetes’ default file permissions, which leaves the ES256 private
key too broadly readable. Update the volume definition for the signing-keys
Secret in the deployment manifest to set defaultMode to 0440, keeping access
limited to root and the fsGroup already configured. Use the existing
signing-keys secret mount block as the place to apply the change.
In `@src/backend/services/authenticator/src/api/handlers.rs`:
- Around line 390-417: The me handler returns session details after
resolve_by_token without verifying that the session is still valid, unlike authz
which checks expiry before continuing. Update me to perform the same now-based
expiration guard using record.expires_at and record.absolute_expires_at before
building the JSON response, and return unauthenticated() when the session is
expired. Reuse the existing session resolution flow in me and keep the expiry
check aligned with authz so both handlers enforce the same validity rules.
In `@src/backend/services/authenticator/src/config.rs`:
- Around line 158-178: AuthenticatorConfig::validate currently only checks
numeric invariants, so boot can succeed with required string fields left empty.
Extend AuthenticatorConfig::validate to fail fast when gateway_issuer,
redirect_uri, idp.issuer_url, idp.client_id, idp.client_secret, or identity_url
are empty, using clear ensure-style checks alongside the existing jwt/session
validations. Keep the change localized to AuthenticatorConfig::validate and
align the error messages with the existing validation style.
In `@src/backend/services/authenticator/src/cookie.rs`:
- Around line 30-40: The Cookie lookup in read_session_token currently aborts on
a single non-UTF8 header because header.to_str().ok()? returns None; change this
so malformed COOKIE entries are skipped and the loop continues checking later
headers for the prefix. Keep the logic in read_session_token and the inner
header iteration intact, but replace the early return-on-error behavior with
per-header filtering so a bad header cannot prevent finding a valid __Host-sid
value elsewhere.
In `@src/backend/services/authenticator/src/main.rs`:
- Around line 95-99: Commands::Check in main should not stop at parsing
AppConfig; it needs to run AuthenticatorConfig::validate() before printing
configuration OK. Update the check path to construct or access the
AuthenticatorConfig and invoke its validation logic so cross-field rules like
jwt_reissue_after_seconds < jwt_ttl_seconds and session_ttl_seconds <=
session_absolute_lifetime_seconds are enforced. Keep the existing command flow,
but only return Ok(()) after validation succeeds.
In `@src/backend/services/authenticator/src/oidc.rs`:
- Around line 255-263: The OIDC token verification in `decode::<IdTokenClaims>`
currently trusts `header.alg` when building `Validation`, which allows
unintended RSA-family variants. Update the `Validation` setup in `oidc.rs` to
explicitly allow only RS256, keeping the existing audience, issuer, and expiry
checks unchanged. Use the `header.alg`-independent path around
`DecodingKey::from_rsa_components` so the `id_token` is rejected unless it is
signed with RS256.
In `@src/backend/services/authenticator/src/session.rs`:
- Around line 354-371: `revoke_user_sessions` leaves expired session IDs behind
in `user_sessions_key`, causing stale ZSET members to accumulate. Update the
cleanup loop in `revoke_user_sessions` (and, if needed, `revoke_session`) so
stale members are removed from the ZSET even when the session hash is already
gone, using the existing `revoke_session`/`user_sessions_key` flow to locate and
delete invalid entries.
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Around line 55-58: The readiness loop in run-e2e.sh currently exits silently
after 30 attempts if the JWKS endpoint never responds, which makes failures
opaque. Update the loop around the curl check so it tracks whether the
authenticator became ready, and after the retries are exhausted, fail explicitly
with a clear “service not ready” error message before continuing. Keep the fix
localized to the existing readiness check logic and the curl probe for
/.well-known/jwks.json.
---
Nitpick comments:
In `@src/backend/services/authenticator/Dockerfile`:
- Around line 66-68: The watchexec download/install step in the Dockerfile does
not verify the release artifact integrity, so update the release fetch flow to
validate the archive against a published SHA256 checksum before extracting it.
Add checksum retrieval and verification alongside the existing curl and tar
pipeline in the watchexec install block, and keep the install step gated on a
successful match so the image build fails on tampering or corruption.
In `@src/backend/services/authenticator/helm/templates/deployment.yaml`:
- Around line 64-65: The container securityContext in the deployment template
should be hardened further beyond allowPrivilegeEscalation and non-root. Update
the pod/container spec to add readOnlyRootFilesystem and a RuntimeDefault
seccompProfile, and verify the authenticator app does not need writable paths
under the image filesystem. If it writes to the configured home_dir (/app/data),
adjust the deployment to mount a writable volume such as emptyDir for that path
while keeping the root filesystem read-only.
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Around line 45-46: The environment variable assignment for the authenticator
test setup uses an empty value with a trailing space, which triggers shellcheck
SC1007. Update the assignment in run-e2e.sh to use an explicit empty string for
APP__gears__authenticator__config__identity_url, and leave the adjacent
APP__gears__authenticator__config__gateway_issuer assignment unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ade23f98-d93b-4c4e-8877-3071ddb8dd9c
⛔ Files ignored due to path filters (2)
deploy/compose/authenticator-dev-keys/current.pemis excluded by!**/*.pemsrc/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.env.compose.example.github/workflows/build-images.ymldeploy/compose/authenticator-dev-keys/README.mddocker-compose.ymldocs/components/backend/authenticator/DESIGN.mdsrc/backend/Cargo.tomlsrc/backend/libs/authenticator-sdk/Cargo.tomlsrc/backend/libs/authenticator-sdk/src/lib.rssrc/backend/services/authenticator/Cargo.tomlsrc/backend/services/authenticator/Dockerfilesrc/backend/services/authenticator/config/insight.yamlsrc/backend/services/authenticator/helm/Chart.yamlsrc/backend/services/authenticator/helm/templates/_helpers.tplsrc/backend/services/authenticator/helm/templates/configmap.yamlsrc/backend/services/authenticator/helm/templates/deployment.yamlsrc/backend/services/authenticator/helm/templates/service.yamlsrc/backend/services/authenticator/helm/values.yamlsrc/backend/services/authenticator/src/api/error.rssrc/backend/services/authenticator/src/api/handlers.rssrc/backend/services/authenticator/src/api/mod.rssrc/backend/services/authenticator/src/config.rssrc/backend/services/authenticator/src/cookie.rssrc/backend/services/authenticator/src/gear.rssrc/backend/services/authenticator/src/identity.rssrc/backend/services/authenticator/src/jwt.rssrc/backend/services/authenticator/src/local_client.rssrc/backend/services/authenticator/src/main.rssrc/backend/services/authenticator/src/oidc.rssrc/backend/services/authenticator/src/session.rssrc/backend/services/authenticator/tests/e2e_login_loop.rssrc/backend/services/authenticator/tests/run-e2e.sh
Comment on lines
+80
to
+82
| - name: signing-keys | ||
| secret: | ||
| secretName: {{ required "signingKeysSecret is required (mount the ES256 current.pem/previous.pem)" .Values.signingKeysSecret | quote }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set defaultMode: 0440 on the signing-keys Secret volume.
The ES256 private key (current.pem) is mounted with Kubernetes' default Secret volume mode 0644, making it world-readable inside the container. With fsGroup: 1000 already set, defaultMode: 0440 restricts read access to root and the fsGroup only — the non-root container user (UID 1000) can still read via group membership, but other UIDs cannot.
🔒️ Proposed fix
- name: signing-keys
secret:
secretName: {{ required "signingKeysSecret is required (mount the ES256 current.pem/previous.pem)" .Values.signingKeysSecret | quote }}
+ defaultMode: 0440📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: signing-keys | |
| secret: | |
| secretName: {{ required "signingKeysSecret is required (mount the ES256 current.pem/previous.pem)" .Values.signingKeysSecret | quote }} | |
| - name: signing-keys | |
| secret: | |
| secretName: {{ required "signingKeysSecret is required (mount the ES256 current.pem/previous.pem)" .Values.signingKeysSecret | quote }} | |
| defaultMode: 0440 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/authenticator/helm/templates/deployment.yaml` around
lines 80 - 82, The signing-keys Secret volume in the deployment template is
using Kubernetes’ default file permissions, which leaves the ES256 private key
too broadly readable. Update the volume definition for the signing-keys Secret
in the deployment manifest to set defaultMode to 0440, keeping access limited to
root and the fsGroup already configured. Use the existing signing-keys secret
mount block as the place to apply the change.
Comment on lines
+158
to
+178
| impl AuthenticatorConfig { | ||
| /// Validate cross-field invariants that the type system can't express. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error when a lifetime relationship is nonsensical (e.g. the | ||
| /// reissue age is not strictly below the JWT TTL, which would erase the | ||
| /// travel margin the whole design depends on). | ||
| pub fn validate(&self) -> anyhow::Result<()> { | ||
| anyhow::ensure!( | ||
| self.jwt_reissue_after_seconds < self.jwt_ttl_seconds, | ||
| "jwt_reissue_after_seconds ({}) must be < jwt_ttl_seconds ({})", | ||
| self.jwt_reissue_after_seconds, | ||
| self.jwt_ttl_seconds | ||
| ); | ||
| anyhow::ensure!( | ||
| self.session_ttl_seconds <= self.session_absolute_lifetime_seconds, | ||
| "session_ttl_seconds must be <= session_absolute_lifetime_seconds" | ||
| ); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
validate() should reject empty required fields at boot.
gateway_issuer, redirect_uri, idp.issuer_url, idp.client_id, idp.client_secret, and identity_url all default to empty strings. The gear starts successfully with these empty, failing only on first request with a less clear error. redis_url is already fail-fast-checked in SessionManager::connect; the same should apply to the other required fields here.
🛡️ Proposed validation additions
pub fn validate(&self) -> anyhow::Result<()> {
anyhow::ensure!(
self.jwt_reissue_after_seconds < self.jwt_ttl_seconds,
"jwt_reissue_after_seconds ({}) must be < jwt_ttl_seconds ({})",
self.jwt_reissue_after_seconds,
self.jwt_ttl_seconds
);
anyhow::ensure!(
self.session_ttl_seconds <= self.session_absolute_lifetime_seconds,
"session_ttl_seconds must be <= session_absolute_lifetime_seconds"
);
+ anyhow::ensure!(!self.gateway_issuer.is_empty(), "gateway_issuer is required");
+ anyhow::ensure!(!self.redirect_uri.is_empty(), "redirect_uri is required");
+ anyhow::ensure!(!self.identity_url.is_empty(), "identity_url is required");
+ anyhow::ensure!(!self.idp.issuer_url.is_empty(), "idp.issuer_url is required");
+ anyhow::ensure!(!self.idp.client_id.is_empty(), "idp.client_id is required");
+ anyhow::ensure!(!self.idp.client_secret.is_empty(), "idp.client_secret is required");
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl AuthenticatorConfig { | |
| /// Validate cross-field invariants that the type system can't express. | |
| /// | |
| /// # Errors | |
| /// Returns an error when a lifetime relationship is nonsensical (e.g. the | |
| /// reissue age is not strictly below the JWT TTL, which would erase the | |
| /// travel margin the whole design depends on). | |
| pub fn validate(&self) -> anyhow::Result<()> { | |
| anyhow::ensure!( | |
| self.jwt_reissue_after_seconds < self.jwt_ttl_seconds, | |
| "jwt_reissue_after_seconds ({}) must be < jwt_ttl_seconds ({})", | |
| self.jwt_reissue_after_seconds, | |
| self.jwt_ttl_seconds | |
| ); | |
| anyhow::ensure!( | |
| self.session_ttl_seconds <= self.session_absolute_lifetime_seconds, | |
| "session_ttl_seconds must be <= session_absolute_lifetime_seconds" | |
| ); | |
| Ok(()) | |
| } | |
| } | |
| impl AuthenticatorConfig { | |
| /// Validate cross-field invariants that the type system can't express. | |
| /// | |
| /// # Errors | |
| /// Returns an error when a lifetime relationship is nonsensical (e.g. the | |
| /// reissue age is not strictly below the JWT TTL, which would erase the | |
| /// travel margin the whole design depends on). | |
| pub fn validate(&self) -> anyhow::Result<()> { | |
| anyhow::ensure!( | |
| self.jwt_reissue_after_seconds < self.jwt_ttl_seconds, | |
| "jwt_reissue_after_seconds ({}) must be < jwt_ttl_seconds ({})", | |
| self.jwt_reissue_after_seconds, | |
| self.jwt_ttl_seconds | |
| ); | |
| anyhow::ensure!( | |
| self.session_ttl_seconds <= self.session_absolute_lifetime_seconds, | |
| "session_ttl_seconds must be <= session_absolute_lifetime_seconds" | |
| ); | |
| anyhow::ensure!(!self.gateway_issuer.is_empty(), "gateway_issuer is required"); | |
| anyhow::ensure!(!self.redirect_uri.is_empty(), "redirect_uri is required"); | |
| anyhow::ensure!(!self.identity_url.is_empty(), "identity_url is required"); | |
| anyhow::ensure!(!self.idp.issuer_url.is_empty(), "idp.issuer_url is required"); | |
| anyhow::ensure!(!self.idp.client_id.is_empty(), "idp.client_id is required"); | |
| anyhow::ensure!(!self.idp.client_secret.is_empty(), "idp.client_secret is required"); | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/authenticator/src/config.rs` around lines 158 - 178,
AuthenticatorConfig::validate currently only checks numeric invariants, so boot
can succeed with required string fields left empty. Extend
AuthenticatorConfig::validate to fail fast when gateway_issuer, redirect_uri,
idp.issuer_url, idp.client_id, idp.client_secret, or identity_url are empty,
using clear ensure-style checks alongside the existing jwt/session validations.
Keep the change localized to AuthenticatorConfig::validate and align the error
messages with the existing validation style.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Around line 43-48: The JWKS readiness check in run-e2e.sh can currently loop
without ever failing, so the script may continue into the login test and report
a misleading connection error. Update the startup wait logic around the JWKS
probe in run-e2e.sh to enforce an explicit timeout and exit with a clear failure
if the JWKS endpoint never becomes ready, keeping the failure tied to the
authenticator/JWKS startup step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 877d0fda-9ce8-4924-8170-bb724d05c86e
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.env.compose.exampledocker-compose.ymldocs/components/backend/authenticator/DESIGN.mdsrc/backend/libs/authenticator-sdk/Cargo.tomlsrc/backend/libs/authenticator-sdk/src/lib.rssrc/backend/services/analytics/Dockerfilesrc/backend/services/api-gateway/Dockerfilesrc/backend/services/authenticator/config/insight.yamlsrc/backend/services/authenticator/helm/templates/configmap.yamlsrc/backend/services/authenticator/src/api/handlers.rssrc/backend/services/authenticator/src/config.rssrc/backend/services/authenticator/src/gear.rssrc/backend/services/authenticator/src/identity.rssrc/backend/services/authenticator/src/jwt.rssrc/backend/services/authenticator/src/local_client.rssrc/backend/services/authenticator/src/main.rssrc/backend/services/authenticator/tests/e2e_login_loop.rssrc/backend/services/authenticator/tests/identity-stub.pysrc/backend/services/authenticator/tests/run-e2e.shsrc/backend/services/fakeidp/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend/services/authenticator/src/main.rs
- src/backend/services/authenticator/config/insight.yaml
- src/backend/services/authenticator/tests/e2e_login_loop.rs
- src/backend/services/authenticator/src/jwt.rs
- src/backend/services/authenticator/src/gear.rs
- src/backend/services/authenticator/src/config.rs
- src/backend/services/authenticator/helm/templates/configmap.yaml
- src/backend/services/authenticator/src/api/handlers.rs
aleksdotbar
approved these changes
Jul 8, 2026
Contributor
Author
|
Thanks @coderabbitai — triaged all 8 findings. Fixed the valid ones in Fixed
Already resolved by a later refactor
Declining (with reasons)
|
|
Tip For best results, initiate chat on the files or code changes.
The fixes address the core concerns well:
Agreed that the On the two declines:
Nice work closing these out in 🐰✨ ✏️ Learnings added
|
cyberantonz
force-pushed
the
nginx-authenticator-04
branch
from
July 8, 2026 17:05
fa2f4e5 to
7d0771d
Compare
ktursunov
approved these changes
Jul 8, 2026
ktursunov
requested changes
Jul 8, 2026
ktursunov
left a comment
ktursunov
left a comment
Contributor
There was a problem hiding this comment.
Please add coverage to the coverage gate
ktursunov
approved these changes
Jul 8, 2026
cyberantonz
force-pushed
the
nginx-authenticator-04
branch
from
July 9, 2026 08:48
7d0771d to
4bfe858
Compare
Contributor
Author
added |
…JWT exchange New authenticator gear (nginx+auth step 04): OIDC code+PKCE login, opaque Redis sessions (asm: schema, identity/credential split), ES256 linked-JWT minting + JWKS, and the /internal/authz cookie->JWT exchange with reissue- ahead. authenticator-sdk carries the AuthenticatorClientV1 contract. §9.6 resolved: mint ES256 (downstream oidc-authn-plugin validates RS256/ES256 today; EdDSA would need a non-trivial plugin change, deferred to Phase 3). Bumps cf-gears-* toolkit pins to gears-rust HEAD (toolkit 0.6.13, auth/security 0.7.2, api-gateway 0.2.9, ...). 15 unit tests; clippy clean. Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
- docker-compose: authenticator service (8083) + dev ES256 signing keys; fakeidp issuer -> in-network fakeidp:8084 (authenticator is now the consumer) - helm chart insight-authenticator (deployment + configmap + service, signing- keys Secret mount, envFrom leaf-config Secret, probes) - build-images.yml: authenticator path filter + build + merge (multi-arch, attested) jobs, wired into bump-descriptors + publish-chart gates - e2e integration test (login -> authz -> me -> logout -> 401) + run-e2e.sh runner; proven green locally against fakeidp - DESIGN: §9.6 recorded as RESOLVED = ES256; cfs toc + validate clean Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…#1687 in code marker Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…ng Dockerfiles Adding the two new workspace members broke the analytics / api-gateway / fakeidp image builds: each Dockerfile's dependency-cache layer copies an explicit list of member Cargo.toml + stub sources so the workspace resolves, and the new members weren't listed (cargo: 'failed to load manifest for workspace member libs/authenticator-sdk'). Verified: analytics + authenticator images now build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…ors, clarify api-gateway naming - Signing keys: no more checked-in dev key. When signing_keys_path is empty the gear generates an EPHEMERAL in-process ES256 key (dev/CI only, loudly warned), like fakeidp; prod still mounts a stable-key Secret. Deletes deploy/compose/authenticator-dev-keys and the compose bind-mount. - SDK: AuthenticatorClientV1 returns Result<u64, CanonicalError> — dropped the hand-rolled AuthenticatorError enum; use canonical errors like every gear (toolkit ADR 0005). LocalClient maps store-down to ServiceUnavailable (503). - Docs: clarify that the 'api-gateway' gear is the gears-rust toolkit REST-host system gear (cf-gears-api-gateway) every service runs on, NOT the Insight platform gateway service the nginx edge replaces (config/helm/main.rs). e2e loop re-verified green against fakeidp with the ephemeral-key path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Per review: no first-admin/empty-table machinery and no RBAC/ACL in step 04 — both move to a separate universe-admin initiative. Local dev seeds the persons table; an unknown person is denied (403, audited login_denied_unknown_person), and every session carries default_roles only. Removed: bootstrap_first_admin config, the one-shot Redis claim, the PersonResolver bootstrap method + is_universe_admin + deterministic id, and the admin-role injection. compose points identity_url at the seeded Identity; run-e2e.sh gains a tiny Identity stub so the loop resolves a person without the full .NET service. DESIGN §4.6 / DD-AUTH-08 marked deferred. Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…ecord + route-group cleanups - deps to latest: jsonwebtoken 9->10 (rust_crypto provider), p256 0.13->0.14 (ToSec1Point + Generate API), cookies via axum-extra 0.12 CookieJar (replaces hand-rolled Set-Cookie building + Cookie-header parsing). - signing keys: no in-binary key generation. The gear always loads from the mounted signing_keys_path (fail closed). dev-compose.sh generates a dev key (openssl, gitignored) and compose bind-mounts it; run-e2e.sh does the same. - SessionRecord owns its Redis mapping (to_fields/from_map) instead of a free function + inline vec. - build_operations split into register_auth/internal/well_known routes. Verified: cargo clippy clean, 15 unit tests, e2e loop green at runtime (mounted key, jsonwebtoken 10 rust_crypto, p256 0.14), authenticator image builds. Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
- oidc: pin id_token verification to RS256 instead of trusting the token's own `alg` header (RFC 8725 algorithm-confusion guard). - session: revoke_user_sessions now trims stale ZSET members whose session hash already expired, so the user-session index can't accumulate dead ids. - handlers: /auth/me applies the same expiry guard as /internal/authz. - config: validate() fails fast on empty required fields (gateway_issuer, redirect_uri, signing_keys_path, identity_url, idp.issuer_url, idp.client_id; client_secret stays optional for public/PKCE clients). - helm: signing-keys Secret volume mounts with defaultMode 0440. - run-e2e.sh: wait for fakeidp + identity-stub readiness and fail loudly if a service never comes up. cargo clippy clean, 15 unit tests, e2e loop green. Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The directory is created at runtime by dev-compose.sh (mkdir -p) and the key is gitignored, so there is nothing to keep committed there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…/from_map) Mirrors SessionRecord — put/take_login_state no longer inline the field list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…ire coverage gate - oidc.rs now builds on `openidconnect` (CoreClient) for discovery, PKCE, code exchange, and id_token validation (signature/iss/aud/nonce/exp/alg) instead of hand-rolled discovery + JWKS fetch + RSA-component parsing + manual checks (NGINX_BFF §11.7). The two non-standard bits the Core typed API doesn't expose — the interim `tenants` claim (constructorfabric#1687) and OIDC `sid` — are read from the already-validated id_token payload; RP-initiated logout keeps a tiny end_session_endpoint read (not part of core discovery). - coverage gate: register authenticator + authenticator-sdk in scripts/ci/components.py. cover=False (mirrors api-gateway): the crate's security-critical flow is proven by the e2e login-loop, which drives the server as a separate process and so can't feed `cargo llvm-cov`; fmt/clippy/ test still gate the pipeline. clippy clean, 15 unit tests, e2e login loop green (two users, no panics). Refs constructorfabric#1583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
nginx-authenticator-04
branch
from
July 9, 2026 11:47
00972c7 to
ab97ab7
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Format check failed CI — the crate was never rustfmt'd. No logic changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
ktursunov
approved these changes
Jul 9, 2026
ktursunov
approved these changes
Jul 9, 2026
mitasovr
approved these changes
Jul 9, 2026
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
4 participants
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.
nginx + authorization — Step 04:
authenticatorgear skeletonPart of the EPIC #1583 (replace the Rust API-gateway Router with nginx
auth_request+ a standalone authenticator). Closes the step-04 subtask #1587.Implements the BFF / token-handler service per
NGINX_BFF.md§4.1/§11 and the step-01 authenticator PRD/DESIGN: OIDC code+PKCE login, opaque Redis sessions, and the cookie→JWT exchange the nginx gateway will call.What's here
Crates
services/authenticator/— the#[toolkit::gear(name = "authenticator", deps = ["types-registry"], capabilities = [rest, stateful])]gear (packageauthenticator), on the api-gateway REST host likeanalytics.libs/authenticator-sdk/—AuthenticatorClientV1trait (revoke_user_sessions) + DTOs + typed error; registered in theClientHub. Consumers depend on the SDK only.Behaviour
AuthenticatorConfig(serde(default, deny_unknown_fields)) — the §4.1 table 1:1 (session/JWT TTLs, jitter,idp.*, roles, bootstrap, csrf, redis/keys/identity URLs).asm:schema, per DESIGN §3.7 — the identity/credential split):asm:token,asm:sessionHASH,asm:jwt,asm:user_sessionsZSET,asm:sid_index,asm:login_state,asm:idp_refresh_due. All multi-key writes inMULTI/EXEC. Fails closed (Redis down → 401/503, boot pings Redis).current.pem+ optionalprevious.pem),kid= RFC 7638 thumbprint.OperationBuilder, all.public()(cookie-authenticated in handler):GET /auth/login,GET /auth/callback(session-fixation guard, person resolve, audited first-admin bootstrap, session+linked-JWT in one pipeline,__Host-sidcookie, sanitizedreturn_to),GET /internal/authz(exchange + reissue-aheadSET NX EX,X-Gateway-Jwt+ computedCache-Control),GET /.well-known/jwks.json,GET /auth/me,POST /auth/logout. Errors asCanonicalError.§9.6 decision — ES256. Downstream
oidc-authn-pluginvalidates RS256/ES256 today; EdDSA would need a non-trivial plugin change, and downstream verification is a later phase. ES256 meets DD-BFF-05's rationale (small sig, fast verify) with zero downstream friction. Recorded in the authenticator DESIGN (RESOLVED (step 04)),cfs toc+validateclean.Wiring —
config/insight.yaml, multi-stageDockerfile, docker-composeauthenticatorservice (:8083, depends redis+fakeidp) with dev ES256 keys,helm/insight-authenticatorchart (deployment + configmap + service + signing-keys Secret + probes),build-images.ymlpath filter + multi-arch build/merge jobs.Tests
Set-Cookiesnapshot, fixation-safe cookie parsing,Cache-Control(60 s travel margin / no-store / disabled), sign↔verify claim contents, JWKS shape, stablekid,sanitize_return_to,jwt_exp.tests/e2e_login_loop.rs,#[ignore]) +tests/run-e2e.shrunner. Proven green locally against fakeidp:login → /authorize → callback (cookie) → /internal/authz (JWT verified against the published JWKS) → /auth/me → /auth/logout → /internal/authz = 401 no-store. The bootstrap-admin audit line fires and the session is revoked on logout.cargo testgreen;cargo clippyclean (pedantic +unwrap/expectdenied).Deliberate scope / follow-ups
0.6.13, auth/security0.7.2, api-gateway0.2.9, …); existinganalyticsstill compiles.GET /v1/persons/{email}returns a person id but no tenant memberships and there is no count/create API, so — per the brief — it sits behind aPersonResolvertrait:person_idfrom Identity,tenantsfrom the id_token, first-admin bootstrap gated on the config flag (loudly audited). Follow-up: INSTALLER + a real Identity membership/emptiness API (to be filed)./auth/refreshrotation,/auth/sessions, back-channel logout, CSRF enforcement, IdP background refresher + janitor (thestatefulrunnable is a returning no-op stub),/internal/tokensecond listener, the nginx gateway itself. Umbrella subchart integration (descriptor +Chart.lock) lands with the deploy step.🤖 Generated with Claude Code
Summary by CodeRabbit