refactor: move scrubbing logger to logx #1319

bassosimone · 2023-09-27T16:28:01Z

This diff is yak shaving for a subsequent diff that attempts to mitigate potential IP addresses leaks caused by using happy eyeballs more aggressively in the codebase. The general idea is that we previously had a netxlite implementation that gave IPv4 preference over IPv6, meaning that we would end up using IPv4 in most cases and very rarely IPv6. As part of my work to make beacons possible, I have introduced happy eyeballs, which means we may sometimes use IPv4 instead of IPv6 if we adopt happy eyeballs more widely. This fact makes it more likely that we include the IPv6 address of a probe when we know its IPv4 address or the other way around into measurements. To mitigate this possible issue before it actually is possible (note that I have not yet changed how we discover the probe IP), I am going to proactively modify the serialization of HTTP bodies and headers to scrub IP endpoints unconditionally. However, to do this, I need to decouple the scrubber package from model such that internal/model/archival.go can use the scrubber package.

Part of ooni/probe#2531

This diff is yak shaving for a subsequent diff that attempts to mitigate potential IP addresses leaks caused by using happy eyeballs more aggressively in the codebase. The general idea is that we previously had a netxlite implementation that gave IPv4 preference over IPv6, meaning that we would end up using IPv4 in most cases and very rarely IPv6. As part of my work to make beacons possible, I have introduced happy eyeballs, which means we may sometimes use IPv4 instead of IPv6. This fact makes it more likely that we include the IPv6 address of a probe when we know its IPv4 address or the other way around into measurements. To mitigate this possible issue before it actually is possible (note that I have not yet changed how we discover the probe IP), I am going to proactively modify the serialization of HTTP bodies and headers to scrub. However, to do this, I need to decouple the scrubber package from model such that internal/model/archival.go can use the scrubber package. Part of ooni/probe#2531

There are cases where we know we have binary data in input and we want the output to be binary data using the dictionary encoding like `{"format":"base64","data":"..."}`. Such cases are, for example, DNS messages bodies. There is no need for us to pass through the MaybeArchivalBinaryData in such cases. Additionally, this makes MaybeArchivalBinaryData a bit more complex than it probably needs to be. What's more, ArchivalBinaryData is for when you do not require to scrub the data. I want to introduce new data types that automatically perform scrubbing when they're used. But this puts even more pressure on MaybeArchivalBinaryData, and hence this commit, to start differentiating between: 1. always binary data vs maybe binary data 2. no scrubbing requires vs scrubbing required The rationale for doing this set of changes has been explained in #1319. The reference issue is ooni/probe#2531. For now, this commit just adds the new type and tests for it without using the type, which we'll do later once we have added better marshal/unmarshal testing for the interested types.

There are cases where we know we have binary data in input and we want the output to be binary data using the dictionary encoding like `{"format":"base64","data":"..."}`. Such cases are, for example, DNS messages bodies. There is no need for us to pass through the MaybeArchivalBinaryData in such cases. Additionally, this makes MaybeArchivalBinaryData a bit more complex than it probably needs to be. What's more, ArchivalBinaryData is for when you do not require to scrub the data. I want to introduce new data types that automatically perform scrubbing when they're used. But this puts even more pressure on MaybeArchivalBinaryData, and hence this commit, to start differentiating between: 1. always binary data vs maybe binary data 2. no scrubbing required vs scrubbing required The rationale for doing this set of changes has been explained in #1319. The reference issue is ooni/probe#2531. For now, this commit just adds the new type and tests for it without using the type, which we'll do later once we have added better marshal/unmarshal testing for the interested types.

I am about to modify this structure to unconditionally use ArchivalBinaryData rather than ArchivalMaybeBinaryData, following the plan that I explained in #1319. Before doing that, I want MORE test coverage for ArchivalTLSOrQUICHandshakeResult. Part of ooni/probe#2531

This diff is yak shaving for a subsequent diff that attempts to mitigate potential IP addresses leaks caused by using happy eyeballs more aggressively in the codebase. The general idea is that we previously had a netxlite implementation that gave IPv4 preference over IPv6, meaning that we would end up using IPv4 in most cases and very rarely IPv6. As part of my work to make beacons possible, I have introduced happy eyeballs, which means we may sometimes use IPv4 instead of IPv6 if we adopt happy eyeballs more widely. This fact makes it more likely that we include the IPv6 address of a probe when we know its IPv4 address or the other way around into measurements. To mitigate this possible issue before it actually is possible (note that I have not yet changed how we discover the probe IP), I am going to proactively modify the serialization of HTTP bodies and headers to scrub IP endpoints unconditionally. However, to do this, I need to decouple the scrubber package from model such that internal/model/archival.go can use the scrubber package. Part of ooni/probe#2531

There are cases where we know we have binary data in input and we want the output to be binary data using the dictionary encoding like `{"format":"base64","data":"..."}`. Such cases are, for example, DNS messages bodies. There is no need for us to pass through the MaybeArchivalBinaryData in such cases. Additionally, this makes MaybeArchivalBinaryData a bit more complex than it probably needs to be. What's more, ArchivalBinaryData is for when you do not require to scrub the data. I want to introduce new data types that automatically perform scrubbing when they're used. But this puts even more pressure on MaybeArchivalBinaryData, and hence this commit, to start differentiating between: 1. always binary data vs maybe binary data 2. no scrubbing required vs scrubbing required The rationale for doing this set of changes has been explained in ooni#1319. The reference issue is ooni/probe#2531. For now, this commit just adds the new type and tests for it without using the type, which we'll do later once we have added better marshal/unmarshal testing for the interested types.

I am about to modify this structure to unconditionally use ArchivalBinaryData rather than ArchivalMaybeBinaryData, following the plan that I explained in ooni#1319. Before doing that, I want MORE test coverage for ArchivalTLSOrQUICHandshakeResult. Part of ooni/probe#2531

bassosimone requested a review from hellais as a code owner September 27, 2023 16:28

bassosimone merged commit 4a86c54 into master Sep 27, 2023
6 of 8 checks passed

bassosimone deleted the issue/2531 branch September 27, 2023 16:43

bassosimone mentioned this pull request Sep 28, 2023

feat(model): introduce ArchivalBinaryData #1321

Merged

bassosimone mentioned this pull request Sep 28, 2023

chore(model): add tests for ArchivalTLSOrQUICHandshakeResult #1322

Merged

bassosimone mentioned this pull request Oct 2, 2023

engine: adapt to use bridges ooni/probe#2531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move scrubbing logger to logx #1319

refactor: move scrubbing logger to logx #1319

bassosimone commented Sep 27, 2023 •

edited

Loading

refactor: move scrubbing logger to logx #1319

refactor: move scrubbing logger to logx #1319

Conversation

bassosimone commented Sep 27, 2023 • edited Loading

bassosimone commented Sep 27, 2023 •

edited

Loading