Add anonymizer tool

Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
Committed: https://crrev.com/4cdaa7cf7778a0fc5e7987942a04a42354e95e40
Cr-Commit-Position: refs/heads/master@{#368052}

[battre, 2015-12-17 09:56:56]

Description was changed from

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
==========

to

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
==========

[battre, 2015-12-17 09:56:56]

battre@chromium.org changed reviewers:
+ vasilii@chromium.org

[battre, 2015-12-17 09:57:29]

Hi Vasilii,

could you please review this? The AnonymizerTool has been taken from go/ozanr
and converted to use RE2.

Thanks,
Dominic

[vasilii, 2015-12-17 12:40:37]

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:55: anonymized =
AnonymizeCustomPatterns(anonymized);
Looking at the implementation you should take the string by value and move it
when calling AnonymizeCustomPatterns.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:89: std::string replacement_mac =
mac_addresses_[mac];
I'd use a reference here.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:104: return result + text.as_string();
You should make it more effective by using operator+=. Otherwise, you create a
copy of result which can be long.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:136: std::string replacement_id =
(*identifier_space)[matched_id];
I'd use reference here too.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:147: result += text.as_string();
Use StringPiece::AppendToString.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.h:35: std::map<std::string, std::string>
mac_addresses_;
Comments needed here.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:10: using std::string;
These don't look beneficial to me.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:91: TEST_F(AnonymizerToolTest,
AnonymizeCustomPattern) {
The name is almost identical to the previous one.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:92: static const char kPattern[]
= "(\\b(?i)id:? ')(\\d+)(')";
Get rid of static.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common.h (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common.h:65: // File attachments in the feedback
data need to be properly anonymized before
The comment above says that the file attachments aren't anonymised.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:90:
feedback->PrepareReport(&report);
The class members aren't marked with '_'. It's very difficult to read the test.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:92: // Verify that uncompressed
data is anonymized.
But you compress them above.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:107: bool success =
base::WriteFile(zip_file, binary_data.data().data(),
Why do you test filesystem and zip here?

[battre, 2015-12-17 13:24:24]

Thanks for the review. PTAL.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:55: anonymized =
AnonymizeCustomPatterns(anonymized);
On 2015/12/17 12:40:36, vasilii wrote:
> Looking at the implementation you should take the string by value and move it
> when calling AnonymizeCustomPatterns.

I don't see that this would save much.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:89: std::string replacement_mac =
mac_addresses_[mac];
On 2015/12/17 12:40:37, vasilii wrote:
> I'd use a reference here.

Are you suggesting to the following?

std::string& replacement_mac = mac_addresses_[mac];
...
replacement_mac = base::StringPrintf(...);
// delete mac_addresses_[mac] = replacement_mac;
?

In this case, I would prefer to keep the current version for readability.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:104: return result + text.as_string();
On 2015/12/17 12:40:37, vasilii wrote:
> You should make it more effective by using operator+=. Otherwise, you create a
> copy of result which can be long.

I have followed the StringPiece::AppendToString idea you gave below.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:136: std::string replacement_id =
(*identifier_space)[matched_id];
On 2015/12/17 12:40:37, vasilii wrote:
> I'd use reference here too.

see above.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:147: result += text.as_string();
On 2015/12/17 12:40:37, vasilii wrote:
> Use StringPiece::AppendToString.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.h:35: std::map<std::string, std::string>
mac_addresses_;
On 2015/12/17 12:40:37, vasilii wrote:
> Comments needed here.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:10: using std::string;
On 2015/12/17 12:40:37, vasilii wrote:
> These don't look beneficial to me.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:91: TEST_F(AnonymizerToolTest,
AnonymizeCustomPattern) {
On 2015/12/17 12:40:37, vasilii wrote:
> The name is almost identical to the previous one.

I think the name reflects what is being tested.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool_unittest.cc:92: static const char kPattern[]
= "(\\b(?i)id:? ')(\\d+)(')";
On 2015/12/17 12:40:37, vasilii wrote:
> Get rid of static.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common.h (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common.h:65: // File attachments in the feedback
data need to be properly anonymized before
On 2015/12/17 12:40:37, vasilii wrote:
> The comment above says that the file attachments aren't anonymised.

I don't understand. Neither AddFile nor PrepareReport takes care of anonymizing
file attachments. This needs to be done be the caller.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:90:
feedback->PrepareReport(&report);
On 2015/12/17 12:40:37, vasilii wrote:
> The class members aren't marked with '_'. It's very difficult to read the
test.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:92: // Verify that uncompressed
data is anonymized.
On 2015/12/17 12:40:37, vasilii wrote:
> But you compress them above.

The feedback can contain two types of data at the same time. Uncompressed data
(report.web_data()) and compressed data
(report.product_sepecific_binary_data()). The size of the data determines
whether the data is compressed or not.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:107: bool success =
base::WriteFile(zip_file, binary_data.data().data(),
On 2015/12/17 12:40:37, vasilii wrote:
> Why do you test filesystem and zip here? 

Not sure I understand you. My goal is to test whether the content of the zip
file has been anonymized.

[vasilii, 2015-12-17 13:39:05]

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:55: anonymized =
AnonymizeCustomPatterns(anonymized);
On 2015/12/17 13:24:24, battre wrote:
> On 2015/12/17 12:40:36, vasilii wrote:
> > Looking at the implementation you should take the string by value and move
it
> > when calling AnonymizeCustomPatterns.
> 
> I don't see that this would save much.

You 100% save one copy of |anonymized|. It can be large.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:92: // Verify that uncompressed
data is anonymized.
On 2015/12/17 13:24:24, battre wrote:
> On 2015/12/17 12:40:37, vasilii wrote:
> > But you compress them above.
> 
> The feedback can contain two types of data at the same time. Uncompressed data
> (report.web_data()) and compressed data
> (report.product_sepecific_binary_data()). The size of the data determines
> whether the data is compressed or not.

Then it should go to a comment explaining why two logs end up in different
buckets.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:107: bool success =
base::WriteFile(zip_file, binary_data.data().data(),
On 2015/12/17 13:24:24, battre wrote:
> On 2015/12/17 12:40:37, vasilii wrote:
> > Why do you test filesystem and zip here? 
> 
> Not sure I understand you. My goal is to test whether the content of the zip
> file has been anonymized.

Isn't there a way to unzip |binary_data| without touching the filesystem?

https://codereview.chromium.org/1530403003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1530403003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.h:37: // where the frist thre bytes
represent the manufacturer. The last three bytes
spelling

[battre, 2015-12-17 14:34:56]

Thank you. I have addressed your comments.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/anonymi...
components/feedback/anonymizer_tool.cc:55: anonymized =
AnonymizeCustomPatterns(anonymized);
On 2015/12/17 13:39:05, vasilii wrote:
> On 2015/12/17 13:24:24, battre wrote:
> > On 2015/12/17 12:40:36, vasilii wrote:
> > > Looking at the implementation you should take the string by value and move
> it
> > > when calling AnonymizeCustomPatterns.
> > 
> > I don't see that this would save much.
> 
> You 100% save one copy of |anonymized|. It can be large.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
File components/feedback/feedback_common_unittest.cc (right):

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:92: // Verify that uncompressed
data is anonymized.
On 2015/12/17 13:39:05, vasilii wrote:
> On 2015/12/17 13:24:24, battre wrote:
> > On 2015/12/17 12:40:37, vasilii wrote:
> > > But you compress them above.
> > 
> > The feedback can contain two types of data at the same time. Uncompressed
data
> > (report.web_data()) and compressed data
> > (report.product_sepecific_binary_data()). The size of the data determines
> > whether the data is compressed or not.
> 
> Then it should go to a comment explaining why two logs end up in different
> buckets.

Done.

https://codereview.chromium.org/1530403003/diff/1/components/feedback/feedbac...
components/feedback/feedback_common_unittest.cc:107: bool success =
base::WriteFile(zip_file, binary_data.data().data(),
On 2015/12/17 13:39:05, vasilii wrote:
> On 2015/12/17 13:24:24, battre wrote:
> > On 2015/12/17 12:40:37, vasilii wrote:
> > > Why do you test filesystem and zip here? 
> > 
> > Not sure I understand you. My goal is to test whether the content of the zip
> > file has been anonymized.
> 
> Isn't there a way to unzip |binary_data| without touching the filesystem?

Ok, found one.

Done.

https://codereview.chromium.org/1530403003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1530403003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.h:37: // where the frist thre bytes
represent the manufacturer. The last three bytes
On 2015/12/17 13:39:05, vasilii wrote:
> spelling

Done.

[vasilii, 2015-12-17 14:38:23]

lgtm

https://codereview.chromium.org/1530403003/diff/80001/components/feedback/ano...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1530403003/diff/80001/components/feedback/ano...
components/feedback/anonymizer_tool.h:37: // where the frist three bytes
represent the manufacturer. The last three
first

[battre, 2015-12-17 14:43:09]

battre@chromium.org changed reviewers:
+ agl@chromium.org, rkc@chromium.org, thakis@chromium.org

[battre, 2015-12-17 14:43:12]

Hi.

could you please review this CL?

thakis for OWNERS approval of adding RE2 to DEPS.

agl for OWNERS approval for generalizing zlib dependency in DEPS

rkc for OWNERS approval and review of the main CL.

Thank you,
Dominic

[battre, 2015-12-17 14:45:45]

battre@chromium.org changed reviewers:
+ bsimonnet@chromium.org

[battre, 2015-12-17 14:45:46]

Just when you thought you had them all...

bsimonnet for owners approval to components/feedback.gypi changes. Feel free to
take over the main owners approval that I asked rkc to provide.

Thank you,
Dominic

[bsimonnet, 2015-12-17 16:37:52]

On 2015/12/17 14:45:46, battre wrote:
> Just when you thought you had them all...
> 
> bsimonnet for owners approval to components/feedback.gypi changes. Feel free
to
> take over the main owners approval that I asked rkc to provide.
> 
> Thank you,
> Dominic

lgtm for feedback.gypi .
You probably want a real owners approval, I think I'm in the list because of the
Chrome OS part of feedback and I don't know much about this :/

[agl, 2015-12-17 16:42:53]

LGTM for using zlib.

[battre, 2015-12-21 10:50:11]

friendly ping at rkc@

[Nico, 2015-12-21 19:13:29]

re2 dep lgtm

[rkc, 2015-12-21 19:36:36]

(+afakhry for FYI)

(sorry for the late review, I was out sick all of last week)

This will have the effect of scrubbing logs 'after' they have been shown to the
user, ending up with different logs being sent to the server than the user sees.
We're actively trying to move away from this behavior (see crbug.com/437762).

Instead, the anonymization should be done in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fee...
by overriding the AddResponse call. As each log data response comes in, we
should anonymize the the logs there.


Additionally, some first party apps (which go through their own privacy review)
add their own custom data to feedback reports, which may get corrupted by the
anonymization process (for example, they may have obtained user permission to
send the e-mail address and/or IP address, which would get scrubbed out by our
anonymizer).

[battre, 2015-12-22 10:29:31]

I have moved the scrubbing as you suggested. I have left the AnonymizerTool in
the components because I would like to discover some extra changes in the future
(making sure that the user sees the data as they get uploaded; changing the
extension API so that callers need to opt out of having their data scrubbed).

PTAL.

Thanks,
Dominic

[battre, 2016-01-04 13:18:12]

@rkc: Just a ping to bring this up in your inbox after the vacation.

Thanks for reviewing this.

Best regards,
Dominic

[rkc, 2016-01-06 18:43:28]

lgtm

https://codereview.chromium.org/1530403003/diff/160001/chrome/browser/feedbac...
File chrome/browser/feedback/system_logs/system_logs_fetcher_base.cc (right):

https://codereview.chromium.org/1530403003/diff/160001/chrome/browser/feedbac...
chrome/browser/feedback/system_logs/system_logs_fetcher_base.cc:53: void
SystemLogsFetcherBase::Rewrite(const std::string& source_name,
Nit: /* source_name */ and /* response */

[battre, 2016-01-07 09:54:02]

Thank you

https://codereview.chromium.org/1530403003/diff/160001/chrome/browser/feedbac...
File chrome/browser/feedback/system_logs/system_logs_fetcher_base.cc (right):

https://codereview.chromium.org/1530403003/diff/160001/chrome/browser/feedbac...
chrome/browser/feedback/system_logs/system_logs_fetcher_base.cc:53: void
SystemLogsFetcherBase::Rewrite(const std::string& source_name,
On 2016/01/06 18:43:28, Rahul Chaturvedi wrote:
> Nit: /* source_name */ and /* response */

Done.

[battre, 2016-01-07 09:57:53]

The CQ bit was checked by battre@chromium.org

[battre, 2016-01-07 09:57:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, agl@chromium.org, bsimonnet@chromium.org,
thakis@chromium.org, rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/1530403003/#ps180001
(title: "Nit")

[commit-bot: I haz the power, 2016-01-07 09:58:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1530403003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1530403003/180001

[commit-bot: I haz the power, 2016-01-07 09:59:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-07 09:59:51]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[battre, 2016-01-07 10:27:29]

The CQ bit was checked by battre@chromium.org

[battre, 2016-01-07 10:27:30]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
vasilii@chromium.org, thakis@chromium.org, agl@chromium.org,
bsimonnet@chromium.org
Link to the patchset: https://codereview.chromium.org/1530403003/#ps200001
(title: "Rebase to ToT")

[commit-bot: I haz the power, 2016-01-07 10:27:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1530403003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1530403003/200001

[commit-bot: I haz the power, 2016-01-07 11:30:37]

Description was changed from

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
==========

to

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
==========

[commit-bot: I haz the power, 2016-01-07 11:30:37]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-01-07 11:31:31]

Description was changed from

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870
==========

to

==========
Add anonymizer tool

This CL adds an anonymizer class that has been written for ChromeOS log files.
This will be extended in the future to remove more PII, in particular emails.

BUG=567870

Committed: https://crrev.com/4cdaa7cf7778a0fc5e7987942a04a42354e95e40
Cr-Commit-Position: refs/heads/master@{#368052}
==========

[commit-bot: I haz the power, 2016-01-07 11:31:31]

Patchset 11 (id:??) landed as
https://crrev.com/4cdaa7cf7778a0fc5e7987942a04a42354e95e40
Cr-Commit-Position: refs/heads/master@{#368052}