[tor-bugs] #7325 [pyobfsproxy]: Scrub IPs before displaying them in logs

[Tor Bug Tracker & Wiki]

#7325: Scrub IPs before displaying them in logs
-------------------------+--------------------------------------------------
 Reporter:  sysrqb       |          Owner:  asn           
     Type:  defect       |         Status:  needs_revision
 Priority:  normal       |      Milestone:                
Component:  pyobfsproxy  |        Version:                
 Keywords:               |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------

Comment(by sysrqb):

 Thanks again for the feedback!

 Replying to [comment:5 asn]:
 > Thanks for the code and the review!
 >
 > Some more comments/nitpicking:
 >
 > * What is the relationship between `ObfsLogger` and `our_logger`? Both
 names sound the same. Could they be the same thing? Maybe we should
 incorporate `our_logger` into `ObfsLogger` and have `ObfsLogger` be
 responsible for the logging subsystem, as its name implies?

 I actually wasn't exactly sure how much of the original logging code you
 wanted moved into the new class, so I kept them separate. They can
 absolutely be the same...and ideally should, I think. Please review
 modified impl.

 >
 > * I'm not really a fan of the `log.obfslogger =
 log.get_obfslogger(args.no_safe_logging)` in `obfsproxy.py`. I don't like
 code of `obfsproxy.py` patching stuff of the logging subsystem.
 >
 > Maybe `ObfsLogger` could be instantiated in start-time (like
 `our_logger` does atm), and have a `set_safe_logging()` method that can be
 called from `obfsproxy.py` when args are parsed? This is related to the
 previous comment.

 Easy fix.

 >
 >
 > * Is `we_should_scrub_address()` useless now? Maybe we should remove it?

 Yes, no reason to keep around dead code, I guess.

 >
 > * You seem to be coding in 2-spaces-per-indentation-level, but the rest
 of pyobfsproxy is in 4-spaces-per-indentation-level. I don't have anything
 against 2-spaces, but let's keep the codebase uniform.

 Fixing that was on my TODO list before posting the original patch. Fixed
 now.

 >
 > * I wonder if `self.safe_logging = not no_safe_logging` would look
 better than the current `ObfsLogger:__init__()`.

 It's definitely cleaner/more concise. Rereading the current code, it does
 currently look a little funny. I think this may actually increase the
 readability of the code, so yes. But, this is actually being removed due
 to the above method.

 >
 > ----
 >
 > If you don't have time to think of these comments, just tell me and I
 will look into it.

 Thanks, but I want to help you - not give you more work.

 Q: In log.py, I left the log-level functions as globals but I'm
 defining/instantiating them in ObfsLogger:___init___(). I think it looks a
 little messier to retrieve the current instance of ObfsLogger, let's call
 it obfslog, and then call obfslog.debug() compared to simply log.debug().
 However, by making these globals into instance variables it leaves
 OBFSLOGGER as the only global, which I also think is advantageous.
 Furthermore, this may actually be a good idea because it is only one
 additional line to retrieve the instance but because it will affect all
 logging code I don't want to make such a change without your input.
 Thoughts?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7325#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online