Skip to content
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

p0f rewritten #1923

Closed
wants to merge 9 commits into from
Closed

p0f rewritten #1923

wants to merge 9 commits into from

Conversation

hissssst
Copy link

@hissssst hissssst commented Mar 21, 2019

  • It is currently written in python3, but I will definetely rewrite it in six
  • It's not much, because I don't want to find out that something went wrong in the beginning after writing many lines of code
  • There are some comments for the reviewer. They will be definetely rewritten in the later versions of this file

Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution !

However, there isn’t much left :/ all the actually useful functions have been removed :/ do you think you could make them work ?

The new parsing is however great !

@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #1923 into master will decrease coverage by 22.64%.
The diff coverage is 10%.

@@             Coverage Diff             @@
##           master    #1923       +/-   ##
===========================================
- Coverage   85.89%   63.24%   -22.65%     
===========================================
  Files         187      126       -61     
  Lines       42746    30506    -12240     
===========================================
- Hits        36716    19295    -17421     
- Misses       6030    11211     +5181
Impacted Files Coverage Δ
scapy/modules/p0f.py 13.54% <10%> (-51%) ⬇️
scapy/sessions.py 26.08% <0%> (-73.92%) ⬇️
scapy/plist.py 18.57% <0%> (-67.49%) ⬇️
scapy/pton_ntop.py 30.13% <0%> (-67.13%) ⬇️
scapy/layers/tls/crypto/hkdf.py 35.55% <0%> (-64.45%) ⬇️
scapy/pipetool.py 26.26% <0%> (-63.26%) ⬇️
scapy/main.py 18.87% <0%> (-56.62%) ⬇️
scapy/utils6.py 33.78% <0%> (-53.43%) ⬇️
scapy/sendrecv.py 32.14% <0%> (-52.81%) ⬇️
scapy/scapypipes.py 33.2% <0%> (-52.15%) ⬇️
... and 132 more

Copy link
Member

@p-l- p-l- left a comment

Choose a reason for hiding this comment

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

Nice work!
I have a couple of suggestions.
BTW, since p0f v2 and v3 differ a lot, what would you think of renaming the existing module (maybe to something like p0fv2) so that we can have both?

Only Python 3 compatible
"""
a = line.split(splitchar)[:n]
yield from a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield from a
for elt in a:
yield elt

"""
a = line.split(splitchar)[:n]
yield from a
yield from [default] * (n - len(a))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield from [default] * (n - len(a))
for _ in range(n - len(a)):
yield default

def lparse(line, n, default='', splitchar=':'):
"""
Function for nice parcing of 'a:b:c:d:e' lines
Only Python 3 compatible
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Only Python 3 compatible

gen = p0fdb.tcp_correl(direction, p0f_out, olayout, quirks)
return max(list(gen), key=lambda x: sum(x[0]))[1]

if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in a separate .uts file rather than here.

@guedou
Copy link
Member

guedou commented Mar 10, 2020

I added a reference to this PR in #399 and I am closing it due to no activity.

@guedou guedou closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants