feat(crypto): add TLS + shared-secret auth to gRPC service - #14
Merged
Conversation
…alytics - Remove barrel file (index.ts with 25+ re-exports), replace with direct imports across 18 files for better tree-shaking - Lazy-load 6 modals with next/dynamic (SecretModal, ViewSecretModal, DeleteVaultModal, NewVaultModal, BulkImportModal, ConnectOrgModal) - Change PostHog script strategy from afterInteractive to lazyOnload Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The two separate Script tags with strategy="lazyOnload" had no guaranteed execution order, so the init script could run before the library was loaded. Merge into a single Script with onLoad callback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rrel The barrel file was removed but 9 test files still mocked the old barrel path. Update mocks to target individual component files, fixing all 89 failing tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Defense-in-depth for the crypto gRPC service that encrypts/decrypts all secrets in the platform. Adds two opt-in security layers: - Shared-secret interceptor: validates x-crypto-auth-token metadata header using SHA-256 + constant-time comparison (prevents timing and length-leaking attacks). gRPC health probes are exempt. - TLS with auto-generated self-signed ECDSA P-256 cert: encrypts traffic between backend and crypto service. Cert is generated at first startup and persisted via Docker volume. Backend verifies via cert pinning (PEM as rootCerts). Both layers are backward-compatible: without env vars configured, the service runs in the current insecure mode with log warnings. CRYPTO_TLS_REQUIRED=true makes TLS failures fatal for production. Also fixes audit finding V22: removes ENV ENCRYPTION_KEY="" from the Dockerfile (was leaking in docker inspect). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds token-based gRPC authentication and optional TLS (with self-signed cert bootstrapping) to the crypto service; backend is updated to pass auth/TLS options to the crypto client and health checks; docker-compose and env examples share TLS assets; dashboard barrel exports removed and many imports switched to granular paths with some modals lazy-loaded. Changes
Sequence DiagramsequenceDiagram
participant Backend as Backend Service
participant RemoteClient as RemoteEncryptionService (client)
participant CryptoServer as Crypto gRPC Server
participant Auth as Auth Interceptor
participant TLS as TLS Layer
Backend->>RemoteClient: invoke RPC (options: authToken, tlsCa/tlsCaPath)
RemoteClient->>CryptoServer: open gRPC channel (with Metadata x-crypto-auth-token) / TLS handshake
CryptoServer->>Auth: authInterceptor intercepts unary call
Auth->>Auth: compute SHA-256, constant-time compare
alt token valid
Auth->>TLS: allow to proceed
TLS->>TLS: verify client TLS using CA (if provided)
alt TLS verified or not required
CryptoServer->>CryptoServer: execute RPC method
CryptoServer-->>RemoteClient: RPC response
RemoteClient-->>Backend: return result
else TLS verification failed
CryptoServer-->>RemoteClient: TLS error
RemoteClient-->>Backend: propagate TLS error
end
else token invalid/missing
Auth-->>RemoteClient: return Unauthenticated
RemoteClient-->>Backend: propagate Unauthenticated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
- Add curly braces to if-return (curly rule) - Replace `Function` type with explicit `(...args: unknown[]) => void` to satisfy @typescript-eslint/no-unsafe-function-type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/dashboard/app/layout.tsx (1)
60-61: Avoid@ts-expect-errorforwindow.posthog.initLine 60 suppresses typing rather than modeling the API. Prefer extending the global
window.posthogtype (e.g., includeinit) and removing the suppression to keep type checks meaningful.Type-safe cleanup
- // `@ts-expect-error` -- posthog is loaded via external script - window.posthog?.init(posthogKey, { + window.posthog?.init?.(posthogKey, { api_host: posthogHost,// packages/dashboard/lib/analytics.ts (or a dedicated global.d.ts) declare global { interface Window { posthog?: { init?: (key: string, config?: Record<string, unknown>) => void capture: (event: string, properties?: Record<string, unknown>) => void identify: (distinctId: string, properties?: Record<string, unknown>) => void } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dashboard/app/layout.tsx` around lines 60 - 61, The code currently suppresses TypeScript with "// `@ts-expect-error`" before calling window.posthog?.init; instead add a proper global type for window.posthog (including init, capture, identify signatures) in a declaration file (e.g., packages/dashboard/lib/analytics.ts or a new global.d.ts) and then remove the ts-expect-error and call window.posthog?.init normally; ensure the declaration defines Window.posthog with init?: (key: string, config?: Record<string, unknown>) => void and the other methods referenced so TypeScript knows the API used by layout.tsx.packages/crypto/auth_test.go (1)
193-236: Consider extracting common server setup to reduce duplication.This test duplicates most of the setup logic from
setupAuthTestServer. You could extend the helper to optionally register the health server:func setupAuthTestServer(t *testing.T, token string, keys map[uint32]string, registerHealth bool) (...)That said, the explicit inline setup makes the test self-contained and clear about what services are registered, so this is a minor refinement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/auth_test.go` around lines 193 - 236, The test TestAuthInterceptor_GrpcHealthExempt duplicates server setup logic; refactor by extending setupAuthTestServer to accept an optional registerHealth bool (e.g., func setupAuthTestServer(t *testing.T, token string, keys map[uint32]string, registerHealth bool) (...)) so callers can request the health server be registered, then update TestAuthInterceptor_GrpcHealthExempt to call setupAuthTestServer with registerHealth=true and remove the in-test server creation/registration code, ensuring authInterceptor, pb.RegisterCryptoServiceServer, and grpc_health_v1.RegisterHealthServer are handled by the helper.packages/backend/src/utils/remoteEncryption.ts (1)
189-198: Consider adding basic PEM validation for clearer error messages.If
tlsCacontains invalid base64 ortlsCaPathpoints to a non-PEM file, the error will surface during the TLS handshake with a less informative message. A simple sanity check could improve debuggability:💡 Optional improvement
private loadTlsCa(options?: CryptoServiceOptions): Buffer | null { if (options?.tlsCa) { - return Buffer.from(options.tlsCa, "base64"); + const pem = Buffer.from(options.tlsCa, "base64"); + if (!pem.toString().includes("-----BEGIN")) { + throw new Error("CRYPTO_TLS_CA does not appear to be valid base64-encoded PEM"); + } + return pem; } if (options?.tlsCaPath) { // Path was explicitly configured -- fail loudly if unreadable - return fs.readFileSync(options.tlsCaPath); + const pem = fs.readFileSync(options.tlsCaPath); + if (!pem.toString().includes("-----BEGIN")) { + throw new Error(`File at CRYPTO_TLS_CA_PATH (${options.tlsCaPath}) does not appear to be valid PEM`); + } + return pem; } return null; }This is optional since the TLS layer will still reject invalid certs, just with less context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/utils/remoteEncryption.ts` around lines 189 - 198, The loadTlsCa method should perform basic PEM and base64 validation to give clearer errors: when options.tlsCa is provided, attempt to decode and verify it looks like a PEM certificate (e.g., contains "-----BEGIN CERTIFICATE-----" / "-----END CERTIFICATE-----") and throw a descriptive error if base64 decode fails or markers are missing; when options.tlsCaPath is provided, read the file and validate its contents similarly, and if unreadable or not PEM-like throw a clear error mentioning tlsCaPath; update loadTlsCa to return the Buffer only after these sanity checks so callers see precise validation failures instead of TLS handshake errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 23-25: The compose defaults cause mismatched TLS behavior because
CRYPTO_TLS_CERT_PATH being empty triggers certificate generation in
packages/crypto/main.go while the backend only enables TLS when CRYPTO_TLS_CA or
CRYPTO_TLS_CA_PATH is set; fix by explicitly defaulting the crypto service cert
path to a sentinel value (e.g. set CRYPTO_TLS_CERT_PATH=none) so an empty docker
compose run means TLS off, and make the same change for the duplicate env
entries (the other CRYPTO_TLS_CERT_PATH lines around 44-45); update any startup
logic that checks CRYPTO_TLS_CERT_PATH to treat "none" as disabling TLS,
referencing CRYPTO_TLS_CERT_PATH, CRYPTO_TLS_CA, CRYPTO_TLS_CA_PATH and
packages/crypto/main.go to locate the relevant checks.
- Line 29: The healthcheck currently hardcodes /data/crypto/tls.crt and fails
when CRYPTO_TLS_CERT_PATH is set to "none" or a custom path; update the
healthcheck command in docker-compose.yml (the test line that references
CRYPTO_TLS_CERT_PATH and GRPC_PORT) to: 1) treat CRYPTO_TLS_CERT_PATH="none" as
valid (skip file existence check), 2) if CRYPTO_TLS_CERT_PATH is non-empty and
not "none" check that that file exists, and 3) if CRYPTO_TLS_CERT_PATH is empty
fall back to the default /data/crypto/tls.crt; keep the existing port check (nc
-z localhost ${GRPC_PORT:-50051}) and exit nonzero on failure.
In `@packages/crypto/main.go`:
- Around line 121-128: When reusing the loaded keypair in the certExists branch
(after tls.LoadX509KeyPair returns a cert), parse the leaf bytes in
cert.Certificate[0] with x509.ParseCertificate and validate its
NotBefore/NotAfter against time.Now(); if now is outside that validity window,
do not return the stale cert and instead trigger regeneration (or return an
explicit error) so rotation happens instead of continuing to use an
expired/not-yet-valid certificate; keep the current SHA-256 fingerprint logic
(sha256.Sum256 and hex.EncodeToString) only for valid certificates.
- Around line 158-176: The current implementation writes certPEM to certPath and
keyPEM to keyPath directly, which can leave a half-written pair on disk; modify
the write logic to write both artifacts to temporary files (e.g.,
certPath+".tmp" and keyPath+".tmp" or use os.CreateTemp), fsync/close them, then
atomically rename (os.Rename) them to certPath and keyPath only after both
writes succeed; on any error during marshal/write/load (including before
tls.LoadX509KeyPair), remove any temp files and avoid leaving partial files, and
ensure file permissions for the key are set to 0600 before rename; update the
code paths around certPEM/keyPEM writes and tls.LoadX509KeyPair to use the final
paths so mismatched TLS files cannot be left behind.
In `@packages/dashboard/app/layout.tsx`:
- Around line 58-69: The PostHog init in layout.tsx uses strategy="lazyOnload"
causing a race where trackEvent calls in login/page.tsx and
auth/callback/page.tsx can be lost; change the script loading strategy to
"afterInteractive" so window.posthog is available on mount, or implement an
event queue (e.g., push events to a temporary array and flush them in the init
callback inside the onLoad closure) to capture early events; also remove the
unnecessary // `@ts-expect-error` before window.posthog since analytics.ts already
types window.posthog as optional.
---
Nitpick comments:
In `@packages/backend/src/utils/remoteEncryption.ts`:
- Around line 189-198: The loadTlsCa method should perform basic PEM and base64
validation to give clearer errors: when options.tlsCa is provided, attempt to
decode and verify it looks like a PEM certificate (e.g., contains "-----BEGIN
CERTIFICATE-----" / "-----END CERTIFICATE-----") and throw a descriptive error
if base64 decode fails or markers are missing; when options.tlsCaPath is
provided, read the file and validate its contents similarly, and if unreadable
or not PEM-like throw a clear error mentioning tlsCaPath; update loadTlsCa to
return the Buffer only after these sanity checks so callers see precise
validation failures instead of TLS handshake errors.
In `@packages/crypto/auth_test.go`:
- Around line 193-236: The test TestAuthInterceptor_GrpcHealthExempt duplicates
server setup logic; refactor by extending setupAuthTestServer to accept an
optional registerHealth bool (e.g., func setupAuthTestServer(t *testing.T, token
string, keys map[uint32]string, registerHealth bool) (...)) so callers can
request the health server be registered, then update
TestAuthInterceptor_GrpcHealthExempt to call setupAuthTestServer with
registerHealth=true and remove the in-test server creation/registration code,
ensuring authInterceptor, pb.RegisterCryptoServiceServer, and
grpc_health_v1.RegisterHealthServer are handled by the helper.
In `@packages/dashboard/app/layout.tsx`:
- Around line 60-61: The code currently suppresses TypeScript with "//
`@ts-expect-error`" before calling window.posthog?.init; instead add a proper
global type for window.posthog (including init, capture, identify signatures) in
a declaration file (e.g., packages/dashboard/lib/analytics.ts or a new
global.d.ts) and then remove the ts-expect-error and call window.posthog?.init
normally; ensure the declaration defines Window.posthog with init?: (key:
string, config?: Record<string, unknown>) => void and the other methods
referenced so TypeScript knows the API used by layout.tsx.
🪄 Autofix (Beta)
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
Run ID: d3334673-ebca-492c-973c-264502ad23b3
📒 Files selected for processing (41)
docker-compose.ymlpackages/backend/.env.examplepackages/backend/src/config/index.tspackages/backend/src/index.tspackages/backend/src/utils/encryption.tspackages/backend/src/utils/remoteEncryption.tspackages/crypto/Dockerfilepackages/crypto/auth_test.gopackages/crypto/main.gopackages/crypto/tls_test.gopackages/dashboard/app/(dashboard)/activity/page.tsxpackages/dashboard/app/(dashboard)/api-keys/page.tsxpackages/dashboard/app/(dashboard)/orgs/[org]/billing/page.tsxpackages/dashboard/app/(dashboard)/orgs/[org]/exposure/page.tsxpackages/dashboard/app/(dashboard)/orgs/[org]/members/page.tsxpackages/dashboard/app/(dashboard)/orgs/[org]/page.tsxpackages/dashboard/app/(dashboard)/orgs/[org]/settings/page.tsxpackages/dashboard/app/(dashboard)/orgs/page.tsxpackages/dashboard/app/(dashboard)/page.tsxpackages/dashboard/app/(dashboard)/security/_components/SecurityAccessLogTab.tsxpackages/dashboard/app/(dashboard)/security/_components/SecurityAlertsTab.tsxpackages/dashboard/app/(dashboard)/security/_components/SecurityExposureTab.tsxpackages/dashboard/app/(dashboard)/security/_components/SecurityOverviewTab.tsxpackages/dashboard/app/(dashboard)/security/page.tsxpackages/dashboard/app/(dashboard)/settings/page.tsxpackages/dashboard/app/(dashboard)/vaults/[owner]/[repo]/collaborators/page.tsxpackages/dashboard/app/(dashboard)/vaults/[owner]/[repo]/environments/page.tsxpackages/dashboard/app/(dashboard)/vaults/[owner]/[repo]/page.tsxpackages/dashboard/app/components/dashboard/index.tspackages/dashboard/app/layout.tsxpackages/dashboard/next-env.d.tspackages/dashboard/tests/components/SecurityAccessLogTab.test.tsxpackages/dashboard/tests/components/SecurityAlertsTab.test.tsxpackages/dashboard/tests/components/SecurityExposureTab.test.tsxpackages/dashboard/tests/components/SecurityOverviewTab.test.tsxpackages/dashboard/tests/pages/ActivityPage.test.tsxpackages/dashboard/tests/pages/ApiKeysPage.test.tsxpackages/dashboard/tests/pages/SecurityPage.test.tsxpackages/dashboard/tests/pages/SettingsPage.test.tsxpackages/dashboard/tests/pages/VaultDetailPage.test.tsxpackages/dashboard/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/dashboard/app/components/dashboard/index.ts
| - crypto_certs:/data/crypto | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "nc -z localhost ${GRPC_PORT:-50051} || exit 1"] | ||
| test: ["CMD-SHELL", "(test -z \"${CRYPTO_TLS_CERT_PATH}\" || test -f /data/crypto/tls.crt) && nc -z localhost ${GRPC_PORT:-50051} || exit 1"] |
There was a problem hiding this comment.
Healthcheck ignores CRYPTO_TLS_CERT_PATH=none and custom cert paths.
This check hardcodes /data/crypto/tls.crt, so disabling TLS with CRYPTO_TLS_CERT_PATH=none or pointing at another cert path leaves the container permanently unhealthy even though packages/crypto/main.go supports both modes. Use the configured path when one is set, and treat none as a valid no-cert case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` at line 29, The healthcheck currently hardcodes
/data/crypto/tls.crt and fails when CRYPTO_TLS_CERT_PATH is set to "none" or a
custom path; update the healthcheck command in docker-compose.yml (the test line
that references CRYPTO_TLS_CERT_PATH and GRPC_PORT) to: 1) treat
CRYPTO_TLS_CERT_PATH="none" as valid (skip file existence check), 2) if
CRYPTO_TLS_CERT_PATH is non-empty and not "none" check that that file exists,
and 3) if CRYPTO_TLS_CERT_PATH is empty fall back to the default
/data/crypto/tls.crt; keep the existing port check (nc -z localhost
${GRPC_PORT:-50051}) and exit nonzero on failure.
Comment on lines
+58
to
+69
| strategy="lazyOnload" | ||
| onLoad={() => { | ||
| // @ts-expect-error -- posthog is loaded via external script | ||
| window.posthog?.init(posthogKey, { | ||
| api_host: posthogHost, | ||
| capture_pageview: true, | ||
| capture_pageleave: true, | ||
| autocapture: true, | ||
| persistence: 'localStorage+cookie', | ||
| capture_performance: true | ||
| })`} | ||
| </Script> | ||
| </> | ||
| capture_performance: true, | ||
| }) | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mount-time analytics calls that can fire before PostHog is initialized.
rg -nP --type=tsx -C3 'useEffect\s*\(\s*\(\)\s*=>[\s\S]{0,350}\btrackEvent\s*\(' packages/dashboard/app packages/dashboard/libRepository: keywaysh/keyway
Length of output: 86
🏁 Script executed:
# Search for useEffect hooks that call trackEvent
rg -n 'useEffect\s*\(' packages/dashboard/app packages/dashboard/lib -A 10 --type=ts | head -100Repository: keywaysh/keyway
Length of output: 7614
🏁 Script executed:
# Look at the specific files mentioned in the review
fd -t f 'login/page\.(tsx|ts)' packages/dashboard/app
fd -t f 'auth/callback/page\.(tsx|ts)' packages/dashboard/appRepository: keywaysh/keyway
Length of output: 41
🏁 Script executed:
# Check the layout.tsx file for the Script block
cat packages/dashboard/app/layout.tsxRepository: keywaysh/keyway
Length of output: 2206
🏁 Script executed:
fd -t f 'login/page\.(tsx|ts)' packages/dashboard/app -exec cat {} \;Repository: keywaysh/keyway
Length of output: 205
🏁 Script executed:
cat packages/dashboard/lib/analytics.tsRepository: keywaysh/keyway
Length of output: 4096
🏁 Script executed:
find packages/dashboard/app -name 'page.tsx' -o -name 'page.ts' | grep -i loginRepository: keywaysh/keyway
Length of output: 96
🏁 Script executed:
find packages/dashboard/app -name 'page.tsx' -o -name 'page.ts' | head -20Repository: keywaysh/keyway
Length of output: 1033
🏁 Script executed:
cat packages/dashboard/app/login/page.tsxRepository: keywaysh/keyway
Length of output: 4883
Race condition between lazyOnload and mount-time event tracking
The lazyOnload strategy defers PostHog initialization until after browser idle, but trackEvent() is called on mount in login/page.tsx (line 28) and auth/callback/page.tsx (lines 16, 22). Since trackEvent silently no-ops when window.posthog is undefined, early analytics events can be lost.
Consider switching to strategy="afterInteractive" or implementing an event queue to capture events before script load.
Additionally, the @ts-expect-error comment is unnecessary—window.posthog is already properly typed as optional in analytics.ts, so the type is known to TypeScript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dashboard/app/layout.tsx` around lines 58 - 69, The PostHog init in
layout.tsx uses strategy="lazyOnload" causing a race where trackEvent calls in
login/page.tsx and auth/callback/page.tsx can be lost; change the script loading
strategy to "afterInteractive" so window.posthog is available on mount, or
implement an event queue (e.g., push events to a temporary array and flush them
in the init callback inside the onLoad closure) to capture early events; also
remove the unnecessary // `@ts-expect-error` before window.posthog since
analytics.ts already types window.posthog as optional.
- Default CRYPTO_TLS_CERT_PATH=none in docker-compose so plain `docker compose up` stays in insecure mode (no TLS mismatch) - Validate cert expiration on load, auto-regenerate if expired, warn if expiring within 30 days - Atomic cert/key writes via temp files + rename to prevent half-written pairs on crash - Simplify healthcheck back to nc -z (cert check unnecessary since default is now TLS off) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/backend/src/utils/remoteEncryption.ts (1)
189-198: Consider wrapping file read with a descriptive error message.If
tlsCaPathpoints to a non-existent or unreadable file,fs.readFileSyncthrows a generic Node.js error (ENOENT/EACCES). A contextual wrapper would help operators diagnose misconfiguration faster.♻️ Suggested improvement
if (options?.tlsCaPath) { // Path was explicitly configured -- fail loudly if unreadable - return fs.readFileSync(options.tlsCaPath); + try { + return fs.readFileSync(options.tlsCaPath); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error( + `Failed to read TLS CA certificate from "${options.tlsCaPath}": ${message}` + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/utils/remoteEncryption.ts` around lines 189 - 198, The file-read in loadTlsCa (class using CryptoServiceOptions) should be wrapped in a try/catch so unreadable/missing tlsCaPath produces a descriptive error; catch errors from fs.readFileSync(options.tlsCaPath) and rethrow a new Error that includes the tlsCaPath value and the original error message (or attach original error) to give clear context for operators when loadTlsCa fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/backend/src/utils/remoteEncryption.ts`:
- Around line 189-198: The file-read in loadTlsCa (class using
CryptoServiceOptions) should be wrapped in a try/catch so unreadable/missing
tlsCaPath produces a descriptive error; catch errors from
fs.readFileSync(options.tlsCaPath) and rethrow a new Error that includes the
tlsCaPath value and the original error message (or attach original error) to
give clear context for operators when loadTlsCa fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 188921bc-bc46-4e64-ad96-691ae71b098e
📒 Files selected for processing (1)
packages/backend/src/utils/remoteEncryption.ts
- .env.example: deployment-specific guidance (Docker Compose uses shared volume, Railway only needs auth token, Kubernetes via Secret) - README.md: updated security section with new env vars and per-platform recommendations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/crypto/README.md (1)
187-187: Soften “TLS unnecessary” wording to avoid weakening security posture.At Line 187, saying TLS is “unnecessary” on Railway is too absolute. Recommend framing it as optional in trusted private networking, while still preferring TLS for defense-in-depth.
✏️ Suggested doc tweak
-6. **TLS**: Set `CRYPTO_TLS_CERT_PATH` to enable auto-generated self-signed TLS. The cert is shared with the backend via Docker volume or env var. Recommended for Docker Compose and Kubernetes. On Railway, the private network isolation makes TLS unnecessary -- use `CRYPTO_AUTH_TOKEN` alone. +6. **TLS**: Set `CRYPTO_TLS_CERT_PATH` to enable auto-generated self-signed TLS. The cert is shared with the backend via Docker volume or env var. Recommended for Docker Compose and Kubernetes. On Railway private networking, TLS can be optional if you enforce `CRYPTO_AUTH_TOKEN`, but TLS is still preferred when feasible for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/README.md` at line 187, Update the README sentence that currently states TLS is "unnecessary" on Railway (referencing CRYPTO_TLS_CERT_PATH and CRYPTO_AUTH_TOKEN) to a softer recommendation: say TLS can be optional in trusted private networking environments like Railway but is still recommended for defense-in-depth; keep mention that CRYPTO_TLS_CERT_PATH enables auto-generated self-signed TLS and that CRYPTO_AUTH_TOKEN can be used alone when private network isolation is trusted. Ensure the revised line preserves guidance about sharing the cert with the backend via Docker volume or env var and recommends preferring TLS when possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/crypto/README.md`:
- Line 187: Update the README sentence that currently states TLS is
"unnecessary" on Railway (referencing CRYPTO_TLS_CERT_PATH and
CRYPTO_AUTH_TOKEN) to a softer recommendation: say TLS can be optional in
trusted private networking environments like Railway but is still recommended
for defense-in-depth; keep mention that CRYPTO_TLS_CERT_PATH enables
auto-generated self-signed TLS and that CRYPTO_AUTH_TOKEN can be used alone when
private network isolation is trusted. Ensure the revised line preserves guidance
about sharing the cert with the backend via Docker volume or env var and
recommends preferring TLS when possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 729c84da-78c2-41de-93cb-19d548ecfa52
📒 Files selected for processing (4)
docker-compose.ymlpackages/backend/.env.examplepackages/crypto/README.mdpackages/crypto/main.go
✅ Files skipped from review due to trivial changes (1)
- packages/backend/.env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- docker-compose.yml
- packages/crypto/main.go
7 tasks
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 free
to 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
ENV ENCRYPTION_KEY=""from DockerfileContext
The crypto gRPC service is the single point of cryptographic trust in Keyway — it encrypts/decrypts all secrets via AES-256-GCM. While network isolation (Docker/Railway) prevents external access, a compromised container on the same network could call Encrypt/Decrypt without authentication. This PR adds defense-in-depth.
What changed
Go crypto service (
packages/crypto/main.go)authInterceptor: validatesx-crypto-auth-tokenmetadata header. Uses SHA-256 hashing before constant-time comparison to prevent both timing and token-length leaking. gRPC health probes (/grpc.health.v1.Health/Check) are exempt so Docker/k8s healthchecks work.loadOrGenerateTLS: loads existing cert/key or generates self-signed ECDSA P-256 with SANs[localhost, 127.0.0.1, crypto, *.railway.internal]. Detects mismatched cert/key files and errors rather than silently regenerating.CRYPTO_TLS_REQUIRED=truemakes TLS failures fatal (recommended for production).TypeScript backend client (
packages/backend/src/utils/remoteEncryption.ts)CryptoServiceOptions(authToken, tlsCa, tlsCaPath)grpc.credentials.createSsl(rootCerts))checkCryptoService()propagates options for startup health checksDocker Compose
crypto_certsvolume (rw for crypto, ro for backend).env.exampleNew environment variables (all optional)
CRYPTO_AUTH_TOKENCRYPTO_TLS_CERT_PATH/data/crypto/tls.crt,noneto disable)CRYPTO_TLS_KEY_PATH/data/crypto/tls.key)CRYPTO_TLS_REQUIREDtrue= fatal on TLS failureCRYPTO_TLS_CA_PATHCRYPTO_TLS_CADeployment modes
CRYPTO_AUTH_TOKENon both servicesCRYPTO_AUTH_TOKEN+ TLS cert path/volumeTest plan
-racedocker compose up --buildwithout new env vars (backward compat)docker compose up --buildwithCRYPTO_AUTH_TOKENsetdocker compose up --buildwith TLS + auth configured🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Infrastructure
Refactor
Tests
Chores