search

liquibase / node-liquibase

Node.js wrap for Liquibase
MIT License
32 stars 14 forks source link

Logger isn't configurable & logs out database password #26

Closed ramblingenzyme closed 3 years ago

ramblingenzyme commented 3 years ago
tabuckner commented 3 years ago

hey @ramblingenzyme I just did some quick testing of the Liquibase CLI and I'm getting the same output from both node-liquibase and Liquibase with a logLevel of 'off'.

I think I understand what you're requesting here but want to be absolutely certain.

There was a typo in our Issue Template, so I don't have quite enough information to move on this Issue yet.

If you close this issue and create a new one, the template will load prompting you for the info I need. If you dont want to open a new PR I've attached the PR template below this message. Just fill out that info in a new comment and I can 🚀 .

Thanks!

Context

Node-Liquibase Version

Liquibase Version (if not bundled)

Description

[Description of the bug or feature]

Steps to Reproduce

  1. [First Step]
  2. [Second Step]
  3. [and so on...]

Expected behavior: [What you expected to happen]

Actual behavior: [What actually happened]

tabuckner commented 3 years ago

@ramblingenzyme just spoke to the Liquibase Core team, the Liquibase CLI is designed to work in this way. Here's a quick excerpt from @nvoxland

The default logLevel is OFF for the CLI The logs are separate from the "UI interface" so turning log-level=off will still be showing the "UI messages" and not turn of all output

Once I have some clarification on your expected behaviors I think you and I can work towards a solution.

ramblingenzyme commented 3 years ago

Hi @tabuckner, I'm talking about the Logger in node-liquibase which only uses process.env.NODE_ENV to decide the log level here and doesn't seem to work properly. https://github.com/liquibase/node-liquibase/blob/master/src/util/logger.ts

The problem is that it logs out the generated command string and includes the password value in the output. The offending log statement is here https://github.com/liquibase/node-liquibase/blob/master/src/util/command-handler.ts#L6

tabuckner commented 3 years ago

The Bug

@ramblingenzyme Thank you for the feedback! You were absolutely right that the LogLevel was not being passed down to the Logger correctly.

That bug is resolved by https://github.com/liquibase/node-liquibase/pull/27, which is now merged to master. The release for this fix will be live by EOD today.

Obfuscation

I do worry, though, that this will not completely satisfy all of your requests. For example, with the Liquibase CLI if I pass a password or other sensitive information to the CLI via flags, that content will not be obfuscated at any LogLevel. node-liquibase is designed as a very thin Node wrapper around Liquibase.

If obfuscating sensitive information without suppressing all CLI output is a requirement for your use case, I would suggest using Environment Variables. Most (all?) CI/CD platforms (e.g. Travis, CircleCI, GitHub Actions) support Environment Variable usages, and this would both satisfy your requirements and take another step into the word of 12 Factor Apps.

Because of this workaround, and the purpose of node-liquibase I don't feel comfortable implementing some custom logic that obfuscates different properties that are passed to the CLI.

ramblingenzyme commented 3 years ago

Hi @tabuckner Thanks for that. What I've setup is liquibase running on the startup of a deployed service, not running in CI, so the password is already passed through environment variables, but I'm unaware if Cloudwatch provides any type of redaction.

I understand your position though, and I'm happy to leave it at that. The only thing I might suggest is to change the level of the log of the command string from log down to debug?