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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

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

  * status:  needs_revision => needs_review


Comment:

 Replying to [comment:2 gsathya]:

 Thanks for the quick feedback! I integrated your suggestions in the patch
 below and in the same branch as above.

 > I was just reading your patch and I had some thoughts. Feel free to
 disagree with anything/everything I've said.
 > 1)
 > {{{
 >
 > +    log.obfslogger = log.ObfsLogger(args)
 >
 > }}}
 >
 > You could just pass args.no_safe_logging instead of passing the entire
 list of args.

 Done.

 >
 > 2)
 > {{{
 > +    log.obfslogger = log.ObfsLogger(args)
 > +
 > }}}
 > Generally good practice to avoid instantiating objects in other modules.

 Makes sense, but see next.

 > {{{
 > +""" Global variable that will track our Obfslogger instance """
 > +obfslogger = None
 > +
 > }}}
 > {{{
 > +def get_obfslogger():
 > +    """ Return our instance of Obfslogger class """
 > +
 > +    assert(obfslogger)
 > +    return obfslogger
 > }}}
 >
 > Wouldn't be better to check if there is a obfslogger, and if not create
 a new instance instead of exiting out?

 Yes, but I was making the assumption that the logger was being
 instantiated in obfsproxy.py so, assuming normal operation, it should
 never be the case that obfslogger was not set.

 >
 > These can be changed to -
 > In bfsproxy.py -
 > {{{
 > obfslogger = log.get_logger(args.no_safe_logging)
 > }}}
 > In obfsproxy/common/log.py -
 > {{{
 > """ Global variable that will track our Obfslogger instance """
 > OBFSLOGGER = None
 > }}}
 > {{{
 > def get_obfslogger(no_safe_logging):
 > """ Return Obfslogger class """
 >   global OBFSLOGGER:
 >   if not OBFSLOGGER: OBFSLOGGER = ObfsLogger(no_safe_logging)
 >   return OBFSLOGGER
 > }}}
 > or even just a create_obfslogger(no_safe_logger) which doesn't return
 anything.
 >

 I agree that this makes much more sense than what I had. I'm curious about
 why the obfslogger variable is all caps, is it so it's assumed to be a
 constant?

 > 3)
 > {{{
 > +      if self.we_should_scrub_address():
 > +        return '[scrubbed]'
 > }}}
 > why use an accessor function?

 Yeah...fixed that.

 Thanks again!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7325#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs