cotag / ruby-tls

Generic TLS for ruby
Other
6 stars 5 forks source link

Move SSL callbacks to from global constants to object level callbacks #10

Closed HoneyryderChuck closed 7 years ago

HoneyryderChuck commented 7 years ago

I've seen the pattern of defining the SSL callbacks in a static variable (ex: VerifyCB). From what I see, this has one downside: since it's defined globally, one loses the local scope. And probably this is the reason why concurrent-ruby is a dependency (to rely on the Concurrent::Map), which has the downside of adding another level of lock contention.

One could instead define the callbacks as normal methods, and just pass them as callbacks. Here's an example I've tested locally:

# to replace VerifyCB
if options[:verify_peer]
  SSL.SSL_set_verify(@ssl, SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE, method(:verify_cb))
end

...
def verify_cb(preverify_ok, x509_store)
  x509 = SSL.X509_STORE_CTX_get_current_cert(x509_store)
  ssl = SSL.X509_STORE_CTX_get_ex_data(x509_store, SSL.SSL_get_ex_data_X509_STORE_CTX_idx)
  ...
 # replace
 # result = InstanceLookup[ssl.address].verify(buffer.read_string(size))
 # with
 result = verify(buffer.read_string(size))
 ...
end

The current solution has one advantage though: it limits the number of FFI::Function allocations. It does so, however, at the cost of adding extra locking and adding one quite big dependency for a small feature (concurrent-ruby). But if that particular advantage was the reason you decided to go with it, you can ignore this request (both solutions involve a trade-off, question is, which one to live with).

stakach commented 7 years ago

I believe the instance lookup is more efficient and uses much less memory.

Otherwise FFI is creating a new C-Function callback for each callback which I've found limits the number of concurrent connections and I need 1000's - I'm not 100% on the internals of FFI however I've hit weird memory limits (around 400mb pure FFI usage) that are independent of ruby's heap which can grow much larger.

stakach commented 7 years ago

Also on JRuby the current method is more likely to be JIT'd as the code path is hot versus an independent code path per connection

HoneyryderChuck commented 7 years ago

Fair enough, I was afraid something like that could be the issue. Thx for the heads up!