-
Notifications
You must be signed in to change notification settings - Fork 115
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(log): Stop logging peer IP addresses, to protect user privacy #6662
Conversation
```sh fastmod SocketAddr PeerSocketAddr zebra-network ```
I manually ran
So this change seems to be working as expected for initial peers. I haven't seen any ongoing peer logs yet. Configured seed peer DNS and IP addresses are ok to log, because they are stored on the disk already in |
I also checked that ongoing peer logs don't show IP addresses any more, here are some debug logs:
|
The address redactions also work on peer errors and block gossips:
|
@oxarbitrage I'm happy to do a video review on this PR if that would help. But it's a lower priority than the release and security fixes. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6662 +/- ##
==========================================
+ Coverage 77.87% 78.02% +0.14%
==========================================
Files 309 310 +1
Lines 40665 40687 +22
==========================================
+ Hits 31669 31745 +76
+ Misses 8996 8942 -54 |
@Mergifyio update |
✅ Branch has been successfully updated |
Failed due to #6667 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went through this PR on a video call. The number of changes are huge but tedious but we have good test coverage for this. The number of file changed is big too so i think we need to merge fast before starting to conflict.
Motivation
We don't want to log peer IP addresses, because lists of peers are privacy and security sensitive.
Close #3320.
Complex Code or Requirements
This PR modifies some canonical IP address code, but these changes shouldn't impact network compatibility. (If they do, we'll see it in the integration tests.)
Solution
New types and functions:
Privacy fixes:
Related fixes:
Testing
See my manual log check below in the comments.
Review
This is a large change, so we might not want to schedule any other
zebra-network
work until it's merged. (@mpguerra I've checked the existingzebra-network
tickets in this sprint and they seem fine.)We might also want to make it a high priority for review, I'll leave that up to Pili.
Reviewer Checklist
Follow Up Work
Check for IP addresses of peers that are logged in other ways: going forward this should be considered a bug.