Why static salt?

[rgl]

Why are you using a static salt?

[rgl]

is the code from this repo what is actually used at https://ci.appveyor.com/tools/encrypt?

[FeodorFitsner]

Not exactly. Any recommendation on how to improve that?

[rgl]

The salt should be crypto random generated (see Salt_(cryptography)) and saved with the secret. For example, passwd creates a strucured secret as $id$salt$hashed (see Passwd#Shadow_file).

How exactly is that tools/encrypt page generating the secret?

[ericnewton76]

I dont think you can use a random salt, since you have to have a way to get same value back eventually. Maybe i'm wrong.

UPDATE: Yeah, so you'll never get the resultant if you dont have a stable salt. You could utilize the salt/pepper method, and save the pepper as an secure variable, but you already have secret... so not really much point there.

So you have to have a static salt in order to reverse the encryption, which is the point of secure-file

[rgl]

You are not wrong, you need to have the salt available. But you also need to understand why you need a dynamic salt (read the links I've posted before).

[jeff-winn]

The issue with static salt is you’ve already compromised half of the encryption to a potential attack.

The salt does not need to be static, you could just as easily require us to encrypt two pieces of data into our YAML files. One piece of information would be the secret (essentially the passphrase passed to the PBKDF2 function) and a second piece of data being the salt also used with the PBKDF2 function to generate the hash. Both values get passed into the decrypt routine thereby unlocking the file.

I’ll see if I can find time to take a crack at this, it’s being used to “secure” my keys right now and it’d be much easier to crack if the salt is already known to the attacker.

Process wise, I like what you did here. Allowing the files to be encrypted within the repository answered a pretty big how for me on the problem I was working on.

[jeff-winn]

I resubmitted the request under PR #8 to address this particular issue. I've also already tested it on one of my builds for both backwards compatibility along with testing it using the upgraded version.

Here's the YAML file I was testing the builds with as I checked out the modified version. https://github.com/winnster/AutomationFoundation/blob/master/appveyor.yml

Before: You can see the warning appeared as expected below during the build process indicating the static salt was used.

After: Here you can see the salt was passed in via an environment variable along with the secret and the warning was no longer a problem.

[jeff-winn]

If you're using something similar on your website to encrypt the data for our YAML files, you'll likely want to address that as well. Which off-hand (without seeing any of it) you probably have a couple options:

• Use RSA to encrypt on the website and use the same RSA key deployed so whatever process is setting up the VMs and configuring the environment variables has access to it. So long as this is something external to the build server VMs that we run our logic on it should stay safe. You won't need to deal with the salt using this option. This is the strongest option, but handling keys can be a pain. This algorithm is also intentionally slow and has size limits to how much data it can encrypt.
• Given we can't see the salt used to encrypt the values via your website, you'd probably have to put a randomized salt at the account or organization level to keep accounts using different salt while keeping everyone safe.

If it were me I'd recommend going the RSA route, as that would be the safest path for your customers. The only machines that could decrypt the values in our YAML files would then be within your network where the RSA keys have been deployed.

[jeff-winn]

Glad you were able to find time! I know releases can be fun at times.

Just need to get it released now, and possibly update the Chocolatey package and this issue will be done with. Yay!

[jeff-winn]

I just redirected both of my builds back to using your install script out of master and everything checked out. So using it with the salt works as expected!

[jeff-winn]

With the code merged and the issue resolved, this issue could now be closed.

Thank you for helping get it merged so others could make use of it!