Skip to content

fix: handle encoding errors in mysql obfuscation #160

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

Merged
robertlaurin merged 4 commits into from
Oct 26, 2022
Merged

fix: handle encoding errors in mysql obfuscation #160

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
cbcad76
fix: handle encoding errors in mysql obfuscation
robertlaurin Oct 24, 2022
bab4c08
fix: db name extraction failures
robertlaurin Oct 25, 2022
6df0122
fix: handle binary encoding
robertlaurin Oct 25, 2022
007b657
Merge branch 'main' into fix-obfuscation-encoding-error
robertlaurin Oct 25, 2022
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
8 changes: 6 additions & 2 deletions instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/patches/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module Instrumentation
module Mysql2
module Patches
# Module to prepend to Mysql2::Client for instrumentation
module Client
module Client # rubocop:disable Metrics/ModuleLength
QUERY_NAMES = [
'set names',
'select',
Expand Down Expand Up @@ -73,10 +73,13 @@ def obfuscate_sql(sql)
if sql.size > 2000
'SQL query too large to remove sensitive data ...'
else
obfuscated = sql.gsub(generated_mysql_regex, '?')
obfuscated = OpenTelemetry::Common::Utilities.utf8_encode(sql, binary: true)
Copy link
Contributor Author

@robertlaurin robertlaurin Oct 25, 2022

Choose a reason for hiding this comment

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

We're setting binary to true in redis, and dalli.

opentelemetry-ruby-contrib/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb

Line 70 in 6df0122

OpenTelemetry::Common::Utilities.utf8_encode(serialized_commands, binary: true)

opentelemetry-ruby-contrib/instrumentation/dalli/lib/opentelemetry/instrumentation/dalli/utils.rb

Line 57 in 6df0122

command = OpenTelemetry::Common::Utilities.utf8_encode(command, binary: true, placeholder: placeholder)

So I think it makes sense here.
https://github.com/open-telemetry/opentelemetry-ruby/blob/18bfd391f2bda2c958d5d6935886c8cba61414dd/common/lib/opentelemetry/common/utilities.rb#L40-L63

obfuscated = obfuscated.gsub(generated_mysql_regex, '?')
obfuscated = 'Failed to obfuscate SQL query - quote characters remained after obfuscation' if detect_unmatched_pairs(obfuscated)
obfuscated
end
rescue StandardError
'OpenTelemetry error: failed to obfuscate sql'
Copy link
Contributor

@ahayworth ahayworth Oct 25, 2022

Choose a reason for hiding this comment

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

Is it possible to make a more specific error message here?

Copy link
Contributor Author

@robertlaurin robertlaurin Oct 25, 2022

Choose a reason for hiding this comment

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

I wasn't sure how specific to make it, this is what will get plunked into the the db.statement attribute field. I'm being prudent about capturing any information about what was failed to be obfuscated.

What did you have in mind? Like the error class or something?

Copy link
Contributor

@ahayworth ahayworth Oct 25, 2022

Choose a reason for hiding this comment

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

Ah, I didn't realize that this would get put into the db.statement field, which is my bad for not looking closely enough. I think this is fine. 👍

Copy link
Contributor

@fbogsany fbogsany Oct 25, 2022

Choose a reason for hiding this comment

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

Probably not something you need to address here, but it'd be useful to use a common prefix for all the "error" type substitutions (this and the two above) - it'd make it easier for Observability teams to monitor this behaviour. Metrics might be nice as well, but a competent engineer can build metrics from the db.statement in a span processor or in the collector.

end

def generated_mysql_regex
Expand Down Expand Up @@ -142,6 +145,7 @@ def extract_statement_type(sql)
QUERY_NAME_RE.match(sql) { |match| match[1].downcase } unless sql.nil?
rescue StandardError => e
OpenTelemetry.logger.debug("Error extracting sql statement type: #{e.message}")
nil
Copy link
Contributor Author

@robertlaurin robertlaurin Oct 25, 2022

Choose a reason for hiding this comment

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

This error is covered by my new test.

end
end
end
Expand Down
13 changes: 13 additions & 0 deletions instrumentation/mysql2/test/opentelemetry/instrumentation/mysql2/instrumentation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,19 @@
_(span.attributes['net.peer.name']).must_equal host.to_s
_(span.attributes['net.peer.port']).must_equal port.to_s
end

it 'encodes invalid byte sequences for db.statement' do
# \255 is off-limits https://en.wikipedia.org/wiki/UTF-8#Codepage_layout
sql = "SELECT * from users where users.id = 1 and users.email = '[email protected]\255'"
obfuscated_sql = 'SELECT * from users where users.id = ? and users.email = ?'

expect do
client.query(sql)
end.must_raise Mysql2::Error

_(span.name).must_equal 'mysql.mysql'
_(span.attributes['db.statement']).must_equal obfuscated_sql
end
end

describe 'when enable_sql_obfuscation is enabled with db_statement set' do
Expand Down