Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature #19641] Enable to pass SSLContext to Net::HTTP #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shouichi
Copy link

Also SSLContext is reused among requests.

Also SSLContext is reused among requests.

Co-authored-by: Seonggi Yang <[email protected]>
@rhenium
Copy link
Member

rhenium commented Aug 20, 2023

Reusing SSLContext might be a breaking change. A Net::HTTP instance may start a connection multiple times, but an SSLContext can only be modified before the first connection due to openssl's limitations for thread safety. An example which won't work if SSLContext is reused:

http = Net::HTTP.new(host)
http.cert = cert_a
http.key = key_a
http.start {
  http.get("/")
}

http.cert = cert_b
http.key = key_b
http.start {
  http.get("/")
}

This concern led to the hack-ish code for TLS session resumption we currently have in #connect. Please see https://bugs.ruby-lang.org/issues/5341 for context.

@rhenium
Copy link
Member

rhenium commented Aug 20, 2023

https://bugs.ruby-lang.org/issues/19641

A potential drawback is conflict handling may not be obvious to users. For example, what happens if a user both sets Net::HTTP#verify_hostname and OpenSSL::SSL::Context#verify_hostname?

For the same reason as above, the new attribute for setting SSLContext has to assume the user-supplied SSLContext may already be frozen. So I think we have no choice but to disallow other existing attributes for configuring SSLContext if user supplies a pre-made SSLContext.

@shouichi
Copy link
Author

@rhenium Thank you for the explanation. How about letting users set arbitrary parameters to SSLContext?

http.ssl_context_params = { security_level: 1}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants