Use existing TCP opts in p0f_impersonate #1023
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1023 +/- ##
=========================================
+ Coverage 82.67% 83.9% +1.22%
=========================================
Files 148 144 -4
Lines 37768 36805 -963
=========================================
- Hits 31226 30882 -344
+ Misses 6542 5923 -619
|
There was a problem hiding this comment.
Hi,
Thanks for this PR! Could you have a look at my remarks?
It would be great to add some unit tests for your changes. If you feel like it, you can create a tiny p0f.uts in test, based on other files you'll find in that repository.
Also, could you rebase your work against current master so that your changes get tested under Python 3?
scapy/modules/p0f.py
Outdated
| @@ -14,6 +14,7 @@ | |||
| import os | |||
| import socket | |||
| import random | |||
| import scapy.modules.six as six | |||
There was a problem hiding this comment.
You can write from scapy.modules import six. Also, since that's a Scapy import, please move it with the other Scapy imports.
| else: | ||
| options.append(('WScale', int(opt[1:]))) | ||
| elif opt == 'T0': | ||
| options.append(('Timestamp', (0, 0))) | ||
| elif opt == 'T': | ||
| if 'T' in pers[5]: | ||
| # Determine first timestamp. | ||
| if uptime is not None: |
There was a problem hiding this comment.
I'm not sure, but shouldn't we always set ts_a = ts_hint[0] when ts_hint[0] and 0 < ts_hint[0] < 2**32?
There was a problem hiding this comment.
Do you mean instead of setting ts_a to uptime when uptime is set?
To me it seems more intuitive to use the value explicitly passed as an argument (uptime), rather than overriding it with the original packet's timestamp. If the caller wanted to use the value in the original packet, I wouldn't expect them to call the function with uptime=x.
Also this way the behavior is more consistent with the way it was before these changes. The first timestamp will be equal to uptime if uptime was passed, as before. An alternative if we aren't worried about consistency might be remove the uptime argument, since people can just set the timestamp in the original packet.
But for now I've kept it the way I had it. I could definitely be wrong, though, so please let me know if you do prefer it the other way.
saturn597
force-pushed
the
p0f_impersonate
branch
from
c9eb474
to
04bd9af
Compare
|
Happy to help! Thank you for your review. I've updated based on your comments, added some tests, and rebased against current master. I also replied back to your comment about setting timestamps. A few things weren't initially working in Python 3. So I fixed the issues I found and added a couple more tests in a second commit (can squash if preferred). |
|
Thanks! |
Trying to address "Check options in p0f.py" from #399 (the contributions roadmap). Making it so p0f_impersonate uses TCP options from the original packet where possible.
If this commit is on the right track, I can make changes/add tests as needed. Thanks!