Closed schicks closed 1 year ago
epugh
commented
1 year ago Thanks @schicks for this... All of this is part of Rails... Do you have any pointers to how to prevent this? I'm actually a bit surprised sicne there is a lot of "sensistive content being hidden" stuff available in Rails...
schicks
commented
1 year ago Honestly I don't know a whole lot about rails. My best guess would be to change the database config so that the prod case looks more like the general case. if rails is trying to hide sensitive content, having the password split out into its own supplied field seems like a good way to communicate that this field should be hidden, which might be hard to identify when the db connection string is given all as one variable.
Another, simpler approach would be to change the log level to be configurable via an environment variable, so that we could just suppress all warning logs in production to avoid the issue.
olivierlacan
commented
1 year ago Edit: misunderstood the issue, see later comment.
Try to add :database_url (it uses a regex match) to Rails.application.config.filter_parameters in https://github.com/o19s/quepid/blob/34d6e45a929dc8626b3aeeb8743de5c1c6d2dc8b/config/initializers/filter_parameter_logging.rb#L6
I can't remember for sure if the Rails logger abides by filter_parameters but it really should even outside request/response cycle calls to the logger.
Should be fairly easy to test.
epugh
commented
1 year ago I am a bit surpirsed this isn't a default convention in Rails! Patch to rails ? ;-)
olivierlacan
commented
1 year ago I am a bit surpirsed this isn't a default convention in Rails! Patch to rails ? ;-)
I'm pretty sure the common database drivers out there filter DATABASE_PASSWORD and DATABASE_URL env vars from logs since it's a common way to configure Rails apps, notably on Heroku.
So I'm not sure why the leak occurred here but I haven't looked very deeply in the configuration yet.
olivierlacan
commented
1 year ago Just realized we're talking about the value of ENV["DATABASE_URL"] here, not the key DATABASE_URL which would get you nowhere. So generally, I'd say it's an extremely bad idea to ever log out DATABASE_URL itself and probably why Rails doesn't default to it.
If you want to log database connection details at best I would try to retrieve whichever DB_HOST (not sure why the prefix is different but that's what you have in config/database.yml) is configured, not the DATABASE_URL.
Otherwise, I'd recommend retrieving the host from the application configuration interface instead perhaps.
You can retrieve that from Rails.configuration.database_configuration at runtime but it is a Ruby object:
{"default"=>{"adapter"=>"mysql2", "encoding"=>"utf8mb4", "pool"=>5},
"development"=>
{"adapter"=>"mysql2",
"encoding"=>"utf8mb4",
"pool"=>5,
"database"=>"development_db_name",
"host"=>"localhost",
"username"=>"olivierlacan",
"password"=>nil},
"test"=>
{"adapter"=>"mysql2",
"encoding"=>"utf8mb4",
"pool"=>5,
"host"=>"localhost",
"database"=>"test_db_name"},
"production"=>
{"adapter"=>"mysql2", "encoding"=>"utf8mb4", "pool"=>5, "url" => "mysql2://user:my_weird^password@mysql:3306/quepid"}}So I'm not sure what is logging out the full DATABASE_URL, could be the mysql2 gem somehow, but if this is something quepid is specifically logging out somewhere, you could do something like:
if Rails.env.production?
logger.info("Failed to connect to database")
elsif
logger.info("Failed to connect to database: #{Rails.configuration.database_configuration[Rails.env]}")
end It would only print out the environment-relevant database config in other environments than production, which would prevent the issue @schicks is having here.
olivierlacan
commented
1 year ago @epugh Sadly, I don't think this issue is fixable by quepid.
As you suggested this seems like something Rails should handle actually. It probably should rescue this URI exception from Ruby to avoid accidental credential leakage.
Might be something to run by Rails folks, I'll open an issue there to see what they think.
I reproduced the issue in development with the following:
# config/database.yml
# ...
development:
<<: *default
database: <%= ENV["DB_NAME"] %>_development
url: <%= ENV["DATABASE_URL"] %># .env
# ...
# DB_HOST=mysql
# DB_USERNAME=root
# DB_PASSWORD=password
# DB_NAME=quepid
DATABASE_URL=mysql2://user:my_weird^password@mysql:3306/quepidThen started development server (after following the excellent README setup instructions):
$ bin/docker server! Unable to load application: URI::InvalidURIError: bad URI(is not URI?): mysql2://user:my_weird^password@mysql:3306/quepid
bundler: failed to load command: puma (/usr/local/bundle/bin/puma)
/usr/local/lib/ruby/3.2.0/uri/rfc2396_parser.rb:176:in `split': bad URI(is not URI?): mysql2://user:my_weird^password@mysql:3306/quepid (URI::InvalidURIError)
from /usr/local/lib/ruby/3.2.0/uri/rfc2396_parser.rb:210:in `parse'
from /usr/local/bundle/gems/activerecord-6.1.7.2/lib/active_record/database_configurations/connection_url_resolver.rb:26:in `initialize'Since Rails defers to Ruby's URI class to parse this DATABASE_URL value, Rails or more specifically ActiveRecord would need to rescue the specific URI::InvalidURIError here to avoid leakage and sanitize output before re-raising the exception.
That might not be something Rails maintainers are willing to take on but if @schicks's DATABASE_URL was actually a valid URL, then... folks we might have a Ruby bug on our hands. Although since the exception is coming straight out of Ruby 3.2.0's URI class RFC2369 parser I would be very surprised.
rbwrightjr91
commented
1 year ago That might not be something Rails maintainers are willing to take on but if @schicks's DATABASE_URL was actually a valid URL, then... folks we might have a Ruby bug on our hands.
@olivierlacan Our password does have characters that, when passed directly, result in an invalid URI (notably the ^ character). In other areas of our codebase, we get around this by URI Encoding the password and I'm sure we could do that here as well. However, we still thought it prudent to bring up the issue as it seems totally reasonable that someone would not think to URI Encode the password while configuring this for the first time.
olivierlacan
commented
1 year ago @rbwrightjr91 Yeah, I created a repro repo to suggest this might be a good idea to do within ActiveRecord. In the meantime URI encoding the whole URL or just the password should work.
I'll see what Rails folks think, but from quick research it's a fairly common issue with DB URLs across languages/frameworks.
olivierlacan
commented
1 year ago As a circumvention of this issue, quepid could allow the log level to be set via an environment variable which is a default that was just added to the upcoming Rails 7.1 (not yet released):
# in config/environments/production.rb
config.log_level = ENV.fetch("RAILS_LOG_LEVEL") { "info" }This would allow anyone running quepid via a Docker image to pass RAILS_LOG_LEVEL=warn in order to silence higher log levels like info.
The available log levels are: :debug, :info, :warn, :error, :fatal, and :unknown, corresponding to the log level numbers from 0 up to 5, respectively. To change the default log level, use https://guides.rubyonrails.org/debugging_rails_applications.html#log-levels
schicks
commented
1 year ago So having now tried that change, it does not seem to fix the problem. As far as we can tell this is because the problematic log message is an error thrown from active record in some context that does not respect the rails log level even when it is configured.
olivierlacan
commented
1 year ago That makes a lot of sense, any error monitoring would leak this stuff as well. This makes me think opening a Rails issue to see if ActiveRecord can rescue these exceptions more cleanly is the best solution.
schicks
commented
1 year ago In that context, I think it's pretty clear this is not really a Quepid issue so I'm going to go ahead and close this thread.
epugh
commented
1 year ago Thank you @schicks and @olivierlacan for working through this issue! Seeing you'all workign through this made me feel like we're finally building out a qommunity that mutual supports each other!
I do want to get us to Rails 7 soon, for many reasons, one of which is that security fixes will come to us.
Describe the bug If the database connection URI does not parse as a URI, an error is thrown containing the whole content of the connection string. This can include a password.
To Reproduce Steps to reproduce the behavior:
DATABASE_URL=mysql2://user:my_weird^password@mysql:3306/quepidExpected behavior The same error would be thrown but would reference the
DATABASE_URLby name rather than showing the value