Uh oh!
There was an error while loading. Please reload this page.
feat: add tls and api key authentication support - #1
Merged
Conversation
Add TLS (withTlsCaCert, withTlsClientCert) and API key auth (withApiKey) builder methods to FilaClient. Uses grpc-java TlsChannelCredentials for TLS and a ClientInterceptor for Bearer token auth. Update admin.proto with API key and ACL management RPCs. All options are optional and backward compatible — plaintext without auth remains the default.
There was a problem hiding this comment.
7 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:54">
P2: Use try-with-resources in the new client-construction examples so copied code closes `FilaClient` correctly.</violation>
</file>
<file name="src/main/java/dev/faisca/fila/FilaClient.java">
<violation number="1" location="src/main/java/dev/faisca/fila/FilaClient.java:273">
P1: Setting client cert/key without a CA cert silently falls back to a plaintext connection, discarding the mTLS configuration with no error. Add a validation check at the start of `build()` to fail fast when `clientCertPem` is set but `caCertPem` is null.</violation>
<violation number="2" location="src/main/java/dev/faisca/fila/FilaClient.java:308">
P2: `parseHost`/`parsePort` break on IPv6 addresses. `lastIndexOf(':')` finds a colon inside the IPv6 address itself, corrupting both the host and port. Consider using `URI` parsing or at minimum handling the `[host]:port` bracket convention.</violation>
</file>
<file name="src/test/java/dev/faisca/fila/TlsAuthClientTest.java">
<violation number="1" location="src/test/java/dev/faisca/fila/TlsAuthClientTest.java:45">
P2: `connectWithTlsOnly` still sets an API key, so it does not validate TLS-only behavior and can miss regressions in no-auth TLS handling.</violation>
<violation number="2" location="src/test/java/dev/faisca/fila/TlsAuthClientTest.java:95">
P2: `rejectWithoutApiKey` should assert `UNAUTHENTICATED`, not just any `RpcException`, to ensure auth rejection is what is being tested.</violation>
</file>
<file name="proto/fila/v1/admin.proto">
<violation number="1" location="proto/fila/v1/admin.proto:15">
P1: Unauthenticated `CreateApiKey` can create superadmin keys. Since this RPC bypasses auth (per the comment), any network-reachable client can call `CreateApiKey(is_superadmin=true)` and obtain a key that bypasses all ACL checks. Consider restricting the unauthenticated bootstrap path—e.g., only allow it when no keys exist yet, or disallow `is_superadmin` on unauthenticated calls—so that an open bootstrap endpoint cannot be exploited post-setup.</violation>
</file>
<file name="src/test/java/dev/faisca/fila/TestServer.java">
<violation number="1" location="src/test/java/dev/faisca/fila/TestServer.java:115">
P2: Binary availability check does not handle executables on PATH, causing TLS/auth integration tests to be skipped incorrectly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Uh oh!
There was an error while loading. Please reload this page.
| rpc Redrive(RedriveRequest) returns (RedriveResponse); | ||
| rpc ListQueues(ListQueuesRequest) returns (ListQueuesResponse); | ||
| // API key management. CreateApiKey bypasses auth (bootstrap); others require a valid key. |
There was a problem hiding this comment.
P1: Unauthenticated CreateApiKey can create superadmin keys. Since this RPC bypasses auth (per the comment), any network-reachable client can call CreateApiKey(is_superadmin=true) and obtain a key that bypasses all ACL checks. Consider restricting the unauthenticated bootstrap path—e.g., only allow it when no keys exist yet, or disallow is_superadmin on unauthenticated calls—so that an open bootstrap endpoint cannot be exploited post-setup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At proto/fila/v1/admin.proto, line 15:
<comment>Unauthenticated `CreateApiKey` can create superadmin keys. Since this RPC bypasses auth (per the comment), any network-reachable client can call `CreateApiKey(is_superadmin=true)` and obtain a key that bypasses all ACL checks. Consider restricting the unauthenticated bootstrap path—e.g., only allow it when no keys exist yet, or disallow `is_superadmin` on unauthenticated calls—so that an open bootstrap endpoint cannot be exploited post-setup.</comment>
<file context>
@@ -11,6 +11,15 @@ service FilaAdmin {
rpc Redrive(RedriveRequest) returns (RedriveResponse);
rpc ListQueues(ListQueuesRequest) returns (ListQueuesResponse);
+
+ // API key management. CreateApiKey bypasses auth (bootstrap); others require a valid key.
+ rpc CreateApiKey(CreateApiKeyRequest) returns (CreateApiKeyResponse);
+ rpc RevokeApiKey(RevokeApiKeyRequest) returns (RevokeApiKeyResponse);
</file context>
MemberAuthor
There was a problem hiding this comment.
This is a server-side authorization concern, not a client SDK issue. The Fila server handles this through the bootstrap_apikey mechanism: when auth is enabled, CreateApiKey requires a valid bootstrap key (configured via config or env var). The proto definition must match the server's proto — changing it here would break compatibility. The SDK is just a client; it doesn't make authorization decisions.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- catch IllegalArgumentException from TLS trustManager for invalid certs (fixes CI build failure) - validate client cert requires CA cert — fail fast instead of silent plaintext fallback - handle IPv6 bracket notation in address parsing - fix connectWithTlsOnly test to actually test without API key - assert UNAUTHENTICATED status code in rejectWithoutApiKey test - handle PATH-based binary lookup in TestServer.isBinaryAvailable - use try-with-resources in README examples
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/test/java/dev/faisca/fila/TestServer.java">
<violation number="1" location="src/test/java/dev/faisca/fila/TestServer.java:122">
P2: Binary detection for PATH commands is platform-dependent because it hard-codes `which`; environments without `which` will incorrectly report the server binary as unavailable.</violation>
</file>
<file name="src/test/java/dev/faisca/fila/TlsAuthClientTest.java">
<violation number="1" location="src/test/java/dev/faisca/fila/TlsAuthClientTest.java:84">
P2: `connectWithTlsOnly()` asserts only exception type, so it can pass on non-auth failures. Assert `UNAUTHENTICATED` to verify the intended behavior.</violation>
</file>
<file name="src/main/java/dev/faisca/fila/FilaClient.java">
<violation number="1" location="src/main/java/dev/faisca/fila/FilaClient.java:297">
P2: This catch is too broad: malformed address/port errors are misreported as "invalid certificate" because `NumberFormatException` is also an `IllegalArgumentException`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- move parseHost/parsePort before tls try block so NumberFormatException from malformed addresses is not misreported as "invalid certificate" - assert UNAUTHENTICATED status code in connectWithTlsOnly test instead of only checking exception type
Add withTls() builder method that enables TLS using the JVM's default trust store (cacerts), so users with servers using public CA certs don't need to pass caCert manually. withTlsCaCert() now implies withTls(). Client cert validation updated to require either method.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/java/dev/faisca/fila/FilaClient.java">
<violation number="1" location="src/main/java/dev/faisca/fila/FilaClient.java:288">
P2: Changing this validation from `FilaException` to `IllegalStateException` is a breaking behavior change for callers that consistently handle SDK errors via `FilaException`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Uh oh!
There was an error while loading. Please reload this page.
Cubic identified that changing the exception type from FilaException to IllegalStateException is a breaking change for callers that catch FilaException consistently. Reverted to FilaException.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add TLS and API key authentication support to the Java SDK, achieving feature parity with the Rust SDK (Epic 15).
withTlsCaCert()for server-side TLS verificationwithTlsClientCert()for mTLSwithApiKey()for Bearer token auth viaApiKeyInterceptorTest plan
🤖 Generated with Claude Code
Summary by cubic
Adds TLS (system trust, custom CA, and mTLS) and API key auth to the Java SDK so clients can connect securely and authenticate. Backward compatible: without options, the client still uses plaintext with no auth.
New Features
FilaClient.Builder:withTls(),withTlsCaCert(byte[]),withTlsClientCert(byte[], byte[]),withApiKey(String)(Bearer viaApiKeyInterceptor)io.grpc.TlsChannelCredentials; sends API key inauthorizationmetadata on every RPCwithTlsCaCert()implies TLS;withTls()uses the JVM default trust storeproto/fila/v1/admin.proto: Create/Revoke/List API keys, Set/Get ACL; added cluster fields to stats and queue infoBug Fixes
FilaExceptionto avoid breaking callersIllegalArgumentExceptionfrom TLS trust manager for invalid certsUNAUTHENTICATEDwithout an API keyspotlessformattingWritten for commit 82eef1c. Summary will update on new commits.