Caideyipi commented on PR #17854: URL: https://github.com/apache/iotdb/pull/17854#issuecomment-4645241906
I think there are two correctness issues in this PR: 1. `ssl_protocol=TLS` is used as both the SSLContext algorithm and Jetty enabled protocol for REST HTTPS. In `RestService.configureSSL`, the value is passed to `sslContextFactory.setIncludeProtocols(protocol)`. The default template value is `TLS`, but JSSE enabled protocol names are normally `TLSv1.3`, `TLSv1.2`, etc., not `TLS`. So enabling REST HTTPS with the default config can leave Jetty with no matching enabled protocol / fail TLS handshakes. The context algorithm and enabled protocol list should be separate, or REST should not call `setIncludeProtocols` for the generic `TLS` value. 2. The legacy SSL overloads in `BaseRpcTransportFactory` now explicitly call the new overload with `null, null`, so internal synchronous SSL clients still do not use `ssl_protocol` / `ssl_provider_class`. For example `SyncDataNodeInternalServiceClient`, `SyncConfigNodeIServiceClient`, and IoT consensus sync clients call the old overload while `enable_internal_ssl` is true. If a custom JSSE provider is required, the server side may register it through `CommonDescriptor.configureRpcSsl()`, but these clients create their SSL transport without that provider and can fail to connect. These internal clients should pass `commonConfig.getSslProtocol()` and `commonConfig.getSslProviderClass()` to the new overload. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
