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

Possible regression in Sidekiq 6.0.7 with Redis and SSL certs #4531

Closed
geoffharcourt opened this issue Apr 17, 2020 · 9 comments · Fixed by #4532
Closed

Possible regression in Sidekiq 6.0.7 with Redis and SSL certs #4531

geoffharcourt opened this issue Apr 17, 2020 · 9 comments · Fixed by #4532

Comments

@geoffharcourt
Copy link
Contributor

geoffharcourt commented Apr 17, 2020

Ruby version:
Sidekiq 6.0.7 / Pro 5.0.1 / Enterprise 2.0.1

We tested this with OSS Sidekiq as well and got the same result.

Please include your initializer and any error message with the full backtrace.

ssl_params = {}
shared_redis_config = {
  url: ENV["REDIS_SSL_URL"] || ENV["REDIS_URL"],
  ssl_params: ssl_params,
}

if ENV["REDIS_SSL_URL"]
  shared_redis_config[:password] = ENV["REDIS_PASSWORD"]

  ssl_params[:cert] = OpenSSL::X509::Certificate.new(ENV["REDIS_CERT"])
  ssl_params[:key] = OpenSSL::PKey::RSA.new(ENV["REDIS_KEY"])
  ssl_params[:cert_store] = OpenSSL::X509::Store.new
  ssl_params[:cert_store].add_cert(
    OpenSSL::X509::Certificate.new(
      File.read(Rails.root.join("config", "redislabs_ca.pem")),
    ),
  )
end

Sidekiq.configure_client do |config|
  config.redis = shared_redis_config.merge(size: 1)
end

Sidekiq.configure_server do |config|
  config.redis = shared_redis_config
end

The crash looks like this and happens on dyno boot:

2020-04-17T20:29:39.633803+00:00 app[worker_ual.1]: no _dump_data is defined for class OpenSSL::X509::Certificate
2020-04-17T20:29:39.633810+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/sidekiq-6.0.7/lib/sidekiq/redis_connection.rb:100:in `dump'
2020-04-17T20:29:39.633811+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/sidekiq-6.0.7/lib/sidekiq/redis_connection.rb:100:in `log_info'
2020-04-17T20:29:39.633811+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/sidekiq-6.0.7/lib/sidekiq/redis_connection.rb:34:in `create'
2020-04-17T20:29:39.633812+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/sidekiq-6.0.7/lib/sidekiq.rb:135:in `redis='
2020-04-17T20:29:39.633812+00:00 app[worker_ual.1]: /app/config/initializers/sidekiq.rb:25:in `block in <main>'
2020-04-17T20:29:39.633813+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/sidekiq-6.0.7/lib/sidekiq.rb:75:in `configure_server'
2020-04-17T20:29:39.633821+00:00 app[worker_ual.1]: /app/config/initializers/sidekiq.rb:24:in `<main>'
2020-04-17T20:29:39.633822+00:00 app[worker_ual.1]: /app/vendor/bundle/ruby/2.6.0/gems/bootsnap-1.4.6/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:55:in `load'

It looks like the failure is something with our cert (we use Redis over SSL through RedisLabs), but we've never seen this happen before. Any ideas? Line 25 is the interior of the Sidekiq.configure_server block.

@geoffharcourt
Copy link
Contributor Author

@geoffharcourt
Copy link
Contributor Author

It looks like the deep cloning by using Marshal.dump blows up if you're using an SSL cert.

@mperham
Copy link
Collaborator

mperham commented Apr 18, 2020

I wonder why it doesn't implement Marshal. A Certificate is just a blob of data...

@mperham
Copy link
Collaborator

mperham commented Apr 18, 2020

Support was added six months ago. I don't know which versions of Ruby will include this change.

ruby/openssl#281

@geoffharcourt
Copy link
Contributor Author

I bundled the gem and tried require-ing it early in several places to work around this without any success. Would you take a PR that pruned the SSL params out of the options before the log info stuff began?

@mperham
Copy link
Collaborator

mperham commented Apr 18, 2020

Absolutely. Please add a test case and changelog entry too.

@geoffharcourt
Copy link
Contributor Author

@mperham PR to resolve: #4532.

mperham pushed a commit that referenced this issue Apr 18, 2020
#4532)

In 3f9c4bf the Redis connection options began to be cloned (via dumping
and re-marshalling) to avoid issues with password redaction in logging
altering the connection options and breaking authentication with Sentinels.

Unfortunately, this change caused an exception on boot for users of
Redis over SSL. The `OpenSSL::X509::Store` object used for SSL certs is
not yet dumpable in the bundled OpenSSL wrapper for current Rubies
(although it does in master as of ruby/openssl#281).

The fix here prunes the `ssl_params` options out of the Redis
configuration options before the dumping and marshalling. It's probably
better not to include those in logging anyway for privacy purposes.

Fix #4531
@geoffharcourt
Copy link
Contributor Author

@mperham thank you for the quick review and merge!

@mperham
Copy link
Collaborator

mperham commented Apr 18, 2020

Your fix was actually better than mine so thank you!

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

Successfully merging a pull request may close this issue.

2 participants