New API for the headers function

[tiesselune]

This pull request addresses issue #50

This new API ( ⚠️ breaking change) is meant to:

• Avoid moving self into the function since it is not necessary (currently, it forces a copy if we want both the headers and the ciphertext because both require moving out of the AesGcmEncryptedBlock)
• Avoid extra allocation by using &'static strs instead of Strings for the keys
• Avoid the use of HashMap that adds extra workload to the process
• Add the possibility to automatically include the VAPID public key in the Crypto-Key header (thus removing the need to merge it, and allowing for the change from a HashMap to a Vector)

Use cases

• The most common use case: using both headers and ciphertext without having to copy one of them to avoid a double move
• Secondary use case: Large user pools to send notifications to in one go. Since notification sending (and content encryption) is done per-browser, sending a simple notification to a lot of users can quickly multiply the amount of extra allocation and hashing done with String and HashMap. (e.g. 1 million users to notify individually in one go) Since the draft for the AESGCM scheme is not likely to change anymore and the AES128GCM scheme is handled separately, we can now assume keys for AESGCM headers will always be fixed and use &'static strs and a tuple Vec instead.

[rfk]

Thanks, and sorry for the delay in looking at this. What you're doing here broadly looks good to me. Please consider adding a basic test-case for completeness, just calling these methods with fixed data and checking that the expected headers come out as a result.

Avoid the use of HashMap that adds extra workload to the process

Can you say more about the "extra workload" aspect of this in practice? Does the Vec make it easier to integrate into calling code than using a HashMap?

[rfk]

The use of an optional vapid key here makes sense to me. I'm tempted to suggest a more open-ended approach here, e.g. allowing the caller to specify arbitrary key-value pairs that get put into the header..but that's probably going to make more opportunities for things to go wrong, so special-casing this p256ecdsa field seems fine 👍

[tiesselune]

The idea with this one is that the Crypto-Key field needs to be merged since there are several sub key-value pairs in its value (dh and p256ecdsa), one for Vapid and the other one being specific to the AESGCM scheme (this odd mix has been streamlined in AES128GCM, where the headers part is far easier to pull off). So allowing users to provide the entire key-pair would require them to effectively merge this Crypto-Key header themselves using the keys from their Vapid Signature and from the returned AESGCMEncryptedBlock, which I'm trying to avoid here.
Apart from this specific case, the function returns an owned Vec so it's pretty easy to push new key-value pairs in it if necessary, outside of the function itself.

[tiesselune]

Can you say more about the "extra workload" aspect of this in practice? Does the Vec make it easier to integrate into calling code than using a HashMap?

I'm referring to the hashing algorithm that is used for hashing keys in HashMaps and the fact that the default implementation uses a cryptographically strong one, that is considered potentially slow by the Book itself, when it's not really necessary anymore. The HashMap would be really useful if we needed to access a value by key outside of the function, (in order to allow the Crypto-Key field to be merged outside of the headers function, for instance, which is not required anymore in my proposal thanks to the Option<&[u8]> additional argument which allows for direct in-function merging).

Since correct headers are provided (including the special Crypto-Key case) and they shouldn't need to be modified anymore once the function returns (aside from potentially adding new ones), the hashing part becomes unnecessary and we can save some CPU cycles by replacing it with a Vec, that can be passed to any HTTP client by just iterating on it (just like the HashMap).

So the user experience should remain the same (or even slightly better since the Crypto-Key header is now merged by the headers function instead of being their responsibility), with a slight gain in performance (with no hashing or unnecessary String allocation) that can stack up quickly when sending a big amount of web push messages.

I hope all that was clear enough 😅

[rfk]

Thanks, that makes sense :-)

[rfk]

Whoops, sorry, I didn't notice that you'd move this out of "draft". Taking a look now.

[rfk]

Thanks again @tiesselune! I'm going to go ahead and merge this as I've got some other changes that I'll be working on shortly that I also expect to be a breaking change, we we might as well batch a few up for the next release.