Skip to content

Drop pcapy-like integrations in favor of winpcapy.py (no acitivity) #2057

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

Merged
merged 2 commits into from
Jun 6, 2019

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Jun 2, 2019

Edit: if you have more questions, have a look at #2057 (comment)

With a few little tricks, it's actually very possible to re-use the Windows integration that uses winpcapy.py to call Npcap, to call libpcap on non-Windows platforms. Indeed, the original winpcapy.py also had plain libpcap support

This PR:

  • removes pcapy, pypcap, pylibpcap integrations, and use winpcapy.py instead
  • updates the docs

Why

Tested on:

  • Windows 10
  • FreeBSD 12
  • Ubuntu 18.04

Notes:

  • even though winpcapy is used on non-winpcap systems, it's still the original name. I'd rather keep it like so for an historical purpose

@gpotter2 gpotter2 added the cleanup Performs some code clean-up label Jun 2, 2019
@codecov
Copy link

codecov bot commented Jun 2, 2019

Codecov Report

Merging #2057 into master will increase coverage by 0.18%.
The diff coverage is 71.92%.

@@            Coverage Diff             @@
##           master    #2057      +/-   ##
==========================================
+ Coverage   86.32%   86.51%   +0.18%     
==========================================
  Files         197      197              
  Lines       44478    44357     -121     
==========================================
- Hits        38396    38374      -22     
+ Misses       6082     5983      -99
Impacted Files Coverage Δ
scapy/arch/windows/native.py 78.84% <ø> (ø) ⬆️
scapy/config.py 84.53% <100%> (-0.04%) ⬇️
scapy/arch/windows/__init__.py 71.96% <64.7%> (-0.05%) ⬇️
scapy/arch/pcapdnet.py 70.64% <69.69%> (+13.6%) ⬆️
scapy/automaton.py 81.81% <0%> (-0.56%) ⬇️
scapy/sendrecv.py 85.37% <0%> (+0.16%) ⬆️
scapy/layers/inet6.py 88.28% <0%> (+0.17%) ⬆️
scapy/layers/tls/basefields.py 80.53% <0%> (+0.67%) ⬆️
scapy/layers/tls/handshake_sslv2.py 92.07% <0%> (+0.75%) ⬆️
... and 1 more

@gpotter2 gpotter2 requested a review from guedou June 2, 2019 15:59
@gpotter2
Copy link
Member Author

gpotter2 commented Jun 2, 2019

@guedou This is ready for reviewing!

As you'll notice, I've removed the pylibpcap receipe from .travis/, hope that's fine for you. Also, If I recall correctly, we now do support MacOS natively right ? It was stated otherwise in the doc.

I'm pretty happy about how this PR turned out, I didn't expect it to work immediately 😄

@gpotter2 gpotter2 mentioned this pull request Jun 3, 2019
27 tasks
Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

Really cool PR! Small comment: did you notice performance issue before and after the PR?

@gpotter2
Copy link
Member Author

gpotter2 commented Jun 6, 2019

I did some tests comparing this against pcapy. Didn't see any difference :/ (though it's hard to make reliable comparison when receiving)

@p-l- p-l- merged commit 5d3c7ef into secdev:master Jun 6, 2019
@carlos-jenkins
Copy link

@gpotter2 you referenced helpsystems/pcapy#60 from this PR, and since it is now merged I would like to ask you:

Now that support for pcapy has been dropped, I cannot find in winpcapy reference to the functionality we implemented on in that very same PR, that is the setdirection() call from libpcap. Am I missing something?

My point is, all libpcap integrations have always been crappy in a way or another, and we are replacing all of them for another crappy one or this is really the best and best of the best?

We use Scapy for packet building and we use open_live from pcapy for packet capture in our Software, together. We want to keep the dependencies right and avoid problems loading different libpcap libraries, so we need that whatever Scapy is using to connect to libpcap can also work for our purposes.

Concretely we use open_live, and reader's setnonblock, setdirection and dispatch, and for what I could see winpcapy doesn't offer those interfaces. Which may not be pretty or fun, but are exactly a one on one match to libpcap.

Futhermore the interfaces by provided by winpcapy aren't enough nor complete to get the same functionality. Simply put, winpcapy doesn't replace pcapy.

Could we discuss this more, it looks like the drop of pcapy was rushed on, in my opinion.

Right now, we have pined Scapy to 2.4.2 to avoid any problem with the drop of this important functionality.

@gpotter2
Copy link
Member Author

gpotter2 commented Jul 2, 2019

You are right about setdirection(). 😄

However I am not sure if this has any effect on users: this change was supposed to only be internal, as only Scapy was supposed to use Winpcapy.py, or it's previous libpcap-providers. If you want to use pcapy on the side, it's still possible. I don't think the bindings can conflict in one way or another

I guess that some users could have tried to use sock.ins.*, but that's scary-unreliable, as the ins provider used to depend from the installed dependencies.

I understand you concerns. It might look crappy, but it brings two things:

  • we stop depending on very low activity libraries.
  • we re-use code that was required to support Npcap (there are no other providers that have added Npcap support for Windows yet :/). We had massive duplicates

I have myself contributed to pcapy (helpsystems/pcapy#47), but I fear the activity is going to drop (even more) after the structural change they encountered.

My point is, all libpcap integrations have always been crappy in a way or another, and we are replacing all of them for another crappy one or this is really the best and best of the best?

It probably isn't. But it wastes less of our time than patching pcapy and waiting for a release.

Futhermore the interfaces by provided by winpcapy aren't enough nor complete to get the same functionality.

We have a very low usage of libpcap calls when it comes to interfaces, because we need to get the routes by another method. It ain't perfectly clean everywhere but it works quite good. That shouldn't affect the end user, since we still provide the same data through Scapy.
What do you mean by "interfaces" ? The Python bindings to libpcap ? It doesn't aim at supporting all libpcap features, but providing what Scapy relies on. I'm not sure I understand this comment :/

We are most likely not going to reintroduce pcapy support (?), whereas it will be easier for us to update winpcapy when needed.
If you have a feature request (setdirection()), we can totally add that to winpcapy and add support for it in the SuperSocket.

Right now, we have pined Scapy to 2.4.2 to avoid any problem with the drop of this important functionality.

Feel free to, but note we really do not recommend the 2.4.1/2.4.2 releases. They were rushed due to security concerns, and have some stuff unfinished :/

We're on verge of releasing 2.4.3, we could add setdirection() (Others?) if I'm quick enough. how about that ?

@gpotter2 gpotter2 changed the title Drop Python-libpcap integrations in favor of winpcapy.py Drop pcaps* integrations in favor of winpcapy.py (no acitivity) Jul 11, 2019
@gpotter2 gpotter2 changed the title Drop pcaps* integrations in favor of winpcapy.py (no acitivity) Drop pcapy-like integrations in favor of winpcapy.py (no acitivity) Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Performs some code clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants