Retain OAuth certs across cert-manager renewal without mass logout - #2489
Retain OAuth certs across cert-manager renewal without mass logout#2489hahn-kev-bot wants to merge 3 commits into
Conversation
…erts in cluster. Add multi-path PEM loading and polled OpenIddict server/validation reload so refresh tokens survive cert-manager renewal without API restarts; add a daily CronJob to copy current/last-seen/previous secrets and mount them on LexBox. Adjust local-dev patches (JWT secret, maildev env, optional retainer delete) for prod-like OAuth testing. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
📦 Lexbox
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOAuth certificate handling now loads current and retained PEM slots, reloads OpenIddict server and validation credentials when PEM content changes, and preserves prior keys through Kubernetes-managed secret retention and deployment mounts. ChangesOAuth certificate rotation
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs (1)
212-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile subject-string sniffing to stagger certificate validity.
Determining
NotAfterviasubject.Contains("New", StringComparison.Ordinal)is implicit and easy to break silently if a future test adds/renames a cert subject. Prefer an explicit parameter.♻️ Proposed refactor
- private static X509Certificate2 CreateSelfSigned(string subject) + private static X509Certificate2 CreateSelfSigned(string subject, bool longerLived = false) { using var rsa = RSA.Create(2048); var request = new CertificateRequest(subject, rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); // Stagger NotAfter so "preferred" signing cert selection is deterministic in redeem-after-swap. var notBefore = DateTimeOffset.UtcNow.AddDays(-1); - var notAfter = subject.Contains("New", StringComparison.Ordinal) - ? DateTimeOffset.UtcNow.AddYears(2) - : DateTimeOffset.UtcNow.AddYears(1); + var notAfter = longerLived + ? DateTimeOffset.UtcNow.AddYears(2) + : DateTimeOffset.UtcNow.AddYears(1); return request.CreateSelfSigned(notBefore, notAfter); }🤖 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 `@backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs` around lines 212 - 222, Update CreateSelfSigned to accept an explicit parameter indicating the desired certificate validity duration or NotAfter value, and use that parameter instead of inferring it from subject.Contains("New"). Update each call site to pass the appropriate value so deterministic preferred-certificate selection remains unchanged.backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs (1)
139-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared OAuth PEM test fixtures duplicated across two new test files. Both new test files independently define identical
GetThumbprint/GetThumbprints,CreateSelfSigned,WritePemPair, andTempPemRoothelpers; the shared root cause is the lack of a common test-support module for OAuth PEM fixtures.
backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs#L139-L182: extract these helpers into a shared internal class (e.g.OauthPemTestHelpers) in theTesting.LexBoxApi.Authnamespace.backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs#L200-L254: remove the duplicated copies and reference the shared helper class instead.🤖 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 `@backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs` around lines 139 - 182, Extract the duplicated OAuth PEM fixture helpers from backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs lines 139-182 into a shared internal OauthPemTestHelpers class in the Testing.LexBoxApi.Auth namespace, preserving GetThumbprint/GetThumbprints, CreateSelfSigned, WritePemPair, and TempPemRoot behavior. Remove the duplicate helper implementations from backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs lines 200-254 and update that test to reference the shared class.deployment/base/oauth-cert-retainer.yaml (2)
55-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound job runtime with
activeDeadlineSeconds.No deadline is set on the
jobTemplate. If akubectlcall hangs (e.g., API server unreachable), the job pod can run indefinitely; combined withconcurrencyPolicy: Forbid, this silently blocks all future scheduled runs until manually intervened.⏱️ Proposed fix
jobTemplate: spec: backoffLimit: 1 + activeDeadlineSeconds: 300 template:🤖 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 `@deployment/base/oauth-cert-retainer.yaml` around lines 55 - 61, Add an appropriate activeDeadlineSeconds value to the CronJob’s jobTemplate.spec, alongside backoffLimit, so hung oauth-cert-retainer pods terminate automatically and do not block subsequent scheduled runs under concurrencyPolicy: Forbid.
63-67: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnmaintained
bitnamilegacyimage for a secret-creating CronJob.
bitnamilegacy/kubectl:1.31.4is Bitnami's frozen legacy repo (post Aug 2025 catalog restructuring) and will not receive security patches going forward. This container also holds RBAC permissions to create/patch Secrets, so running an unpatched image here increases blast radius if a vulnerability surfaces.Consider a maintained alternative with bash+kubectl (e.g., building a minimal custom image on top of the official
kubectlimage, oralpine/k8s), since the comment indicates only the missing shell drove this choice.🤖 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 `@deployment/base/oauth-cert-retainer.yaml` around lines 63 - 67, Replace the unmaintained bitnamilegacy/kubectl image in the retain CronJob with a maintained image that provides both bash and kubectl, preserving the existing command and retainer script behavior. Prefer the project’s maintained custom image or an established maintained alternative such as alpine/k8s, and keep the required Kubernetes tooling available.
🤖 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 `@deployment/base/oauth-cert-retainer.yaml`:
- Around line 58-67: Harden the retain container in the deployment template by
adding an explicit securityContext that disables privilege escalation and
prevents running as root. Apply these settings to the container identified by
name retain, preserving its existing command, image, and arguments.
---
Nitpick comments:
In `@backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs`:
- Around line 139-182: Extract the duplicated OAuth PEM fixture helpers from
backend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.cs lines 139-182
into a shared internal OauthPemTestHelpers class in the Testing.LexBoxApi.Auth
namespace, preserving GetThumbprint/GetThumbprints, CreateSelfSigned,
WritePemPair, and TempPemRoot behavior. Remove the duplicate helper
implementations from
backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs lines 200-254
and update that test to reference the shared class.
In `@backend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.cs`:
- Around line 212-222: Update CreateSelfSigned to accept an explicit parameter
indicating the desired certificate validity duration or NotAfter value, and use
that parameter instead of inferring it from subject.Contains("New"). Update each
call site to pass the appropriate value so deterministic preferred-certificate
selection remains unchanged.
In `@deployment/base/oauth-cert-retainer.yaml`:
- Around line 55-61: Add an appropriate activeDeadlineSeconds value to the
CronJob’s jobTemplate.spec, alongside backoffLimit, so hung oauth-cert-retainer
pods terminate automatically and do not block subsequent scheduled runs under
concurrencyPolicy: Forbid.
- Around line 63-67: Replace the unmaintained bitnamilegacy/kubectl image in the
retain CronJob with a maintained image that provides both bash and kubectl,
preserving the existing command and retainer script behavior. Prefer the
project’s maintained custom image or an established maintained alternative such
as alpine/k8s, and keep the required Kubernetes tooling available.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22b4901a-61cb-4a1e-a5fe-23f87fc09644
📒 Files selected for processing (13)
backend/LexBoxApi/Auth/AuthKernel.csbackend/LexBoxApi/Auth/OauthPemCertificateLoader.csbackend/LexBoxApi/Auth/OauthPemCertificatePaths.csbackend/LexBoxApi/Auth/OauthPemOpenIddictServerConfigurer.csbackend/LexBoxApi/Auth/OauthPemOptionsChangeTokenSource.csbackend/Testing/LexBoxApi/Auth/OauthPemCertificateLoaderTests.csbackend/Testing/LexBoxApi/Auth/OauthPemCredentialHotReloadTests.csdeployment/base/kustomization.yamldeployment/base/lexbox-deployment.yamldeployment/base/oauth-cert-retainer.yamldeployment/local-dev/delete-oauth-cert-retainer.yamldeployment/local-dev/kustomization.yamldeployment/local-dev/lexbox-deployment.patch.yaml
| template: | ||
| spec: | ||
| serviceAccountName: oauth-cert-retainer | ||
| restartPolicy: Never | ||
| containers: | ||
| - name: retain | ||
| # Distroless rancher/kubectl has no shell; retainer script needs bash. | ||
| image: bitnamilegacy/kubectl:1.31.4 | ||
| command: ["/bin/bash", "-c"] | ||
| args: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add container securityContext hardening.
Static analysis flags this container for allowing privilege escalation and admitting a root container. Add explicit hardening.
🔒 Proposed fix
containers:
- name: retain
# Distroless rancher/kubectl has no shell; retainer script needs bash.
image: bitnamilegacy/kubectl:1.31.4
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
+ capabilities:
+ drop: ["ALL"]
command: ["/bin/bash", "-c"]📝 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.
| template: | |
| spec: | |
| serviceAccountName: oauth-cert-retainer | |
| restartPolicy: Never | |
| containers: | |
| - name: retain | |
| # Distroless rancher/kubectl has no shell; retainer script needs bash. | |
| image: bitnamilegacy/kubectl:1.31.4 | |
| command: ["/bin/bash", "-c"] | |
| args: | |
| template: | |
| spec: | |
| serviceAccountName: oauth-cert-retainer | |
| restartPolicy: Never | |
| containers: | |
| - name: retain | |
| # Distroless rancher/kubectl has no shell; retainer script needs bash. | |
| image: bitnamilegacy/kubectl:1.31.4 | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: | |
| drop: ["ALL"] | |
| command: ["/bin/bash", "-c"] | |
| args: |
🤖 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 `@deployment/base/oauth-cert-retainer.yaml` around lines 58 - 67, Harden the
retain container in the deployment template by adding an explicit
securityContext that disables privilege escalation and prevents running as root.
Apply these settings to the container identified by name retain, preserving its
existing command, image, and arguments.
Source: Linters/SAST tools
2 participants
Now when our certs rotate we remember the old one so we don't logout everyone. I'll give an example for the
oauth-signing-cert, there's also an encryption one but they work the same for this purpose.How this works:
Day 1, cert A is created, users login and it's used for auth. Stored in the secret
oauth-signing-certDay 2, the new job
oauth-cert-retainerruns for the first time, copiesoauth-signing-certtooauth-signing-cert-last-seenandoauth-signing-cert-previousbecause they don't existDay 3,
oauth-cert-retainerruns again, comparesoauth-signing-cert == oauth-signing-cert-last-seen, both are cert A so does nothing. Etc for each day following.Day 75, cert A is 15 days from expiring,
cert-managerissues a new one, cert B which overwrites the secretoauth-signing-cert.Day 75/76,
oauth-cert-retainerruns again, comparesoauth-signing-cert == oauth-signing-cert-last-seen, false this time because last seen is cert A, butoauth-signing-certis cert B, so it takesoauth-signing-cert-last-seenmoves it intooauth-signing-cert-previous, then updatesoauth-signing-cert-last-seento be cert B.Day 150, cert B is 1t days from expiring, cert C is created and overwrites
oauth-signing-certDay 150/151, the retainer job runs, sees that there's a new cert and updates previous and last seen.
Meanwhile, asp.net is watching the folders with
oauth-signing-cert,oauth-signing-cert-last-seenandoauth-signing-cert-previousdeduplicates them, and gives them to OpenIddict which will check each one it turn when authenticating a refresh token. It will use the newest one when creating new tokens which will then be valid for the next 90 days.One thing to note with this current setup is that a refresh token is valid for 14 days today, and our certs are renewed 15 days before they expire. This means if someone gets a brand new token right before a new cert is created, then it's valid for 14 days, and the cert to validate it is good for 15 days. No problems. However, if we want to increase the length that a refresh token is valid for, then we also need to increase how early we create new certs, otherwise we might create a token that should be valid for 20 days, but the cert it's based on will expire before then making the extra long refresh token length moot.
Another fun thing to think about, is that if we were to add a weekly background job to sync projects, those will naturally need to authenticate with Lexbox update their refresh token, so users would functionally never be logged out due to an expired token, where today users would have to use the app every 14 days or less to stay logged in.
Summary
oauth-cert-retainerCronJob that shifts cert-manager TLS secrets into last-seen/previous copies, plus LexBox volume mounts; local-dev keeps optional retainer delete and patches JWT/maildev for prod-like OAuth testing.Fixes #2332. Relates to #843.
Test plan
dotnet test backend/Testing --filter FullyQualifiedName~OauthPemoauth2cwithoffline_access)