Resolves issue #2 - Why static salt?

jeff-winn commented 5 years ago

This pull request is to correct issue #2 wherein the salt used is static rather than a randomized value.

About the change... I had to do a bit of restructuring on the code to support what needed to be done, so there will be quite a few new files added, and quite a bit of the logic that was within Main has been lifted out to other classes. Hopefully this will make the tool easier to maintain and make changes to in the future.

A couple things:

I made sure tool still retains backwards compatibility for decrypt operations only, so it should be a minor upgrade rather than a major. It will also output a warning to the console (thereby showing up in builds) that a static salt was used to decrypt the value. I hope this will get people to re-encrypt their data.
The salt should be stored within secure environment variables along side the secret and passed into the tool. I've already made the necessary documentation changes to the README.md for this. Normally you'd store the salt near the value, but given the value is a file I made the assumption using another secure environment variable would be better.
I added FIPS compliance support if required people can use the new -fips option to enable this. This will cause it to use AES instead of Rijndael. Some companies may require using FIPS compliant algorithms.

If you have any questions, please feel free to ask!

FeodorFitsner commented 5 years ago

What if we re-target the project to .NET Core 3.0 and try publishing it as a single self-contained executable? It's quite inconvenient that the tool requires the presence of .NET Core runtime on the machine.

jeff-winn commented 5 years ago

You could very well choose to do so, I did a quick regression test targeting this for netcoreapp3.0 and it was able to decrypt the files using your existing deployed version along with encrypting and decrypting new files (using this version + .NET Core 3.0). So it should be fine if you wish to modify the target runtime in a separate change.

jeff-winn commented 5 years ago

Just out of curiosity, are there plans to merge this or should I close the request?

FeodorFitsner commented 5 years ago

Sorry about the delay with reviewing this PR! We've been busy with releasing some new stuff here. I'm going to finish with the PR this week.

sehcheese commented 5 years ago

@FeodorFitsner Will you give us a heads up when this is live? Or is it live already?

FeodorFitsner commented 5 years ago

It should be live already, but it's fully backward compatible and shouldn't break existing builds. That's the trick.

jeff-winn commented 5 years ago

I’m pretty sure your website documentation indicates to use the install.ps1 which is currently pointed at your 1.0.0 version. Which I didn’t update that, I wouldn’t have had access to create the release.

FeodorFitsner commented 5 years ago

Correct. Release artifact was just updated with a newer version.

appveyor / secure-file

Resolves issue #2 - Why static salt? #8