This restores previous functionality where "aes128gcm" could chunk
large payloads into multiple records. It is using a different
algorithm than the previous implementation, but one that I think
is easier to understand (or at least, better documented 😅).
Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking.
This is a significant refactor of the guts of the crypto code, but I think overall it
makes things easier to understand and to audit.
First, I've removed the `EceWebPush` trait that was previously used to share parts
of the encrypt/decrypt logic between the two schemes. The schemes are not that similar
in practice and on balance, I think the attempt to share code between them was
actually making both schemes harder to understand.
Second, I've cut all the record-chunking code out of "aesgcm". It now supports only
a single record on both encryption and decryption, in line with what the spec says
that a webpush client should support. We were already throwing errors when encountering
multiple records in "aesgcm"; this cleanup takes advantage of that fact to actually
remove the code without breaking the public API.
Finally, I've removed the record-chunking during encryption for "aes128gcm", instead
opting to support larger payloads by increasing the record size. I've also added
several layers of abstraction in the hope of making the code easier to understand -
for example there is a separate `Header` struct for reading/writing the header,
and a separate `PlaintextRecord` struct for reading/writing an individual record.
Of course "easier to understand" is subjective, but I think it's an improvement
(and I certainly understand things better as a result of having worked through it!).
Feedback and/or pushback on this is most welcome.
I'd like to try adding record chunking in back here, but as a separate PR building
atop these abstractions.
Connects to #55.
This is one of the padding techniques suggested in the RFC, and while
we can't choose a default scheme that will work well for all applications,
this one at least seems easier to reason about.
Fixes#54.
This is a significant refactor of the public API of the crate, simplifying
the API surface and removing some of the footgun potential noted by Martin
in his review at https://github.com/mozilla/application-services/issues/1068.
In particular:
* The public `encrypt` functions no longer take a `salt` parameter. The
right thing to do is to generate a new random `salt` for each encryption
so we just do that for you automatically.
* Many internal implementation details are now `pub(crate)` rather than `pub`,
to avoid potential confusion from consumers.
* We refuse to encrypt or decrypt across multiple records in the legacy
`aesgcm` scheme, because the only consumer of that schema is webpush,
and webpush restricts consumers to using only a single record.
We still have the code lying around to encrypt/decrypt across record
boundaries, but we don't have high confidence that it works correctly
for `aesgcm` and intend to refactor that away in a future commit.
So, may as well adjust the interface to reflect that while we're in here
making breaking changes.
To go along with the revised interface, this commit also significantly
expands to docs in order to help set consumer expectations and context.
* Use salt from WebPushParams if provided (Mainly for test purposes against the IETF example from draft 4)
* Implemented legacy aes public functions with first successful tests from spec
* Fixed documentation to have nice-looking parameter descriptions and remove erroneous types
* Added a documentation disclaimer to redirect to the top-level functions instead of the Encryption structs
Co-authored-by: John Tiesselune <jsos10@pm.me>
* replace failure with thiserror and backtrace
* adds thiserror::Error derive and uses #[from] to auto generate from impl
* adds thiserror::Error derive and uses #[from] to auto generate from impl
JR Conlin
Ryan Kelly
Ryan Kelly
Julien SOSTHENE
Jordandev678
Edouard Oger
Tarik Eshaq
Tarik Eshaq
JR Conlin
Mozilla-GitHub-Standards