feat(connectivity): protect connectivity-api with the stack auth issuer - #522
feat(connectivity): protect connectivity-api with the stack auth issuer#522Dav-14 wants to merge 8 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
✅ Approve — automated reviewNo actionable defects remain in the current diff. The active prior route-revocation concern is addressed at HEAD, and the earlier findings remain resolved. No findings. |
24b0a89 to
a1c84af
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #522 (comment)
gfyrag
left a comment
There was a problem hiding this comment.
Reviewed against its canonical stacked base, #521, rather than against main.
The security need is valid and important: once a Stack has Auth, Connectivity must not remain the only unauthenticated module API. Reusing auths.GetProtectedConfiguration, explicitly writing checkScopes=false to override the delegated CRD default, and pruning stale spec.api.auth when Auth is absent are the right mechanics.
I confirm the existing unresolved blocker: the reconciler reads the Auth dependency but does not watch it. Adding or removing Auth on an existing Stack therefore does not enqueue Connectivity, so the API may remain unauthenticated after Auth is enabled (the exact exposure this PR intends to close), or retain stale auth after removal. Please add WithWatchDependency[*v1beta1.Connectivity](&v1beta1.Auth{}) and an event-level regression test; direct calls to Reconcile do not prove controller convergence.
There is also a scope/need mismatch at the current head: the second commit adds logging.json propagation and substantially rewrites monitoring composition, but the PR title/problem/fix sections describe only API authentication. Is JSON logging required to deliver this security fix? If not, split it into its own PR. If it is intentionally stacked here, update the PR contract and test plan so reviewers and release notes expose both behavior changes.
Non-blocking documentation point: the new page says Connectivity is protected “like the other stack modules,” while the implementation intentionally cannot propagate additional auth.issuers. Please document that single-issuer limitation so operators know tokens accepted by Ledger/Payments may still be rejected by Connectivity.
The focused Connectivity package tests and GitHub CI pass. I did not duplicate the existing inline Auth-watch thread.
a1c84af to
3e4f6d7
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #522 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).
Summary: #522 (comment)
gfyrag
left a comment
There was a problem hiding this comment.
Follow-up review on ebdd182, compared against the canonical stacked base (#521) and only against the previously reviewed a1c84af series.
The range-diff shows the original auth and JSON-logging commits are unchanged. The two new commits address the convergence blockers correctly:
- Auth changes are now watched through
WithWatchDependency[*v1beta1.Connectivity](&v1beta1.Auth{}). - Gateway changes are also watched, which is necessary because
GetProtectedConfigurationderives the issuer from the Gateway URL. - Both additions use the shared stack-dependent event mapper, so create/update/delete events reach the matching Connectivity resource. The new handler-level tests prove the stack mapping and enqueue target.
Verification:
go test ./internal/resources/connectivities -count=1passes locally, including both new watch tests and the auth/logging reconciliation tests.- GitHub's Tests job is green.
- The failing E2E jobs (Kubernetes 1.31, 1.34 and 1.35) stop while installing Chainsaw because
sum.golang.orgreturns HTTP/2 stream errors; no E2E scenario executes. Kubernetes 1.32 and 1.33 pass.
Need check: the security need is now technically covered end-to-end—adding/removing Auth and changing the Gateway issuer will converge without waiting for an unrelated event. I am leaving this as COMMENT rather than APPROVE because the previously recorded scope question remains unanswered: does this security PR actually need to ship the independent JSON-logging behavior too? Please resolve that existing review point by either separating it or making it part of the PR's stated contract and test plan. I have not duplicated the resolved Auth/Gateway threads.
ebdd182 to
8f99c76
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #522 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #522 (comment)
8a7c84a to
180dcce
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #522 (comment)
180dcce to
5746f0c
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #522 (comment)
gfyrag
left a comment
There was a problem hiding this comment.
Follow-up review on the current head. Two new blockers were reproduced locally; the existing Auth/Gateway/transient-gate findings remain resolved.
5746f0c to
6c5a82a
Compare
gfyrag
left a comment
There was a problem hiding this comment.
Re-review of the new fail-closed head found one blocking regression.
When the stack has an Auth module, wire spec.api.auth on the delegated Connectivity resource through auths.GetProtectedConfiguration, mirroring how ledger and payments self-verify bearer tokens: issuer from the stack auth URL, scope checking driven by the auth.connectivity.check-scopes Setting. Without an Auth module the api.auth block is cleared and the API keeps running unauthenticated.
c85ca6e to
f238653
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #522 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).
Summary: #522 (comment)
Problem
The
connectivity-apicompanion is exposed through the stack gateway at/api/connectivity, but nothing wires its OIDC protection: the module reconciler never setsspec.api.authon the delegatedconnectivity.formance.com/Connectivity, so the connectivity operator renders the API pod withAUTH_ENABLED=false. The gateway does not enforce auth per route either (theSecuredrule flag is not rendered in the Caddyfile), so on a stack with an Auth module — where ledger and payments self-verify bearer tokens — connectivity-api is the only module API reachable without a token.Fix
connectivities.Reconcilenow resolves the module's protection through the sharedauths.GetProtectedConfiguration(the same machinery behind ledger/paymentsAUTH_ENABLED/AUTH_ISSUER):spec.api.auth.issueris set to the stack auth issuer andspec.api.auth.checkScopesfollows theauth.connectivity.check-scopesSetting (platform default: false);spec.api.authis removed and the API keeps running unauthenticated (behind-VPN / in-cluster deployments);APIAuthResolveFailedcondition.checkScopesis always set explicitly because the connectivity CRD defaults it to true while the platform convention defaults to false. Auth and Gateway changes are watched so issuer changes converge immediately. After a resolved non-v3 Ledger hard gate has been handled, Auth is synchronized to an existing delegated Connectivity before transient prerequisite gates, keeping an already-running API protected while its Ledger is temporarily unready. The early patch validates the delegated resource's controller ownership before changing it.The connectivity CRD models a single trusted issuer, so additional
auth.issuersSettings entries are not propagated. The module documentation calls out that limitation.Also adds
docs/04-Modules/12-Connectivity.md(the module had no page), covering the module basics and the auth behavior.Tests
internal/resources/connectivities/init_test.go:checkScopes: falsewired when the stack has an Auth module (written first, watched fail);checkScopes: truewhen theauth.connectivity.check-scopesSetting is enabled (written first, watched fail);spec.api.authcleared when the stack has no Auth module (written first, watched fail);APIAuthResolveFailedcondition when the Auth lookup fails;just pre-commit(tidy, lint, generate, manifests, helm validate, docs, settings catalog) — no generated drift.Stack created with GitHub Stacks CLI • Give Feedback 💬