fix: report hostname validation from active SSL engine - #999
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change removes hostname-validation accessors from SSL engine factories. Sequence Diagram(s)sequenceDiagram
participant ProtocolInitHandler
participant DriverConfigReporter
participant JdkSslHandlerFactory
participant SSLEngine
ProtocolInitHandler->>DriverConfigReporter: pass control channel
DriverConfigReporter->>JdkSslHandlerFactory: read channel validation state
JdkSslHandlerFactory->>SSLEngine: inspect SSL parameters
DriverConfigReporter-->>ProtocolInitHandler: build configuration report
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java-47-53 (1)
47-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat subclasses as custom factories when the algorithm is absent.
instanceof ProgrammaticSslEngineFactoryincludes user subclasses. A subclass can implement hostname validation without setting an endpoint-identification algorithm. This branch then reportsfalseinstead of omitting an unknown value. Use exact-class checks for the built-in fallback.🤖 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 `@core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java` around lines 47 - 53, Update the hostnameValidationRequired fallback in JdkSslHandlerFactory to use exact-class checks for DefaultSslEngineFactory, ProgrammaticSslEngineFactory, and SniSslEngineFactory rather than instanceof checks. Ensure subclasses are treated as arbitrary custom factories, leaving the value unknown when no endpoint-identification algorithm is configured.
🤖 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.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java`:
- Around line 33-55: Make hostname-validation state connection-specific instead
of storing it in the shared JdkSslHandlerFactory.hostnameValidationRequired
field. In newSslHandler, attach the computed value to the created SslHandler or
channel, then ensure the DRIVER_CONFIG/reporting path reads that per-connection
state for the same channel rather than the factory field.
---
Other comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.java`:
- Around line 47-53: Update the hostnameValidationRequired fallback in
JdkSslHandlerFactory to use exact-class checks for DefaultSslEngineFactory,
ProgrammaticSslEngineFactory, and SniSslEngineFactory rather than instanceof
checks. Ensure subclasses are treated as arbitrary custom factories, leaving the
value unknown when no endpoint-identification algorithm is configured.
🪄 Autofix
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: QUIET
Plan: Pro Plus
Run ID: 9b308576-cae4-46e5-88fb-c10cd5bbd93c
📒 Files selected for processing (5)
core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporter.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/JdkSslHandlerFactory.javacore/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverConfigReporterTest.java
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
scylladb/github-automation(auto-detected)scylladb/scylladb(auto-detected)
💤 Files with no reviewable changes (2)
- core/src/main/java/com/datastax/oss/driver/api/core/ssl/ProgrammaticSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java
6dd1b17 to
b9096d0
Compare
7767841 to
29fe44b
Compare
Pass the control channel to the configuration reporter and retain the driver-installed JDK SSL handler on that channel. Read the endpoint-identification algorithm from the exact engine used for the connection instead of exposing diagnostic accessors on SSL engine factories. Omit hostname verification when the driver-installed handler is unavailable or a disabled state cannot be determined safely.
Treat endpoint-identification parameters as authoritative only when the driver built the trust-manager path that interprets them. Mark configured truststore or keystore contexts and standard cloud bundles as known. Omit hostname verification for process-wide, programmatic, subclassed, and arbitrary SSL contexts, whose trust managers may enforce or ignore hostname checks independently. Keep only a weak reference to the recorded handler.
29fe44b to
c53ad68
Compare
2 participants
Follow-up to #968.
Removes the newly added hostname-validation accessors and derives the reported value from the active JDK SSLEngine instead. Unknown custom factories remain omitted.
Unrelated changes were extracted into:
Tests:
mvn -pl core -DskipITs -Dtest=DefaultDriverConfigReporterTest,ProtocolInitHandlerTest,CloudConfigFactoryTest test