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

Avoid mutating SSLContext in SSLServer. #742

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

Conversation

ioquatix
Copy link
Member

I think that we shouldn't mutate the SSLContext in SSLServer and instead be more proactive about setting the right defaults.

               |   FrozenError: can't modify frozen OpenSSL::SSL::SSLContext: #<OpenSSL::SSL::SSLContext:0x0000000104d3f3b0 @verify_mode=0, @verify_hostname=true, @servername_cb=#<Proc:0x0000000104d3f1a8 /Users/samuel/Developer/socketry/async-http-native-io/external/falcon/lib/falcon/environment/proxy.rb:78>, @session_id_context=nil, @max_proto_version=771, @min_proto_version=771>
               |   → /Users/samuel/.gem/ruby/3.3.0/bundler/gems/openssl-d3d857cd1e4d/lib/openssl/ssl.rb:536 in `initialize'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:125 in `new'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:125 in `block in bind'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:124 in `map'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:124 in `bind'
               |     /Users/samuel/Developer/socketry/async-http-native-io/lib/async/http/endpoint.rb:185 in `bind'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/bound_endpoint.rb:13 in `bound'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/bound_endpoint.rb:83 in `bound'
               |     lib/falcon/service/server.rb:41 in `block in start'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/async-2.10.2/lib/async/task.rb:163 in `block in run'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/async-2.10.2/lib/async/task.rb:376 in `block in schedule'

@rhenium
Copy link
Member

rhenium commented Apr 26, 2024

Can we just skip assigning the session ID context if SSLContext is already frozen, for now?

I agree SSLServer shouldn't mutate SSLContext, but I don't feel safe to change the behavior without deprecating it first, as that could break existing user programs in a tricky way.

session_id = prng.bytes(16).unpack1('H*')
self.session_id_context = session_id
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSLContext#set_params is useful only for TLS clients so the session ID context should not be set here (which only makes sense for servers).

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.

None yet

2 participants