fix(security): Enforce TLS 1.2 minimum for UTxORPC and Bark servers#2869
Conversation
…rage for UTxORPC and Bark TLS configurations. Signed-off-by: Akhil Repala <arepala@blinklabs.io>
📝 WalkthroughWalkthroughAdds TLS configuration helpers for the UTxO RPC and Bark servers that initialize 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
arepala-uml
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/utxorpc/utxorpc.go (1)
300-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate TLS configuration logic.
The
configureServerTLShelper and its tests are identically duplicated across the UTxO RPC and Bark server packages. Consider extracting this logic into a shared internal utility package (for example,internal/netutilorinternal/tlsutil) to keep server TLS policies centralized and DRY.
api/utxorpc/utxorpc.go#L300-L311: Replace this helper with a call to the extracted shared function.bark/bark.go#L196-L207: Replace this helper with a call to the extracted shared function.api/utxorpc/utxorpc_test.go#L150-L198: Move this test to the shared package alongside the extracted helper.bark/bark_test.go#L25-L68: Remove this duplicated test once the logic is covered in the shared package.🤖 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 `@api/utxorpc/utxorpc.go` around lines 300 - 311, The duplicate configureServerTLS implementations and tests should be centralized in a shared internal TLS utility. In api/utxorpc/utxorpc.go lines 300-311 and bark/bark.go lines 196-207, replace the local helpers with calls to the shared function; move the corresponding tests from api/utxorpc/utxorpc_test.go lines 150-198 into the shared package, and remove the duplicate tests from bark/bark_test.go lines 25-68 while preserving coverage of the TLS 1.2 minimum-version behavior.
🤖 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.
Nitpick comments:
In `@api/utxorpc/utxorpc.go`:
- Around line 300-311: The duplicate configureServerTLS implementations and
tests should be centralized in a shared internal TLS utility. In
api/utxorpc/utxorpc.go lines 300-311 and bark/bark.go lines 196-207, replace the
local helpers with calls to the shared function; move the corresponding tests
from api/utxorpc/utxorpc_test.go lines 150-198 into the shared package, and
remove the duplicate tests from bark/bark_test.go lines 25-68 while preserving
coverage of the TLS 1.2 minimum-version behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3545c29f-4198-46bd-94f0-0c806b1e2787
📒 Files selected for processing (4)
api/utxorpc/utxorpc.goapi/utxorpc/utxorpc_test.gobark/bark.gobark/bark_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… TLS 1.2 minimum configuration across UTxORPC and Bark. Signed-off-by: Akhil Repala <arepala@blinklabs.io>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…oth scenarios (max version below raised floor, ECH keys with unset minimum) Signed-off-by: Akhil Repala <arepala@blinklabs.io>
|
@cubic-dev-ai reveiw |
@arepala-uml I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
|
@cubic-dev-ai reveiw |
@arepala-uml I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
|
@cubic-dev-ai reveiw |
@arepala-uml I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…onfig.Clone() Signed-off-by: Akhil Repala <arepala@blinklabs.io>
|
@arepala-uml I have started the AI code review. It will take a few minutes to complete. |
2 participants
Closes #2436
Summary by cubic
Enforces TLS 1.2 as the minimum for
api/utxorpcandbarkservers viainternal/tlsutil. TLS 1.3 is automatically required when ECH keys are configured; addresses #2436.Bug Fixes
tlsutil.ServerConfigto both servers; wrapGetConfigForClientand clone before applying the floor to avoid mutating shared configs.MaxVersionwhen below the floor; tests cover nil/zero, <1.2, 1.3, ECH (static/dynamic), MaxVersion, and callback cases.Migration
Written for commit e870e53. Summary will update on new commits.
Summary by CodeRabbit
Security
Bug Fixes
Tests