Convert the encryption header parsers to be iterator-based.

components/gcm_driver/crypto/encryption_header_parsers.cc

Index: components/gcm_driver/crypto/encryption_header_parsers.cc
diff --git a/components/gcm_driver/crypto/encryption_header_parsers.cc b/components/gcm_driver/crypto/encryption_header_parsers.cc
index ccfae9fe57dfa18bc599bd435692a97dd745c57a..eced6ad82a24577c50fe81b8a566bbf6ed401da5 100644
--- a/components/gcm_driver/crypto/encryption_header_parsers.cc
+++ b/components/gcm_driver/crypto/encryption_header_parsers.cc
@@ -6,38 +6,38 @@
 
 #include "base/base64url.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/strings/string_piece.h"
+
 #include "base/strings/string_util.h"
-#include "net/http/http_util.h"
+
 
 namespace gcm {
 
 namespace {
 
 // The default record size in bytes, as defined in section two of
-// https://tools.ietf.org/html/draft-thomson-http-encryption-02.
+// https://tools.ietf.org/html/draft-thomson-http-encryption.
 const uint64_t kDefaultRecordSizeBytes = 4096;
 
-// Decodes the string between |begin| and |end| using base64url, and writes the
-// decoded value to |*salt|. Returns whether the string could be decoded.
-bool ValueToDecodedString(const std::string::const_iterator& begin,
-                          const std::string::const_iterator& end,
-                          std::string* salt) {
-  const base::StringPiece value(begin, end);
+// Decodes the string in |value| using base64url and writes the decoded value to
+// |*salt|. Returns whether the string could be decoded.
+bool ValueToDecodedString(base::StringPiece value, std::string* salt) {
   if (value.empty())
     return false;
 
-  return base::Base64UrlDecode(
-      value, base::Base64UrlDecodePolicy::IGNORE_PADDING, salt);
+  std::string candidate_salt;
+  if (!base::Base64UrlDecode(value, base::Base64UrlDecodePolicy::IGNORE_PADDING,
+                             &candidate_salt)) {
+    return false;
+  }
+
+  salt->swap(candidate_salt);
+  return true;
 }
 
-// Parses the record size between |begin| and |end|, and writes the value to
-// |*rs|. The value must be an unsigned, 64-bit integer greater than zero that
-// does not start with a plus. Returns whether the record size was valid.
-bool RecordSizeToInt(const std::string::const_iterator& begin,
-                     const std::string::const_iterator& end,
-                     uint64_t* rs) {
-  const base::StringPiece value(begin, end);
+// Parses the record size in |value| and writes the value to |*rs|. The value
+// must be an unsigned, 64-bit integer greater than zero that does not start
+// with a plus. Returns whether the record size was valid.
+bool RecordSizeToInt(base::StringPiece value, uint64_t* rs) {
   if (value.empty())
     return false;
 
@@ -47,53 +47,51 @@ bool RecordSizeToInt(const std::string::const_iterator& begin,
   if (value[0] == '+')

[Ryan Sleevi, 2015/12/08 00:22:00]

But is '-' allowed? Or are you relying on the Uint64 to cause that to be
rejected?

[Peter Beverloo, 2015/12/08 01:02:58]

IteratorRangeToNumber::Invoke will consider the input to be invalid when an '-'
is included for a non-signed type (which uint64_t is), so yes.

     return false;
 
-  if (!base::StringToUint64(value, rs))
+  uint64_t candidate_rs;
+  if (!base::StringToUint64(value, &candidate_rs) ||
+      candidate_rs <= 1 /* the record size MUST be greater than 1 */)

[Ryan Sleevi, 2015/12/08 00:22:00]

Please add braces to this conditional if kept as multi-line.

Also, the inline comment style seems... less common for Chromium code

if (!base::StringToUint64(value, &candidate_rs))
  return false;

// The record size MUST be greater than one byte.
if (candidate_rs <= 1)
  return false;

[Peter Beverloo, 2015/12/16 21:09:57]

Done.

     return false;
 
-  // The record size MUST be greater than 1.
-  return *rs > 1;
+  *rs = candidate_rs;
+  return true;
 }
 
-// Parses the string between |input_begin| and |input_end| according to the
-// extended ABNF syntax for the Encryption HTTP header, per the "parameter"
-// rule from RFC 7231 (https://tools.ietf.org/html/rfc7231).
-//
-// encryption_params = [ parameter *( ";" parameter ) ]
-//
-// This implementation applies the parameters defined in section 3.1 of the
-// HTTP encryption encoding document:
-//
-// https://tools.ietf.org/html/draft-thomson-http-encryption-02#section-3.1
-//
-// This means that the three supported parameters are:
-//
-//     [ "keyid" "=" string ]
-//     [ ";" "salt" "=" base64url ]
-//     [ ";" "rs" "=" octet-count ]
-bool ParseEncryptionHeaderValuesImpl(std::string::const_iterator input_begin,
-                                     std::string::const_iterator input_end,
-                                     EncryptionHeaderValues* values) {
+}  // namespace
+
+EncryptionHeaderIterator::EncryptionHeaderIterator(
+    std::string::const_iterator header_begin,
+    std::string::const_iterator header_end)
+    : iterator_(header_begin, header_end, ','),
+      rs_(kDefaultRecordSizeBytes) {}
+
+EncryptionHeaderIterator::~EncryptionHeaderIterator() {}
+
+bool EncryptionHeaderIterator::GetNext() {
+  keyid_.clear();
+  salt_.clear();
+  rs_ = kDefaultRecordSizeBytes;
+
+  if (!iterator_.GetNext())
+    return false;
+
   net::HttpUtil::NameValuePairsIterator name_value_pairs(
-      input_begin, input_end, ';',
+      iterator_.value_begin(), iterator_.value_end(), ';',
       net::HttpUtil::NameValuePairsIterator::VALUES_NOT_OPTIONAL);
 
   while (name_value_pairs.GetNext()) {
     const base::StringPiece name(name_value_pairs.name_begin(),
                                  name_value_pairs.name_end());
+    const base::StringPiece value(name_value_pairs.value_begin(),
+                                  name_value_pairs.value_end());
 

[Ryan Sleevi, 2015/12/08 00:22:00]

SPEC BUG: The spec is unclear whether multiple parameters are valid, and if so,
what the behaviour of the client should be.

Presently, you've opted for the "last parameter wins", but in other places, it's
"one and only one instance of this parameter permitted". It would be good for
the spec to be clearer on this.

[Peter Beverloo, 2015/12/08 01:02:58]

I will take an action item to generate a PR against the spec.

[Peter Beverloo, 2015/12/16 21:09:57]

https://github.com/martinthomson/http-encryption/pull/6

     if (base::LowerCaseEqualsASCII(name, "keyid")) {
-      values->keyid.assign(name_value_pairs.value_begin(),
-                           name_value_pairs.value_end());
+      value.CopyToString(&keyid_);
     } else if (base::LowerCaseEqualsASCII(name, "salt")) {
-      if (!ValueToDecodedString(name_value_pairs.value_begin(),
-                                name_value_pairs.value_end(), &values->salt)) {
+      if (!ValueToDecodedString(value, &salt_))
         return false;
-      }
     } else if (base::LowerCaseEqualsASCII(name, "rs")) {
-      if (!RecordSizeToInt(name_value_pairs.value_begin(),
-                           name_value_pairs.value_end(), &values->rs)) {
+      if (!RecordSizeToInt(value, &rs_))
         return false;
-      }
     } else {
       // Silently ignore unknown directives for forward compatibility.
     }
@@ -102,47 +100,39 @@ bool ParseEncryptionHeaderValuesImpl(std::string::const_iterator input_begin,
   return name_value_pairs.valid();
 }
 
-// Parses the string between |input_begin| and |input_end| according to the
-// extended ABNF syntax for the Crypto-Key HTTP header, per the "parameter" rule
-// from RFC 7231 (https://tools.ietf.org/html/rfc7231).
-//
-// encryption_params = [ parameter *( ";" parameter ) ]
-//
-// This implementation applies the parameters defined in section 4 of the
-// HTTP encryption encoding document:
-//
-//https://tools.ietf.org/html/draft-thomson-http-encryption-02#section-4
-//
-// This means that the three supported parameters are:
-//
-//     [ "keyid" "=" string ]
-//     [ ";" "aesgcm128" "=" base64url ]
-//     [ ";" "dh" "=" base64url ]
-bool ParseCryptoKeyHeaderValuesImpl(std::string::const_iterator input_begin,
-                                    std::string::const_iterator input_end,
-                                    CryptoKeyHeaderValues* values) {
+CryptoKeyHeaderIterator::CryptoKeyHeaderIterator(
+    std::string::const_iterator header_begin,
+    std::string::const_iterator header_end)
+    : iterator_(header_begin, header_end, ',') {}
+
+CryptoKeyHeaderIterator::~CryptoKeyHeaderIterator() {}
+
+bool CryptoKeyHeaderIterator::GetNext() {
+  keyid_.clear();
+  aesgcm128_.clear();
+  dh_.clear();
+
+  if (!iterator_.GetNext())
+    return false;
+
   net::HttpUtil::NameValuePairsIterator name_value_pairs(
-      input_begin, input_end, ';',
+      iterator_.value_begin(), iterator_.value_end(), ';',
       net::HttpUtil::NameValuePairsIterator::VALUES_NOT_OPTIONAL);
 
   while (name_value_pairs.GetNext()) {
     const base::StringPiece name(name_value_pairs.name_begin(),
                                  name_value_pairs.name_end());
+    const base::StringPiece value(name_value_pairs.value_begin(),
+                                  name_value_pairs.value_end());
 
     if (base::LowerCaseEqualsASCII(name, "keyid")) {
-      values->keyid.assign(name_value_pairs.value_begin(),
-                           name_value_pairs.value_end());
+      value.CopyToString(&keyid_);
     } else if (base::LowerCaseEqualsASCII(name, "aesgcm128")) {
-      if (!ValueToDecodedString(name_value_pairs.value_begin(),
-                                name_value_pairs.value_end(),
-                                &values->aesgcm128)) {
+      if (!ValueToDecodedString(value, &aesgcm128_))
         return false;
-      }
     } else if (base::LowerCaseEqualsASCII(name, "dh")) {
-      if (!ValueToDecodedString(name_value_pairs.value_begin(),
-                                name_value_pairs.value_end(), &values->dh)) {
+      if (!ValueToDecodedString(value, &dh_))
         return false;
-      }
     } else {
       // Silently ignore unknown directives for forward compatibility.
     }
@@ -151,52 +141,4 @@ bool ParseCryptoKeyHeaderValuesImpl(std::string::const_iterator input_begin,
   return name_value_pairs.valid();
 }
 
-}  // namespace
-
-bool ParseEncryptionHeader(const std::string& input,
-                           std::vector<EncryptionHeaderValues>* values) {
-  DCHECK(values);
-
-  std::vector<EncryptionHeaderValues> candidate_values;
-
-  net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ',');
-  while (value_iterator.GetNext()) {
-    EncryptionHeaderValues candidate_value;
-    candidate_value.rs = kDefaultRecordSizeBytes;
-
-    if (!ParseEncryptionHeaderValuesImpl(value_iterator.value_begin(),
-                                         value_iterator.value_end(),
-                                         &candidate_value)) {
-      return false;
-    }
-
-    candidate_values.push_back(candidate_value);
-  }
-
-  values->swap(candidate_values);
-  return true;
-}
-
-bool ParseCryptoKeyHeader(const std::string& input,
-                          std::vector<CryptoKeyHeaderValues>* values) {
-  DCHECK(values);
-
-  std::vector<CryptoKeyHeaderValues> candidate_values;
-
-  net::HttpUtil::ValuesIterator value_iterator(input.begin(), input.end(), ',');
-  while (value_iterator.GetNext()) {
-    CryptoKeyHeaderValues candidate_value;
-    if (!ParseCryptoKeyHeaderValuesImpl(value_iterator.value_begin(),
-                                        value_iterator.value_end(),
-                                        &candidate_value)) {
-      return false;
-    }
-
-    candidate_values.push_back(candidate_value);
-  }
-
-  values->swap(candidate_values);
-  return true;
-}
-
 }  // namespace gcm