Jens-G commented on PR #3652: URL: https://github.com/apache/thrift/pull/3652#issuecomment-5032372565
### Code review Found 1 issue: 1. This PR correctly removes PR #3639's in-place mutation of a caller-supplied `ssl_context`. One side effect: `configured_ssl_context`'s `@ssl_context ||= ...` now applies safe defaults (`VERIFY_PEER` + system cert store) only when no context is supplied at all — any caller-supplied context passes to `OpenSSL::SSL::SSLSocket.new` unchanged, regardless of its `verify_mode`. A plain `OpenSSL::SSL::SSLContext.new` defaults to `verify_mode == VERIFY_NONE`; setting a trust source like `ca_file` alone doesn't change that. `post_connection_check`, now called unconditionally, checks hostname match against the presented certificate but not chain trust. This PR's own diff adds `context.verify_mode = OpenSSL::SSL::VERIFY_PEER` to the "should use a configured certificate authority" spec to keep it exercising real verification, and the README's "supply a context configured with `VERIFY_NONE` to disable verification" wording doesn't state what verify_mode a supplied-but-unconfigured context actually gets. Worth confirming "verification is the caller's responsibility once any context is supplied" is the intended contract, and documenting that default explicitly if so. https://github.com/apache/thrift/blob/4b2ed9ebecc57f87056912e6c6564e055b9012b6/lib/rb/lib/thrift/transport/ssl_socket.rb#L89-L96 https://github.com/apache/thrift/blob/4b2ed9ebecc57f87056912e6c6564e055b9012b6/lib/rb/spec/ssl_socket_spec.rb#L347-L355 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
