Update to the latest version of the crypto refresh#3
Open
lubux wants to merge 47 commits intotwiss:update-packet-versionsfrom
Open
Update to the latest version of the crypto refresh#3lubux wants to merge 47 commits intotwiss:update-packet-versionsfrom
lubux wants to merge 47 commits intotwiss:update-packet-versionsfrom
Conversation
Require the application to load a polyfill instead.
In terms of API, this feature is backwards compatible, no breaking changes. However, since a Wasm module is loaded for the Argon2 computation, browser apps might need to make changes to their CSP policy in order to use the feature. Newly introduced config fields: - `config.s2kType` (defaulting to `enums.s2k.iterated`): s2k to use on password-based encryption as well as private key encryption; - `config.s2kArgon2Params` (defaulting to "uniformly safe settings" from Argon RFC): parameters to use on encryption when `config.s2kType` is set to `enums.s2k.argon2`;
Compared to v5 keys, v6 keys contain additional length fields to aid in parsing the key, but omit the secret key material length field. Additionally, unencrypted v6 secret key packets don't include the count of the optional fields, as per the updated crypto refresh. Since they are always absent, the count is not needed.
Compared to v5 signatures, v6 signatures include a salt, and the subpacket lengths are increased from 2 to 4 bytes.
Store key flags, features and preferences in a direct-key signature instead of user ID signatures, for V6 keys.
The AEAD Encrypted Data packet has been removed from the draft in favor of version 2 of the Sym. Encrypted Integrity Protected Data packet. It also has a new feature flag to match.
This flag has been removed from the draft specification.
Also, set it as the preferred AEAD algorithm.
This subpacket replaces both symmetric algorithm preferences and AEAD algorithm preferences when AEAD is supported, by providing sets of preferred symmetric and AEAD algorithm pairs. We still keep the symmetric algorithm preferences in case AEAD is not supported.
Chrome's Web Crypto implementation doesn't support it, and it seems unnecessary to list it when AES-256 is available.
This has been changed in the crypto refresh.
Key flags, expiration time, algorithm preferences, et cetera, are now read from the direct-key signature instead of the primary User ID binding signature for v6 keys. This also requires a direct-key signature to be present for v6 keys.
The crypto refresh says that we MUST NOT reject messages where the CRC24 checksum is incorrect. So, we remove the check for it. Also, remove the checksumRequired config.
The latest crypto refresh specifies an HKDF step to be used for deriving the key to encrypt the session key with. It also specifies two additional length fields.
Instead of calling getPreferredAlgo('symmetric') and
getPreferredAlgo('aead'), we define and call getPreferredCipherSuite()
to determine the preferred symmetric and AEAD algorithm.
Additionally, we remove isAEADSupported(), instead we return
aeadAlgorithm = 0 from getPreferredCipherSuite() if AEAD is not
supported.
And finally, we define getPreferredCompressionAlgo() to replace
getPreferredAlgo('compression').
Rather than using the config to determine which algorithms to try to decrypt session keys for, try the algorithm we know the message was encrypted with.
Rather than padding in the PKESK packet, add padding inside the public-key algorithms, so that we can later add algorithms without any padding. Additionally, simplify the constant-time decryption logic, by always using the assumed algorithm, rather than doing a constant-time select based on whether the decrypted algorithm identifier was correct.
7eccf32 to
0e7f20c
Compare
Introduces v6 one-pass signature packets required for v6 signatures. Includes the changes from !305 of the crypto refresh: https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/305
0e7f20c to
09f5d0f
Compare
Implement the changes from !304 of the crypto refresh: https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/304 "Introduces an explicit length octet for the fingerprint. This length also covers the key version number. This has the advantage of nicely covering the anonymous recipient case, where a zero length indicates not having a fingerprint in the packet without having to resort to a fictitious key version zero."
The latest version of the crypto refresh (i.e., !313, !314) specifies that the "Hash" header is depricated. An implementation that is verifying a cleartext signed message MUST ignore this header. Thus, the header check can be removed. This commit does not change the writing part of cleartext messages.
09f5d0f to
c261097
Compare
The latest version of the crypto refresh (i.e., !313, !314) specifies that the "Hash" header is depricated. This commit changes that the Hash header is only generated if a cleartext message contains a non-V6 signature.
891d3e7 to
7723b60
Compare
6 tasks
7eceef2 to
d051167
Compare
larabr
reviewed
Aug 4, 2023
| const sizeFields = bytes[offset++]; | ||
| if (sizeFields !== 0) { | ||
| this.publicKeyVersion = bytes[offset++]; | ||
| const fingerprintLength = this.publicKeyVersion ? (this.publicKeyVersion >= 5 ? 32 : 20) : 0; |
There was a problem hiding this comment.
Suggested change
| const fingerprintLength = this.publicKeyVersion ? (this.publicKeyVersion >= 5 ? 32 : 20) : 0; | |
| const fingerprintLength = this.publicKeyVersion >= 5 ? 32 : 20; |
public key version of 0 has been removed in favour of the explicit size
There was a problem hiding this comment.
and we could actually use the sizeFields - 1 value instead of hardcoding the fingerprint length
larabr
reviewed
Aug 4, 2023
| ]; | ||
|
|
||
| if (this.version === 6) { | ||
| if (this.publicKeyFingerprint !== null) { |
There was a problem hiding this comment.
I think it might be easier to follow what's going on by referencing the wildcard keyID?
Suggested change
| if (this.publicKeyFingerprint !== null) { | |
| if (!this.publicKeyID.isWildcard()) { |
larabr
reviewed
Aug 4, 2023
|
|
||
| if (this.version === 6) { | ||
| const saltLength = saltLengthForHash(this.hashAlgorithm); | ||
| if (this.salt === null) { |
There was a problem hiding this comment.
This conditional regeneration seems risky; to double check if needed.
larabr
reviewed
Aug 7, 2023
| return true; | ||
| }; | ||
|
|
||
| function verifyHeaders(headers) { |
d051167 to
79e45df
Compare
|
I've cherry picked the changes to the target branch manually 👍 We'll also continue the review on the original PR since I cannot push to lubux's branch |
e1a4816 to
2231abd
Compare
7498223 to
2a67f02
Compare
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
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.
Includes the updates for the latest merged pull requests in the crypto refresh draft:
Further changes:
createSignaturePacketshas an additional optional argumentsignatureSaltsthat allows to pass previously generated salts. This argument is used for signature creation with ops packets.createSignaturePackethas an additional optional argumentsaltthat allows to pass a previously generated salt.