Upgrade nats.c to latest stable release#903
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds leaf-certificate SAN validation and verification-callback wiring into NATS TLS connections; switches NATS error retrieval to a fixed-size buffer API; expands NATS build inputs and BoringSSL compatibility defines; adds unit tests for SAN logic; silences a Clang unused-function warning around a utility include. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SNTPush as SNTPushClientNATS
participant NATSlib as NATS lib
participant SSL as OpenSSL/BoringSSL
participant X509 as X509 parser
Client->>SNTPush: Initiate NATS connect (TLS)
SNTPush->>NATSlib: set SSL options + NATSSSLVerifyCallback
NATSlib->>SSL: perform TLS handshake
SSL->>SNTPush: invoke NATSSSLVerifyCallback(depth, preverifyOk, cert)
SNTPush->>X509: NATSLeafCertHasPushDomain(cert)
X509-->>SNTPush: return hasPushDomain (true/false)
SNTPush-->>SSL: return verification result
SSL-->>NATSlib: handshake outcome
NATSlib-->>Client: connection established or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/BUILD.nats`:
- Around line 41-46: The macro aliases in BUILD.nats change TLS semantics: do
not remap SSL_CTX_set_ciphersuites to SSL_CTX_set_cipher_list or
SSL_use_certificate_chain_file to SSL_use_certificate_file(...,
SSL_FILETYPE_PEM) because this breaks nats.c usage (e.g., TLS 1.3 cipher names
like TLS_AES_256_GCM_SHA384 and full chain loading). Remove these two mappings
(the "SSL_CTX_set_ciphersuites=SSL_CTX_set_cipher_list" and
"SSL_use_certificate_chain_file(s,f)=SSL_use_certificate_file(s,f,SSL_FILETYPE_PEM"
entries) or replace them with platform-aware shims that preserve TLS1.3 cipher
handling and full chain loading semantics for callers in nats.c (ensure
SSL_CTX_set_ciphersuites and SSL_use_certificate_chain_file behaviors are
preserved or explicitly implemented).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6e409ea-106c-41a5-b454-ca9fcc0797b7
📒 Files selected for processing (4)
Source/common/NKeyTokenValidator.mmSource/santasyncservice/SNTPushClientNATS.mmdeps/BUILD.natsdeps/non_module_deps.bzl
This PR updates the nats.c library to the latest stable release v3.12.0.
There were some interface changes between v3.12.0 and v3.8.2. Most notably around the TLS validation and callbacks.