Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716443002:
Implement support for draft-ietf-webpush-encryption-08 (Closed)
|
Created:
3 years, 10 months ago by Peter Beverloo Modified:
3 years, 7 months ago CC:
chromium-reviews, johnme+watch_chromium.org, zea+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for draft-ietf-webpush-encryption-08
This CL builds upon the previous refactorings to implement support for
the latest drafts (which is WGLC) of the Web Push Encryption scheme.
Support is not yet enabled for incoming messages - since the message
format changed slightly as well, a more trivial update to
GCMEncryptionProvider is necessary as well.
BUG=679789
Review-Url: https://codereview.chromium.org/2716443002
Cr-Commit-Position: refs/heads/master@{#473876}
Committed: https://chromium.googlesource.com/chromium/src/+/8b48bde152a8e3a8bb1a1066854ab6e9c28a7e86
Patch Set 1 #Patch Set 2 : Implement support for draft-ietf-webpush-encryption-08 #
Total comments: 5
Patch Set 3 : Implement support for draft-ietf-webpush-encryption-08 #Patch Set 4 : test vectors #Patch Set 5 : fix windows #Patch Set 6 : fix windows^2 #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 30 (21 generated)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
peter@chromium.org changed reviewers: + davidben@chromium.org
This needs to be covered with TestVectors, but should be ready other than that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
eroman@chromium.org changed reviewers: + eroman@chromium.org
lgtm https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_message_cryptographer.cc:177: std::stringstream info_stream; same comment as on other CLs -- i don't think stringstream is the best way to do appends. https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_message_cryptographer.cc:246: record.remove_suffix(padding_length); Is this correct in the case where we don't loop? i.e. if record.size() <= 1, we will remove 1 byte from record, without actually having checked its value. Maybe it would be clearer to structure it so the stripping is only done after finding 0x02, and falling-through outside the loop returns false.
Thanks! PS3 contains the semantical changes, I'd appreciate another quick look on that. It includes a new ValidateCiphertextSize method following your comment on the first patch -- -03 has a minimum of two padding bytes, -08 of a single byte. https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_message_cryptographer.cc:59: info_stream << "Content-Encoding: auth" << '\x00'; Updated this too per the previous CL. https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_message_cryptographer.cc:177: std::stringstream info_stream; On 2017/05/16 21:03:27, eroman wrote: > same comment as on other CLs -- i don't think stringstream is the best way to do > appends. Done. https://codereview.chromium.org/2716443002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_message_cryptographer.cc:246: record.remove_suffix(padding_length); On 2017/05/16 21:03:27, eroman wrote: > Is this correct in the case where we don't loop? > > i.e. if record.size() <= 1, we will remove 1 byte from record, without actually > having checked its value. > > Maybe it would be clearer to structure it so the stripping is only done after > finding 0x02, and falling-through outside the loop returns false. I added a DCHECK to verify that record.size() >= 1, since the ciphertext length validation covers that. To make sure we cover the record.size() == 1 case I've changed the "<" on line 236 to "<=".
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/gcm_message_cryptographer.cc:58: const char kInfo[] = "Content-Encoding: auth"; What about adding \0 at the end of the literal? I think the array will still be initialized properly. https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/gcm_message_cryptographer.cc:197: const char kInfo[] = "WebPush: info"; Same comment here, assuming it works.
On 2017/05/22 18:49:48, eroman wrote: > lgtm > > https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... > File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): > > https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... > components/gcm_driver/crypto/gcm_message_cryptographer.cc:58: const char kInfo[] > = "Content-Encoding: auth"; > What about adding \0 at the end of the literal? I think the array will still be > initialized properly. > > https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... > components/gcm_driver/crypto/gcm_message_cryptographer.cc:197: const char > kInfo[] = "WebPush: info"; > Same comment here, assuming it works. Even with a bunch of sizeof()s and std::string constructors with the explicit size that yields test failures.. I want to understand why it doesn't work so I'll continue playing with it, and will follow up if I find the solution. (Or please tell me if you know!) const char kInfo[] = "Content-Encoding: auth\x00"; std::string info(kInfo, sizeof(kInfo));
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495542968554060,
"parent_rev": "a4581b46788c22124a6fd749e793c3015aaa3891", "commit_rev":
"8b48bde152a8e3a8bb1a1066854ab6e9c28a7e86"}
Message was sent while issue was closed.
Description was changed from ========== Implement support for draft-ietf-webpush-encryption-08 This CL builds upon the previous refactorings to implement support for the latest drafts (which is WGLC) of the Web Push Encryption scheme. Support is not yet enabled for incoming messages - since the message format changed slightly as well, a more trivial update to GCMEncryptionProvider is necessary as well. BUG=679789 ========== to ========== Implement support for draft-ietf-webpush-encryption-08 This CL builds upon the previous refactorings to implement support for the latest drafts (which is WGLC) of the Web Push Encryption scheme. Support is not yet enabled for incoming messages - since the message format changed slightly as well, a more trivial update to GCMEncryptionProvider is necessary as well. BUG=679789 Review-Url: https://codereview.chromium.org/2716443002 Cr-Commit-Position: refs/heads/master@{#473876} Committed: https://chromium.googlesource.com/chromium/src/+/8b48bde152a8e3a8bb1a1066854a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8b48bde152a8e3a8bb1a1066854a...
Message was sent while issue was closed.
On Tue, May 23, 2017 at 5:35 AM, <peter@chromium.org> wrote: > On 2017/05/22 18:49:48, eroman wrote: > > lgtm > > > > > https://codereview.chromium.org/2716443002/diff/100001/ > components/gcm_driver/crypto/gcm_message_cryptographer.cc > > File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): > > > > > https://codereview.chromium.org/2716443002/diff/100001/ > components/gcm_driver/crypto/gcm_message_cryptographer.cc#newcode58 > > components/gcm_driver/crypto/gcm_message_cryptographer.cc:58: const char > kInfo[] > > = "Content-Encoding: auth"; > > What about adding \0 at the end of the literal? I think the array will > still > be > > initialized properly. > > > > > https://codereview.chromium.org/2716443002/diff/100001/ > components/gcm_driver/crypto/gcm_message_cryptographer.cc#newcode197 > > components/gcm_driver/crypto/gcm_message_cryptographer.cc:197: const > char > > kInfo[] = "WebPush: info"; > > Same comment here, assuming it works. > > Even with a bunch of sizeof()s and std::string constructors with the > explicit > size that yields test failures.. I want to understand why it doesn't work > so > I'll continue playing with it, and will follow up if I find the solution. > (Or > please tell me if you know!) > > const char kInfo[] = "Content-Encoding: auth\x00"; > std::string info(kInfo, sizeof(kInfo)); > How many nulls are you trying to get at the end? When you initialize an array using a C-string literal, you already get a NUL at the end. For instance: const char kData1[] = "Foo"; strlen(kData1) == 3 However, sizeof(kData1) == 4 kData1[3] == '\0'. You can add more nulls (either as \0 or \x00 as in your patch) if you like, just note that the very last one is always going to be NUL too given initialization from c-string. > > https://codereview.chromium.org/2716443002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right): https://codereview.chromium.org/2716443002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/gcm_message_cryptographer.cc:58: const char kInfo[] = "Content-Encoding: auth"; On 2017/05/22 18:49:48, eroman wrote: > What about adding \0 at the end of the literal? I think the array will still be > initialized properly. Updated this to use sizeof() here: https://codereview.chromium.org/2901923002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
