pjfanning commented on PR #3660:
URL: https://github.com/apache/pekko/pull/3660#issuecomment-6360557541
Thanks for writing this up. I checked it against the code: `PekkoSSLConfig`,
`outgoingTlsConnection`, `bindTls` and `bindAndHandleTls` existed in 1.x and
were removed by #2127. The replacements exist on `main`:
`outgoingConnectionWithTls` and `bindWithTls` in both DSLs,
`bindAndHandleWithTls` in scaladsl only, plus the SSLEngine-factory overloads
of `TLS.apply` and `TLS.create`. A few suggestions:
1. **Hostname verification and SNI are not mentioned (the important one).**
When `hostInfo` was given, the removed `TLS.apply(sslContext, sslConfig,
firstSession, role, closing, hostInfo)` also:
- created the engine with `createSSLEngine(host, port)`;
- added SNI;
- enabled hostname verification, via
`setEndpointIdentificationAlgorithm("HTTPS")` or ssl-config's
`hostnameVerifier`.
The new text only asks for "client or server mode and the required trust
and authentication settings". A client that follows it with
`sslContext.createSSLEngine()` + `setUseClientMode(true)` silently loses
hostname verification. Please tell clients to:
- create the engine with the peer's host and port;
- set `SSLParameters.setEndpointIdentificationAlgorithm("HTTPS")`;
- apply what `NegotiateNewSession` used to configure (cipher suites,
protocols, client auth) directly on the engine.
2. **Link the existing example.**
[`stream/stream-io.md#tls`](https://pekko.apache.org/docs/pekko/current/stream/stream-io.html#tls)
already has Scala and Java snippets for setting up the engine, so an `@ref` to
it would help more than prose. That snippet doesn't set endpoint identification
either. That's fine for its server/test use, but worth a follow-up if it
becomes the migration example.
3. **Placement.** This describes a removed API rather than configuration. It
currently sits as an unbulleted paragraph under "Removed configuration". It
would read better as a bullet under "Additional Breaking Changes in Pekko 2.x"
with the (PR2127) link, like the other entries there.
4. **Java `bindAndHandleTls`.** The Java DSL never had `bindAndHandleTls`,
and the earlier "`bindTls` → `bindWithTls`" already covers Java, so that clause
can be Scala-only.
5. **PR description.**
- The content is entirely Pekko-native. Could the akka-core reference be
dropped? We avoid citing recent Akka PRs.
- "Tests: Not run - docs only" next to "Passed: `docs/paradox`" reads as
contradictory. Listing the paradox run is enough.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
