Opened 7 years ago

Closed 7 years ago

#7325 closed defect (fixed)

Scrub IPs before displaying them in logs

Reported by: sysrqb Owned by: asn
Priority: Medium Milestone:
Component: pyobfsproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

pyobfsproxy needs a SafeLogging option, with True as its default value.

Note: reimplementation of #5079

Child Tickets

Attachments (5)

0001-Accept-no-safe-logging-and-add-scrubbing-func.patch (6.1 KB) - added by sysrqb 7 years ago.
0001-Improved-safe-logging-with-suggestions-from-gsathya.patch (2.4 KB) - added by sysrqb 7 years ago.
Delta to first patch. I can squash and repost if requested.
0001-Improved-based-on-asn-s-suggestions.patch (7.3 KB) - added by sysrqb 7 years ago.
0001-Improved-based-on-asn-s-suggestions.2.patch (12.0 KB) - added by sysrqb 7 years ago.
0001-Accept-no-safe-logging-and-add-scrubbing-func.2.patch (13.3 KB) - added by sysrqb 7 years ago.
Squashed

Download all attachments as: .zip

Change History (17)

comment:1 Changed 7 years ago by sysrqb

Status: newneeds_review

Please find my initial patch attached and in branch bug7325 on git://gitweb.evolvesoftware.cc/pyobfsproxy.git

It's a relatively simple patch, but adds a new class to the log module to encapsulate the value. I also added, in the module, a wrapper around the class's method to slightly simplify the calling. The rest of the patch is just a wrapper around an IP addr when it's an output value to a log entry.

Any comments and criticisms appreciated.

comment:2 Changed 7 years ago by gsathya

Status: needs_reviewneeds_revision

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.

2)

+    log.obfslogger = log.ObfsLogger(args)
+

Generally good practice to avoid instantiating objects in other modules.

+""" 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?

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.

3)

+      if self.we_should_scrub_address():
+        return '[scrubbed]'

why use an accessor function?

comment:3 in reply to:  2 ; Changed 7 years ago by sysrqb

Status: needs_revisionneeds_review

Replying to 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!

Changed 7 years ago by sysrqb

Delta to first patch. I can squash and repost if requested.

comment:4 in reply to:  3 Changed 7 years ago by gsathya

Replying to sysrqb:

Great! Thanks for making the changes :)

Replying to gsathya:

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.

I completely agree that in normal operation, it should never be the case that obfslogger was not set. But I'm just wondering if it'd be better to make it modular if other modules wanted to access it in the future.

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?

Yep. From the PEP8 style guide - http://www.python.org/dev/peps/pep-0008/#constants

The rest look good, thanks! I'm leaving the status as 'needs_review' for asn's comments.

comment:5 Changed 7 years ago by asn

Status: needs_reviewneeds_revision

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'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.

  • Is we_should_scrub_address() useless now? Maybe we should remove it?
  • 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.
  • I wonder if self.safe_logging = not no_safe_logging would look better than the current ObfsLogger:__init__().

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

comment:6 in reply to:  5 ; Changed 7 years ago by sysrqb

Thanks again for the feedback!

Replying to 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?

comment:7 Changed 7 years ago by sysrqb

I'm leaving in needs_revision until you have a chance to comment on my last question. The patch may or may not help you understand what I'm trying to say, if not then you can see the update in branch bug7325 on https://gitweb.evolvesoftware.cc/pyobfsproxy.git

Thanks!

comment:8 in reply to:  6 Changed 7 years ago by asn

Replying to sysrqb:

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?

FWIW, I'm also not fond of the globals in log.py. Still, I think that it's a better solution than requiring two calls for a single logging message.

The Python documentation in http://docs.python.org/2/howto/logging.html#advanced-logging-tutorial suggests to do logger = logging.getLogger(__name__) per-module, and then use logger through the module.

This means that it would change all the import obfsproxy.common.log as log to import logging; log = logging.getLogger('our_logger'). Do you like this better? I think I do. Also, the 'our_logger' string is a bit awkward, maybe we should change it to 'obfslogger' or something (since its our main logger anyway).

comment:9 Changed 7 years ago by sysrqb

I'm trying to find a more elegant solution. As you suggest, we can use

  logger = logging.getLogger(__name__)

per-module. However, this starts to separate the two logging classes (logging and ObfsLogger) which I was under the impression we wanted to combine. If we do initialize the logger in each module then we're still adding a little extra code overhead, which isn't ideal. There should be a way for us to accomplish what we want in log.py and then only need to import it into the other modules and call log.debug() et al.

I'm hopeful, but the more I type the more I think we will need at least one or two global lines in each module to initialize it.

comment:10 Changed 7 years ago by sysrqb

Status: needs_revisionneeds_review

Not ideal, so feedback is appreciated. bug7325-2 on https://gitweb.evolvesoftware.cc/pyobfsproxy.git and patch below. Also, I think I fixed everything when I moved them into the class, but please check that I didn't miss anything.

comment:11 Changed 7 years ago by sysrqb

Not sure if this is anymore readable, but I squashed in branch bug7325-3. Patch to follow.

Changed 7 years ago by sysrqb

Squashed

comment:12 in reply to:  11 Changed 7 years ago by asn

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

Not sure if this is anymore readable, but I squashed in branch bug7325-3. Patch to follow.

Merged. Thanks!

Note: See TracTickets for help on using tickets.