gomoob / grunt-flyway

Run Flyway database migration tool with Grunt.
MIT License
17 stars 11 forks source link

mask the credentials in the build output #9

Open travi opened 8 years ago

travi commented 8 years ago

no matter how carefully the credentials are passed to the task, they are printed in the build output, where anyone with access to the build can see them. flyway itself is careful to prevent this.

Could something be done to mask at least the credentials in the output?

zero5100 commented 8 years ago

Good idea, I'll look into that when I find some time. You can feel free to submit a patch as well.

travi commented 8 years ago

The PR that I just opened is pretty aggressive, because it ends up removing most of the output. I'm not sure that it is a huge problem because the if check was too specific for the error that was output from the current version anyway.

If you think there is some value to keeping some of the output behind some sort of debug flag, I would suggest at least hiding it by default. I know I made the incorrect assumption that credentials would be hidden by default and only realized it when I was going through the log to understand a different issue, meaning I will need to reset my database url and credentials to keep things secure.

Thanks for being open to getting this handled.

travi commented 8 years ago

@zero5100 did you happen to have thoughts about this PR? I would love to see this pulled in and have a release cut that includes the jars since this is currently blocking me from moving forward with one of my projects. I would normally reference my own branch, but since master does not include the jars, I can't do that easily. I assume the addition of the jars must be part of publishing to npm?

zero5100 commented 8 years ago

@travi I have not taken the time to thoroughly look into the problem yet, but it looks like that PR would not be able to be merged right now as written. It should be pointed at the upstream develop branch instead of master, and we would probably want to add a verbose/debug flag that enables the extra output. I think ideally it would show the normal debug output that it does right now except we would mask the password as ***\ or something. It would probably be safe to leave the username in the output, but that could be moved the verbose output as well if you wanted to be extra safe.

I would encourage you to create and use your own fork for now if it is blocking you; I can't guarantee when any new changes would make it into master. Once #7 is merged into develop and then master the jars will be included.

travi commented 8 years ago

Thank you for the additional information. I agree that truly masking the sensitive information would be ideal, but since the output is currently directly dumped, it would take a bit of fairly specific processing to do that type of masking. I do think that it is important to mask the url, user, and password all need to be masked. Some of these may be less sensitive on an internal build system, but any of these details can be a big deal for a fully public build on Travis or similar.

It sounds like some pieces need to come together for this to be included in a release, but it also sounds like you are on top of it. For now, it probably makes sense for me to close the current PR and let you take the first steps. I'm happy to help out as things move closer, so don't hesitate to ping me at the appropriate time.

zero5100 commented 8 years ago

Sounds good, thanks.