WIP Cleanups based on review feedback #27
Conversation
| [RFC8188](https://tools.ietf.org/html/rfc8188) | ||
| * `aesgcm`: the draft scheme described in | ||
| [draft-ietf-webpush-encryption-04](https://tools.ietf.org/html/draft-ietf-webpush-encryption-04) and | ||
| [draft-ietf-httpbis-encryption-encoding-03](https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-03_) |
There was a problem hiding this comment.
I tried to track down the specific specs, which includes both ECE and WebPush. Did I find the right ones? (In particular, are these the right draft numbers for the draft specs? I reverse-engineered which draft to link based on the details of how the code currently behaves).
| The sender can encrypt a Web Push message to the receiver's public key: | ||
|
|
||
| ``` | ||
| let ciphertext = ece::encrypt(pubkey, auth_secret, b"payload")?; |
There was a problem hiding this comment.
I propose we remove the salt argument from this function, since it seems like a footgun we can easily help the caller avoid by just generating a fresh random salt each time. (So I did so pre-emptively in the docs here)
There was a problem hiding this comment.
fair enough. The salt is mostly a legacy thing now with the sunset of aesgcm. If we have this default to using aes128gcm for encryption/decryption then there's no problem.
|
|
||
| * We do not implement streaming encryption or decryption, although the ECE scheme is designed to permit it. | ||
| * The application cannot control what padding is apply during encryption; we randonly select a length of padding to add | ||
| during encryption, but the RFC suggests that the best choice of strategy [is application-dependant](https://tools.ietf.org/html/rfc8188#section-4.8) |
There was a problem hiding this comment.
I'd like to add more explanation about how the padding thing works but I don't really understand it - are we just randomly selecting some length to pad to? I wonder if we should give the caller control over that rather than doing it behind the scenes.
There was a problem hiding this comment.
The padding is mostly there to obscure the length of the actual data, with the provision that a minimum set be present. There's concern that left to the application, no padding is used which could compromise the encryption (as noted in the spec). I'll add that we don't currently support multi-segment messages, so it could be a bit easier to predict payloads if no (or consistently sized) padding is used. Using a randomized padding size like this does kind of remove a footgun.
There was a problem hiding this comment.
Thanks for the explanation.
I expect there are better things we could do than "random padding" here, such as bucketing to the nearest power of 2. IIUC the problem with adding random noise is that it's easy to remove when doing statistical analysis, you just have to collect a greater volume of traffic. But we can explore that in followups.
There was a problem hiding this comment.
Seems reasonable to me.
Also nit on line 84: s/apply/applied/
|
Closing in favour of #52 |
I finally made the time to go through Martin's feedback in mozilla/application-services#1068 (comment) and I have a few suggested cleanups in progress. First though, I wanted to make sure I understood the scope and intention of everything in this crate, so I started with trying to flesh out the README.
@eoger @jrconlin, does this sound about right so far?