-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the public API to remove footguns, and document it. #52
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
[package] | ||
name = "ece" | ||
version = "1.3.0" | ||
version = "1.4.0-alpha1" | ||
authors = ["Firefox Sync Team <[email protected]>", "JR Conlin <[email protected]>"] | ||
license = "MPL-2.0" | ||
edition = "2018" | ||
|
@@ -12,20 +12,19 @@ keywords = ["http-ece", "web-push"] | |
byteorder = "1.3" | ||
thiserror = "1.0" | ||
base64 = "0.12" | ||
hex = "0.4" | ||
hkdf = { version = "0.9", optional = true } | ||
lazy_static = { version = "1.4", optional = true } | ||
once_cell = "1.4" | ||
openssl = { version = "0.10", optional = true } | ||
serde = { version = "1.0", features = ["derive"], optional = true } | ||
sha2 = { version = "0.9", optional = true } | ||
|
||
[dev-dependencies] | ||
hex = "0.4" | ||
|
||
[features] | ||
default = ["backend-openssl", "serializable-keys"] | ||
serializable-keys = ["serde"] | ||
backend-openssl = ["openssl", "lazy_static", "hkdf", "sha2"] | ||
backend-test-helper = [] | ||
|
||
[package.metadata.release] | ||
no-dev-version = true |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really be in favor of dropping the oldest one of these. I'll file a separate ticket. Most of what we're seeing is either 'aes128gcm' or 'aesgcm'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can definitely consider dropping the really old
aesgcm128
format:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the code actually supports that format, this could just be a documentation issue or misunderstanding on my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrconlin I'm not very familiar with the various versions of these specs, could you please help me understand what you mean here? The two linked "draft" documents only seem to mention "aesgcm", which IIUC is the "legacy" line in your graph and the corresponding "legacy" API in this crate. Neither contains the string "aesgcm128" that I can see.
I suspect we don't have anything to remove in the code, but I'd be happy to update the doc references here to make things more clear, if you can suggest any revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh. The fun of a long lived and somewhat popular draft is that you get stuff like this. I don't expect that this PR should drop support for the very old protocol, but I do think we should add it as a future issue.
In essence, three "versions" of the encryption format started being used over time. The very first version "aesgcm128" used a combination of header and body content in order to encrypt/decrypt the message. It was reasonably early so not a lot of libraries adopted it, but firefox said that they'd support it as part of the WebPush pre-cursor. The next version cleaned things up, but still used header/body format (albeit, with different headers and formats), and was labeled "aesgcm". This version was WIDELY adopted by third party encoder libraries and (as you can see) is still very much in use. The final RFC proposed "aes128gcm", which made encryption/decryption self contained within the body of the message. While folks have been encouraging subscription providers to update their libraries to use the RFC format (since it's both more efficient and far more secure), a lot of subscription services have not.
There are not a lot of changes between "aesgcm128" and "aesgcm", but they are present. Supporting the very old format just adds unneeded complexity and potential surface area for the library. For most client side / decryption models, we can definitely control this from the Push Server by simply blocking messages that use "aesgcm128" with a 415 or something. For encryption, however, it would be best to remove that particular format as a potential option.
All things must come to an end, and for "aesgcm128", it's way overdue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dug in a bit more here, and I don't believe this crate has support for
aesgcm128
. I've been using the desktop implementation for reference to help get my head around it.In order to support
aesgcm128
, we would need to be:Encryption-Key
header as part of the encryption process; I can only find support for emitting theCrypto-Key
andEncryption
headers of the newer-but-still-legacyaesgcm
scheme.I've made the following updates to help clarify what I believe the situation is, but if I've misunderstood, please correct me:
Encryption-Key
header in one of the comments, which was the only place I could find such a string, and which is incorrect since the comment is specifically in the context ofaesgcm
which uses theEncryption
field.draft-thomson-http-encryption-02
, which I dug out of the comments in the desktop implementation).