Skip to content

Conversation

@but-i-am-dominator
Copy link

Smudge V1.0

Copy link

@Zalgo2462 Zalgo2462 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I saw this PR drop, I just had to take a look. I'm so excited to see this project developing!

I haven't historically been too involved with this project, so take everything I've said here with a heaping pinch of of salt. I more or less used this PR as an opportunity to learn about the codebase. Nonetheless, I think I've found a few comments/ names that ought to be updated.

PS: I really enjoyed reading the Smudge code. What a nice, clean approach!

Cheers,
LL

print(dframe)


def wraper_function(dframe, options):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in wraper

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider filter_by_options

@@ -0,0 +1,133 @@
import pandas as pd
from numpy import sum

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import appears to be unused. Pandas .sum should call out to numpy internally.

warnings = (df['description'].str.startswith('Warning')).sum()
suspicious = (df.State.values == 'suspicious').sum()
n = len(pd.unique(df['IPAddress']))
print(len(df), "records,", n, "distinct addresses,", op, "open ports", suspicious, "suspicious entries,", warnings,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider either returning these stats along with the dataframe or moving this analysis out to a separate function.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this logic is repeated in show()

reader = csv.reader(aih, delimiter='\t')
#Format: range_start range_end AS_number country_code AS_description
for first_ip, last_ip, as_num, country, as_description in reader:
if sys.version_info < (3, 0):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using six to implement python version compatibility.
https://stackoverflow.com/a/29213992

pass
#elif as_num == '0' and as_description != 'Not routed':
# sys.stderr.write('as == 0, desc != not routed\n')
#elif as_num != '0' and as_description == 'Not routed':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either delete these comments or add some text comments explaining their usefulness.

except UnicodeDecodeError:
raw_addr_string = ''

#if Devel:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug comments.


sys.path.insert(0, '.') #Allows us to load from the current directory (There was one claim that we need to create an empty file __init__.py , but this does not appear to be required.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider bundling ip2asn.py as a module as you have done with smudge. This would allow you to import the file without this work around. Modules can be run with python -m ./path/to/module. Alternatively, move ip2asn.py into a folder ip2asn, and make an empty __init__.py file in the folder. Then, you can import the file as ip2asn.ip2asn and still run the file from the cli.

maxminddb-geolite2
pytz
scapy>=2.4.0
scapy>=2.4.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might consider adding pandas here or building out a separate requirements file for the analysis script.

parser.add_argument('-p', '--per_packet_timeout', help='Time to wait for a reply for a single packet, can be fractional (default: ' + str(per_packet_timeout_default) + ' ).', required=False, default=per_packet_timeout_default)
parser.add_argument('-f', '--forced_interface', help='Force packets through this interface (needed on macos, default: ' + str(forced_interface_default) + ' ).', required=False, default=forced_interface_default)
parser.add_argument('-t', '--ttl', help='Maximum number of hops to try (default: ' + str(ttl_default) + ')', required=False, default=ttl_default)
parser.add_argument('-c', '--cache_dir', help='Directory tree to hold cached traceroutes (default: ' + str(traceroute_cache_dir_default) + ' ). Use None to not cache results.', required=False, default=traceroute_cache_dir_default)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that the string None passed on the cli will be properly cast to None in python. Instead, I believe it will come in as the string "None".

I haven't tested this, so please dismiss this if this is not an issue.

"""

import json
import urllib.request

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving these imports to the top of the file and accessing the libraries as normal rather than through the class.

@william-stearns
Copy link
Contributor

This code has been merged into the passer-ng branch. Once you're set with Zalgo's comments, dominator, please consider retracting this PR.

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.

4 participants