Conversation
github-project-automation
bot
moved this to Milestone 1
in Scalable Cloud Tools Project
📝 WalkthroughWalkthroughRestructures auth from a single-user model to role-based reader/writer credentials across Helm charts and application code; adds separate Kubernetes secrets for reader/writer, updates deployment env bindings, refactors config and middleware to enforce role-aware auth, and bumps chart version to 0.3.1. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Middleware as CacheAuth
participant Config as AuthConfig
Client->>Server: HTTP request (method, credentials)
Server->>Middleware: invoke CacheAuth(requireWriter?)
Middleware->>Config: compare creds with Reader/Writer
alt creds valid & role allowed
Middleware->>Server: allow request
Server->>Client: 200/2xx response
else creds invalid
Middleware->>Server: deny (401)
Server->>Client: 401 Unauthorized (WWW-Authenticate)
else creds valid but insufficient role
Middleware->>Server: deny (403)
Server->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
📦 Chart Preview Readyhelm install test oci://ghcr.io/ls1intum/charts/theia-shared-cache --version 0.3.1-pr.25Updated: 2383f38 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/config/config.go (1)
157-163:⚠️ Potential issue | 🟠 MajorMissing validation for the
Rolefield onUserAuth.
Validate()checksUsernameandPasswordbut notRole. If a user entry has an empty role (e.g., omitted in config), theRequireRole("writer")middleware will silently deny all writes with 403, making the issue hard to diagnose.🛡️ Proposed fix
for i, user := range c.Auth.Users { if user.Username == "" { return fmt.Errorf("auth.users[%d].username is required", i) } if user.Password == "" { return fmt.Errorf("auth.users[%d].password is required", i) } + if user.Role == "" { + return fmt.Errorf("auth.users[%d].role is required", i) + } }
🤖 Fix all issues with AI agents
In `@chart/values.yaml`:
- Around line 19-21: Fix the typos in the comments near the password entry:
change "optimaly" to "optimally" and "envrionment" to "environment" in the
comment lines surrounding the password: "changeme-reader" (these comment lines
appear twice). Leave the password value unchanged but update both occurrences of
the misspelled words so the comment reads e.g. "optimally from github
environment secrets".
In `@src/internal/config/config.go`:
- Around line 120-122: The current env-override uses fixed indices via
overrideUserFromEnv(&cfg, 0, ...) and overrideUserFromEnv(&cfg, 1, ...) which is
brittle if cfg.Auth.Users order or length changes; change the logic to locate
the user by role (e.g., "reader" and "writer") instead of by index: implement a
new helper (or modify overrideUserFromEnv) to iterate cfg.Auth.Users, match on
Users[i].Role, and then set Users[i].Username and Users[i].Password from the
corresponding env vars if present, returning after a match so it safely skips
missing roles.
🧹 Nitpick comments (1)
src/internal/middleware/auth.go (1)
59-67: Unsafe type assertion could panic if context value is unexpected.Line 62 uses a direct type assertion
role.(string)which will panic if the value stored under"role"is not a string. While in the current flowBasicAuthalways sets a string, a defensive comma-ok pattern is safer against future middleware changes.♻️ Defensive fix
func RequireRole(requiredRole string) gin.HandlerFunc { return func(c *gin.Context) { role, exists := c.Get("role") - if !exists || role.(string) != requiredRole { + roleStr, ok := role.(string) + if !exists || !ok || roleStr != requiredRole { c.AbortWithStatus(http.StatusForbidden) return } c.Next() } }
KevinGruber2001
moved this from Milestone 1
to In Progress
in Scalable Cloud Tools Project
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chart/templates/deployment.yaml (1)
22-22:⚠️ Potential issue | 🟠 MajorChecksum annotation doesn't cover the new auth secrets — credential changes won't trigger a rollout.
Line 22 computes a checksum from
secrets.yaml, but reader/writer credentials live inauth-secrets.yaml. If auth credentials are rotated, pods won't restart automatically.Proposed fix: add a checksum for auth-secrets
checksum/secret: {{ include (print $.Template.BasePath "/secrets.yaml") . | sha256sum }} + {{- if .Values.auth.enabled }} + checksum/auth-secret: {{ include (print $.Template.BasePath "/auth-secrets.yaml") . | sha256sum }} + {{- end }}
🤖 Fix all issues with AI agents
In `@chart/templates/auth-secrets.yaml`:
- Line 1: The auth secrets template is guarded by .Values.enabled but not
.Values.auth.enabled, so secrets are created when the chart is enabled even if
auth is disabled; update the top-level conditional in the auth-secrets template
to also require .Values.auth.enabled (e.g. change the if to test both
.Values.enabled and .Values.auth.enabled or replace with .Values.auth.enabled
alone) so the Secret resource is only rendered when auth is actually enabled;
ensure references to the template (and any include calls) still work with the
updated condition.
In `@src/internal/middleware/auth.go`:
- Around line 11-12: Update the stale doc comment that references BasicAuth to
correctly describe CacheAuth: find the top-of-file or above the CacheAuth
function declaration (symbol CacheAuth) in auth.go and replace "BasicAuth" with
"CacheAuth" and adjust wording if necessary so the comment accurately describes
that the middleware validates cached authentication and stores the user's role
in the gin context.
🧹 Nitpick comments (2)
chart/Chart.yaml (1)
8-8: Consider a minor version bump for this breaking change.Switching from a single-user auth model to reader/writer roles changes the secret structure, environment variables, and values schema. This is a breaking change for existing deployments — a bump to
0.4.0would better signal incompatibility to consumers.chart/templates/auth-secrets.yaml (1)
12-13: Plaintext credentials invalues.yamlare double-base64-encoded if users supply pre-encoded values.Using
b64encon thedatafield is correct whenvalues.yamlcontains plaintext credentials. However, consider documenting that values must be plaintext (not pre-encoded), or alternatively usestringDatainstead ofdatato avoid theb64enccall entirely and let Kubernetes handle encoding. This is less error-prone.Simpler alternative using stringData
-data: - username: {{ .Values.auth.reader.username | b64enc | quote }} - password: {{ .Values.auth.reader.password | b64enc | quote }} +stringData: + username: {{ .Values.auth.reader.username | quote }} + password: {{ .Values.auth.reader.password | quote }}Same change for the writer secret.
src/internal/middleware/auth.go
Outdated
| // BasicAuth creates a middleware that validates HTTP Basic Authentication | ||
| // and stores the user's role in the gin context. |
There was a problem hiding this comment.
Stale doc comment references BasicAuth instead of CacheAuth.
Fix
-// BasicAuth creates a middleware that validates HTTP Basic Authentication
-// and stores the user's role in the gin context.
+// CacheAuth creates a middleware that validates HTTP Basic Authentication
+// with role-based access control (reader/writer).📝 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.
| // BasicAuth creates a middleware that validates HTTP Basic Authentication | |
| // and stores the user's role in the gin context. | |
| // CacheAuth creates a middleware that validates HTTP Basic Authentication | |
| // with role-based access control (reader/writer). |
🤖 Prompt for AI Agents
In `@src/internal/middleware/auth.go` around lines 11 - 12, Update the stale doc
comment that references BasicAuth to correctly describe CacheAuth: find the
top-of-file or above the CacheAuth function declaration (symbol CacheAuth) in
auth.go and replace "BasicAuth" with "CacheAuth" and adjust wording if necessary
so the comment accurately describes that the middleware validates cached
authentication and stores the user's role in the gin context.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
chart/templates/auth-secrets.yaml (1)
12-13: Consider usingrequiredto fail fast on missing credentials.If any of the auth credential values are unset or empty in
values.yaml, Helm will silently render an empty base64-encoded string. Usingrequiredwould surface misconfigurations athelm template/helm installtime rather than at runtime.Proposed fix
- username: {{ .Values.auth.reader.username | b64enc | quote }} - password: {{ .Values.auth.reader.password | b64enc | quote }} + username: {{ required "auth.reader.username is required" .Values.auth.reader.username | b64enc | quote }} + password: {{ required "auth.reader.password is required" .Values.auth.reader.password | b64enc | quote }}Apply the same pattern to the writer secret (lines 25-26).
src/internal/middleware/auth.go (1)
22-26: Timing side-channel: username compared with==and short-circuit skips the constant-time password check.The
&&short-circuits, so when the username doesn't match,ConstantTimeCompareis never called. An attacker can distinguish "wrong username" (fast) from "right username, wrong password" (slower due toConstantTimeCompare). This partially negates the benefit of the constant-time comparison.For a build cache this is low risk, but if you want full timing-attack resistance, compare both username and password in constant time and combine the results:
Proposed fix
- isReader := username == auth.Reader.Username && - subtle.ConstantTimeCompare([]byte(password), []byte(auth.Reader.Password)) == 1 - isWriter := username == auth.Writer.Username && - subtle.ConstantTimeCompare([]byte(password), []byte(auth.Writer.Password)) == 1 + isReader := subtle.ConstantTimeCompare([]byte(username), []byte(auth.Reader.Username)) == 1 && + subtle.ConstantTimeCompare([]byte(password), []byte(auth.Reader.Password)) == 1 + isWriter := subtle.ConstantTimeCompare([]byte(username), []byte(auth.Writer.Username)) == 1 && + subtle.ConstantTimeCompare([]byte(password), []byte(auth.Writer.Password)) == 1Note:
&&still short-circuits in Go, so you'd need to evaluate both and combine to be fully constant-time:readerUser := subtle.ConstantTimeCompare([]byte(username), []byte(auth.Reader.Username)) readerPass := subtle.ConstantTimeCompare([]byte(password), []byte(auth.Reader.Password)) isReader := readerUser & readerPass == 1 writerUser := subtle.ConstantTimeCompare([]byte(username), []byte(auth.Writer.Username)) writerPass := subtle.ConstantTimeCompare([]byte(password), []byte(auth.Writer.Password)) isWriter := writerUser & writerPass == 1
1 participant
Summary by CodeRabbit
New Features
Chores