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.

Sign up for GitHub

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

fix: do not expose auth params with Redis 5 #1370

Merged
merged 4 commits into from
Jan 30, 2025
Merged

fix: do not expose auth params with Redis 5 #1370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8090e29
fix: do not expose auth params with Redis 5
zvkemp Jan 27, 2025
cfe9734
Merge branch 'main' into redis-5.x
arielvalentin Jan 30, 2025
ff8f705
Merge branch 'main' into redis-5.x
arielvalentin Jan 30, 2025
4884133
Merge branch 'main' into redis-5.x
kaylareopelle Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions instrumentation/redis/Appraisals
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ appraise 'redis-4.x' do
gem 'redis-client', '~> 0.22'
gem 'redis', '~> 4.8'
end

appraise 'redis-5.x' do
gem 'redis', '~> 5.0'
end
2 changes: 1 addition & 1 deletion instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def serialize_commands(commands)

serialized_commands = commands.map do |command|
# If we receive an authentication request command we want to obfuscate it
if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/)
if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/i)
command[0].to_s.upcase + (' ?' * (command.size - 1))
else
command_copy = command.dup
Expand Down
207 changes: 180 additions & 27 deletions instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
require_relative '../../../../../lib/opentelemetry/instrumentation/redis/patches/redis_v4_client'

describe OpenTelemetry::Instrumentation::Redis::Patches::RedisV4Client do
# NOTE: These tests should be run for redis v4 and redis v5, even though the patches won't be installed on v5.
# Perhaps these tests should live in a different file?
let(:instrumentation) { OpenTelemetry::Instrumentation::Redis::Instrumentation.instance }
let(:exporter) { EXPORTER }
let(:password) { 'passw0rd' }
Expand All @@ -21,12 +23,24 @@
# will generate one extra span on connect because the Redis client will
# send an AUTH command before doing anything else.
def redis_with_auth(redis_options = {})
redis_options[:password] = password
redis_options[:host] = redis_host
redis_options[:port] = redis_port
redis_options[:password] ||= password
redis_options[:host] ||= redis_host
redis_options[:port] ||= redis_port
Redis.new(redis_options)
end

def redis_version
Gem.loaded_specs['redis']&.version
end

def redis_version_major
redis_version&.segments&.first
end

def redis_gte_5?
redis_version_major&.>=(5)
end

before do
# ensure obfuscation is off if it was previously set in a different test
config = { db_statement: :include }
Expand Down Expand Up @@ -95,19 +109,49 @@ def redis_with_auth(redis_options = {})
end

it 'reflects db index' do
skip if redis_gte_5?

redis = redis_with_auth(db: 1)
redis.get('K')

_(exporter.finished_spans.size).must_equal 3

select_span = exporter.finished_spans[1]
_(select_span.name).must_equal 'SELECT'
_(select_span.attributes['db.system']).must_equal 'redis'
_(select_span.attributes['db.statement']).must_equal('SELECT 1')

get_span = exporter.finished_spans.last

_(select_span.attributes['db.system']).must_equal 'redis'
_(select_span.attributes['net.peer.name']).must_equal redis_host
_(select_span.attributes['net.peer.port']).must_equal redis_port
_(select_span.attributes['db.redis.database_index']).must_equal 1

_(get_span.name).must_equal 'GET'
_(get_span.attributes['db.system']).must_equal 'redis'
_(get_span.attributes['db.statement']).must_equal('GET K')
_(get_span.attributes['db.redis.database_index']).must_equal 1
_(get_span.attributes['net.peer.name']).must_equal redis_host
_(get_span.attributes['net.peer.port']).must_equal redis_port
end

it 'reflects db index v5' do
skip unless redis_gte_5?

redis = redis_with_auth(db: 1)
redis.get('K')

_(exporter.finished_spans.size).must_equal 2
select_span = exporter.finished_spans.first
get_span = exporter.finished_spans.last
_(select_span.name).must_equal 'PIPELINED'
_(select_span.attributes['db.statement']).must_equal("AUTH ?\nSELECT 1")

_(select_span.attributes['db.system']).must_equal 'redis'
_(select_span.attributes['net.peer.name']).must_equal redis_host
_(select_span.attributes['net.peer.port']).must_equal redis_port
_(select_span.attributes['db.redis.database_index']).must_equal 1

_(get_span.name).must_equal 'GET'
_(get_span.attributes['db.system']).must_equal 'redis'
_(get_span.attributes['db.statement']).must_equal('GET K')
Expand All @@ -134,6 +178,8 @@ def redis_with_auth(redis_options = {})
end

it 'records exceptions' do
skip if redis_gte_5?

expect do
redis = redis_with_auth
redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG'
Expand All @@ -150,21 +196,68 @@ def redis_with_auth(redis_options = {})
_(last_span.status.code).must_equal(
OpenTelemetry::Trace::Status::ERROR
)

_(last_span.status.description.tr('`', "'")).must_include(
"ERR unknown command 'THIS_IS_NOT_A_REDIS_FUNC', with args beginning with: 'THIS_IS_NOT_A_VALID_ARG"
)
end

it 'records net.peer.name and net.peer.port attributes' do
it 'records exceptions v5' do
skip unless redis_gte_5?

expect do
Redis.new(host: 'example.com', port: 8321, timeout: 0.01).auth(password)
end.must_raise Redis::CannotConnectError
redis = redis_with_auth
redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG'
end.must_raise Redis::CommandError

_(last_span.name).must_equal 'AUTH'
_(exporter.finished_spans.size).must_equal 2
_(last_span.name).must_equal 'THIS_IS_NOT_A_REDIS_FUNC'
_(last_span.attributes['db.system']).must_equal 'redis'
_(last_span.attributes['db.statement']).must_equal 'AUTH ?'
_(last_span.attributes['net.peer.name']).must_equal 'example.com'
_(last_span.attributes['net.peer.port']).must_equal 8321
_(last_span.attributes['db.statement']).must_equal(
'THIS_IS_NOT_A_REDIS_FUNC THIS_IS_NOT_A_VALID_ARG'
)
_(last_span.attributes['net.peer.name']).must_equal redis_host
_(last_span.attributes['net.peer.port']).must_equal redis_port
_(last_span.status.code).must_equal(
OpenTelemetry::Trace::Status::ERROR
)

_(last_span.status.description.tr('`', "'")).must_include(
'Unhandled exception of type: RedisClient::CommandError'
)
end

it 'records net.peer.name and net.peer.port attributes' do
skip if redis_gte_5?

client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01)
expect { client.auth(password) }.must_raise Redis::CannotConnectError

if redis_gte_5?
skip(
'Redis 5 is a wrapper around RedisClient, which calls' \
'`ensure_connected` before any of the middlewares are invoked.' \
'This is more appropriately instrumented via a `#connect` hook in the middleware.'
)
else
_(last_span.name).must_equal 'AUTH'
_(last_span.attributes['db.system']).must_equal 'redis'
_(last_span.attributes['db.statement']).must_equal 'AUTH ?'
_(last_span.attributes['net.peer.name']).must_equal 'example.com'
_(last_span.attributes['net.peer.port']).must_equal 8321
end
end

it 'records net.peer.name and net.peer.port attributes v5' do
skip unless redis_gte_5?

client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01)
expect { client.auth(password) }.must_raise Redis::CannotConnectError

# NOTE: Redis 5 is a wrapper around RedisClient, which calls
# ensure_connected` before any of the middlewares are invoked, so we don't
# have a span here. This is more appropriately instrumented via a `#connect`
# hook in the middleware.
end

it 'traces pipelined commands' do
Expand All @@ -185,10 +278,11 @@ def redis_with_auth(redis_options = {})

it 'traces pipelined commands on commit' do
redis = redis_with_auth
redis.queue([:set, 'v1', '0'])
redis.queue([:incr, 'v1'])
redis.queue([:get, 'v1'])
redis.commit
redis.pipelined do |pipeline|
pipeline.set('v1', '0')
pipeline.incr('v1')
pipeline.get('v1')
end

_(exporter.finished_spans.size).must_equal 2
_(last_span.name).must_equal 'PIPELINED'
Expand Down Expand Up @@ -225,16 +319,17 @@ def redis_with_auth(redis_options = {})
it 'truncates long db.statements' do
redis = redis_with_auth
the_long_value = 'y' * 100
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.queue([:set, 'v1', the_long_value])
redis.commit
redis.pipelined do |pipeline|
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
pipeline.set('v1', the_long_value)
end

expected_db_statement = <<~HEREDOC.chomp
SET v1 yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
Expand Down Expand Up @@ -291,13 +386,39 @@ def redis_with_auth(redis_options = {})
end

it 'omits db.statement attribute' do
skip if redis_gte_5?

redis = redis_with_auth
_(redis.set('K', 'xyz')).must_equal 'OK'
_(redis.get('K')).must_equal 'xyz'
_(exporter.finished_spans.size).must_equal 3

set_span = exporter.finished_spans[0]
_(set_span.name).must_equal('AUTH')
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes).wont_include('db.statement')

set_span = exporter.finished_spans[1]
_(set_span.name).must_equal 'SET'
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes).wont_include('db.statement')

set_span = exporter.finished_spans[2]
_(set_span.name).must_equal 'GET'
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes).wont_include('db.statement')
end

it 'omits db.statement attribute v5' do
skip unless redis_gte_5?

redis = redis_with_auth
_(redis.set('K', 'xyz')).must_equal 'OK'
_(redis.get('K')).must_equal 'xyz'
_(exporter.finished_spans.size).must_equal 3

set_span = exporter.finished_spans[0]
_(set_span.name).must_equal 'AUTH'
_(set_span.name).must_equal('PIPELINED')
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes).wont_include('db.statement')

Expand All @@ -320,13 +441,45 @@ def redis_with_auth(redis_options = {})
end

it 'obfuscates arguments in db.statement' do
skip if redis_gte_5?

redis = redis_with_auth
_(redis.set('K', 'xyz')).must_equal 'OK'
_(redis.get('K')).must_equal 'xyz'
_(exporter.finished_spans.size).must_equal 3

set_span = exporter.finished_spans[0]
_(set_span.name).must_equal('AUTH')
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes['db.statement']).must_equal(
'AUTH ?'
)

set_span = exporter.finished_spans[1]
_(set_span.name).must_equal 'SET'
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes['db.statement']).must_equal(
'SET ? ?'
)

set_span = exporter.finished_spans[2]
_(set_span.name).must_equal 'GET'
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes['db.statement']).must_equal(
'GET ?'
)
end

it 'obfuscates arguments in db.statement v5' do
skip unless redis_gte_5?

redis = redis_with_auth
_(redis.set('K', 'xyz')).must_equal 'OK'
_(redis.get('K')).must_equal 'xyz'
_(exporter.finished_spans.size).must_equal 3

set_span = exporter.finished_spans[0]
_(set_span.name).must_equal 'AUTH'
_(set_span.name).must_equal('PIPELINED')
_(set_span.attributes['db.system']).must_equal 'redis'
_(set_span.attributes['db.statement']).must_equal(
'AUTH ?'
Expand Down
Loading
Loading