Replace strings with integer algorithm identifiers in packet classes #1410
Conversation
| @@ -117,8 +117,7 @@ class S2K { | |||
| if (this.type === 'gnu-dummy') { | |||
| return new Uint8Array([101, 0, ...util.stringToUint8Array('GNU'), 1]); | |||
| } | |||
|
|
|||
| const arr = [new Uint8Array([enums.write(enums.s2k, this.type), enums.write(enums.hash, this.algorithm)])]; | |||
| const arr = [new Uint8Array([enums.write(enums.s2k, this.type), this.algorithm])]; | |||
|
|
|||
| switch (this.type) { | |||
There was a problem hiding this comment.
It'd be nice to change this as well, but I didn't for now since we treat the gnu-dummy key as a special case, and I think it would require re-thinking more logic.
src/packet/aead_encrypted_data.js
Outdated
| this.cipherAlgorithm = enums.write(enums.symmetric, await reader.readByte()); | ||
| this.aeadAlgorithm = enums.write(enums.aead, await reader.readByte()); |
There was a problem hiding this comment.
I would prefer not to get into the habit of using enums.write for checking an integer value. I know it supports it, but I think we should get a bit more strict about the types, and use read only for int -> string, and write only for string -> int. And then we can do validation in some other way - for example, in this case we will already throw below, in the read call in getAEADMode, if the AEAD mode is unknown (and similarly for the cipher, but during decryption).
There was a problem hiding this comment.
I strongly disagree about doing optimistic parsing, I think we should detect wrong enums ASAP and not let this propagate to when we use the packet, with potentially unexpected side effects. It also makes it much easier to debug issues (as opposed to now). I'd rather add an enum function to validate the enum value -- so we also don't need to make exceptions for certain packets (about "where to throw").
There was a problem hiding this comment.
Conceptually speaking, they're not necessarily "wrong enums", it might be that it's an algorithm that we just don't support. In other words, the spec does not forbid any values here, it's just that we might not be able to use all of them. And sometimes it might be useful to parse the packet, e.g. when building a packet listing, and just show it with an "unsupported algorithm", rather than showing the entire packet as "unsupported packet".
with potentially unexpected side effects
What side effects are you thinking of?
(There also shouldn't be a need for explicit exceptions, the "rule" would just be "throw when you use the enum value if we don't support it.")
By the way, in v5 public keys, we don't actually have to throw, as it includes a count of the public key material. We don't really use it today, but that was added exactly for the purpose of being able to parse a key without knowing the public key algorithm (and indeed bring it more in line with the other packets in that way).
There was a problem hiding this comment.
I understand that in some cases it can be useful to allow parsing, but IMO it'll get in the way of a more solid error handling for the standard usage. I fundamentally think that an unknown algo inside a known packet leads to effectively having an unsupported packet, since it cannot be used as intended.
Plus, if we are going to be more strict in terms of typings, then we cannot really pass around values that are not necessarily of the specified enum type.
What side effects are you thinking of?
Nothing specific in mind, but we'd be keeping a potentially invalid value, trusting that the rest of the functions in the code can handle that securely.
Re. the packet listing example, I personally don't consider it a target use case of the library, but I think it could still be implemented by manually adding all the missing enums values and mapping them to 'unknown', for instance.
There was a problem hiding this comment.
There are other implications, for example if you have a key block with multiple keys, and one of them has a public key algorithm that we don't support, then it would be nice to still properly parse them as separate keys, and have one of them be a key that we just can't use, (similarly to if it is a DSA key, for example), rather than throwing an error. And then, if you have a message that's signed with both, for example, it can still verify. Of course, then if you try to encrypt a message to both keys, it will still throw, as expected, but then at least the application could choose to encrypt with only one of the keys, if it wants to.
Nothing specific in mind, but we'd be keeping a potentially invalid value, trusting that the rest of the functions in the code can handle that securely.
I don't think there should be any security implications of parsing these packets, since we don't support the algorithm we will always throw when we try to use it.
larabr
force-pushed
the
algo-enum-only
branch
from
065732a
to
5a600d7
Compare
In several packet classes, we used to store string identifiers for public-key, aead, cipher or hash algorithms. To make the code consistent and to avoid having to convert to/from string values, we now always store integer values instead, e.g.
enums.symmetric.aes128is used instead of'aes128'.This is not expected to be a breaking change for most library users. Note that the type of
Key.getAlgorithmInfo()and of the session key objects returned and accepted by top-level functions remain unchanged.Affected classes (type changes for some properties and method's arguments):
PublicKeyPacket,PublicSubkeyPacket,SecretKeyPacket,SecretSubkeyPacketSymEncryptedIntegrityProtectedDataPacket,AEADEncryptedDataPacket,SymmetricallyEncryptedDataPacketLiteralDataPacket,CompressedDataPacketPublicKeyEncryptedSessionKey,SymEncryptedSessionKeyPacketSignaturePacketOther potentially breaking changes:
AEADEncryptedDataPacket.aeadAlgo, since it was redudant given.aeadAlgorithm.AEADEncryptedDataPacket.cipherAlgo->.cipherAlgorithmTODO: