feat(#7821): let a listed trusted proxy vouch for the gRPC CreateApiToken transport - #8891

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant GrpcServerPlugin
  participant GrpcTransportSecurityInterceptor
  participant ArcadeDbGrpcAdminService
  GrpcServerPlugin->>GrpcTransportSecurityInterceptor: Install interceptor with server configuration
  GrpcTransportSecurityInterceptor->>GrpcTransportSecurityInterceptor: Check peer address and x-forwarded-proto metadata
  GrpcTransportSecurityInterceptor->>ArcadeDbGrpcAdminService: Publish transport-safety result in gRPC context
  ArcadeDbGrpcAdminService->>ArcadeDbGrpcAdminService: Reject token creation if transport is unsafe

Merge Risk: ⚪ Minimal · up to 62c6b

No actionable merge-blocking issue is established. Trusted-proxy support preserves the existing TLS and loopback paths and requires trusted peers with HTTPS-only forwarded metadata; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 62c6b

The change adds an explicit proxy trust path while preserving authentication, administrator authorization, and fail-closed transport checks. Its security depends on accurate proxy headers and a protected proxy-to-server network. No introduced authorization bypass was established, but deployment and concurrent revocation coverage remain incomplete.

Retained concerns
No architecture-level concerns identified.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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 help to get the list of available commands.}

Review of #8891

Overall this is a clean, well-scoped change. The security shape is right: the metadata is read only when the direct peer is on the list, the default (empty list) leaves it unread, an unparseable list fails closed, every x-forwarded-proto value must be https, and TLS/loopback can never be narrowed. Reusing IPAddressBlocklist and the HTTP-side forwardedProtoIsFullyEncrypted avoids a second matcher. I found no blocking issues.

Suggestions (non-blocking)

1. Coupling a protocol module to an HTTP handler class. grpcw now imports PostApiTokenHandler just for two pure static helpers, and the PR widens them to public on a handler class. It works (server is provided), but the gRPC trust decision now lives in an HTTP handler. Moving parseTrustedProxies / forwardedProtoIsFullyEncrypted into a small neutral utility (e.g. next to IPAddressBlocklist or in com.arcadedb.server.security) would keep "one trust boundary" structural rather than by convention. Fine as a follow-up.

2. requireTransportSafeForSecrets became static package-private only to be testable, and the test only asserts the message text. The actual gate (key absent or false -> FAILED_PRECONDITION, key true -> returns) is not pinned in the unit test. A short assertion for the key-true and key-unset cases would lock in the fail-closed behaviour its javadoc describes.

3. Hot-path cost. The interceptor now calls configuration.getValueAsString(...) on every cleartext, non-loopback call for any RPC, not only CreateApiToken. It is a map lookup plus a String.equals against the cached text, so cheap, but worth knowing given the per-call overhead emphasis in CLAUDE.md. Probably not worth restricting to the mint method.

4. Test gaps worth a tiny addition: an IPv6 / IPv4-mapped peer (::ffff:10.0.0.5 against an entry 10.0.0.5), since parseTrustedProxies documents that as intended and it is now also reachable via gRPC; and a whitespace-padded value (" https ") through the gRPC path.

5. Style nits.

   • ServerControlPlane.java: the edited javadoc line runs well past the wrap used by the surrounding paragraph; please re-wrap.
   • ArcadeDbGrpcAdminService.requireTransportSafeForSecrets: the comment sits between an un-braced if and its throw; moving it above the if is easier to scan.
   • GrpcTransportSecurityInterceptorTest: the extra blank line between the com.arcadedb and io.grpc import groups differs from the rest of the file.

Operational note (docs)

The scheme relies on the proxy listed being the address ArcadeDB sees as the direct peer (not the client). Envoy overwrites x-forwarded-proto, and nginx grpc_set_header replaces it too; the "every value must be https" rule additionally covers appending proxies. A short note in the user docs would save operators a support round trip.

Test coverage

Good: 12 new unit tests covering the forgery case, CIDR, live setting change, throwing supplier and the forConfiguration factory, plus the mutation check. A real non-loopback listed proxy against a live server is not reachable from a test, which is understandable.

LGTM modulo the nits above.

Review of #8891 (gRPC CreateApiToken behind a trusted proxy)

Overall this is a clean, well-scoped change. The trust decision is the same as the HTTP gate from #7804, the shared helpers avoid a second CIDR/https parser, the default (empty list) leaves the metadata unread, and failure modes all fail closed (unparseable list, throwing supplier, null/unresolved peer, non-IP transport). I found no blocking issues.

What I checked

• Order of checks in isSafePeer: loopback first, then list membership, and the metadata is read only for a listed peer. A forging unlisted client gains nothing (anUnlistedPeerCannotVouchForItself).
• Every value must be https: String.join(",", headers.getAll(...)) fed into forwardedProtoIsFullyEncrypted keeps the -1 split limit, so https, and appended https,http are refused.
• Parse cache (currentTrustedProxies): keyed on the setting text, so a live SET SERVER SETTING is honoured. An unparseable text is cached as empty, so the SEVERE log fires once per distinct text, not per call. The volatile record swap with a benign race is fine.
• Module layout: grpcw -> server stays provided; ApiTokenTrustedProxies lives in server, so no protocol-module cross dependency is introduced.

Suggestions (non-blocking)

1. PR description does not match the code. It says PostApiTokenHandler.parseTrustedProxies and forwardedProtoIsFullyEncrypted "became public", but in the diff they are still package-private one-line wrappers delegating to ApiTokenTrustedProxies, and the interceptor calls ApiTokenTrustedProxies directly (which is the better design). Either drop the wrappers and point their callers (presumably the Issue7804* tests) at ApiTokenTrustedProxies, or fix the description. Pass-through wrappers kept only for tests are a small indirection that could go.
2. No direct unit test for ApiTokenTrustedProxies. It is now a public shared class, but is only covered indirectly through the HTTP wrappers and the interceptor. A tiny ApiTokenTrustedProxiesTest (null/blank, https,, https,,https, hostname entry returns empty and does not throw) would pin the contract independent of both callers, and would let you delete the wrappers in item 1 without losing coverage.
3. Document the L4 caveat on the setting. The gate trusts x-forwarded-proto from a listed peer. If an operator lists a proxy that works at L4 (nginx stream, HAProxy mode tcp), the client's own forged x-forwarded-proto: https metadata passes through untouched and vouches for a cleartext leg. This is identical to the HTTP behaviour, so it is not a regression, but the new gRPC wording could add one sentence saying only an L7 proxy that sets/overwrites the header should be listed. gRPC deployments use L4 balancers more commonly than HTTP ones, so it is a likelier mistake here.
4. requireTransportSafeForSecrets visibility. It went from private to package-private static purely so the test can call it. That is acceptable (it only reads the Context key), but a short note would make the intent clear.
5. Observability (optional). The HTTP path logs an unprotected mint at WARNING. Consider a FINE/INFO log when the gRPC gate accepts a call only because a listed proxy vouched for it, so an operator can tell loopback, TLS and proxy apart.
6. Test coverage gap (minor). All 12 new tests drive the interceptor with synthetic Attributes; no test sends a real cleartext gRPC call from a non-loopback peer through a listed proxy to CreateApiToken. That is hard to arrange in CI, so I am fine with it, but Issue7309GrpcSecurityControlPlaneIT (loopback) does not exercise the new branch.

Style nits

• Style matches the repo (final params, brace-less single statements, assertThat(...) form, no new dependencies, imports not fully qualified).
• The comment blocks added in GrpcServerPlugin and requireTransportSafeForSecrets are long and reference issue numbers; CLAUDE.md prefers short WHY-only comments. The interceptor Javadoc already carries the rationale.

Nice work on the mutation check and the live-setting tests. Fine to merge from my side once the description/wrapper mismatch in item 1 is settled.

🤖 Generated with Claude Code

Review of #8891 (gRPC CreateApiToken trusted-proxy vouching)

Overall this is a clean, well-scoped change. I found no blockers.

What I checked

• Trust boundary parity with HTTP: the interceptor reads x-forwarded-proto only when the direct peer is in the list, and requires every value to be https (Metadata.getAll joined with , goes through the same forwardedProtoIsFullyEncrypted as HTTP, so the trailing-comma and appended-value cases are covered). TLS and loopback are never narrowed by the list. With the default empty list the metadata is never read.
• Fail-closed paths: unresolved or non-IP peer, null or throwing supplier, blank setting, unparseable list (parses to empty), null headers. All deny.
• Shared code extraction: ApiTokenTrustedProxies.parse and forwardedProtoIsFullyEncrypted are moved verbatim. LogManager, Level and IPAddressBlocklist are all still used in PostApiTokenHandler, so no dangling imports.
• Module scoping: grpcw -> server stays provided, so the new import of com.arcadedb.server.security.ApiTokenTrustedProxies respects the wire-protocol dependency rule.
• Per-call cost: the setting lookup only happens for remote cleartext peers, and the parse is cached by setting text, so a bad list logs SEVERE once per change rather than per call. The cache holds an immutable record in a volatile field, so the documented race is benign.
• GrpcServerPlugin change: the unit tests that go through configureServer (Issue5050, Issue6756, Issue7035) stub mockServer.getConfiguration(), so the new Objects.requireNonNull in forConfiguration will not NPE there.

Suggestions (all minor, non-blocking)

1. Which configuration object: configureServer(serverBuilder, config) already receives a ContextConfiguration, but the interceptor is built from arcadeServer.getConfiguration(). SET SERVER SETTING writes to the server's configuration, so this is right in production, but a short comment saying why config is not used would save the next reader the question.
2. Bad-list diagnostics are lazy: the SEVERE log for an unparseable list is emitted on the first remote cleartext call rather than at startup or on SET SERVER SETTING. Acceptable (HTTP behaves the same), but an operator with a typo only learns about it after a mint is refused. Just noting.
3. Delegates in PostApiTokenHandler: parseTrustedProxies / forwardedProtoIsFullyEncrypted are now one-line pass-throughs kept only for existing tests. Consider pointing those tests at ApiTokenTrustedProxies and deleting the delegates in a follow-up, to avoid two entry points to one rule.
4. Registration reachability test: theServerFactoryReadsTheTrustedProxySettingLive proves the factory works, but nothing asserts that GrpcServerPlugin.configureServer actually registers the forConfiguration interceptor rather than new GrpcTransportSecurityInterceptor() (which would silently ignore the list). Issue7309GrpcSecurityControlPlaneIT only exercises loopback, which passes either way. A small test that pins that wiring would close the gap.
5. Comment volume: per CLAUDE.md the default is minimal comments. The new Javadoc and in-method comments are long. Most of it is genuinely about non-obvious trust decisions, so keep the security rationale and trim restatements of what the code does.

Test coverage

Good: 14 new interceptor tests covering true/false/unset gate, CIDR, forged key from an unlisted peer, IPv4-mapped peer, padded and appended values, live setting change, and a throwing supplier, plus a direct ApiTokenTrustedProxiesTest. Assertion style follows the project convention. The only gap is item 4.

Docs / config

The setting description now includes the L4-balancer warning, the most important operational caveat for this feature. The refusal message names the setting and the metadata key, which is helpful.

LGTM with the optional suggestions above.

Review of #8891 (trusted proxy vouching for gRPC CreateApiToken)

No blocking issues. The trust model matches the HTTP gate from #7804 and is fail-closed throughout.

What I checked

• Default unchanged. With the list empty the metadata is never read, and the decision stays TLS-or-loopback. TLS and loopback are evaluated first, so a proxy list cannot narrow them.
• Forgery resistance. The x-forwarded-proto metadata is read only when the direct peer is in the list. Every value must be https, using String.join(",", headers.getAll(...)) and then the shared split(",", -1) check, so appended or trailing-empty values (https,, https,http) are refused.
• Failure modes. A throwing supplier, a blank or null setting, an unparseable list, a null or unresolved peer, and a non-IP transport all resolve to "not safe".
• Concurrency and cost. The volatile record cache keyed by the setting text is correct, and the benign double-parse race is fine. The hot-path cost on cleartext remote calls is one config lookup plus String.equals. An unparseable list is parsed once per text change, so the SEVERE log does not repeat per call.
• Reuse. Moving parse and forwardedProtoIsFullyEncrypted into ApiTokenTrustedProxies removes duplication, and the package-private delegates in PostApiTokenHandler keep existing tests intact.

Suggestions (non-blocking)

1. Unused imports in PostApiTokenHandler. After parseTrustedProxies lost its LogManager/Level usage, please confirm LogManager, Level and (if now only the return type) IPAddressBlocklist are still used elsewhere in the file. If not, drop them. I could not verify this from the diff alone.
2. Envoy wording in the setting description. "Envoy adds it on its own" is true only when Envoy is the edge (use_remote_address: true, which overwrites the header). Envoy behind another hop, with use_remote_address: false, preserves an inbound value. Since the description already warns about L4 pass-through, a short "and does not trust a client-supplied value" qualifier would keep that guidance consistent.
3. Module coupling. grpcw now calls a class in arcadedb-server (provided scope), which is compliant with CLAUDE.md. Because it is provided, ApiTokenTrustedProxies must stay binary compatible with the server in any distribution that mixes versions. The Modular Builder is a likely place for that to show up, so it is worth a quick check that a grpcw-only distribution always includes the server module.
4. Operator visibility (optional). The interceptor runs for every RPC, so you rightly skipped a per-call log. A single DEBUG/FINE log only when CreateApiToken is accepted via a proxy (inside requireTransportSafeForSecrets would need a second context key) would help audit trails. A follow-up is fine.

Tests

Coverage is thorough: the shared helper has its own test, the interceptor tests cover true/false/unset, CIDR, IPv4-mapped, padded and appended values, live setting change and the factory, and a mutation check was done. The Issue7309GrpcSecurityControlPlaneIT run confirms loopback mints still work. I did not rerun the tests locally. The CLAUDE.md conventions (assertThat(...), final, no FQNs, no System.out) look followed in the diff.

Nice work; good to merge once the CI checks are green and CodeRabbit threads are resolved.

Codecov Report

❌ Patch coverage is 98.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.38%. Comparing base (6c2a724) to head (62c6b99).

@@            Coverage Diff            @@
##               main    #8891   +/-   ##
=========================================
  Coverage     76.37%   76.38%           
- Complexity     3800     3802    +2     
=========================================
  Files          2104     2105    +1     
  Lines        206367   206401   +34     
  Branches      43491    43497    +6     
=========================================
+ Hits         157620   157659   +39     
+ Misses        31762    31760    -2     
+ Partials      16985    16982    -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.