dd-trace-rb icon indicating copy to clipboard operation
dd-trace-rb copied to clipboard

ActiveRecord `describes:` with an invalid value logs full database config

Open hamiltop opened this issue 11 months ago • 2 comments

Current behaviour If I configure the active_record tracing like so:

          Datadog.configure do |c|
            c.tracing.instrument :active_record, describes: 'some invalid string' do |gadget_db|
              gadget_db.service_name = gadget_db_service_name
            end
          end

It understandably logs an error when starting up:

E, [2024-03-14T03:15:30.910202 #1816] ERROR -- ddtrace: [ddtrace] (/app/lib/datadog/tracing/contrib/active_record/configuration/resolver.rb:87:in `rescue in parse_matcher') Failed to resolve ActiveRecord configuration key "some invalid string". Cause: URI::InvalidURIError bad URI(is not URI?): "some invalid string" Source: /usr/local/lib/ruby/3.0.0/uri/rfc3986_parser.rb:67:in `split'

However, as the application continues it will start logging the following:

E, [2024-03-14T03:15:30.912526 #1816] ERROR -- ddtrace: [ddtrace] (/app/lib/datadog/tracing/contrib/active_record/configuration/resolver.rb:68:in `rescue in resolve') Failed to resolve ActiveRecord configuration key {:prepared_statements=>false, :adapter=>"mysql2", :username=>"root", :password=>"root", :port=>3306, :database=>"mysql", :host=>"mysql", :flags=>2}. Cause: NoMethodError undefined method `none?' for true:TrueClass Source: /app/lib/datadog/tracing/contrib/active_record/configuration/resolver.rb:61:in `block in resolve'

This is problematic for two reasons:

  1. It seems like it registered a configuration with matcher true. I believe this is because of this code, which will rescue an error and return true (because Datadog.logger.error returns true).
  2. Because of the invalid matcher, we end up logging the database configuration including the password.

Expected behaviour

  1. Continue to log the resolve error at startup and then not include the bad configuration in matchers.
  2. Do not ever blindly log ActiveRecord matchers (which will contain the db password).

Steps to reproduce This patch adds a test case that will produce the above logs:

diff --git a/spec/datadog/tracing/contrib/active_record/multi_db_spec.rb b/spec/datadog/tracing/contrib/active_record/multi_db_spec.rb
index dca6f8bbd..9e802e1c1 100644
--- a/spec/datadog/tracing/contrib/active_record/multi_db_spec.rb
+++ b/spec/datadog/tracing/contrib/active_record/multi_db_spec.rb
@@ -215,6 +215,22 @@ RSpec.describe 'ActiveRecord multi-database implementation' do
       end
     end
 
+    context 'an invalid value' do
+      context 'for a typical server' do
+        before do
+          Datadog.configure do |c|
+            c.tracing.instrument :active_record, describes: 'some invalid string' do |gadget_db|
+              gadget_db.service_name = gadget_db_service_name
+            end
+          end
+        end
+
+        it do
+          count
+        end
+      end
+    end
+
     context 'a Hash that describes a connection' do
       before do
         widget_db_connection_hash = { adapter: 'sqlite3', database: ':memory:' }

How does ddtrace help you?

Environment

  • ddtrace version: 1.20.0
  • Configuration block (Datadog.configure ...): See above patch
  • Ruby version: All
  • Operating system: Linux
  • Relevant library versions:

hamiltop avatar Mar 14 '24 03:03 hamiltop

Update: It seems that any incorrect value causes logs to leak.

Example: If I specify describes: :replica2 and there is no replica2 entry in datadog.yml I will get this error:

[2024-03-13T22:17:38] ERROR Datadog: Failed to resolve ActiveRecord configuration key :replica2. Cause: ActiveRecord::AdapterNotSpecified The `replica2` database is not configured for the `development` environment.

  Available databases configurations are:

  default
development: primary, replica
production: primary, replica
test: primary, replica
 Source: /Users/peterhamilton/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activerecord-6.1.7.6/lib/active_record/database_configurations.rb:194:in `resolve_symbol_connection'

And then when my code is running I will get:

[2024-03-13T22:17:45] ERROR Datadog: Failed to resolve ActiveRecord configuration key {:prepared_statements=>false, :adapter=>"mysql2", :encoding=>"utf8mb4", :collation=>"utf8mb4_unicode_ci", :pool=>2, :database=>"railsdev", :host=>"127.0.0.1", :username=>"root", :port=>"3306", :password=>"", :variables=>{"sql_mode"=>"NO_AUTO_VALUE_ON_ZERO,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION"}, :flags=>2}. Cause: NoMethodError undefined method `none?' for true:TrueClass Source: /Users/peterhamilton/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/ddtrace-1.13.1/lib/datadog/tracing/contrib/active_record/configuration/resolver.rb:61:in `block in resolve'

Again, passwords leaking into logs is the main problem here.

hamiltop avatar Mar 14 '24 05:03 hamiltop

@hamiltop Thanks for reporting! I will take care of this!

TonyCTHsu avatar Mar 14 '24 13:03 TonyCTHsu