Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Pre-empt accidentally leaking PII in logs #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tlater-famedly
Copy link
Contributor

@tlater-famedly tlater-famedly commented Oct 16, 2024

This allows using the debug impls of our User structs without worrying too much about accidentally exposing PII, trying to start adhering to our logging policy.

Note that this means external_user_id should never contain PII, and as such we'll have to change the CSV source. An issue about this will be opened separately after we merge this - my suggestion would be to add a new column to the CSV sources so that we can simply give users non-sensitive IDs, but we will need to discuss the exact use cases in which we expect to be able to use the CSV source (can't mix it with non-CSV use cases if the external IDs don't match, but more on that on a future ticket).

@tlater-famedly tlater-famedly requested a review from a team as a code owner October 16, 2024 16:40
@tlater-famedly tlater-famedly force-pushed the tlater/dont-log-pii branch 3 times, most recently from 6328593 to 6e54da6 Compare October 16, 2024 16:52
This allows using the debug impls of our User structs without worrying
too much about accidentally exposing PII.

Note that this means `external_user_id` should *never* contain PII,
and as such we'll have to change the CSV source. An issue about this
will be opened separately.
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.05%. Comparing base (3d725da) to head (c4ce6ae).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/user.rs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   86.94%   87.05%   +0.10%     
==========================================
  Files           7        7              
  Lines        1310     1321      +11     
==========================================
+ Hits         1139     1150      +11     
  Misses        171      171              
Files with missing lines Coverage Δ
src/user.rs 91.13% <92.30%> (+1.43%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d725da...c4ce6ae. Read the comment docs.

@emgrav
Copy link
Member

emgrav commented Oct 17, 2024

Note that the logging policy CR is not yet accepted, there is still opportunity for changes. The reason we want to avoid logging PII is to ensure it is kept only where we know it is, so that it can easily be removed to comply with f.x. GDPR rights. As such, I would say that avoiding logging PII is mainly important for service logs which are ingested into loki. CSV imports (at least in how we are currently imagining them) will generally be carried out interactively, where logging PII is less of a concern.

One option could be to log f.x. email addresses when running interactively, and add a --non-interactive flag which enables complete logging policy compliance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants