Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1244803002:
Add parsers for the Encryption and Encryption-Key HTTP headers. (Closed)
|
Created:
5 years, 5 months ago by Peter Beverloo Modified:
5 years, 1 month ago CC:
chromium-reviews, zea+watch_chromium.org, svaldez, davidben Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd parsers for the Encryption and Encryption-Key HTTP headers.
These headers are defined in the following IETF draft:
https://tools.ietf.org/html/draft-thomson-http-encryption-01
They contain the information required to decrypt an encryption GCM
message send through either the Web Push protocol, or using the
same properties in the existing GCM protocol.
The messages will be received by the Google Cloud Messaging (GCM)
driver, either through the GCM engine (on desktop) or Google
Play Services on Android, together with these headers. The
messages then have to be decrypted before being forwarded to the
feature consumer, which will initially be Push Notifications.
BUG=486040
Committed: https://crrev.com/1f99ff0f7ccd143dcfcd46161500bc9d62d031c6
Cr-Commit-Position: refs/heads/master@{#352027}
Patch Set 1 #
Total comments: 28
Patch Set 2 : #
Total comments: 3
Patch Set 3 : rewrite #
Total comments: 30
Patch Set 4 : Anton's comments #
Total comments: 25
Patch Set 5 : address comments #
Total comments: 12
Patch Set 6 : #
Total comments: 16
Patch Set 7 : #
Dependent Patchsets: Messages
Total messages: 29 (6 generated)
peter@chromium.org changed reviewers: + johnme@chromium.org
+johnme for early review, before I add OWNERS.
Initial comments (haven't reviewed the code yet). https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:51: { "keyid=foo; salt=" ENCODED_SALT "; rs=1024", Are you sure "; " is allowed? https://tools.ietf.org/html/draft-thomson-http-encryption-01#section-3 seems to imply that no whitespace should be used, though I'm not an expert at ABNF. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:53: { "keyid='foo'; salt='" ENCODED_SALT "'; rs='1024'", It seems according to https://tools.ietf.org/html/rfc7231 that "parameter name tokens are case-insensitive" (though it's unclear if that applies in all cases). If that applies, can you add tests for mixed case and uppercase parameter names? Also, "Parameter values might or might not be case-sensitive, depending on the semantics of the parameter name."; clearly base64 needs to be case-sensitive, but are there e.g. any hex parameters? https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:106: TEST(EncryptionHeaderParsersTest, ParseMultipleEncryptionHeaders) { Please add a test that you support the freaky empty list elements that https://tools.ietf.org/html/rfc7230#section-7 requires you to support, e.g.: "foo,bar" "foo ,bar," "foo , ,bar,charlie " where foo/bar/charlie are valid keyid;salt;rs tuples. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:129: { "keyid=foo; key=" ENCODED_KEY "; dh=" ENCODED_DH, Similarly are you sure "; " is allowed?
peter@chromium.org changed reviewers: + rsleevi@chromium.org
+Ryan, given his love for this stuff. We'll initially use |salt| and |rs| from EncryptionHeaderValue, and |dh| from EncryptionKeyHeaderValue. Support for |key| from EncryptionKeyHeaderValue will be added a bit later. I wrote this in a way that we can quite easily componentize it when there's a second customer. What's your feel on moving a URL-safe base64 decoder/encoder into //base (or //net)? These functions are duplicated way too much. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:51: { "keyid=foo; salt=" ENCODED_SALT "; rs=1024", On 2015/07/20 19:15:19, johnme wrote: > Are you sure "; " is allowed? > https://tools.ietf.org/html/draft-thomson-http-encryption-01#section-3 seems to > imply that no whitespace should be used, though I'm not an expert at ABNF. The examples in the same draft use spaces, and this seems common for HTTP headers (e.g. Content-Type). I agree that the syntax is confusing, I'll poke Martin on Thursday. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:53: { "keyid='foo'; salt='" ENCODED_SALT "'; rs='1024'", On 2015/07/20 19:15:19, johnme wrote: > It seems according to https://tools.ietf.org/html/rfc7231 that "parameter name > tokens are case-insensitive" (though it's unclear if that applies in all cases). > If that applies, can you add tests for mixed case and uppercase parameter names? Good point. Will do. > Also, "Parameter values might or might not be case-sensitive, depending on the > semantics of the parameter name."; clearly base64 needs to be case-sensitive, > but are there e.g. any hex parameters? |keyid| would be case sensitive, but we're not using it right now. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:129: { "keyid=foo; key=" ENCODED_KEY "; dh=" ENCODED_DH, On 2015/07/20 19:15:19, johnme wrote: > Similarly are you sure "; " is allowed? See earlier comment.
https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:40: net::HttpUtil::NameValuePairsIterator parser(tokenizer.token_begin(), I notice that NameValuePairsIterator allows arbitrary LWS. But linear whitespace is advised against by https://tools.ietf.org/html/std68 as "no longer legal in mail headers and have caused interoperability problems in other contexts. Do not use when defining mail headers and use with caution in other contexts." So if the webpush spec allows LWS, it should perhaps stop doing so. Or if it doesn't allow LWS, then this implementation doesn't strictly match the webpush spec, and we should add support to NameValuePairsIterator for whatever whitespace the webpush spec does allow. Then again, there's value in consistency. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:42: if (!parser.valid()) It's not actually possible for valid to be false before you call GetNext. Conversely, you don't check valid() after calling GetNext, so given partially valid input like "salt=abcd;rs" your implementation will return true having parsed salt only, even though the rs was invalid. I suspect what you want here is to move the !parser.valid() check after your parse.GetNext() while loop, so that you return false for inputs like "salt=abcd;rs". https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:43: return false; It might also make sense to call output->clear() before returning false? https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:54: // TODO(peter): Generalize a URL-safe base64 implementation. Please link to https://tools.ietf.org/html/rfc4648#section-5 https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:56: if (input.find_first_of("+/=") != std::string::npos) Why do you disallow '='? Whilst padding is technically unnecessary, https://tools.ietf.org/html/rfc4648#section-3.2 says "Implementations MUST include appropriate pad characters at the end of encoded data unless the specification referring to this document explicitly states otherwise.", and https://tools.ietf.org/html/draft-thomson-http-encryption-01 does not say otherwise. And if Martin updates the spec to make it optional, it might still make sense to allow the padding even though redundant (unless the spec disallows that). https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:71: // Parses the "salt" field of the Encryption header. Must be a base64 URL Nit: "base64url" for consistency with the naming in the RFC (and also to make it very clear that this != ordinary base64)? https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:136: return false; If value_map is completely empty, it should be ok to skip it and continue (rather than returning false). See earlier comment about allowing "foo , ,bar,charlie ". https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.h:38: bool ParseEncryptionKeyHeader(const std::string& input, I'm not sure this is the optimal API. Unless I'm misreading the spec, it seems that an Encryption-Key entry must always be paired with an Encryption entry that has the same keyid (the reverse is not true, since there may be no Encryption-Key entry). So it seems this parser would be easier to use if it took care of matching the entries based on their keyids, e.g.: // Pass an empty string for encryption_key_header if it was omitted. ParseEncryptionHeaders( const std::string& encryption_header, const std::string& encryption_key_header, std::vector<EncryptionHeadersValue>* result); struct EncryptionHeadersValue { // keyid, key and dh may be empty. std::string keyid; std::string salt; int64_t rs = 4096; std::string key; std::string dh; } https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:72: SCOPED_TRACE(i); Nice :) https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:93: // Record sizes smaller than 2 are not supported. Please also check that a record size of 9,223,372,036,854,775,808 (i.e. one greater than the largest 64-bit signed value) fails to parse (or set a smaller limit).
https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:40: net::HttpUtil::NameValuePairsIterator parser(tokenizer.token_begin(), On 2015/07/21 14:34:43, johnme wrote: > I notice that NameValuePairsIterator allows arbitrary LWS. But linear whitespace > is advised against by https://tools.ietf.org/html/std68 as "no longer legal in > mail headers and have caused interoperability problems in other contexts. Do not > use when defining mail headers and use with caution in other contexts." > > So if the webpush spec allows LWS, it should perhaps stop doing so. > Or if it doesn't allow LWS, then this implementation doesn't strictly match the > webpush spec, and we should add support to NameValuePairsIterator for whatever > whitespace the webpush spec does allow. > > Then again, there's value in consistency. I'd like to defer to Ryan for final judgement here, but as far as I can tell Chrome isn't very strict in header parsing. It feels silly for these things to be sensitive to whitespace. Mismatch between reality and spec theoretics? NVPI has more relaxed semantics. Additionally, by being able to use NameValuePairsIterator we avoid duplicating a lot of logic. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:42: if (!parser.valid()) On 2015/07/21 14:34:43, johnme wrote: > It's not actually possible for valid to be false before you call GetNext. > Conversely, you don't check valid() after calling GetNext, so given partially > valid input like "salt=abcd;rs" your implementation will return true having > parsed salt only, even though the rs was invalid. > > I suspect what you want here is to move the !parser.valid() check after your > parse.GetNext() while loop, so that you return false for inputs like > "salt=abcd;rs". Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:43: return false; On 2015/07/21 14:34:43, johnme wrote: > It might also make sense to call output->clear() before returning false? Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:54: // TODO(peter): Generalize a URL-safe base64 implementation. On 2015/07/21 14:34:43, johnme wrote: > Please link to https://tools.ietf.org/html/rfc4648#section-5 Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:56: if (input.find_first_of("+/=") != std::string::npos) On 2015/07/21 14:34:43, johnme wrote: > Why do you disallow '='? Whilst padding is technically unnecessary, > https://tools.ietf.org/html/rfc4648#section-3.2 says "Implementations MUST > include appropriate pad characters at the end of encoded data unless the > specification referring to this document explicitly states otherwise.", and > https://tools.ietf.org/html/draft-thomson-http-encryption-01 does not say > otherwise. And if Martin updates the spec to make it optional, it might still > make sense to allow the padding even though redundant (unless the spec disallows > that). Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:71: // Parses the "salt" field of the Encryption header. Must be a base64 URL On 2015/07/21 14:34:43, johnme wrote: > Nit: "base64url" for consistency with the naming in the RFC (and also to make it > very clear that this != ordinary base64)? Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.cc:136: return false; On 2015/07/21 14:34:43, johnme wrote: > If value_map is completely empty, it should be ok to skip it and continue > (rather than returning false). See earlier comment about allowing "foo , > ,bar,charlie ". Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers.h:38: bool ParseEncryptionKeyHeader(const std::string& input, On 2015/07/21 14:34:43, johnme wrote: > I'm not sure this is the optimal API. Unless I'm misreading the spec, it seems > that an Encryption-Key entry must always be paired with an Encryption entry that > has the same keyid (the reverse is not true, since there may be no > Encryption-Key entry). > > So it seems this parser would be easier to use if it took care of matching the > entries based on their keyids, e.g.: > > // Pass an empty string for encryption_key_header if it was omitted. > ParseEncryptionHeaders( > const std::string& encryption_header, > const std::string& encryption_key_header, > std::vector<EncryptionHeadersValue>* result); > > struct EncryptionHeadersValue { > // keyid, key and dh may be empty. > std::string keyid; > std::string salt; > int64_t rs = 4096; > std::string key; > std::string dh; > } My thinking here is that they are two separate headers, with separate value requirements. Furthermore, since the "keyid" name may be omitted when the UA has other means of matching key information with a request, for example the used subscription Id (which we use), and header values could theoretically be repeated, it will not always be clear what to match with what. Additionally, while we will be the DH parameter of the Encryption-Key header, other consumers may only need Encryption. draft-thomson-http-encryption-01 has been set up to be generic, which means it's likely that we'll get a second consumer in Chrome, in which case componentizing this (or moving it elsewhere) is beneficial. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:53: { "keyid='foo'; salt='" ENCODED_SALT "'; rs='1024'", On 2015/07/20 22:44:33, Peter Beverloo wrote: > On 2015/07/20 19:15:19, johnme wrote: > > It seems according to https://tools.ietf.org/html/rfc7231 that "parameter name > > tokens are case-insensitive" (though it's unclear if that applies in all > cases). > > If that applies, can you add tests for mixed case and uppercase parameter > names? > > Good point. Will do. > > > Also, "Parameter values might or might not be case-sensitive, depending on the > > semantics of the parameter name."; clearly base64 needs to be case-sensitive, > > but are there e.g. any hex parameters? > > |keyid| would be case sensitive, but we're not using it right now. Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:93: // Record sizes smaller than 2 are not supported. On 2015/07/21 14:34:43, johnme wrote: > Please also check that a record size of 9,223,372,036,854,775,808 (i.e. one > greater than the largest 64-bit signed value) fails to parse (or set a smaller > limit). Done. https://codereview.chromium.org/1244803002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:106: TEST(EncryptionHeaderParsersTest, ParseMultipleEncryptionHeaders) { On 2015/07/20 19:15:19, johnme wrote: > Please add a test that you support the freaky empty list elements that > https://tools.ietf.org/html/rfc7230#section-7 requires you to support, e.g.: > > "foo,bar" > "foo ,bar," > "foo , ,bar,charlie " > > where foo/bar/charlie are valid keyid;salt;rs tuples. Added a comma.
Friendly ping :)
Sorry for the delay. This seems like it's doing a lot of (unnecessary) string manipulation. It also looks like you're expecting the input to be collated by the caller (that is, turning multiple header values into a joined comma-separated value), but doesn't handle some of the edge cases well. I'm incredibly biased, but perhaps it may help to restructure the parsing similar to net's ParseHPKPHeader, which I think is quite conceptually similar, and helps you reduce any string copying to the bare minimum necessary for the output result. https://codereview.chromium.org/1244803002/diff/20001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:28: using NameValueMap = std::map<std::string, std::string>; This seems like a lot of unnecessary copying https://codereview.chromium.org/1244803002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:35: tokenizer.set_quote_chars("'\""); Seems like this is hardcoding what properly belongs to HttpUtil::IsQuote, but without the caveat about single quotes not being standard https://codereview.chromium.org/1244803002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:123: std::vector<EncryptionHeaderValue>* result) { This all seems very inefficient
I should also add that chrome-security@ generally dislikes HTTP parsing in the browser :) From a layering perspective, is this something that could/should go to the Blink layer? I realize the answer is "probably not", but it might help to make sure to document that.
I have rewritten this patch based on http_security_headers.cc - thank you for that pointer, Ryan, it is much better indeed! As you expected, we can't do the header parsing on Blink. This code will be used for messages received from GCM containing the information required to decrypt the message payload. The renderer must only get the decrypted payload, and must not be exposed to the private key.
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
drive-by lgtm with a bunch of nits you can mostly ignore ;) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:17: const int64_t kDefaultRecordSize = 4096; Is this size in bytes? Kb? https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:19: bool Base64DecodeURLSafe(const base::StringPiece& input, std::string* output) { perhaps a comment what you mean by safe here? You add a padding and replace two chars with other two chars. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:20: size_t padded_size = (input.size() + 3) - (input.size() + 3) % 4; Does it have to be an arithmetic expression? I had to wrap my head around it for a couple of minutes to extract the logic of: padded_size = (input.size() % 4) ? input.size() : input.size() + 4 - (input.size() % 4); If it needs to be superfast, you could go with bitwise logic perhaps: size_t padded_size = without_last_two_bits(input.size()) + (is_last_or_penultimate_bit_set(input.size)) << 2; where without_last_two_bits(i) => i & ~3 and is_last_or_penultimate_bit_set(i) => is_bit_set(i, 0) | is_bit_set(i, 1) and is_bit_set(i, n) => (i & (1U << n) >> n) :) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:33: bool ValueToDecodedString(std::string::const_iterator begin, can you pass iterators by reference (perhaps only valuable in debug builds on some platforms but who knows)? https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:37: if (value.empty()) you could check if begin == end before constructing StringPiece? also, if the value is empty, does Base64Decode return true? https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:43: bool RecordSizeToInt(std::string::const_iterator begin, Read this a "record (verb) a size to an int". Maybe different naming? (SizeToInt, HeaderRecordSizeToInt) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:54: return *rs > 1; this has a possible unintended effect of changing |rs| even if false is returned. you're safe by not using this value later on in the public function though. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:66: const base::StringPiece name = can't you do: const base::StringPiece name(n_v_p.name_begin(), n_v_p.name_end()) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:70: if (base::LowerCaseEqualsASCII(name, "keyid")) { give names to the magic constants? you only reuse "keyid" though. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:8: #include <stdint.h> can we include <cstdint> ? (.h is much more common throughout the codebase) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:15: // time. The base64url encoded salt will be decoded, |rs| will be validated. s/salt/|salt|? what about |keyid|? https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:19: // Returns whether the |input| could successfully be parsed, and the resulting s/could successfully be parsed/was (or has been) successfully parsed Do you want to say something that if the returned value is false, the output parameters remain unchanged? https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:28: // time. The base64url encoded key and dh will be decoded. s/key/|key| and s/dh/|dh|? Mostly to be consistent with the comment above. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:32: // Returns whether |input| could be successfully parsed, and the resulting ditto about the phrasing and the return value https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:83: } Check that the output values are not modified perhaps?
rsleevi@chromium.org changed reviewers: + eroman@chromium.org
I'm swamped with a few other things for this week and next, so I'm gonna see if Eric has any spare bandwidth to do this review, and CC'ing svaldez@ as just FYI for "//net-stack newbie review training" :)
Thanks Anton! https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:17: const int64_t kDefaultRecordSize = 4096; On 2015/09/14 14:57:43, whywhat wrote: > Is this size in bytes? Kb? Done. Also clarified the origin of this value. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:19: bool Base64DecodeURLSafe(const base::StringPiece& input, std::string* output) { On 2015/09/14 14:57:43, whywhat wrote: > perhaps a comment what you mean by safe here? > You add a padding and replace two chars with other two chars. Renamed to the more appropriate Base64URLDecode. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:20: size_t padded_size = (input.size() + 3) - (input.size() + 3) % 4; On 2015/09/14 14:57:43, whywhat wrote: > Does it have to be an arithmetic expression? I had to wrap my head around it for > a couple of minutes to extract the logic of: > > padded_size = (input.size() % 4) ? input.size() : input.size() + 4 - > (input.size() % 4); > > If it needs to be superfast, you could go with bitwise logic perhaps: > size_t padded_size = without_last_two_bits(input.size()) + > (is_last_or_penultimate_bit_set(input.size)) << 2; > where without_last_two_bits(i) => i & ~3 and is_last_or_penultimate_bit_set(i) > => is_bit_set(i, 0) | is_bit_set(i, 1) and is_bit_set(i, n) => (i & (1U << n) >> > n) > > :) Hah. I adopted your suggestion. This is not performance sensitive, so the branch is OK. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:33: bool ValueToDecodedString(std::string::const_iterator begin, On 2015/09/14 14:57:43, whywhat wrote: > can you pass iterators by reference (perhaps only valuable in debug builds on > some platforms but who knows)? Done. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:37: if (value.empty()) On 2015/09/14 14:57:43, whywhat wrote: > you could check if begin == end before constructing StringPiece? > also, if the value is empty, does Base64Decode return true? Construction of the StringPiece should be inline, so this shouldn't matter a lot. Yes, Base64Decode will return true. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:43: bool RecordSizeToInt(std::string::const_iterator begin, On 2015/09/14 14:57:43, whywhat wrote: > Read this a "record (verb) a size to an int". Maybe different naming? > (SizeToInt, HeaderRecordSizeToInt) As discussed offline - I understand you concern but the implementation is trivial enough that it will quickly become obvious to the reader. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:54: return *rs > 1; On 2015/09/14 14:57:43, whywhat wrote: > this has a possible unintended effect of changing |rs| even if false is > returned. you're safe by not using this value later on in the public function > though. Ack, that's why we use temps in the public :) https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:66: const base::StringPiece name = On 2015/09/14 14:57:43, whywhat wrote: > can't you do: > > const base::StringPiece name(n_v_p.name_begin(), n_v_p.name_end()) Done. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:70: if (base::LowerCaseEqualsASCII(name, "keyid")) { On 2015/09/14 14:57:43, whywhat wrote: > give names to the magic constants? you only reuse "keyid" though. As you say, the gain is minimal while this is much easier to read. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:8: #include <stdint.h> On 2015/09/14 14:57:43, whywhat wrote: > can we include <cstdint> ? (.h is much more common throughout the codebase) That just includes stdint.h, except that it also puts the types in the std:: namespace. stdint.h is the canonical one in Chromium :). https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:15: // time. The base64url encoded salt will be decoded, |rs| will be validated. On 2015/09/14 14:57:43, whywhat wrote: > s/salt/|salt|? what about |keyid|? s/|rs|/rs/ in fact. This part of the comment refers to the input parsed from the header, not to the values written to the out arguments. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:19: // Returns whether the |input| could successfully be parsed, and the resulting On 2015/09/14 14:57:43, whywhat wrote: > s/could successfully be parsed/was (or has been) successfully parsed > Do you want to say something that if the returned value is false, the output > parameters remain unchanged? Done. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:28: // time. The base64url encoded key and dh will be decoded. On 2015/09/14 14:57:43, whywhat wrote: > s/key/|key| and s/dh/|dh|? > Mostly to be consistent with the comment above. Acknowledged. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:32: // Returns whether |input| could be successfully parsed, and the resulting On 2015/09/14 14:57:43, whywhat wrote: > ditto about the phrasing and the return value Done. https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/40001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:83: } On 2015/09/14 14:57:43, whywhat wrote: > Check that the output values are not modified perhaps? We check for that in InvalidHeadersDoNotModifyOutput :)
Friendly ping. If you would like me to do something to make reviewing this easier please let me know. We're hoping to have this feature hooked up for M47 (behind a flag), and one more patch has to happen after this.
The implementation LGTM. I didn't review the spec itself though to judge if we really want it in Chrome, deferring to rsleevi/peter for that judgement call https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:19: const int64_t kDefaultRecordSizeBytes = 4096; uint64_t https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:21: bool Base64URLDecode(const base::StringPiece& input, std::string* output) { There are already several duplications of base64url parsing in our code. (i.e. https://code.google.com/p/chromium/issues/detail?id=364749#c6) Would be good to extract this first. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:29: // Convert to standard base64 alphabet. It looks like this was copied from GCMNetworkChannel::Base64DecodeURLSafe ? https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:31: base::ReplaceChars(padded_input, "_", "/", &padded_input); This parsing is too permissive, in that it will allow inputs which contain + and / (since those are valid to Base64Decode) However an input containing + and / is an invalid base64url encoded value. For a workaround see: https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp... (although that also rejects padding characters which isn't necessary here). https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:53: if (!base::StringToInt64(value, rs)) why not use a uint64 and StringToUint64() since negatives are not in range? https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:25: int64_t* rs); uint64_t, since |rs| must be non-negative (in fact needs to be > 1) https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:33: { "keyid=foo", "foo", "", kDefaultRecordSize }, Does this file incldue a test for unrecognized roperties as in keyid=foo; foopy=sup ? https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:69: "rs=1", These are all good tests! I would additionally suggest: rs=0 rs=012 rs=0x13 rs=+5 (Since historically these are the kind of values that non-strict http parsers have recognized) https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:79: int64_t rs = kDefaultRecordSize; Does this initialization matter? (I don't mind it, just wondering) https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:129: "keyid=foo;novaluekey", I suggest also testing: * characters which are not part of base64url (+ and /) * Incorrect base64 padding https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:138: for (size_t i = 0; i < arraysize(expected_failures); i++) { optional: Though the magic of c++11 I believe you can write this as: for (const auto& expected_failure : expected_failures) { ... } But then you wouldn't be able to write SCOPED_TRACE(i)
Thank you for the review! All addressed, PTAL. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:19: const int64_t kDefaultRecordSizeBytes = 4096; On 2015/09/25 18:18:19, eroman wrote: > uint64_t Done. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:21: bool Base64URLDecode(const base::StringPiece& input, std::string* output) { On 2015/09/25 18:18:19, eroman wrote: > There are already several duplications of base64url parsing in our code. > (i.e. https://code.google.com/p/chromium/issues/detail?id=364749#c6) > > Would be good to extract this first. Certainly agreed. I filed https://crbug.com/536745 to track this. That said, while I'm happy to sign up to work on this, I don't think it should block this patch given the proximity to the branch point. I wouldn't be surprised if it takes a while. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:29: // Convert to standard base64 alphabet. On 2015/09/25 18:18:19, eroman wrote: > It looks like this was copied from GCMNetworkChannel::Base64DecodeURLSafe ? Yes, and I now mixed in parts from components/proximity_auth/cryptauth/base64url.cc per the following comment. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:31: base::ReplaceChars(padded_input, "_", "/", &padded_input); On 2015/09/25 18:18:19, eroman wrote: > This parsing is too permissive, in that it will allow inputs which contain + and > / (since those are valid to Base64Decode) > > However an input containing + and / is an invalid base64url encoded value. > > For a workaround see: > https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp... > > (although that also rejects padding characters which isn't necessary here). I've included a check that the characters "/" and "-" do not occur in the value, per the following implementation: components/proximity_auth/cryptauth/base64url.cc https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:53: if (!base::StringToInt64(value, rs)) On 2015/09/25 18:18:19, eroman wrote: > why not use a uint64 and StringToUint64() since negatives are not in range? Done. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:25: int64_t* rs); On 2015/09/25 18:18:19, eroman wrote: > uint64_t, since |rs| must be non-negative (in fact needs to be > 1) Done. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:33: { "keyid=foo", "foo", "", kDefaultRecordSize }, On 2015/09/25 18:18:19, eroman wrote: > Does this file incldue a test for unrecognized roperties as in keyid=foo; > foopy=sup ? Line 40 (Encryption) and 106 (Encryption-Key). https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:69: "rs=1", On 2015/09/25 18:18:19, eroman wrote: > These are all good tests! > I would additionally suggest: > rs=0 > rs=012 > rs=0x13 > rs=+5 > > (Since historically these are the kind of values that non-strict http parsers > have recognized) Done, except for the following two which pass: rs=012 rs=+5 (Both are allowed by the StringToUint64 implementation, and don't sound entirely unreasonable, despite 012 == 12 rather than 10) https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:79: int64_t rs = kDefaultRecordSize; On 2015/09/25 18:18:19, eroman wrote: > Does this initialization matter? (I don't mind it, just wondering) No, it does not. Muscle memory reminds me of cases where not initializing a value like this (even though it should only fail on access) causes odd compiler errors, but that may be outdated by now. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:129: "keyid=foo;novaluekey", On 2015/09/25 18:18:20, eroman wrote: > I suggest also testing: > * characters which are not part of base64url (+ and /) > * Incorrect base64 padding Done, also for the Encryption header. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:138: for (size_t i = 0; i < arraysize(expected_failures); i++) { On 2015/09/25 18:18:20, eroman wrote: > optional: Though the magic of c++11 I believe you can write this as: > > for (const auto& expected_failure : expected_failures) { > ... > } > > > But then you wouldn't be able to write SCOPED_TRACE(i) Acknowledged.
https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:21: bool Base64URLDecode(const base::StringPiece& input, std::string* output) { On 2015/09/28 11:27:15, Peter Beverloo wrote: > On 2015/09/25 18:18:19, eroman wrote: > > There are already several duplications of base64url parsing in our code. > > (i.e. https://code.google.com/p/chromium/issues/detail?id=364749#c6) > > > > Would be good to extract this first. > > Certainly agreed. I filed https://crbug.com/536745 to track this. > > That said, while I'm happy to sign up to work on this, I don't think it should > block this patch given the proximity to the branch point. I wouldn't be > surprised if it takes a while. sgtm https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:69: "rs=1", On 2015/09/28 11:27:15, Peter Beverloo wrote: > On 2015/09/25 18:18:19, eroman wrote: > > These are all good tests! > > I would additionally suggest: > > rs=0 > > rs=012 > > rs=0x13 > > rs=+5 > > > > (Since historically these are the kind of values that non-strict http parsers > > have recognized) > > Done, except for the following two which pass: > > rs=012 > rs=+5 > > (Both are allowed by the StringToUint64 implementation, and don't sound entirely > unreasonable, despite 012 == 12 rather than 10) Regarding rs=+5: While this behavior my be reasonable, it still strikes me as problematic. At a minimum it is something that is underspecified by the specification, which merely says that rs is a "positive decimal integer". The problem with extra permissiveness in parsing is it leads to incompatible implementations (I feel strongly about this from past experience, having to deal with a world where other UAs implemented number parsing via sscanf(%d) to horrifying results). For this case in particular given that details in your target specification are scant, I would look to RFC 2616 for precedent on how integers are represented. For instance here is how Content-Length is specified: DIGIT = <any US-ASCII digit "0".."9"> ... Content-Length = "Content-Length" ":" 1*DIGIT So working off this I would expect that: * leading 0 is acceptable, and is to be interpreted as decimal (not octal) * Leading + is invalid. So too are any other prefix like 0x, or any numerical separator. So anyway my expectation is that rs=+5 is something to reject. Can you follow-up with the spec on this?
This is a spec that I really don't like, but where I *especially* don't want to get involved in the standardization part of it given the parties involved. Regarding "Who can sign off on the crypto", I think your best bet is discussing with the internal crypto-help team, so that we can avoid the (many) (recent) mistakes made in IETF encryption standards that, as a result of getting the imprimatur of IETF, have led our development teams to think they're a good idea. I realize much of the spec tries to handwave away its security considerations, but it makes me considerably uncomfortable... (The asymmetry between Encryption and Encryption-Key, for example) https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:30: input.size() % 4 ? input.size() + 4 - (input.size() % 4) : input.size(); Erm, can't this overflow in some (extremely pedantic) scenarios? https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:34: padded_input.resize(padded_size, '='); Blergh; having to copy this string makes it all the more questionable whether or not we should just fix this in //base first, so that we can do a copy-less operation and just signify via the API whether or not padding is expected. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:138: // [ ";" "rs" "=" octet-count ] Comment gripe: So, this isn't what the spec says, but it conveniently highlights the bug in your implementation "Encryption" follows the list-rule of #encryption_params, which means multiple headers can be coalesced via commas. Your parsing ignores the commas, which seems like it can lead to all sorts of weird situations. Consider the following header: Encryption: keyid=a,keyid=b;salt=foo;rs=8 You'd end up parsing this as "keyid" = "a,keyid=b", "salt" = "foo", "rs" = "8". So I think you have to account for the comma separation here, and I'd argue it's cleaner & clearer to use the ABNF from the RFC rather than this form, along with perhaps a table of expected forms. If we're not going to support multiple encryption layers, that's fine, but we should parse the header at least. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:13: // Parses |input| following the syntax of the Encryption HTTP header. Chrome I don't believe //components should be talking about Chrome, especially since //components are designed to be reusable. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:14: // only supports a single set of properties to be read from the header at this I'm having trouble parsing this, based on header + draft alone. When you say 'single set of properties', are you describing the multiple layers of encryption that may be applied? If so, why the arbitrary limitation for a single layer of encryption? That seems non-conformant - if we don't want to support that, we should be clear about that (and arguably the spec should adapt), otherwise, we should support multiple layers, and it may make more sense from an API contract to have some form of parser/iterator that takes a combined header value and then outputs the parameters for each subsequent layer of encryption. As the API is designed now, it's not possible to even support multiple layers without recoding these interfaces, that's why I'm trying to figure out the reasoning. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:28: // only supports a single set of properties to be read from the header at this Ditto confusion
Ryan, Eric, thank you for the comments! The parser can now parse the header according to the specification, and I have updated the comments and tests to reflect this. I have also applied your other comments. https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right): https://codereview.chromium.org/1244803002/diff/60001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:69: "rs=1", On 2015/09/28 16:25:29, eroman wrote: > On 2015/09/28 11:27:15, Peter Beverloo wrote: > > On 2015/09/25 18:18:19, eroman wrote: > > > These are all good tests! > > > I would additionally suggest: > > > rs=0 > > > rs=012 > > > rs=0x13 > > > rs=+5 > > > > > > (Since historically these are the kind of values that non-strict http > parsers > > > have recognized) > > > > Done, except for the following two which pass: > > > > rs=012 > > rs=+5 > > > > (Both are allowed by the StringToUint64 implementation, and don't sound > entirely > > unreasonable, despite 012 == 12 rather than 10) > > Regarding rs=+5: > > While this behavior my be reasonable, it still strikes me as problematic. > > At a minimum it is something that is underspecified by the specification, which > merely says that rs is a "positive decimal integer". > > The problem with extra permissiveness in parsing is it leads to incompatible > implementations (I feel strongly about this from past experience, having to deal > with a world where other UAs implemented number parsing via sscanf(%d) to > horrifying results). > > For this case in particular given that details in your target specification are > scant, I would look to RFC 2616 for precedent on how integers are represented. > For instance here is how Content-Length is specified: > > DIGIT = <any US-ASCII digit "0".."9"> > ... > Content-Length = "Content-Length" ":" 1*DIGIT > > So working off this I would expect that: > * leading 0 is acceptable, and is to be interpreted as decimal (not octal) > * Leading + is invalid. So too are any other prefix like 0x, or any numerical > separator. > > So anyway my expectation is that rs=+5 is something to reject. > > Can you follow-up with the spec on this? That's a great explanation, thank you! Since this is an addition on top of the string to integer routines in //base, I will add an if-statement in RecordSizeToInt() and re-included the test. In general, the spec defines both headers as accepting a number of "parameter" rules, which basically accept anything in the "key=value" syntax, and then imposes limitations on the values based on the expected kinds of data. There probably is a way to enforce the exact syntax for each of these rules. I'll follow up. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:30: input.size() % 4 ? input.size() + 4 - (input.size() % 4) : input.size(); On 2015/09/28 19:35:48, Ryan Sleevi wrote: > Erm, can't this overflow in some (extremely pedantic) scenarios? Yes. However, the only scenario in which *this* would overflow, and not std::string::size() (which is also of type size_t), is when the size of the input string is (> SIZE_T_MAX - 3) and (<= SIZE_T_MAX). This class will only handle input that is received from Google GCM servers, which impose a limit of 4K - six orders of magnitude lower on 32-bit systems, 15 orders of magnitude on 64-bit systems :-). Switched to a CheckedNumeric<size_t>. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:34: padded_input.resize(padded_size, '='); On 2015/09/28 19:35:48, Ryan Sleevi wrote: > Blergh; having to copy this string makes it all the more questionable whether or > not we should just fix this in //base first, so that we can do a copy-less > operation and just signify via the API whether or not padding is expected. Acknowledged. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.cc:138: // [ ";" "rs" "=" octet-count ] On 2015/09/28 19:35:48, Ryan Sleevi wrote: > Comment gripe: So, this isn't what the spec says, but it conveniently highlights > the bug in your implementation > > "Encryption" follows the list-rule of #encryption_params, which means multiple > headers can be coalesced via commas. Your parsing ignores the commas, which > seems like it can lead to all sorts of weird situations. > > Consider the following header: > Encryption: keyid=a,keyid=b;salt=foo;rs=8 > > You'd end up parsing this as "keyid" = "a,keyid=b", "salt" = "foo", "rs" = "8". > > So I think you have to account for the comma separation here, and I'd argue it's > cleaner & clearer to use the ABNF from the RFC rather than this form, along with > perhaps a table of expected forms. > > If we're not going to support multiple encryption layers, that's fine, but we > should parse the header at least. Done. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:13: // Parses |input| following the syntax of the Encryption HTTP header. Chrome On 2015/09/28 19:35:48, Ryan Sleevi wrote: > I don't believe //components should be talking about Chrome, especially since > //components are designed to be reusable. Done. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:14: // only supports a single set of properties to be read from the header at this On 2015/09/28 19:35:48, Ryan Sleevi wrote: > I'm having trouble parsing this, based on header + draft alone. > > When you say 'single set of properties', are you describing the multiple layers > of encryption that may be applied? If so, why the arbitrary limitation for a > single layer of encryption? That seems non-conformant - if we don't want to > support that, we should be clear about that (and arguably the spec should > adapt), otherwise, we should support multiple layers, and it may make more > sense from an API contract to have some form of parser/iterator that takes a > combined header value and then outputs the parameters for each subsequent layer > of encryption. > > As the API is designed now, it's not possible to even support multiple layers > without recoding these interfaces, that's why I'm trying to figure out the > reasoning. Done. https://codereview.chromium.org/1244803002/diff/80001/components/gcm_driver/c... components/gcm_driver/crypto/encryption_header_parsers.h:28: // only supports a single set of properties to be read from the header at this On 2015/09/28 19:35:48, Ryan Sleevi wrote: > Ditto confusion Done.
LGTM, although with a *lot* of angst (read the .h file comments first) and mod several nits. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:55: bool RecordSizeToInt(const std::string::const_iterator& begin, Document https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:75: bool ParseEncryptionHeaderValuesImpl(std::string::const_iterator input_begin, Document https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:107: bool ParseEncryptionKeyHeaderValuesImpl(std::string::const_iterator input_begin, Document https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:144: // [ ";" "rs" "=" octet-count ] Seems like this comment should be moved to 75? https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:151: net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ','); Worth documenting this header follows #list rule and some reference to the relevant 7230/7231 (or whatever the RFC is that covers the modified ABNF with the #list rule) https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:172: // [ ";" "dh" "=" base64url ] And this to 107? https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:179: net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ','); Ditto :) https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.h:50: std::vector<EncryptionKeyHeaderValues>* values); I know I'm being a pain, but this is still fairly inconsistent with the //net HTTP parsing logic, which opts for iterator interfaces over headers. That is, example interfaces to consider HeadersIterator (//net/http/http_util) ValuesIterator (//net/http/http_util) NameValuePairsIterator (//net/http/http_util) It's unclear to me how often this will be in the fast path, or the performance v simplicity tradeoffs. My gut instinct is that we'd be better off with an iterator approach with no (long-term) copying, as that would reduce memory *and* allocations. I tried to refer to this in my original remark ("it may make more sense from an API contract to have some form of parser/iterator"), but I realize in re-reading that it may have been less clear as to *why* I was recommending this. I'm torn on how hard to push back on this; I'd want to push back and say "do it as an iterator", but what you have is valid and works, and my arguments are "consistency and performance", and I'm not sure how relevant either are (the former because this isn't part of //net, the latter because maybe performance doesn't matter here?). I also don't want to be saying "Do it my way", since that's bad review feedback. I guess use your best judgement here, with a *strong* encouragement for doing it my way :P
Thank you for the review. I have filed crbug.com/538576 for updating the parser to be iterator-based (see my reply to the comment), and crbug.com/536745 exists for the base64url issue. These will be followed up on Soon(tm). https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/encryption_header_parsers.cc (right): https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:55: bool RecordSizeToInt(const std::string::const_iterator& begin, On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:75: bool ParseEncryptionHeaderValuesImpl(std::string::const_iterator input_begin, On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:107: bool ParseEncryptionKeyHeaderValuesImpl(std::string::const_iterator input_begin, On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:144: // [ ";" "rs" "=" octet-count ] On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Seems like this comment should be moved to 75? Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:151: net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ','); On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Worth documenting this header follows #list rule and some reference to the > relevant 7230/7231 (or whatever the RFC is that covers the modified ABNF with > the #list rule) Done in the header, so that this function doesn't have two places with documentation. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:172: // [ ";" "dh" "=" base64url ] On 2015/10/01 22:50:45, Ryan Sleevi wrote: > And this to 107? Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.cc:179: net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ','); On 2015/10/01 22:50:45, Ryan Sleevi wrote: > Ditto :) Done. https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... File components/gcm_driver/crypto/encryption_header_parsers.h (right): https://codereview.chromium.org/1244803002/diff/100001/components/gcm_driver/... components/gcm_driver/crypto/encryption_header_parsers.h:50: std::vector<EncryptionKeyHeaderValues>* values); On 2015/10/01 22:50:45, Ryan Sleevi wrote: > I know I'm being a pain, but this is still fairly inconsistent with the //net > HTTP parsing logic, which opts for iterator interfaces over headers. > > That is, example interfaces to consider > > HeadersIterator (//net/http/http_util) > ValuesIterator (//net/http/http_util) > NameValuePairsIterator (//net/http/http_util) > > It's unclear to me how often this will be in the fast path, or the performance v > simplicity tradeoffs. My gut instinct is that we'd be better off with an > iterator approach with no (long-term) copying, as that would reduce memory *and* > allocations. I tried to refer to this in my original remark ("it may make more > sense from an API contract to have some form of parser/iterator"), but I realize > in re-reading that it may have been less clear as to *why* I was recommending > this. > > I'm torn on how hard to push back on this; I'd want to push back and say "do it > as an iterator", but what you have is valid and works, and my arguments are > "consistency and performance", and I'm not sure how relevant either are (the > former because this isn't part of //net, the latter because maybe performance > doesn't matter here?). I also don't want to be saying "Do it my way", since > that's bad review feedback. I guess use your best judgement here, with a > *strong* encouragement for doing it my way :P You are saying "do it my way" because of consistency and performance - that's entirely different from saying "do it my way" before of personal preferences. You are of course completely right in that regard. I am going to play the branch point card here. I'll use two tokens, one for keeping the base64url method, a second one for this comment, and will repay them by following up with the proper fixes next week, and apply this feedback. To repeat what you said earlier, it is important to start gathering and sharing feedback as early as we can, in order to get this out the door when we're satisfied. Maximizing the ability for developers to try out encryption will play an important part in that, which is why I feel that having end-to-end in M47 makes a difference. Retrospectively, I regret not understanding your earlier feedback so that I could have put this through immediately, but this CL has been a long road as it is.
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, eroman@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1244803002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1244803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1244803002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1f99ff0f7ccd143dcfcd46161500bc9d62d031c6 Cr-Commit-Position: refs/heads/master@{#352027} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
