fix: Allow empty passwords for encrypted pem files

[gmpinder]

Summary

I've been working on integrating this crate into my CLI program and I ran into a situation where pem files generated by the cosign tool with an empty password couldn't be loaded. I was getting an invalid key type error, so I dug into why that was and found that the keys were being processed differently if the password had no length. Removing that constraint allowed my empty password cosign keys to be loaded properly. I'm hoping this change will be accepted so that this rust implementation is a bit closer to parity with the cosign cli tool.

Closes #380

Release Note

• Allows the use of encrypted PEM files with an empty password to keep in parity with cosign
• Allows reading cosign generated encrypted pem files with a different scrypt N parameter

Documentation

N/A

[gmpinder]

After doing some more testing, there seems to be another spot that's preventing this from working with empty password cosign generated keys. Working on figuring that out

[gmpinder]

Ok so I found the other issue, there was apparently a hard check for the N param for the kdf encryption of the pem. I've removed it for now since cosign appears to use a value of 65536 which is what was causing the key to not be able to be read.

example test cosign pem:

{"kdf":{"name":"scrypt","params":{"N":65536,"r":8,"p":1},"salt":"/67J9VwY8a6ravOCALfO1P3NGD4Xsk6/M9hNbaXC7+A="},"cipher":{"name":"nacl/secretbox","nonce":"/ht529R6XgnAFmUz/u4jyQTMeom4T6MT"},"ciphertext":"ZJYYk2GQaZgJtHzS0JtUnMhSnQWsnGpD3a5B23weYKodDo2dxUNexEIDp88FRC3jURM4b56EHEcnVUZaDL3cgfkN7Meeo18VYSvZmwI7XhRZs119G6yifi8HqZFbgI3mQwNv0EDp1DzL4wfIZ0pyP+xA/3lNy9myfRd6R3RhGHyIkz5QxxrvZ0uTvGTLNrgKHus/sSn+5ZKl/Q=="}

[flavio]

I'm fine with the changes done to allow empty passwords.

@Xynnn007: would you mind taking a look too?

[Xynnn007]

The logic looks good to me. I relooked inside the cosign code and behavior and ensure that if no password is given, still a encrypted privatekey will be given rather than a plaintext privatekey. The encryption key is derived from "" with scrypt.

[loosebazooka]

@woodruffw brought this up in the sigstore clients meeting today. Two points to consider here:

1. Empty password vs unencrypted: Empty password may lead to a assumption of stronger security
2. Should sigstore-rs even be generating long lived keys (and serializing them to PEM)? Should this instead be the job of another tool?

Also, perhaps the behavior in cosign should be changed.

[woodruffw]

(Thanks @loosebazooka! I forgot to comment on this earlier.)

• Empty password vs unencrypted: Empty password may lead to a assumption of stronger security

To elaborate a bit more on this concern: IMO there's a security risk with enabling people to encrypt with the empty password, insofar as the "encrypted" version is indistinguishable from a version with an actual, strong password. In other words: a user who lacks a full picture of the system may assume that any PEM blob with the BEGIN ENCRYPTED SIGSTORE PRIVATE KEY tag is safe to leak/commit to a public source, without realizing that the encryption is effectively nonexistent.

I think the "fix" involves both cosign and sigstore-rs:

1. It looks like sigstore-rs (and maybe cosign too?) generates key-bearing PEMs with the ENCRYPTED tag unconditionally, even when the private key isn't actually encrypted. When the key is actually unencrypted, the PEM tag probably shouldn't include that marker.
2. Rather than encrypting with an empty password (when an empty one is explicitly provided), IMO sigstore-rs and cosign should refuse the operation and fail.

(2) is arguably a slippery slope towards "weak" password/entropy testing, which is an anti-pattern. But IMO stopping at empty passwords is perfectly fine; that's the point of having a strong memory-hard hash function like scrypt 🙂

[gmpinder]

(Thanks @loosebazooka! I forgot to comment on this earlier.)

• Empty password vs unencrypted: Empty password may lead to a assumption of stronger security

To elaborate a bit more on this concern: IMO there's a security risk with enabling people to encrypt with the empty password, insofar as the "encrypted" version is indistinguishable from a version with an actual, strong password. In other words: a user who lacks a full picture of the system may assume that any PEM blob with the BEGIN ENCRYPTED SIGSTORE PRIVATE KEY tag is safe to leak/commit to a public source, without realizing that the encryption is effectively nonexistent.

I think the "fix" involves both cosign and sigstore-rs:

1. It looks like sigstore-rs (and maybe cosign too?) generates key-bearing PEMs with the ENCRYPTED tag unconditionally, even when the private key isn't actually encrypted. When the key is actually unencrypted, the PEM tag probably shouldn't include that marker.
2. Rather than encrypting with an empty password (when an empty one is explicitly provided), IMO sigstore-rs and cosign should refuse the operation and fail.

(2) is arguably a slippery slope towards "weak" password/entropy testing, which is an anti-pattern. But IMO stopping at empty passwords is perfectly fine; that's the point of having a strong memory-hard hash function like scrypt 🙂

What would happen to the users who have already generated a cosign key with an empty password? Would that just stop working all of a sudden? Would those users then have to create new keys and then update their users to know that they have a different key? I feel like changing that now could break compatibility with previous use cases.

At the end of the day, this is y'all's project, so it's your decision. I just want to voice my concerns.

[woodruffw]

What would happen to the users who have already generated a cosign key with an empty password?

There would definitely need to be a transition period here, at least for cosign (since AFAIK they follow strict SemVer). Maybe something like this:

1. Loading empty-password keys continues to work, with a warning, until the next major version (3.x), when it becomes a failure.
2. Generating empty-password keys becomes a hard error immediately, since cosign generate-key-pair is already allowed to fail as part of its invocation contract

For sigstore-rs, I don't think a transition period is needed: the project is still explicitly under active development and is not considered stable.

Do you think that would accomodate your use case?

[gmpinder]

Do you think that would accomodate your use case?

Not really as we have hundreds of users of our system that has been relying on using those empty cosign passwords. I don't see why breaking user-space to remove something that cosign allows in the first place is a good idea. If the cosign team wants to make that breaking change, then so be it. Until then, I think sigstore-rs should at least accommodate the use-cases that cosign has created just so that interoperability can exist, because otherwise we will have to stop using this library altogether so that we don't break our user's builds.

[Xynnn007]

For compatibility, we probably can judge the reading logic of the keys by pem header, s.t.

1. if the private key starts with SIGSTORE ... and no password is given, will use empty password to derive key to decrypt
2. if the private key starts with PRIVATE ... and no password is given, it will be treated as a plaintext key
3. if the private key starts with SIGSTORE ... and some password is given, will use the password to derive key to decrypt
4. if the private key starts with PRIVATE ... and some password is given, will raise an error to show that the key is not encrypted.

case 2 is for compability. For key generation logic, we could turn to new logic, s.t. empty password still derive a key to encrypt.

[gmpinder]

For compatibility, we probably can judge the reading logic of the keys by pem header, s.t.

1. if the private key starts with SIGSTORE ... and no password is given, will use empty password to derive key to decrypt
2. if the private key starts with PRIVATE ... and no password is given, it will be treated as a plaintext key
3. if the private key starts with SIGSTORE ... and some password is given, will use the password to derive key to decrypt
4. if the private key starts with PRIVATE ... and some password is given, will raise an error to show that the key is not encrypted.

case 2 is for compability. For key generation logic, we could turn to new logic, s.t. empty password still derive a key to encrypt.

I can add that logic in another commit. Are there any changes that you think I should make to my first two commits?

[gmpinder]

Any update on whether this will be merged in?

[Xynnn007]

Thanks for this! More comments about code style.

[gmpinder]

Cool, I can work on these this weekend

[gmpinder]

Alright, got the tests updated.

[gmpinder]

Got the last few comments resolved

[Xynnn007]

@gmpinder Thanks for your adopting the suggestions! Let's see how CI works

[Xynnn007]

Good. Passed

[Xynnn007]

LGTM. ptal @flavio

[gmpinder]

Awesome! Would you be able to get these changes out in a v0.9.1 patch so my crate is able to make use of it?

[flavio]

@gmpinder JFYI, this is going to be part of 0.10.0, which we expect to release soon (see #391)