Update the Default Digest for Envelope Signing to SHA2-256 #44
Conversation
defaults.go
Outdated
| // ContentEncryptionAlgorithm determines the algorithm used to encrypt the | ||
| // plaintext message. Change the value of this variable to change which | ||
| // algorithm is used in the Encrypt() function. | ||
| var ContentEncryptionAlgorithm = EncryptionAlgorithmAES256CBC |
There was a problem hiding this comment.
The CBC mode is increasingly deprecated (e.g. dropped in TLS 1.3 and a number of other products). I suggest changing the default to EncryptionAlgorithmAES256GCM
There was a problem hiding this comment.
PKCS7 is often used in systems/environments that require compatibility with older systems. I've seen it fail with users trying to use SCEP with GCM, so defaulting to that (in this library, with existing systems relying on this default automatically) will likely be a good way to break things.
Using a different encryption algorithm is on the docket for v2.
defaults_fips.go
Outdated
| // ContentEncryptionAlgorithm determines the algorithm used to encrypt the | ||
| // plaintext message. Change the value of this variable to change which | ||
| // algorithm is used in the Encrypt() function. | ||
| var ContentEncryptionAlgorithm = EncryptionAlgorithmDESCBC |
There was a problem hiding this comment.
DES is not secure under any mode of operation and should not be used. Consider alternatives such as AES256GCM
| // plaintext message. Change the value of this variable to change which | ||
| // algorithm is used in the Encrypt() function. | ||
| var ContentEncryptionAlgorithm = EncryptionAlgorithmDESCBC | ||
|
|
||
| // ErrUnsupportedEncryptionAlgorithm is returned when attempting to encrypt | ||
| // content with an unsupported algorithm. | ||
| var ErrUnsupportedEncryptionAlgorithm = errors.New("pkcs7: cannot encrypt content: only DES-CBC, AES-CBC, and AES-GCM supported") |
There was a problem hiding this comment.
In line with the comment above, DES-CBC must be deprecated.
defaults_fips.go
Outdated
| // SignatureAlgorithm determines the algorithm used to sign the message. | ||
| // Change the value of this variable to change which algorithm is used in | ||
| // the PKCS7 Envelope signing | ||
| var SignatureDigestAlgorithm = OIDDigestAlgorithmSHA1 |
There was a problem hiding this comment.
What is the reason to maintain SHA1 here? Consider using a different hashing algorithm, such as SHA256
There was a problem hiding this comment.
Ah, this is the fips one. Yeah, that probably can be updated.
Name of feature:
Default Digest Update
Pain or issue this feature alleviates:
SHA1 is no longer allowed for signature digests under FIPS 140-3 and FIPS 140-2 goes end of life in 2026. Most cryptographic backends no longer allow the use of SHA1 as a digest (for example Go doesn't even allow it, it requires the ENV Variable).
Allowing people to use and support SHA1 is fine, but it definitely should not be the default.
Why is this important to the project (if not answered above):
In the spirit of go - Defaults should be safe defaults.
Is there documentation on how to use this feature? If so, where?
No functional change
In what environments or workflows is this feature supported?
SCEP is my main familiarity with PKCS7, but any PKCS7 usage with a signed envelope
In what environments or workflows is this feature explicitly NOT supported (if any)?
Extremely legacy environments (i.e. pre-2003), however they can use the digest algorithm setter as documented.
Supporting links/other PRs/issues:
#27
Less configurable, but simpler approach than here:
https://github.com/smallstep/pkcs7/pull/29/files
💔Thank you!