feat(oauth): implement oidc contract with a generic provider and preset based on google and stackit - #36
Conversation
created oauth provider interfaces and registry created preset for google and stackit use generic approach for all other until implemented implement oauth claims into a map for rbac Signed-off-by: Maximilian Hagen <git@saxy.dev>
that example shows how to connect with google oidc Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds configurable OAuth/OIDC authentication with generic and preset providers, cached JWKS verification, claim-to-role RBAC mapping, bearer-token support in binary and RESP protocols, configuration flags, tests, documentation, and an OAuth example. ChangesOAuth/OIDC authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Tellstone
participant OIDCProvider
participant RBAC
Client->>Tellstone: AUTH bearer token
Tellstone->>OIDCProvider: Verify JWT
OIDCProvider-->>Tellstone: Claims
Tellstone->>RBAC: Resolve claims to role and subject
RBAC-->>Tellstone: Session context
Tellstone-->>Client: Authentication result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 13
🧹 Nitpick comments (10)
internal/oauth/registry.go (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the doc comment verb and consider
slices.Sort.Line 79 reads "Providers return the sorted names". The Go convention for a single function is "Providers returns".
slices.Sortis also the current form for a string slice.♻️ Proposed change
-// Providers return the sorted names of all registered providers. The sort +// Providers returns the sorted names of all registered providers. The sort // keeps help output and error messages deterministic. func Providers() []string { names := make([]string, 0, len(registry)) for name := range registry { names = append(names, name) } - sort.Strings(names) + slices.Sort(names) return names }Replace
"sort"with"slices"in the import block.🤖 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 `@internal/oauth/registry.go` around lines 79 - 88, Update the Providers doc comment to say “Providers returns” and replace sort.Strings with slices.Sort, updating the import from sort to slices accordingly.internal/oauth/generic/generic.go (4)
187-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the discovery and JWKS response bodies.
getJSONdecodes whatever the endpoint returns.p.client.Timeoutbounds the total call duration, but a peer that streams JSON steadily stays inside the timeout whilejson.Decoderallocates without limit. For the JWKS path this is reachable after startup, becauserefreshJWKSruns duringVerify.Wrap the body in an
io.LimitReader. A real discovery document and JWKS are a few kilobytes.🛡️ Proposed fix
+// maxDocSize bounds discovery and JWKS responses. Real documents are a few +// kilobytes, so this only stops a peer that streams JSON inside the timeout. +const maxDocSize = 1 << 20 + func (p *Provider) getJSON(ctx context.Context, url string, out any) error { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return err } resp, err := p.client.Do(req) if err != nil { return err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("generic: GET %s returned %s", url, resp.Status) } - return json.NewDecoder(resp.Body).Decode(out) + return json.NewDecoder(io.LimitReader(resp.Body, maxDocSize)).Decode(out) }This needs
"io"in the import block.🤖 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 `@internal/oauth/generic/generic.go` around lines 187 - 201, Update Provider.getJSON to wrap resp.Body with io.LimitReader before passing it to json.Decoder, using a sufficiently bounded limit for discovery and JWKS responses. Add the required io import and preserve the existing request, status validation, and decoding behavior.
142-156: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the discovery document's
issueragainst the configured issuer.
doc.Issueris decoded at Line 145 and then discarded. OpenID Connect Discovery requires theissuervalue in the document to match the issuer the client used to build the request URL. Without the check, a redirect or a misconfigured endpoint can hand back a document that pointsjwks_uriat an unrelated key set.Claim validation at Line 325 still compares the token's
issagainstcfg.Issuer, so this is defense in depth rather than an open bypass. It is one comparison at startup.🔒️ Proposed fix
if err := p.getJSON(ctx, url, &doc); err != nil { return err } + // OIDC Discovery requires the document issuer to equal the issuer used to + // build the request, so a redirected or wrong endpoint cannot supply keys. + if doc.Issuer != strings.TrimRight(p.cfg.Issuer, "/") && doc.Issuer != p.cfg.Issuer { + return fmt.Errorf("generic: discovery issuer %q does not match configured issuer %q", doc.Issuer, p.cfg.Issuer) + } if doc.JWKSURI == "" { return errors.New("oauth: discovery document has no jwks_uri") }The in-process identity providers in
internal/oauth/generic/generic_test.goandinternal/oauth/presets/google_test.goalready returnsrv.URLasissuer, so the existing tests keep passing.🤖 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 `@internal/oauth/generic/generic.go` around lines 142 - 156, In Provider.refreshDiscovery, validate doc.Issuer against the configured issuer after decoding the discovery document and before accepting doc.JWKSURI; normalize trailing slashes consistently with the request URL and return an error on mismatch. Preserve the existing missing-jwks_uri check and refreshJWKS flow for a matching issuer.
244-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeprecated
ecdsa.PublicKeycoordinate fields ininternal/oauth/generic/generic.goandinternal/oauth/generic/generic_test.go. Both sites move EC public keys through the rawXandYbig.Intfields, which Go deprecated. The Go 1.26 API audit listsPublicKey.XandPublicKey.Yas deprecated. The documentation directs callers toPublicKey.BytesandParseUncompressedPublicKey. golangci-lint already fails on the test site with SA1019, sotask checkdoes not pass.
internal/oauth/generic/generic.go#L244-L256: inparseJWK, build the P-256 key from the uncompressed point withecdsa.ParseUncompressedPublicKey(elliptic.P256(), append([]byte{4}, xPadded..., yPadded...))instead of assigningXandY. Left-pad each coordinate to 32 bytes, because a JWK omits leading zero bytes. This also validates that the point lies on the curve, which the current code does not do.internal/oauth/generic/generic_test.go#L98-L110: injwksFromKeys, derive the JWK coordinates frompk.Bytes()and slice the uncompressed point intoXandYrather than readingpk.Xandpk.Y.Confirm the repository's declared Go toolchain first, because
ParseUncompressedPublicKeyandPublicKey.Bytesneed Go 1.25 or later. The verification script oninternal/oauth/generic/generic.goLine 244-256 reports the declared version.🤖 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 `@internal/oauth/generic/generic.go` around lines 244 - 256, Update internal/oauth/generic/generic.go:244-256 in parseJWK to confirm the declared Go toolchain supports the newer APIs, then left-pad decoded EC coordinates to 32 bytes and construct the P-256 key with ecdsa.ParseUncompressedPublicKey using the uncompressed point format, preserving decode errors and validating curve membership. Update internal/oauth/generic/generic_test.go:98-110 in jwksFromKeys to derive coordinates from pk.Bytes() and slice the uncompressed point instead of accessing deprecated pk.X and pk.Y.Source: Linters/SAST tools
126-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the split from
splitTokeninstead of splitting the token again.
splitTokenat Line 98 already split the token. Line 126 re-runsstrings.Split(string(token), "."), which allocates a second string copy and a second slice, then Line 127 concatenates the first two segments into a third allocation.Verifyruns once per AUTH on the auth-worker pool, so this is not a correctness problem, but the file states thatVerify"stays local and allocation-light".Return the signing input from
splitTokenand drop the second split.♻️ Proposed refactor
- segments := strings.Split(string(token), ".") - if err = verifySignature(hdr.Alg, key, []byte(segments[0]+"."+segments[1]), sig); err != nil { + if err = verifySignature(hdr.Alg, key, signing, sig); err != nil { return nil, oauth.ErrInvalidToken }Change the helper to hand back the signed bytes, which are a subslice of the input and cost no allocation:
// splitToken decodes the three JWT segments and returns the signing input // (header.payload) as a subslice of token, so the caller never re-splits. func splitToken(token []byte) (header, payload, sig, signing []byte, err error) { first := bytes.IndexByte(token, '.') if first < 0 { return nil, nil, nil, nil, oauth.ErrInvalidToken } second := bytes.IndexByte(token[first+1:], '.') if second < 0 { return nil, nil, nil, nil, oauth.ErrInvalidToken } second += first + 1 sigPart := token[second+1:] if bytes.IndexByte(sigPart, '.') >= 0 { return nil, nil, nil, nil, oauth.ErrInvalidToken } // ... decode the three segments with b64 as today ... return header, payload, sig, token[:second], nil }Measure the change with a benchmark and report the actual before/after numbers rather than an estimate.
As per coding guidelines: "Never invent benchmark numbers; if a benchmark was not run, state that explicitly."
🤖 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 `@internal/oauth/generic/generic.go` around lines 126 - 129, Update splitToken to return the signing input as an additional value, using the existing token subslice covering header.payload without allocating. In Verify, consume that returned signing input and remove the second strings.Split and concatenation before verifySignature. Add or update a benchmark to measure the allocation change, and report actual results only if the benchmark is run.Source: Coding guidelines
internal/oauth/presets/google.go (1)
32-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExported preset constructors return unexported types in
internal/oauth/presets/google.goandinternal/oauth/presets/stackit.go. Both constructors are exported but hand back a pointer to a package-private struct, so a caller in another package cannot name the returned type, cannot declare a variable of it, and cannot embed it. Both structs already satisfyoauth.Provider, which is the typeserverstores andoauth.Registeraccepts.
internal/oauth/presets/google.go#L32-L44: change the signature tofunc NewGoogle(cfg oauth.Config, logger log.Logger) (oauth.Provider, error)and keep theinternal/oauth/presets/stackit.go#L33-L45: change the signature tofunc NewStackit(cfg oauth.Config, logger log.Logger) (oauth.Provider, error)and keep thestackitstruct unexported.Both preset test files call
p.Config()andp.Verify(...), which the interface exposes, so the existing tests compile unchanged.🤖 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 `@internal/oauth/presets/google.go` around lines 32 - 44, Exported preset constructors currently expose unexported concrete return types. Update NewGoogle in internal/oauth/presets/google.go:32-44 and NewStackit in internal/oauth/presets/stackit.go:33-45 to return (oauth.Provider, error), while keeping the google and stackit structs unexported and preserving their existing construction logic.internal/oauth/oauth.go (1)
60-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
IsJWTwith its documented contract.The comment at Line 72-74 does not match the code.
"a.b"contains one dot, not two, and two dots always produce three segments, so the parenthetical is inverted.The
len(b) >= 5guard also does not enforce non-empty segments."..abc"has two dots and length 5, soIsJWTreturnstrueeven though the header and payload segments are empty. The unit test case"two dots empty head"uses"..c", which is rejected by the length check alone, so the empty-segment case is not covered.There is no security impact:
generic.splitTokenrejects the empty segment later. The routing contract should still say what it does.♻️ Proposed segment check
func IsJWT(b []byte) bool { - dots := 0 - for _, c := range b { - if c == '.' { - dots++ - } - } - // At least one non-empty segment each side of two dots, so a bare ".." or - // "a.b" (three segments is impossible with two dots) cannot slip through. - return dots == 2 && len(b) >= 5 + // Require three non-empty dot-separated segments. A bare "..", "..c" or + // "a.b" is therefore never routed to token verification. + first := bytes.IndexByte(b, '.') + if first <= 0 { + return false + } + rest := b[first+1:] + second := bytes.IndexByte(rest, '.') + if second <= 0 { + return false + } + tail := rest[second+1:] + return len(tail) > 0 && bytes.IndexByte(tail, '.') < 0 }This needs
"bytes"in the import block.🤖 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 `@internal/oauth/oauth.go` around lines 60 - 75, Update IsJWT to validate exactly three dot-separated, non-empty segments rather than relying on dots == 2 and len(b) >= 5; use the proposed bytes-based segment check and add the required bytes import. Correct the surrounding comment and test the empty-header/payload case such as "..c" or "..abc" so the documented contract matches the routing behavior.internal/oauth/README.md (1)
18-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one line-break syntax in the flowchart labels.
Line 20 uses
\ninside the node label. Line 21 and Line 22 use<br>. Mermaid does not treat\nas a line break in node text across renderers, so the first node can render with a literal backslash-n on GitHub.📝 Proposed fix
- A[AUTH command arrives] --> B{Does the password\nlook like a JWT?} + A[AUTH command arrives] --> B{"Does the password<br>look like a JWT?"} B -- "no (oauth.IsJWT false)" --> C[Password path:<br>bcrypt hash vs policy user] B -- "yes (2 dots, >= 5 bytes)" --> D[Token path] D --> E{oauth provider\nconfigured?}Apply the same change to the
Enode on Line 23.🤖 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 `@internal/oauth/README.md` around lines 18 - 31, Standardize the Mermaid flowchart labels on the supported `<br>` line-break syntax: update the `A` and `B` node labels that currently use `\n`, and ensure the `E` node uses the same syntax. Preserve the existing label text and flowchart structure.internal/resp/oauth_test.go (1)
97-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the three-argument AUTH form.
internal/resp/server.go:840routes a token to the provider only whenlen(args) == 2. No test pins that rule. A regression that also acceptsAUTH <user> <token>would pass silently and would change the session identity source. Add a case that sends the valid token with a username and asserts the rejection.🤖 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 `@internal/resp/oauth_test.go` around lines 97 - 111, Extend TestRESPServer_OAuthDenyNoRoleMatch to cover the three-argument AUTH form by sending the valid token with a username and asserting it is rejected, then verify the connection remains unauthenticated. Keep the existing two-argument denial case unchanged and use the same token/session setup.internal/rbac/policy.go (1)
226-247: 🩺 Stability & Availability | 🔵 TrivialConsider surfacing the verification error to the caller.
ResolveOAuthTokendiscardserrfromverify. The listeners then log every failure asinvalid token. Theoauth.Providercontract distinguishesErrInvalidTokenfrom transient errors, such as an unreachable identity provider. Operators cannot separate a forged token from an IdP outage in the audit log or in metrics.Deny in both cases, but return the error so the callers can log or count it separately.
🤖 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 `@internal/rbac/policy.go` around lines 226 - 247, The ResolveOAuthToken signature currently discards verification failures; update it and its callers to return or propagate the verify error while still denying access. Preserve nil results for invalid claims or unavailable policy, and ensure listeners can distinguish ErrInvalidToken from transient identity-provider errors when logging or counting failures.
🤖 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 `@ARCHITECTURE.md`:
- Line 83: Add OAuth/OIDC to the “Opt-In Features” table in ARCHITECTURE.md with
a row documenting the --oauth-provider and --oauth-issuer options, and mark the
feature as disabled by default. Keep the existing internal/oauth component entry
unchanged.
In `@cmd/example/oauth/main.go`:
- Around line 101-105: The stdin token-reading flow around scanner.Scan must
configure Scanner.Buffer for the supported protocol message size, preserve
successful tokens up to that limit, and return scanner.Err() when scanning fails
instead of always returning os.ErrNotExist. Add a regression test covering a
valid id_token larger than the default 64 KiB scanner limit.
In `@cmd/example/oauth/policy.yaml`:
- Around line 26-30: Align cmd/example/oauth/policy.yaml:26-30 with the
documented identities by configuring an `@saxy.dev` admin rule and an
`@tellstone.io` readonly rule; update cmd/example/oauth/policy.yaml:7-16 to
describe those domains and roles, and update cmd/example/oauth/main.go:58-59 to
assert the role granted by the aligned policy.
In `@internal/network/client.go`:
- Around line 217-224: Add explicit math.MaxUint16 bounds before payloadLen
computation in the authentication method containing the shown password-only
frame in internal/network/client.go (lines 217-224), returning a clear error
when len(password) exceeds the limit; apply the same validation in
internal/network/client_role.go (lines 151-161) for both username and password
before computing payloadLen, preventing uint16 length-field wrapping.
In `@internal/network/server.go`:
- Around line 677-686: The OAuth token verification calls in both AUTH worker
pools use an unbounded context. In internal/network/server.go:677-686, update
the callback passed to ResolveOAuthToken to create and defer-cancel a
context.WithTimeout around s.oauth.Verify; apply the same bounded timeout value
and context lifecycle in internal/resp/server.go:1000-1009. Keep the existing
session and username resolution behavior unchanged.
- Around line 612-622: Update the JWT routing condition in the binary
authentication flow to dispatch OAuth tokens only when the supplied username is
empty, matching the RESP listener’s token-only form. Preserve the existing
non-empty-username authentication path and JWT error response behavior.
In `@internal/oauth/generic/generic_test.go`:
- Around line 131-141: Update signJWT to support HS256 by adding the required
HMAC implementation and produce a valid signature, so disallowed-algorithm
coverage reaches the allowedAlgs check. Also update the “wrong signature” test
to use kid "key-1" with a different private key, ensuring verification reaches
verifySignature instead of failing on unknown kid.
In `@internal/oauth/generic/generic.go`:
- Around line 115-125: Add a throttled, single-flight refresh helper near
refreshJWKS, using a mutex, timestamp, and in-flight state to enforce a minimum
refresh interval and collapse concurrent calls without new dependencies. Update
the unknown-key path in the token-validation method to call
refreshJWKSThrottled; return oauth.ErrInvalidToken when cooldown prevents
refresh, while preserving refreshJWKS errors and the existing post-refresh key
lookup behavior.
- Around line 316-333: Update validateClaims to require a valid exp claim:
reject tokens when raw["exp"] is absent or num cannot parse it, and retain the
existing expiration check for present claims. Add a regression case to
TestVerifyRejectsInvalidToken that signs a payload with exp removed and verifies
validation fails.
In `@internal/oauth/presets/google_test.go`:
- Around line 51-56: In the test setup that encodes the Google JWKS response,
update the QF1008-triggering references in the map construction to use the
promoted rsa.PrivateKey fields directly: replace priv.PublicKey.E and
priv.PublicKey.N with priv.E and priv.N. Run task fmt and task check afterward.
In `@internal/oauth/presets/google.go`:
- Around line 48-57: Update google.Verify so the hosted-domain value is added to
claims["groups"] only when no groups claim already exists; preserve the existing
groups value otherwise. Add a google preset test covering a token containing
both hd and groups, asserting the original groups remain unchanged.
In `@server/server.go`:
- Around line 229-237: Update Server.initOAuth to handle an empty
cfg.GetOAuthClientID before constructing oauth.Config: require the client ID
when OAuth token authentication is enabled, or emit an explicit startup warning
that audience validation is disabled if empty IDs are intentionally supported.
- Around line 238-256: In initOAuth, validate s.policy before entering the OAuth
provider switch, returning the existing --oauth-provider requires --rbac-config
error immediately when it is nil. Remove the later duplicate policy check while
preserving provider construction and error handling for valid configurations.
---
Nitpick comments:
In `@internal/oauth/generic/generic.go`:
- Around line 187-201: Update Provider.getJSON to wrap resp.Body with
io.LimitReader before passing it to json.Decoder, using a sufficiently bounded
limit for discovery and JWKS responses. Add the required io import and preserve
the existing request, status validation, and decoding behavior.
- Around line 142-156: In Provider.refreshDiscovery, validate doc.Issuer against
the configured issuer after decoding the discovery document and before accepting
doc.JWKSURI; normalize trailing slashes consistently with the request URL and
return an error on mismatch. Preserve the existing missing-jwks_uri check and
refreshJWKS flow for a matching issuer.
- Around line 244-256: Update internal/oauth/generic/generic.go:244-256 in
parseJWK to confirm the declared Go toolchain supports the newer APIs, then
left-pad decoded EC coordinates to 32 bytes and construct the P-256 key with
ecdsa.ParseUncompressedPublicKey using the uncompressed point format, preserving
decode errors and validating curve membership. Update
internal/oauth/generic/generic_test.go:98-110 in jwksFromKeys to derive
coordinates from pk.Bytes() and slice the uncompressed point instead of
accessing deprecated pk.X and pk.Y.
- Around line 126-129: Update splitToken to return the signing input as an
additional value, using the existing token subslice covering header.payload
without allocating. In Verify, consume that returned signing input and remove
the second strings.Split and concatenation before verifySignature. Add or update
a benchmark to measure the allocation change, and report actual results only if
the benchmark is run.
In `@internal/oauth/oauth.go`:
- Around line 60-75: Update IsJWT to validate exactly three dot-separated,
non-empty segments rather than relying on dots == 2 and len(b) >= 5; use the
proposed bytes-based segment check and add the required bytes import. Correct
the surrounding comment and test the empty-header/payload case such as "..c" or
"..abc" so the documented contract matches the routing behavior.
In `@internal/oauth/presets/google.go`:
- Around line 32-44: Exported preset constructors currently expose unexported
concrete return types. Update NewGoogle in
internal/oauth/presets/google.go:32-44 and NewStackit in
internal/oauth/presets/stackit.go:33-45 to return (oauth.Provider, error), while
keeping the google and stackit structs unexported and preserving their existing
construction logic.
In `@internal/oauth/README.md`:
- Around line 18-31: Standardize the Mermaid flowchart labels on the supported
`<br>` line-break syntax: update the `A` and `B` node labels that currently use
`\n`, and ensure the `E` node uses the same syntax. Preserve the existing label
text and flowchart structure.
In `@internal/oauth/registry.go`:
- Around line 79-88: Update the Providers doc comment to say “Providers returns”
and replace sort.Strings with slices.Sort, updating the import from sort to
slices accordingly.
In `@internal/rbac/policy.go`:
- Around line 226-247: The ResolveOAuthToken signature currently discards
verification failures; update it and its callers to return or propagate the
verify error while still denying access. Preserve nil results for invalid claims
or unavailable policy, and ensure listeners can distinguish ErrInvalidToken from
transient identity-provider errors when logging or counting failures.
In `@internal/resp/oauth_test.go`:
- Around line 97-111: Extend TestRESPServer_OAuthDenyNoRoleMatch to cover the
three-argument AUTH form by sending the valid token with a username and
asserting it is rejected, then verify the connection remains unauthenticated.
Keep the existing two-argument denial case unchanged and use the same
token/session setup.
🪄 Autofix
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 Plus
Run ID: d5f556e5-3183-4cfc-b986-a796b58c1da5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
ARCHITECTURE.mdREADME.mdROADMAP.mdcmd/example/client/main.gocmd/example/oauth/main.gocmd/example/oauth/policy.yamlcmd/tellstone/main.goconfig/config.gogo.modinternal/metrics/metrics_test.gointernal/network/acl_test.gointernal/network/benchmark_tls_test.gointernal/network/client.gointernal/network/client_role.gointernal/network/oauth_test.gointernal/network/protocol_test.gointernal/network/role_test.gointernal/network/server.gointernal/network/server_test.gointernal/oauth/README.mdinternal/oauth/generic/generic.gointernal/oauth/generic/generic_test.gointernal/oauth/oauth.gointernal/oauth/oauth_test.gointernal/oauth/presets/google.gointernal/oauth/presets/google_test.gointernal/oauth/presets/stackit.gointernal/oauth/presets/stackit_test.gointernal/oauth/registry.gointernal/oauth/registry_test.gointernal/rbac/config.gointernal/rbac/oauth_rules_test.gointernal/rbac/policy.gointernal/resp/acl_test.gointernal/resp/handshake_test.gointernal/resp/oauth_test.gointernal/resp/role_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/resp/starttls_test.goserver/server.go
…blilty Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/oauth/presets/google_test.go (1)
113-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStop after a scope-length mismatch.
If
cfg.Scopescontains fewer entries thanwant, Line 113 records the failure but the loop still indexescfg.Scopes[i]and panics. Uset.Fatalfor return after the length check so the test reports the assertion failure directly.💚 Proposed fix
if len(cfg.Scopes) != len(want) { - t.Errorf("Scopes = %v, want %v", cfg.Scopes, want) + t.Fatalf("Scopes = %v, want %v", cfg.Scopes, want) }🤖 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 `@internal/oauth/presets/google_test.go` around lines 113 - 118, Update the scope-length assertion in the test around cfg.Scopes and want to stop execution when their lengths differ, using t.Fatalf or an immediate return before the comparison loop. Preserve the per-scope equality checks for matching-length slices.
🤖 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 `@internal/network/server.go`:
- Around line 614-617: Update the JWT handling around dispatchOAuth so token
storage is obtained from a preallocated reusable OAuth job buffer only after
worker-queue admission succeeds, avoiding per-request allocation when the pool
is full. Propagate the buffer through the OAuth dispatch path and return it when
the job is rejected, preserving the existing AUTHERR behavior.
---
Outside diff comments:
In `@internal/oauth/presets/google_test.go`:
- Around line 113-118: Update the scope-length assertion in the test around
cfg.Scopes and want to stop execution when their lengths differ, using t.Fatalf
or an immediate return before the comparison loop. Preserve the per-scope
equality checks for matching-length slices.
🪄 Autofix
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 Plus
Run ID: 100e613f-b73c-4e12-8b83-3e9b7bb8d410
📒 Files selected for processing (7)
internal/network/client.gointernal/network/client_role.gointernal/network/oauth_test.gointernal/network/server.gointernal/oauth/presets/google.gointernal/oauth/presets/google_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/oauth/presets/google.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/network/server.go (2)
49-52: 🚀 Performance & Scalability | 🟠 MajorMake pooled token ownership explicit and allocation-free.
buf.Bytes()passes a non-owning slice to the asynchronous job. TheauthJobchange does not show ownership of the*bytes.Buffer. Reusing the buffer beforeVerifyfinishes can overwritejob.password. Never releasing it after an accepted job also defeats pooling.Carry the buffer owner through
authJob. Release it exactly once after verification and on queue rejection. This path also usesmake,interface{}boxing, and a growingbytes.Buffer, so it can allocate.Verification commands
#!/bin/bash set -euo pipefail rg -n -C 8 'tokenPool|dispatchOAuth|type authJob|job\.password|oauth\.Verify' internal/network/server.go rg -n 'AllocsPerRun|AllocsPerOp|Benchmark.*(Auth|OAuth)' internal/network -g '*_test.go'As per coding guidelines, the
internal/networkrequest-handling hot path must not allocate, usemake(), or useinterface{}boxing, and allocation behavior must be verified with benchmarks. The previous review also required releasing the reusable buffer on rejected dispatches.Also applies to: 592-597, 622-625, 685-694, 777-793
🤖 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 `@internal/network/server.go` around lines 49 - 52, Update authJob and the OAuth dispatch/verification flow around dispatchOAuth and oauth.Verify to carry ownership of the pooled *bytes.Buffer alongside the non-owning token slice. Release that buffer exactly once after verification, including every accepted-job completion path and queue rejection, and ensure no reuse occurs before verification finishes. Remove hot-path make calls, interface{} boxing, and growing-buffer allocations, then add or update internal/network benchmarks to verify zero allocations.Source: Coding guidelines
179-182: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject OAuth configuration without an RBAC policy store.
NewServeracceptsoauth != nilwithpolicy == nil,handleAuthMessagebypasses authentication when bothrequirePassHashandpolicyare nil, startup leaves connections authenticated withpolicy == nil, and the OAuth worker path dereferencess.policy. Either reject provider-only startup, includes.oauth == nilin the no-auth fast path, and update startup auth state together, and add a regression test for provider-only configuration.🤖 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 `@internal/network/server.go` around lines 179 - 182, Update NewServer’s OAuth/RBAC validation to reject any provider-only configuration where provider is non-nil and policy is nil, preventing startup with an OAuth worker that cannot access s.policy. Preserve valid password-only, policy-only, and combined configurations, and add a regression test covering provider-only startup rejection.
🤖 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.
Outside diff comments:
In `@internal/network/server.go`:
- Around line 49-52: Update authJob and the OAuth dispatch/verification flow
around dispatchOAuth and oauth.Verify to carry ownership of the pooled
*bytes.Buffer alongside the non-owning token slice. Release that buffer exactly
once after verification, including every accepted-job completion path and queue
rejection, and ensure no reuse occurs before verification finishes. Remove
hot-path make calls, interface{} boxing, and growing-buffer allocations, then
add or update internal/network benchmarks to verify zero allocations.
- Around line 179-182: Update NewServer’s OAuth/RBAC validation to reject any
provider-only configuration where provider is non-nil and policy is nil,
preventing startup with an OAuth worker that cannot access s.policy. Preserve
valid password-only, policy-only, and combined configurations, and add a
regression test covering provider-only startup rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4efb4cd1-3ee4-45ff-a1cd-830f62c5b8f1
📒 Files selected for processing (2)
ARCHITECTURE.mdinternal/network/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ARCHITECTURE.md
…le-flight Signed-off-by: Maximilian Hagen <git@saxy.dev>
1 participant
Description
Component:
Authentication
Type of Change:
Related Issue
Closes #10
Closes #34
Technical Deep Dive & Context
Connection-time OIDC bearer-token authentication: a client presents a Google/STACKIT/any-issuer
id_token(signed JWT) as itsAUTHcredential; its claims are mapped to an RBAC role via the policy'soauth.rulesand pinned to the connection exactly like a password session.Provider contract (
internal/oauth/oauth.go):Provider.Verify(ctx, token) (Claims, error)withClaims = map[string][]string(JWT claims may repeat or be lists; never flattened). Two error classes —ErrInvalidTokenfor bad credentials (callers find it viaerrors.Is), unwrapped transient errors so "IdP down" is distinguishable from "bad credential". No secrets inConfig: verification is signature + issuer + audience against the public JWKS, so no client secret is needed server-side.Generic OIDC provider (
internal/oauth/generic, stdlib-only, fail-fast): synchronous discovery + JWKS fetch inNew(wrong issuer/IdP fails at startup, not firstAUTH); algorithm allowlist RS256/ES256 only (HS* rejected to prevent algorithm-confusion);kidrequired; exactly one JWKS refresh on unknownkid(rotation without restarts);exp/nbf/iss/audenforced; JWKS cache behind anRWMutex, whole-set replacement on refresh. Presetsgoogle/stackitare thin wrappers — Google maps thehdclaim intogroupssoclaim: groups, match: "*@example.com"works one-line. Registry (internal/oauth/registry.go) is first-wins, read-only after startup, lock-free reads.RBAC integration (
internal/rbac/policy.go):oauth.rulescompiled at policy load (unknown target role fails the load; a middle*wildcard is rejected).Store.ResolveOAuthTokenis a closure sorbacstays independent of theoauthpackage. First-match-wins; no match or verify failure = no role = deny-all (fail-closed). Session subject = tokensub(falls back to"default"); audit events andACL LOGaccount failures identically to the password path. SIGHUP hot-reload never re-evaluates pinned sessions.Hot-path allocation (binary client,
client.go/client_role.go): realid_tokens routinely exceed the 512-byte stack buffer used byAuth/AuthUser. The stack path stays allocation-free; payloads that overflow fall back to a one-time heap buffer. Before this, >512-byte credentials were rejected withErrRequestTooLarge— the bundled client could not authenticate over the binary protocol at all.Startup config (
config/config.go,server/server.go):--oauth-provider(google|stackit, empty → generic),--oauth-issuer,--oauth-client-id, each with aTSD_*env fallback. No flags → providernil→ zero behavior change and zero overhead. Token auth without--rbac-configis a startup error (a token can only reach a role throughoauth.rules).Example (
cmd/example/oauth): a client that presents anid_token(from-token/-token-file/TSD_ID_TOKEN/stdin), AUTHs, issues SET/GET, and proves a forged token is denied.Performance & Benchmarks
Workload: N/A — no measurable change to the data path.
How Has This Been Tested?
go test ./...— all packages pass (internal/oauth,internal/oauth/generic,internal/oauth/presets,internal/rbac,internal/respoauth suites included).go vet ./...— clean.AUTH <google id_token>→+OK; SET/GET succeed; forged token →-ERR invalid password; session reset to-NOAUTHafter failed AUTH.INVALID_AUTH; AUTH →MsgAuthOk; SET/GET OK; forged token denied; large-token path exercised byTestClientOAuthLargeToken(700-byte JWT-shaped credential throughclient.Auth).kid→ JWKS refresh once then reject; missingkidrejected; HS256 token rejected; expired/not-yet-valid tokens rejected; multi-valueaudaccepted;expabsent allowed; policy with*@domainprefix-glob,*@domainsuffix-glob, exact, and bare*matches; first-match-wins ordering; fail-closed when no rule matches; middle-*pattern rejected at load; unknown target role fails load;--oauth-providerunknown name → startup error; token auth without--rbac-config→ startup error.cmd/example/oauthagainst a live server with real Google token (AUTH OK, SET OK, GET value, forged deniedINVALID_AUTH).Checklist
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
New Features
Bug Fixes
Documentation