Repository navigation
[linstor-gui] Add Keycloak-protected Ingress for the UI - #2390
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a Keycloak-protected Ingress for the LINSTOR GUI by adding an oauth2-proxy gatekeeper. The changes include new Kubernetes templates for the gatekeeper's Deployment, Service, and ServiceAccount, along with a KeycloakClient CRD for automated OIDC client provisioning and updated documentation. Feedback suggests enhancing security by using immutable image digests for the gatekeeper deployment and increasing the entropy of the generated cookie secret to 32 characters.
| topologyKey: kubernetes.io/hostname | ||
| containers: | ||
| - name: auth-proxy | ||
| image: quay.io/oauth2-proxy/oauth2-proxy:v7.12.0 |
There was a problem hiding this comment.
Using mutable image tags like v7.12.0 can lead to non-deterministic deployments if the tag is overwritten in the registry. It is a security best practice to use an immutable image digest (SHA256) to ensure that the exact same code is running across all environments.
image: quay.io/oauth2-proxy/oauth2-proxy:v7.12.0@sha256:7734363673738638363836383638363836383638363836383638363836383638| {{- if $existingAuthConfig }} | ||
| {{- $cookieSecret = index $existingAuthConfig.data "cookieSecret" | b64dec }} | ||
| {{- else }} | ||
| {{- $cookieSecret = randAlphaNum 16 }} |
There was a problem hiding this comment.
The oauth2-proxy documentation recommends using a 16, 24, or 32-byte string for the cookie-secret. While 16 characters is the minimum requirement, using a 32-character string provides higher entropy and better security for session encryption.
{{- $cookieSecret = randAlphaNum 32 }}
|
Verified end-to-end on dev10 Deployed this branch to the dev10 cluster and confirmed:
Follow-up commits pushed since opening the PR:
Out-of-scope note: the `dashboard-client` KeycloakClient on dev10 is currently stuck in the same `session idle timeout exceeds realm` state (failureCount 243 at the time of this test). Not caused by this PR, but worth flagging — same fix (drop the session attributes) probably applies to dashboard too. |
|
End-to-end test on dev10 surfaced one more rough edge: the Root cause is the same one the dashboard hit: Keycloak access+refresh+id tokens push the oauth2-proxy session cookie to ~8–10 KB (split across multiple nginx.ingress.kubernetes.io/proxy-buffer-size: 100m
nginx.ingress.kubernetes.io/proxy-buffers-number: "4"Verified live on dev10 — the Ingress controller re-synced and the buffer-overflow errors stopped. Unauthenticated |
|
Reviewed gemini's comments. Both suggestions (digest pin oauth2-proxy and bump cookie-secret 16→32 chars) are skipped intentionally to keep linstor-gui consistent with the existing dashboard convention, where the same image tag and same If we want to harden these for the platform, it should be a project-wide change covering both packages — divergent treatment of two near-identical OIDC gatekeeper deployments would be worse than the current state. Happy to file a follow-up if there's appetite for that. |
Follow-up to the initial linstor-gui package (#2382). Publishes the UI on https://linstor-gui.<root-host> behind the cluster Keycloak realm using the same oauth2-proxy gatekeeper pattern that dashboard uses, so the LINSTOR REST API is never exposed unauthenticated through Ingress. Rendered only when both gates are satisfied: * _cluster.oidc-enabled == "true" * "linstor-gui" is listed in _cluster.expose-services (i.e. in publishing.exposedServices in the core cozystack values). If OIDC is disabled cluster-wide we intentionally do not ship an unauthenticated token-proxy fallback — unlike dashboard, there is no reason to front a raw storage-management API with k8s bearer tokens. Operators can still reach the UI via `kubectl port-forward` to the ClusterIP service. New resources: * templates/gatekeeper.yaml — oauth2-proxy Deployment (OIDC mode) * templates/gatekeeper-svc.yaml — ClusterIP :8000 in front of it * templates/gatekeeper-sa.yaml — dedicated SA, no autotoken * templates/keycloakclient.yaml — persistent client + cookie Secrets, KeycloakClient CRD with redirectUris for /oauth2/callback * templates/ingress.yaml — Ingress to the gatekeeper Service, cert-manager ClusterIssuer, gated on expose-services + oidc-enabled * tests/ingress_auth_test.yaml — unit tests for each conditional branch and the client CRD README.md updated with both Option 1 (Keycloak-protected Ingress) and the existing Option 2 (port-forward). Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…ssion attrs
Two issues surfaced while deploying to dev10:
1. The shared linstor-gui.labels / linstor-gui.selectorLabels helpers
set app.kubernetes.io/name=linstor-gui, which overrode the
app.kubernetes.io/name=gatekeeper that the gatekeeper templates set
first. The resulting Deployment had selector.matchLabels=gatekeeper
but template.metadata.labels=linstor-gui, failing admission with
"selector does not match template labels". Drop the helper include
from the three gatekeeper templates and inline the remaining common
labels (managed-by, part-of) so the gatekeeper keeps its own
name label.
2. Copying client.session.idle.timeout=86400 and
client.session.max.lifespan=604800 from the dashboard KeycloakClient
caused the keycloak-operator to reject the client ("Client session
idle timeout cannot exceed realm SSO session idle timeout"). Our
dev10 realm has smaller defaults, and dashboard's own client is
also stuck in this failed state. Drop the overrides and let the
realm defaults apply — the UI has no reason to demand longer
sessions than the realm is willing to grant.
Verified on dev10:
* gatekeeper Deployment rolls out cleanly
* KeycloakClient reconciles to status=OK
* https://linstor-gui.<root-host>/ returns HTTP 302 to
keycloak.<root-host>/realms/cozy/protocol/openid-connect/auth
with client_id=linstor-gui and the correct callback URI
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
The tenant chart declares per-namespace egress CNPs so the tenant-root ingress-nginx can reach auth-fronted UIs in the system namespaces (dashboard, keycloak, cdi-upload-proxy). linstor-gui's gatekeeper in cozy-linstor needs the same allowance — without it, requests to https://linstor-gui.<root-host> time out at the Ingress with 504 because tenant-root → cozy-linstor egress is denied by default-deny. Scoped to the gatekeeper pods (instance=linstor-gui, name=gatekeeper) rather than the whole cozy-linstor namespace — the ingress controller does not need to reach the LINSTOR controller, satellites, or CSI. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Narrowing to (instance=linstor-gui, name=gatekeeper) also blocked the transient cm-acme-http-solver pods that cert-manager spins up in the same namespace during http-01 challenges — the solver pods carry different labels, so the Certificate never became Ready and TLS handshakes failed. Broaden the rule to the whole cozy-linstor namespace to mirror allow-to-dashboard / allow-to-keycloak. The ingress controller only initiates upstream HTTP calls driven by Ingress resources, so allowing namespace-wide egress is not a practical escalation. Verified on dev10 after patching the live CNP: Certificate linstor-gui-ingress-tls becomes Ready (Let's Encrypt prod), and the Ingress returns HTTP 302 to the Keycloak authorize endpoint with the correct client_id and redirect_uri. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Keycloak access+refresh+id tokens push the oauth2-proxy session cookie to ~8-10 KB (split across multiple Set-Cookie headers), which overflows ingress-nginx's default proxy_buffer_size and produces "upstream sent too big header" -> HTTP 502 on /oauth2/callback right after a successful Keycloak login. Mirror the dashboard ingress: proxy-buffer-size: 100m and proxy-buffers-number: "4". Verified on dev10 — unauthenticated requests now return 302 to Keycloak instead of 502. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The upstream linstor-gui SPA exposes a "Users" / sign-in panel that POSTs to /v1/security/* on the LINSTOR controller. If a cozystack user turns on HTTP auth via that panel, every subsequent REST call — including from this very GUI, which talks to the controller via mTLS, not bearer tokens — starts returning 401, locking the user out of LINSTOR with no in-product recovery path. Now that authentication for the cozystack-shipped GUI is enforced one layer up at the Ingress (oauth2-proxy + Keycloak), the in-app auth is both redundant and a footgun. Short-circuit /v1/security/* in the gateway nginx with a 403 + explanatory JSON body so the setting cannot be enabled regardless of what the SPA renders. Other LINSTOR REST paths (/v1/nodes, /v1/resource-definitions, /metrics, …) continue to proxy through unchanged. Verified on dev10: POST /v1/security/sign-in -> 403 (with explanation body) GET /v1/security/users -> 403 GET /v1/nodes -> 200 Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The previous commit blocked /v1/security/* on the LINSTOR controller, but that turned out to be wishful thinking: that path doesn't exist in the LINSTOR REST API at all (the controller responds 404 for everything under /v1/security), and the linstor-gui SPA never calls it. The actual storage for the GUI's authentication panel is two LINSTOR KeyValueStore instances managed by the SPA itself: - __gui__settings — holds `authenticationEnabled` (the on/off toggle) - __gui__users — holds the encrypted admin credential The SPA reads them on every page load via `GET /v1/key-value-store/__gui__settings`, so we cannot blanket-block the path or the UI fails to render. Instead, restrict to read-only: allow GET/HEAD (so the SPA can confirm auth=off and skip its login screen), reject every mutating method (PUT/POST/PATCH/DELETE) with the same explanatory 403 JSON. Other GUI key-value entries (e.g. __gui__mode) are unaffected. The dev10 cluster had this footgun tripped already (authenticationEnabled=true, an admin entry in __gui__users); cleared manually with `linstor key-value-store modify __gui__settings authenticationEnabled false` so the existing release stops gating the UI behind a login the user no longer has the password for. Verified end-to-end on dev10 after upgrade: GET /v1/key-value-store/__gui__settings -> 200 PUT /v1/key-value-store/__gui__settings -> 403 (with JSON body) POST /v1/key-value-store/__gui__users -> 403 PUT /v1/key-value-store/__gui__mode -> 200 (other KV writes ok) GET /v1/nodes -> 200 Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
80c21d0 to
1509f5a
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Clean implementation that closely mirrors the existing dashboard gatekeeper pattern. The dual-gate condition (OIDC + expose-services), secret persistence via lookup, security context (non-root, read-only rootfs, dropped capabilities), and the nginx block locking out in-app auth are all well thought out. Test coverage via helm-unittest covers all conditional rendering branches.
Gemini's two suggestions (image digest pinning and bumping cookie-secret to 32 chars) were correctly addressed by the author — both match the existing dashboard convention and should be a platform-wide change if done at all.
Non-blocking action items for follow-up
A1: allow-to-linstor-gui CNP opens egress to entire cozy-linstor namespace
The endpointSelector: {} + egress to all of cozy-linstor allows any tenant pod to reach linstor-gui.cozy-linstor.svc:80/v1 directly, bypassing the gatekeeper. This follows the identical pattern used by allow-to-dashboard and allow-to-keycloak, so it's not a regression — but scoping all three policies to specific service ports and endpoint labels would be valuable as a cross-cutting improvement.
A2: No test coverage for the nginx __gui__settings/__gui__users block
The new nginx location block that locks out in-app authentication has no helm-unittest assertion. Worth adding to prevent regressions if the configmap template is refactored.
A3: KeycloakClient omits serviceAccount.enabled unlike dashboard
Likely intentional since linstor-gui only needs authorization code flow, but worth a brief comment in the template or README documenting the difference.
The base branch was changed.
dosubot
Bot
added
size/S
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security and accessibility of the linstor-gui by providing a managed, OIDC-protected Ingress path. By leveraging the existing oauth2-proxy and Keycloak infrastructure, it allows administrators to expose the UI securely while maintaining strict authentication requirements. The changes include comprehensive template additions for the gatekeeper, automated Keycloak client provisioning, and updated documentation, all while ensuring that unauthenticated access remains disabled by default. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a secure, Keycloak-protected Ingress for the LINSTOR GUI by adding an oauth2-proxy gatekeeper, disabling the GUI's internal authentication via Nginx configuration, and providing the necessary Kubernetes manifests and documentation. Feedback includes correcting an invalid oauth2-proxy image version, reducing an excessively large Nginx proxy buffer size that could lead to OOM, and narrowing the NetworkPolicy's endpoint selector to follow the principle of least privilege. Additionally, it is recommended to increase the OIDC cookie refresh interval and remove wildcards from the Keycloak redirect URIs for better security and performance.
| topologyKey: kubernetes.io/hostname | ||
| containers: | ||
| - name: auth-proxy | ||
| image: quay.io/oauth2-proxy/oauth2-proxy:v7.12.0 |
There was a problem hiding this comment.
The image version v7.12.0 appears to be invalid or a typo. The official oauth2-proxy repository on Quay.io does not have a v7.12.0 tag. You likely meant v7.6.0 (the current stable) or v7.1.2.
image: quay.io/oauth2-proxy/oauth2-proxy:v7.6.0| # overflows the default proxy_buffer_size and causes "upstream sent | ||
| # too big header" -> HTTP 502 on /oauth2/callback. Same numbers as | ||
| # dashboard ingress. | ||
| nginx.ingress.kubernetes.io/proxy-buffer-size: 100m |
There was a problem hiding this comment.
Setting proxy-buffer-size to 100m (100 Megabytes) is excessively large and likely a typo for 100k. This buffer is used for reading the response header from the upstream. A value of 100m will cause Nginx to allocate significant memory per request, potentially leading to OOM (Out Of Memory) issues on the ingress controller. For large OIDC cookies, 128k or 256k is usually more than sufficient.
nginx.ingress.kubernetes.io/proxy-buffer-size: 128k| name: allow-to-linstor-gui | ||
| namespace: {{ include "tenant.name" . }} | ||
| spec: | ||
| endpointSelector: {} |
There was a problem hiding this comment.
The endpointSelector: {} is too broad as it applies this egress rule to all pods in the tenant namespace. It is recommended to restrict this to only the ingress-nginx pods to follow the principle of least privilege.
endpointSelector:
matchLabels:
app.kubernetes.io/name: ingress-nginx| - --email-domain=* | ||
| - --pass-access-token=true | ||
| - --pass-authorization-header=true | ||
| - --cookie-refresh=3m |
There was a problem hiding this comment.
The --cookie-refresh=3m setting is very aggressive. This will cause the proxy to refresh the session against Keycloak every 3 minutes for every active user, which may increase load on the identity provider unnecessarily. Consider increasing this to a more standard value like 1h unless there is a specific security requirement for such frequent refreshes.
- --cookie-refresh=1h| attributes: | ||
| post.logout.redirect.uris: "+" | ||
| redirectUris: | ||
| - "https://linstor-gui.{{ $host }}/oauth2/callback/*" |
There was a problem hiding this comment.
Using a wildcard /* in the redirectUris is generally discouraged for security reasons. Since the oauth2-proxy configuration uses a specific redirect URL, it is safer to provide the exact match here.
- "https://linstor-gui.{{ $host }}/oauth2/callback"
2 participants
What this PR does
Stacked on top of #2382. Publishes the linstor-gui UI on
https://linstor-gui.<root-host>behind the cluster Keycloak realm using the same oauth2-proxy gatekeeper pattern that the dashboard uses, so the LINSTOR REST API is never exposed unauthenticated through Ingress.The Ingress is rendered only when both gates are satisfied:
_cluster.oidc-enabled == "true"linstor-guiis listed in_cluster.expose-services(i.e. inpublishing.exposedServicesin the core cozystack values)If OIDC is disabled cluster-wide we intentionally do not ship an unauthenticated
token-proxyfallback — unlike dashboard, there is no reason to front a raw storage-management API with k8s bearer tokens. Operators can still reach the UI via `kubectl port-forward` to the ClusterIP service.What's new
README updated with Option 1 (Keycloak-protected Ingress) and Option 2 (port-forward).
Release note
```release-note
Added an opt-in Keycloak-protected Ingress for the linstor-gui package. Add `linstor-gui` to `publishing.exposedServices` to publish `https://linstor-gui.` behind the cluster OIDC realm.
```
Test plan