Closed
Conversation
maraino
suggested changes
May 31, 2023
safebags.go
Outdated
| kdf.Algorithm = oidPBKDF2 | ||
| kdfParams.Salt.Tag = asn1.TagOctetString | ||
| kdfParams.Salt.Bytes = salt | ||
| kdfParams.Iterations = 2048 |
There was a problem hiding this comment.
2048 iterations are very low for current standards.
Consider the following facts:
- In 2017, Nist minimum recommendation was 10,000
- In December 2022, the OWASP Foundation increased its previous recommendation of 310,000 to 600,000 or more.
- 1password uses 650,000.
- LastPass now recommends at least 600,000 - it was very criticized because old users only used 5,000 iterations by default.
My recommendation would be to use 600k by default and add some way to change it.
safebags.go
Outdated
| if err != nil { | ||
| return nil, errors.New("pkcs12: error encoding PBES2 params: " + err.Error()) | ||
| } | ||
| } else if paramBytes, err = asn1.Marshal(pbeParams{Salt: randomSalt, Iterations: 2048}); err != nil { |
There was a problem hiding this comment.
PBES1 will likely use only SHA-1. The OWASP recommendation for PBKDF2-HMAC-SHA1 is 1,300,000 iterations. And PBKDF2 supersedes PBKDF1 which is the one used by PBES1.
Author
|
I've defaulted to 2048 in most places, since that's still what OpenSSL uses, but I've increased SafeEncode to the standard 600,000 iterations. |
This was referenced Jul 10, 2023
AGWA
added a commit
that referenced
this pull request
Jul 14, 2023
Instead of calling pkcs12.Encode, call the Encode method of an Encoder. (Likewise for EncodeTrustStore and EncodeTrustStoreEntries.) The following Encoders are available: * Legacy, for the current behavior. * LegacyDESCerts, to use 3DES for encrypting certificates (Closes #36, Closes #35). * Passwordless, to create password-less trust stores like Java 18 (Closes #10). * Openssl3, to match OpenSSL's modern algorithm choices (Closes #47). * Modern, to always use the most modern choices.
Author
|
Closed in favor of #48 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With the release of OpenSSL v3, openssl now defaults to "AES password based encryption (PBES2 with PBKDF2 and AES-256-CBC) for private keys and certificates, the PBKDF2 and MAC key derivation iteration count of PKCS12_DEFAULT_ITER (currently 2048), and MAC algorithm HMAC with SHA2-256."
https://www.openssl.org/docs/man3.0/man3/PKCS12_create.html
All other formats are now considered legacy.
This PR builds on the configuration work done in #39 , simply adding support for PBES2 as an algorithm choice.
This doesn't support encoding in every algorithm that could be used for PBES2, but only the exact format used by OpenSSL.
Other formats are left as a future enhancement, and will likely require a rethink of the interface used to select algorithms in #39.
Additionally this solves #44 by bumping the
x/cryptodependency to0.9.0. This appears to work without any changes required.