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
| if (!isHostnameValidationKnown()) { | ||
| return null; | ||
| } | ||
| String endpointIdentificationAlgorithm = |
There was a problem hiding this comment.
After the second commit this read can no longer differ from what the factory already knows: isHostnameValidationKnown() passes only for exact-class DefaultSslEngineFactory (sets HTTPS iff requireHostnameValidation) and SniSslEngineFactory (always sets it). The only thing the channel attribute, weak reference and pipeline walk still distinguish is a NettyOptions hook that replaced the driver's handler — paid for on every connection, plus the Channel parameter now threaded through DriverConfigReporter, NoopDriverConfigReporter and ProtocolInitHandler. Asking the two built-ins for the boolean collapses all of it.
| /** Builds a new instance from the driver configuration. */ | ||
| public DefaultSslEngineFactory(DriverContext driverContext) { | ||
| DriverExecutionProfile config = driverContext.getConfig().getDefaultProfile(); | ||
| this.hostnameValidationKnown = |
There was a problem hiding this comment.
This makes the field absent for the most common TLS setup — no keystore or truststore, server cert chaining to cacerts — where it used to be reported. If the reason is that SSLContext.getDefault() can be swapped process-wide via SSLContext.setDefault, that belongs here as a comment: the javadoc on isHostnameValidationKnown ("this factory built the trust-manager path") doesn't describe the keystore-only branch either, where context.init(km, null, …) leaves the JDK's default trust managers in place. It is also a verbatim copy of buildContext's condition below, with nothing keeping the two in step.
| /* programmaticLocalDc= */ null); | ||
| Map<String, String> options = new HashMap<>(); | ||
| r.populateControlConnectionOptions(options, reportingChannel); // must not throw | ||
| assertThat(options).isEmpty(); |
There was a problem hiding this comment.
This pins more than the name says: one throwing getSSLParameters() drops the whole DRIVER_CONFIG blob — reconnection, timeouts, node location — through populateControlConnectionOptions's blanket catch, not just the optional field. A try/catch around the single getHostnameValidationRequired call would omit only hostname-verification, which the schema already allows.
| } | ||
|
|
||
| /** Builds the factory for a cloud-bundle context whose trust managers the driver created. */ | ||
| public static SniSslEngineFactory forCloudBundle(SSLContext sslContext) { |
There was a problem hiding this comment.
Public and takes any SSLContext, so any caller can mint the "the driver built these trust managers" claim for a context it didn't build — the false positive this PR exists to remove. Package-private, with a package-private entry point for CloudConfigFactory, would keep the guarantee tied to the provenance. Nothing covers the getClass() == CloudConfigFactory.class branch either, so a subclass on the production path would silently drop the field for every cloud session with the suite green.
| @Test | ||
| public void should_report_tls_enabled_with_hostname_verification() throws Exception { | ||
| // hostname-verification comes from the factory's own state, not the config option. | ||
| public void should_treat_a_programmatic_ssl_context_with_an_algorithm_as_unknown() |
There was a problem hiding this comment.
These four …_as_unknown tests now stop at the same return false in isHostnameValidationKnown() without ever reaching getSSLParameters(), so the anonymous subclasses that set or omit the algorithm are inert setup and imply the algorithm still decides something. should_report_only_built_in_factories_with_known_validators already covers the branch.
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