Skip to content

Commit

Permalink
Remove SSL parameters from Redis connection logging to avoid exception (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
geoffharcourt committed Apr 18, 2020
1 parent e3c5551 commit dd0a847
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 3 deletions.
5 changes: 5 additions & 0 deletions Changes.md
Expand Up @@ -2,6 +2,11 @@

[Sidekiq Changes](https://github.com/mperham/sidekiq/blob/master/Changes.md) | [Sidekiq Pro Changes](https://github.com/mperham/sidekiq/blob/master/Pro-Changes.md) | [Sidekiq Enterprise Changes](https://github.com/mperham/sidekiq/blob/master/Ent-Changes.md)

Unreleased
---------

- Avoid exception dumping SSL store in Redis connection logging [#4532]

6.0.7
---------

Expand Down
2 changes: 1 addition & 1 deletion lib/sidekiq/redis_connection.rb
Expand Up @@ -97,7 +97,7 @@ def log_info(options)
redacted = "REDACTED"

# deep clone so we can muck with these options all we want
scrubbed_options = Marshal.load(Marshal.dump(options))
scrubbed_options = Marshal.load(Marshal.dump(options.except(:ssl_params)))
if scrubbed_options[:url] && (uri = URI.parse(scrubbed_options[:url])) && uri.password
uri.password = redacted
scrubbed_options[:url] = uri.to_s
Expand Down
19 changes: 17 additions & 2 deletions test/test_redis_connection.rb
Expand Up @@ -197,8 +197,8 @@ def server_connection(*args)
{ host: 'host1', port: 26379, password: 'secret'},
{ host: 'host2', port: 26379, password: 'secret'},
{ host: 'host3', port: 26379, password: 'secret'},
],
password: 'secret'
],
password: 'secret'
}

output = capture_logging do
Expand All @@ -211,6 +211,21 @@ def server_connection(*args)
assert_includes(output, ':host=>"host3", :port=>26379, :password=>"REDACTED"')
assert_includes(output, ':password=>"REDACTED"')
end

it 'prunes SSL parameters from the logging' do
options = {
ssl_params: {
cert_store: OpenSSL::X509::Store.new
}
}

output = capture_logging do
Sidekiq::RedisConnection.create(options)
end

assert_includes(options.inspect, "ssl_params")
refute_includes(output, "ssl_params")
end
end
end

Expand Down

0 comments on commit dd0a847

Please sign in to comment.