Chrome Participated Tamper Detect

components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_
+#define COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_
+
+#include "components/data_reduction_proxy/common/data_reduction_proxy_headers.h"
+
+#include "net/http/http_response_headers.h"

[bengr, 2014/07/11 18:22:48]

#include <string>
#include <vector>

[xingx, 2014/07/15 04:51:36]

Done.

+

[bengr, 2014/07/11 18:22:47]

namespace net {
class HttpResponseHeaders;
}

[xingx, 2014/07/15 04:51:36]

Done.

+namespace data_reduction_proxy {
+
+// The class for detecting tampering.

[bengr, 2014/07/11 18:22:48]

Explain what that means. E.g., "This class that detects if response header
information sent by the data reduction proxy has been modified or deleted by
intermediaries on the Web. Specifically, it detects these forms of tampering:
..."

[xingx, 2014/07/15 04:51:35]

Done.

+// For each type of fingerprint, we have a pair of functions:
+//    * checking function:  returns true if such fingerprint got tampered;
+//                          (function name Is...Tampered)
+//    * reporting function: reporting tampering to corresponding UMA.
+//                          (function name Report...TamperedUMA)
+class DataReductionProxyTamperDetect {
+ public:
+  DataReductionProxyTamperDetect(

[bengr, 2014/07/11 18:22:48]

suggest: DataReductionProxyTamperDetection

[bengr, 2014/07/11 18:22:48]

Add a comment. What are all these parameters? How are they used?

[xingx, 2014/07/15 04:51:35]

Done.

[xingx, 2014/07/15 04:51:36]

Done.

+      const net::HttpResponseHeaders* response_headers,
+      bool is_secure_scheme,
+      unsigned mcc_mnc,

[bengr, 2014/07/11 18:22:49]

rename: carrier_id.

[xingx, 2014/07/15 04:51:36]

Done.

+      std::vector<std::string>* chrome_proxy_header_values);
+
+  virtual ~DataReductionProxyTamperDetect();
+
+  // The main function for detecting tampering, which checks whether there is
+  // tamper detect request (i.e., contains fingerprints added by data reduction

[bengr, 2014/07/11 18:22:48]

Be clearer, e.g.:

"Checks if the response contains tamper detection fingerprints added by the data
reduction proxy, and determines if the response had been tampered with if so.
Results are reported to UMA. HTTP and..."

[xingx, 2014/07/15 04:51:35]

Done.

+  // proxy) in the response from data reduction proxy. if so, it checks whether
+  // there are tampers and report the results to UMA. HTTP and HTTPS traffic
+  // would be reported independently, specified by |is_secure_scheme|.
+  static void CheckResponseFingerprint(const net::HttpResponseHeaders* header,

[bengr, 2014/07/11 18:22:48]

Move this static method above the constructor.

[xingx, 2014/07/15 04:51:36]

Done.

+                                       bool is_secure_scheme);
+
+ private:
+  friend class DataReductionProxyTamperDetectTest;
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           TestFingerprintCommon);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           ChromeProxy);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           Via);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           OtherHeaders);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           ContentLength);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           Parsing);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectTest,
+                           Completed);
+
+  // Enum for fingerprint type.

[bengr, 2014/07/11 18:22:48]

Put each value on its own line and add a comment for each.

[xingx, 2014/07/15 04:51:36]

Done.

+  enum FingerprintCode { CHROMEPROXY,
+                         VIA,
+                         OTHERHEADERS,
+                         CONTENTLENGTH,
+                         NONEXIST };
+
+  // Returns true if the Chrome-Proxy header has been tampered.
+  bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const;

[bengr, 2014/07/11 18:22:48]

Where's the reporting function fo this one?

[xingx, 2014/07/15 04:51:35]

Done.

+
+  // Returns true if the Via header has been tampered.
+  bool IsViaHeaderTampered(const std::string& fingerprint) const;
+  // Reports UMA for tampering of the Via header.
+  void ReportViaHeaderTamperedUMA() const;
+
+  // Returns true if a list of data reductin proxy defined headers have been
+  // tampered.
+  bool AreOtherHeadersTampered(const std::string& fingerprint) const;
+  // Reports UMA for tampering of values of the list of headers.
+  void ReportOtherHeadersTamperedUMA() const;
+
+  // Returns true if Content-Length has been tampered.
+  bool IsContentLengthHeaderTampered(const std::string& fingerprint) const;
+  // Reports UMA for tampering of Content-Length.
+  void ReportContentLengthHeaderTamperedUMA() const;
+
+  // Returns fingerprint code (enum) for the given fingerprint tag.
+  FingerprintCode GetFingerprintCode(const std::string& fingerprint_tag);
+
+  // Check whether values of a Chrome-Proxy header contains fingerprints added

[bengr, 2014/07/11 18:22:48]

contain

[xingx, 2014/07/15 04:51:35]

Done.

+  // by data reduction proxy. If it does, return true, and save Chrome-Proxy

[bengr, 2014/07/11 18:22:48]

the data reduction

If they do

and save the

[xingx, 2014/07/15 04:51:36]

Done.

+  // header's fingerprint to |chrome_proxy_fingerprint|; also save other
+  // fingerprints to |other_fingerprints|. Return false if there is no
+  // fingerprint found. Chrome-Proxy header's fingerprint will be removed from
+  // |values| to get |clean_chrome_proxy_header_values| for later use.
+  static bool GetTamperDetectFingerprints(std::vector<std::string>* values,

[bengr, 2014/07/11 18:22:48]

Move this first param to a new line or align the subsequent ones with it.

[xingx, 2014/07/15 04:51:36]

Done.

+      std::string* chrome_proxy_fingerprint,
+      std::string* other_fingerprints);
+
+  // Utility function. Return string of sorted values of |values|.

[bengr, 2014/07/11 18:22:48]

Returns

And no need to call it a utility function.

[xingx, 2014/07/15 04:51:36]

Done.

+  static std::string ValuesToSortedString(std::vector<std::string> &values);

[bengr, 2014/07/11 18:22:48]

Do not use non-const references.
And the & should be next to the type, not the name.

[xingx, 2014/07/15 04:51:36]

Done.

+
+  // Utility function. Return MD5 hash value for a given string |input|.

[bengr, 2014/07/11 18:22:48]

Remove "Utility function"

Returns the

[xingx, 2014/07/15 04:51:35]

Done.

+  // We need raw MD5 hash value so it's different to base::MD5String which is

[bengr, 2014/07/11 18:22:48]

Remove "We". I.e., don't personify code or comment on the goals of the
programmer.

[xingx, 2014/07/15 04:51:36]

Done.

+  // base16 encoded. It's similar to base::MD5Sum but with digest converted to
+  // string.
+  static std::string GetMD5(const std::string& input);
+
+  // Utility function. Return all the values of a header field |header_name|
+  // of the response header |headers|, as a vector.
+  static std::vector<std::string> GetHeaderValues(
+      const net::HttpResponseHeaders* headers, const std::string& header_name);
+
+  const net::HttpResponseHeaders* response_headers_;
+
+  // If true, the connection to the data reduction proxy is over HTTPS.
+  const bool is_secure_scheme_;
+
+  // Carrier ID.
+  const unsigned mcc_mnc_;

[bengr, 2014/07/11 18:22:48]

Don't use abbreviations in variable names. Can you just call the variable
carrier_id_?

[xingx, 2014/07/15 04:51:36]

Done.

+
+  // Values for Chrome-Proxy header, with its fingerprint value removed.
+  // Save it as temporary result so we don't need to parse Chrome-Proxy header

[bengr, 2014/07/11 18:22:49]

as a

parse the

[xingx, 2014/07/15 04:51:35]

Done.

+  // twice.
+  std::vector<std::string>* clean_chrome_proxy_header_values_;
+
+  // Map a fingerprint tag (string) to a fingerprint code (enum).
+  std::map<std::string, FingerprintCode> fingperprint_tag_code_map_;
+};
+
+}  // namespace data_reduction_proxy
+#endif  // COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_