Chrome Participated Tamper Detect

components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h

Index: components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h
diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h b/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h
new file mode 100644
index 0000000000000000000000000000000000000000..099e656f9c0f17bca71b7a943620c3701f1fc9d7
--- /dev/null
+++ b/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h
@@ -0,0 +1,126 @@
+// 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 <map>
+
+#include "components/data_reduction_proxy/common/data_reduction_proxy_headers.h"
+
+#include "net/http/http_response_headers.h"
+
+//namespace net {

[bengr, 2014/07/07 17:01:33]

Remove dead code. Though I think you need this for below.

[xingx, 2014/07/08 00:22:25]

Done.

+//class HttpResponseHeaders;
+//}
+
+namespace data_reduction_proxy {
+
+// Two fingerprints will be added to Chrome-Proxy header.

[bengr, 2014/07/07 17:01:34]

Fill the comment out to 80 characters on each line.

[xingx, 2014/07/08 00:22:26]

Done.

+// One starts with |kTamperDetectFingerprintChromeProxy|, which is the
+// fingerprint for Chrome-Proxy header.

[bengr, 2014/07/07 17:01:33]

for the

[xingx, 2014/07/08 00:22:26]

Done.

+// The other one starts with |kTamperDetectFingerprint|, which includes
+// all other fingerprints.
+extern const char kTamperDetectFingerprint[];
+extern const char kTamperDetectFingerprintChromeProxy[];
+
+// Fingerprint |kTamperDetectFingerprint| contains multiple
+// fingerprints, each starts with a tag followed by "=" and its fingerprint
+// value. Three fingerprints and their respective tags are defined below.
+extern const char kTamperDetectFingerprintVia[];

[bengr, 2014/07/07 17:01:35]

Do these really need to be visible to the entire namespace? It looks like you
only need these in the .cc and in your tests. I'd suggest putting these in an
anonymous namespace in the .cc and not using these directly from your tests.

[xingx, 2014/07/08 00:22:25]

Done.

+extern const char kTamperDetectFingerprintOther[];
+extern const char kTamperDetectFingerprintContengLength[];
+
+// Utility function, exposed for unittest.
+// Check whether values of a header field |values| contains the Chrome-Proxy
+// header's fingerprint (starts with |kTamperDetectFingerprintChromeProxy|).
+// If there is, return true, and save Chrome-Proxy header's fingerprint to

[bengr, 2014/07/07 17:01:33]

If there is --> If it does

[xingx, 2014/07/08 00:22:26]

Done.

+// |chrome_proxy_fingerprint|;
+// also save other fingerprints (starts with |kTamperDetectFingerprintOther|)
+// to |other_fingerprints|.
+// Return false if there is no Chrome-Proxy header's fingerprint found.

[bengr, 2014/07/07 17:01:34]

header's -> header

[xingx, 2014/07/08 00:22:26]

Done.

+bool ContainsTamperDetectFingerprints(std::vector<std::string>& values,

[bengr, 2014/07/07 17:01:33]

Make this a private or protected static member and friend your tests.

+                                      std::string& chrome_proxy_fingerprint,

[bengr, 2014/07/07 17:01:34]

Do not use non-const references.

[xingx, 2014/07/08 00:22:27]

Will discuss with you.

+                                      std::string& other_fingerprints);
+
+// The main function for detecting tamper. It takes two parameters as input,

[bengr, 2014/07/07 17:01:33]

tamper -> tampering.

[xingx, 2014/07/08 00:22:26]

Done.

+//   1. a pointer to HttpResponseHeaders,
+//   2. a boolean variable indicates whether the connection

[bengr, 2014/07/07 17:01:34]

variable indicates -> variable that indicates

[xingx, 2014/07/08 00:22:27]

Done.

+//      between Chrome and data reduction proxy is on HTTPS or not.
+// For such response, the function checks whether there is a tamper detect

[bengr, 2014/07/07 17:01:34]

What is "such response"? Be clearer

[xingx, 2014/07/08 00:22:26]

Done.

+// request (contains fingerprints) from data reduction proxy, if so, it checks
+// whether there are tampers and report the results to UMA.
+void CheckResponseFingerprint(const net::HttpResponseHeaders*, const bool);
+
+//extern enum FingerprintCode;
+enum FingerprintCode { CHROMEPROXY, VIA, OTHERHEADERS,

[bengr, 2014/07/07 17:01:33]

Put each value on a separate line.

[xingx, 2014/07/08 00:22:26]

Done.

+                       CONTENTLENGTH, NONEXIST };
+
+// The class for detecting tamper.

[bengr, 2014/07/07 17:01:34]

tampering.

[xingx, 2014/07/08 00:22:26]

Done.

+// It wraps up the functionalities for tamper detection.

[bengr, 2014/07/07 17:01:35]

Remove this line. It doesn't add anything. Try to add a more descriptive
comment.

[xingx, 2014/07/08 00:22:25]

Done.

+// For each fingerprint, we need to implement two functions:
+//    * checking function:  returns tamper or not for such fingerprint;
+//                          (function name starts with Check...)
+//    * reporting function: reporting results to corresponding UMA
+//                          when there are tampers detected.
+//                          (function name starts with Report...)
+class DataReductionProxyTamperDetect {
+ public:
+  DataReductionProxyTamperDetect(const net::HttpResponseHeaders*, const bool,

[bengr, 2014/07/07 17:01:33]

provide variable names.

[xingx, 2014/07/08 00:22:26]

Done.

+                                 const unsigned, std::vector<std::string>*);

[bengr, 2014/07/07 17:01:34]

Don't make the bool and the unsigned const. I don't see how that helps.

[xingx, 2014/07/08 00:22:26]

Done.

+  virtual ~DataReductionProxyTamperDetect();
+
+  // Returns true if Chrome-Proxy has been tampered.

[bengr, 2014/07/07 17:01:33]

what is the parameter?

[bengr, 2014/07/07 17:01:34]

if Chrome-Proxy --> if the Chrome-Proxy header

[xingx, 2014/07/08 00:22:25]

Done.

[xingx, 2014/07/08 00:22:25]

Done.

[xingx, 2014/07/08 00:22:26]

Done.

[xingx, 2014/07/08 00:22:26]

Done.

+  bool CheckHeaderChromeProxy(const std::string&) const;

[bengr, 2014/07/07 17:01:35]

variable name

[bengr, 2014/07/07 17:01:35]

Rename as IsChromeProxyHeaderModified(const std::string& header_value) or
similar, e.g., IsChromeProxyHeaderTampered()

[xingx, 2014/07/08 00:22:26]

Done.

[xingx, 2014/07/08 00:22:27]

Done.

+
+  // Returns true if Via has been tampered.

[bengr, 2014/07/07 17:01:34]

if Via --> if the Via header

[xingx, 2014/07/08 00:22:26]

Done.

+  bool CheckHeaderVia(const std::string&) const;

[bengr, 2014/07/07 17:01:34]

rename as IsViaHeaderModified(const std::string& header_value)

[bengr, 2014/07/07 17:01:35]

variable name

[xingx, 2014/07/08 00:22:25]

Done.

[xingx, 2014/07/08 00:22:26]

Done.

[xingx, 2014/07/08 00:22:26]

Done.

+  // Report UMA for tampering of Via header.
+  void ReportHeaderVia() const;

[bengr, 2014/07/07 17:01:34]

suggest ReportViaHeaderTamperedUMA()

[xingx, 2014/07/08 00:22:26]

Done.

+
+  // Returns true if other headers (a list of headers) hav been tampered.
+  bool CheckHeaderOtherHeaders(const std::string&) const;

[bengr, 2014/07/07 17:01:34]

What is the parameter? The concatenation of all other headers? That's weird.

Also, suggest: AreOtherHeadersModified()

[xingx, 2014/07/08 00:22:26]

Done.

+  // Report UMA for tampering of values of the list of headers.
+  void ReportHeaderOtherHeaders() const;

[bengr, 2014/07/07 17:01:35]

ReportOtherHeadersTamperedUMA()

[xingx, 2014/07/08 00:22:26]

Done.

+
+  // Returns true if Content-Length has been tampered.
+  bool CheckHeaderContentLength(const std::string&) const;
+  // Report UMA for tampering of Content-Length.
+  void ReportHeaderContentLength() const;

[bengr, 2014/07/07 17:01:34]

ReportContentLengthHeaderTamperedUMA()

[xingx, 2014/07/08 00:22:25]

Done.

+
+  // Return string of sorted values of |values|.
+  static std::string ValuesToSortedString(std::vector<std::string> &values);

[bengr, 2014/07/07 17:01:33]

Why does this (and many of these functions) need to be public?

[xingx, 2014/07/08 00:22:25]

Done.

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

[bengr, 2014/07/07 17:01:34]

Return --> Returns

Add to the comment why you can't just use the MD5 hash method directly.

[xingx, 2014/07/08 00:22:26]

Done.

+  static std::string GetMD5(const std::string& input);
+
+  // 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);
+
+  // Return fingerprint code (enum) for the given fingerprint tag.
+  FingerprintCode GetFingerprintCode(const std::string&);

[bengr, 2014/07/07 17:01:33]

provide a variable name here and everywhere.

[xingx, 2014/07/08 00:22:26]

Done.

+
+ private:
+  // Response header.
+  const net::HttpResponseHeaders* response_headers;
+
+  // HTTPS or HTTP.
+  const bool is_secure_scheme;
+
+  // Carrier ID.
+  const unsigned mcc_mnc;
+
+  // Values for Chrome-Proxy header, with |kTamperDetectFingerprintChromeProxy|
+  // removed. Save it as temporary result so we don't need to parse
+  // Chrome-Proxy header 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_