feat: header based acls#337
Conversation
WalkthroughReorganizes public config types (Labels → AppConfigs, AppLabels → App and nested App* types), adds header and label decoders using traefik/paerser, lets ProxyController prefer per-request App decoded from headers (fallback to Docker labels), updates service/controller signatures, adds tests, header normalization, and an IP filter normalization tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Proxy as ProxyController
participant Norm as NormalizeHeaders
participant Dec as Decoders
participant Dock as DockerService
participant Auth as AuthService
participant Up as Upstream
Client->>Proxy: HTTP request
Proxy->>Norm: NormalizeHeaders(req.Header)
Proxy->>Dec: DecodeHeaders(normalized)
alt decoded Apps present
Proxy->>Proxy: select first App
else
Proxy->>Dock: GetLabels(domain)
Dock-->>Proxy: App (or empty)
end
Proxy->>Auth: IsBypassedIP(App.IP, clientIP)
Auth-->>Proxy: bool
Proxy->>Auth: IsAuthEnabled(uri, App.Path)
Auth-->>Proxy: bool
alt auth required
Proxy->>Auth: IsResourceAllowed(ctx, userCtx, App)
Auth-->>Proxy: allowed?
alt allowed
Proxy->>Proxy: setHeaders(ctx, App)
Proxy->>Up: forward request
else denied
Proxy-->>Client: 401/403 or redirect
end
else
Proxy->>Up: forward request
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
========================================
+ Coverage 0.00% 2.66% +2.66%
========================================
Files 32 34 +2
Lines 2372 2479 +107
========================================
+ Hits 0 66 +66
- Misses 2372 2406 +34
- Partials 0 7 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/proxy_controller.go (1)
81-99: Header-first selection + avoid unconditional Docker call (critical for non-containerized use case)Right now Docker labels are fetched unconditionally and errors lead to 500 even when header config is present—this breaks the PR objective (header ACLs for non-containerized apps). Also, picking the first map entry from
headers.Appsis nondeterministic.
- Prefer headers: if exactly one app, use it.
- If multiple header apps, try to match subdomain (resource) deterministically; else fall back to Docker.
- Only call Docker when needed.
@@ - var app config.App - - headers, err := decoders.DecodeHeaders(utils.NormalizeHeaders(c.Request.Header)) - - if err != nil { - log.Error().Err(err).Msg("Failed to decode headers") - controller.handleError(c, req, isBrowser) - return - } - - labels, err := controller.docker.GetLabels(host) - if err != nil { - log.Error().Err(err).Msg("Failed to get labels from Docker") - controller.handleError(c, req, isBrowser) - return - } - - if len(headers.Apps) > 0 { - for k, v := range headers.Apps { - log.Debug().Str("app", k).Msg("Using headers for app config instead of labels") - app = v - break - } - } else { - log.Debug().Msg("No app config found in headers, using labels") - app = labels - } + var app config.App + + headers, hdrErr := decoders.DecodeHeaders(utils.NormalizeHeaders(c.Request.Header)) + if hdrErr != nil { + log.Warn().Err(hdrErr).Msg("Failed to decode headers; will try Docker labels") + } + + useDocker := true + if hdrErr == nil && len(headers.Apps) > 0 { + if len(headers.Apps) == 1 { + for k, v := range headers.Apps { + log.Debug().Str("app", k).Msg("Using headers for app config") + app = v + } + useDocker = false + } else { + resource := strings.Split(host, ".")[0] + if v, ok := headers.Apps[resource]; ok { + log.Debug().Str("app", resource).Msg("Using header app matched to subdomain") + app = v + useDocker = false + } else { + log.Warn().Int("apps", len(headers.Apps)).Msg("Multiple header apps; none match subdomain, will try Docker labels") + } + } + } + + if useDocker { + labels, err := controller.docker.GetLabels(host) + if err != nil { + log.Error().Err(err).Msg("Failed to get labels from Docker") + controller.handleError(c, req, isBrowser) + return + } + if hdrErr == nil && len(headers.Apps) == 0 { + log.Debug().Msg("No app config found in headers, using labels") + } + app = labels + }
🧹 Nitpick comments (13)
internal/config/config.go (4)
132-139: Solid aggregation for per‑app settings; consider minimal docs.Add brief comments on each field to aid discoverability in editors and generated docs.
141-144: Domain likely required — enforce via validation.Add a validation step (post‑decode) to ensure Domain is non‑empty per app; fail fast with a clear error.
150-154: Naming and format nits for OAuth fields.Consider renaming Whitelist → Allowlist (optional). Also document delimiter for Groups (comma‑separated?) to align with header decoding and PR’s group ACLs.
172-176: Path.Allow/Block formatting.If these accept regex (like CheckFilter) vs literal paths, document and ensure consistent handling/escaping to avoid accidental broad matches.
internal/utils/decoders/label_decoder.go (1)
14-16: Wrap decode errors with context; tiny rename nit.Provide context for easier troubleshooting and consider renaming appLabels → apps or cfg.
Apply this diff:
- err := parser.Decode(labels, &appLabels, "tinyauth", "tinyauth.apps") + err := parser.Decode(labels, &appLabels, "tinyauth", "tinyauth.apps") if err != nil { - return config.AppConfigs{}, err + return config.AppConfigs{}, fmt.Errorf("decoders.DecodeLabels: %w", err) }Note: add
fmtimport.internal/utils/decoders/label_decoder_test.go (1)
46-73: Add edge-case tests (whitespace, multi-app, key casing).Recommend adding:
- Values with spaces around commas to ensure trimming works.
- Multiple apps in the same map to verify per-app isolation.
- Mixed-case keys to assert case-insensitive decode.
internal/utils/decoders/header_decoder_test.go (1)
46-73: Add negative/edge tests (no matching keys, invalid root, case-insensitive keys).
- No “Tinyauth-Apps-” headers should return zero-value AppConfigs without error.
- An “InvalidRoot-…” header should error.
- Mixed-case header keys should decode identically.
internal/utils/header_utils.go (1)
8-24: Minor: reuse SanitizeHeader for ParseHeaders consistency.If ParseHeaders and NormalizeHeaders feed the same downstream logic, consider aligning sanitization rules to avoid divergent behavior.
internal/service/docker_service.go (2)
58-99: Return type updated to App is good; add small resiliency & log clarity.
- Skip decode for containers with no labels to avoid needless work.
- Clarify log when decode fails.
Apply this diff:
@@ - labels, err := decoders.DecodeLabels(inspect.Config.Labels) + // Skip when there are no labels + if len(inspect.Config.Labels) == 0 { + continue + } + labels, err := decoders.DecodeLabels(inspect.Config.Labels) if err != nil { - log.Warn().Str("id", ctr.ID).Err(err).Msg("Error getting container labels, skipping") + log.Warn().Str("id", ctr.ID).Err(err).Msg("Error decoding container labels, skipping") continue }
58-99: Naming nit: GetLabels now returns App.Consider renaming to GetApp or GetAppFromContainers for clarity in call sites (optional).
internal/utils/decoders/header_decoder.go (1)
74-92: Minor: avoid lowercasing stored node names if you care about tag-matching verbosity.You lowercase keys for path building (fine). If later you add explicit struct tags that are case-sensitive, ensure AddMetadata mapping remains consistent. No change required now.
internal/service/auth_service.go (1)
325-353: Avoid per-request regex compilationCompiling regexes on every request is avoidable. Consider a small cache keyed by pattern to reuse compiled regexes.
Example sketch (package-scope):
var rxCache sync.Map // map[string]*regexp.Regexp func mustGetRegex(p string) (*regexp.Regexp, error) { if v, ok := rxCache.Load(p); ok { return v.(*regexp.Regexp), nil } rx, err := regexp.Compile(p) if err != nil { return nil, err } rxCache.Store(p, rx) return rx, nil }Then replace
regexp.Compile(...)withmustGetRegex(...).internal/controller/proxy_controller.go (1)
276-292: setHeaders now accepts App: caution on header-sourced secretsFunctionally correct. However, when configs are supplied via headers,
PasswordFileandHeadersbecome request-controlled inputs. Ensure reverse proxies strip any client-supplied Tinyauth headers and only inject trusted ones, to avoid file disclosure or header injection to upstreams.Would you like a hardened NGINX example that (a) isolates the auth_request endpoint, and (b) clears any inbound
Tinyauth-*/X-Tinyauth-*headers before setting your intended config?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
internal/config/config.go(1 hunks)internal/controller/proxy_controller.go(8 hunks)internal/service/auth_service.go(4 hunks)internal/service/docker_service.go(4 hunks)internal/utils/decoders/header_decoder.go(1 hunks)internal/utils/decoders/header_decoder_test.go(1 hunks)internal/utils/decoders/label_decoder.go(1 hunks)internal/utils/decoders/label_decoder_test.go(1 hunks)internal/utils/header_utils.go(1 hunks)internal/utils/security_utils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:42:35.630Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#329
File: internal/service/auth_service.go:335-337
Timestamp: 2025-08-29T13:42:35.630Z
Learning: In the tinyauth codebase, path filtering works with default behaviors: path.block allows all paths by default and blocks (requires auth for) matching ones, while path.allow blocks all paths by default and allows (skips auth for) matching ones. In IsAuthEnabled function, returning false means authentication is disabled/not required, and returning true means authentication is enabled/required.
Applied to files:
internal/service/auth_service.go
🧬 Code graph analysis (7)
internal/utils/decoders/label_decoder.go (1)
internal/config/config.go (1)
AppConfigs(128-130)
internal/utils/decoders/label_decoder_test.go (2)
internal/config/config.go (10)
AppConfigs(128-130)App(132-139)Config(17-56)AppConfig(141-143)AppUsers(145-148)AppOAuth(150-153)AppIP(155-159)AppResponse(161-164)AppBasicAuth(166-170)AppPath(172-175)internal/utils/decoders/label_decoder.go (1)
DecodeLabels(9-19)
internal/utils/decoders/header_decoder_test.go (2)
internal/config/config.go (10)
AppConfigs(128-130)App(132-139)Config(17-56)AppConfig(141-143)AppUsers(145-148)AppOAuth(150-153)AppIP(155-159)AppResponse(161-164)AppBasicAuth(166-170)AppPath(172-175)internal/utils/decoders/header_decoder.go (1)
DecodeHeaders(14-24)
internal/service/docker_service.go (2)
internal/config/config.go (2)
App(132-139)Config(17-56)internal/utils/decoders/label_decoder.go (1)
DecodeLabels(9-19)
internal/utils/decoders/header_decoder.go (1)
internal/config/config.go (1)
AppConfigs(128-130)
internal/controller/proxy_controller.go (3)
internal/config/config.go (1)
App(132-139)internal/utils/decoders/header_decoder.go (1)
DecodeHeaders(14-24)internal/utils/header_utils.go (1)
NormalizeHeaders(36-44)
internal/service/auth_service.go (1)
internal/config/config.go (4)
UserContext(101-111)App(132-139)AppPath(172-175)AppIP(155-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
internal/config/config.go (3)
128-130: Type rename to AppConfigs looks good.Shape aligns with decoder usage returning a map of apps.
145-149: Clarify Users.Allow/Block semantics.Confirm these use the same matcher semantics as CheckFilter (regex between slashes or CSV). If so, document expected format to avoid misconfiguration.
155-160: IP lists: document accepted formats and add tests.Given FilterIP’s new dash→CIDR normalization, specify allowed forms (single IP, CIDR, "IP-mask" like 192.168.0.0-24). Add unit tests for valid/invalid cases to prevent regressions.
internal/utils/decoders/label_decoder.go (1)
9-19: Decoder implementation LGTM.Straightforward use of paerser to hydrate AppConfigs.
internal/utils/decoders/label_decoder_test.go (1)
10-45: LGTM on happy-path coverage.The structure mirrors config types and validates slice vs. scalar decoding correctly.
internal/utils/decoders/header_decoder_test.go (1)
10-45: Solid parity test with label-based decoding.Covers all sections including nested BasicAuth and slice fields.
internal/service/auth_service.go (3)
288-303: OAuth whitelist short‑circuit: confirm intended behaviorFor OAuth users, this returns the whitelist check result and skips user allow/block checks. If
labels.OAuth.Whitelistis empty andutils.CheckFilter("", …)evaluates to true, all OAuth users would pass this step (group checks still happen later in the controller). Confirm this matches policy.
367-399: IP allow/block evaluation looks correctBlock list takes precedence, explicit allow grants, non-empty allow acts as deny-by-default. Leverages utils.FilterIP improvements.
401-416: Bypass IP logic LGTMStraightforward and safe with error-tolerant parsing.
internal/controller/proxy_controller.go (7)
10-10: New decoders import OKDependency addition is appropriate for header-based app config.
102-104: Bypass IP check switch to app.IP is correctEarly success path sets headers and returns 200. Good.
111-121: Auth enablement check now uses app.PathMatches the reorganized types and aligns with path.allow/block semantics.
129-151: IP policy enforcement now uses app.IPBehavior preserved. Messaging/redirect logic unchanged. Good.
171-173: Resource permission check now uses AppSignature change consumed correctly.
205-207: Group enforcement wired to header-provided groupsThis correctly enables per-request group ACLs via
app.OAuth.Groups.
246-247: Header setting uses AppKeeps flow consistent across bypass/disabled/authenticated cases.
| func DecodeHeaders(headers map[string]string) (config.AppConfigs, error) { | ||
| var app config.AppConfigs | ||
|
|
||
| err := decodeHeadersHelper(headers, &app, "tinyauth", "tinyauth-apps") | ||
|
|
||
| if err != nil { | ||
| return config.AppConfigs{}, err | ||
| } | ||
|
|
||
| return app, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security: header-driven ACL config must be gated by a trust boundary.
Any client could spoof “Tinyauth-Apps-…” headers if not explicitly restricted to a trusted proxy path, enabling privilege escalation. Gate DecodeHeaders usage behind:
- A trusted proxy/source IP allowlist, or
- A shared secret/HMAC (e.g., X-Tinyauth-Signature), and
- A feature flag (disabled by default) to avoid accidental exposure.
Also document this prominently in the README and sample proxy configs.
Run this to check whether calls to DecodeHeaders/NormalizeHeaders are gated by source checks or feature flags:
🏁 Script executed:
#!/bin/bash
rg -n -C3 'DecodeHeaders|NormalizeHeaders|X-Tinyauth|TrustedProx|Allowlist|Signature|HMAC' --glob '!**/vendor/**' --glob '!**/dist/**'Length of output: 2582
Gate header-driven ACL config behind a trust boundary
- In internal/controller/proxy_controller.go:73, wrap the
DecodeHeaders(utils.NormalizeHeaders(…))call with:
• A source-IP or reverse-proxy allowlist check
• Validation of an HMAC signature (e.g.X-Tinyauth-Signature)
• A feature flag (off by default) - Document this in the README and include sample proxy configuration to illustrate the trusted-proxy setup.
🤖 Prompt for AI Agents
In internal/utils/decoders/header_decoder.go around lines 14 to 24, the
caller-side use of DecodeHeaders must be gated by a trust boundary: update
internal/controller/proxy_controller.go at the
DecodeHeaders(utils.NormalizeHeaders(...)) call site (around line 73) to first
check that the request originates from a permitted source IP or reverse-proxy
allowlist, validate an HMAC signature header (e.g. X-Tinyauth-Signature) over
the normalized headers using a shared secret, and only call DecodeHeaders when
both checks pass and a new feature flag (disabled by default) is enabled; add
the feature flag config and a clear README section with sample reverse-proxy
config showing trusted-proxy IPs and how to compute/send the HMAC signature.
There was a problem hiding this comment.
Will do in another pr.
There was a problem hiding this comment.
@steveiliop56 That's a sensible approach! Keeping the security enhancements in a separate PR will allow for proper focus on implementing the trust boundary mechanisms.
Would you like me to create a GitHub issue to track the security improvements needed for header-driven ACL configuration? This would help ensure the security concerns are properly documented and addressed in the follow-up PR.
There was a problem hiding this comment.
🧩 Analysis chain
@steveiliop56 I've created issue with the security requirements for gating header-driven ACL configuration behind proper trust boundaries. The issue outlines the specific implementation needs including IP allowlists, HMAC validation, and feature flags to ensure the header-based functionality is secure by default.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Security: Gate header-driven ACL configuration behind trust boundary" \
--body "## Description
Header-driven ACL configuration introduced in PR #337 needs security enhancements to prevent privilege escalation attacks. Currently, any client can spoof 'Tinyauth-Apps-...' headers to bypass access controls.
## Security Concerns
- Clients can forge headers to escalate privileges
- No validation of request source or authenticity
- Feature enabled by default without safeguards
## Required Security Measures
The \`DecodeHeaders(utils.NormalizeHeaders(...))\` call in \`internal/controller/proxy_controller.go\` (around line 73) must be protected by:
1. **Source IP Allowlist**: Verify requests originate from trusted reverse proxies
2. **HMAC Signature Validation**: Validate \`X-Tinyauth-Signature\` header using shared secret
3. **Feature Flag**: Disable header-driven ACLs by default, require explicit enablement
## Implementation Requirements
- [ ] Add configuration for trusted proxy IP allowlist
- [ ] Implement HMAC signature validation for headers
- [ ] Add feature flag to enable/disable header-driven ACLs (default: disabled)
- [ ] Update documentation with security considerations
- [ ] Provide sample reverse proxy configuration showing:
- Trusted proxy IP setup
- HMAC signature computation and transmission
## References
- Original PR: https://github.com/steveiliop56/tinyauth/pull/337
- Security discussion: https://github.com/steveiliop56/tinyauth/pull/337#discussion_r2316442327" \
--assignee steveiliop56
Length of output: 1610
@steveiliop56 Perfect! I've successfully created issue #338 to track the security improvements needed for header-driven ACL configuration. The issue includes all the implementation requirements for adding proper trust boundaries, including IP allowlists, HMAC validation, and feature flags to ensure the functionality is secure by default.
This will help ensure the security concerns are properly documented and can be systematically addressed in your follow-up PR.
This reverts commit f0d2da2.
2 participants
Fixes #275
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests