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

Reduce raw() usage/ compatibility calls + fix cryptography #1735

Merged
merged 7 commits into from
Feb 4, 2019

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Dec 8, 2018

"There is no small job"

This PR:

  • separates raw() calls, that encode packets, from bytes_encode() calls, that encode text. It's clearer and smally reduces the time it takes to call a raw(). Now we have raw() to build packets, and bytes_encode to get a string to bytes. It’s much cleaner (and faster) and forces us to know exactly what to call where
  • speeds up a few of the compat.py functions (by not much each, but they are used a lot)
  • removes oldish cmp calls

This PR also contains a cryptography update

It was found during the implementation

  • update cryptography decrypt() calls to work with latest version of cryptography
  • some of the in-use ciphers for TLS 1.0 and 1.1 have been removed from OpenSSL newer versions, so use other still available ciphers for those 2 tests
  • increase verbosity of the TLS tests + stabilize their failure. Will be easier to debug in the future
  • added: fixes Health checks (Deprecation/Garbage) failing #1814

Tests

Using the version of the benchmarker from #642, with an extra

if WINDOWS:
    route_add_loopback()

Results

  • WINDOWS 10:
Type  Before - 2.7   Before - 3.6 After - 2.7 After - 3.6
Build 13.68s 10.20s 13.56s (+1%) 9.74s (+4%)
Dissect  5.27s 3.85s 4.96s (+5%) 3.59s (+7%)
Build & dissect 19.31s  14.17s 18.90s (+2%) 13.07s (+7%)
  • UBUNTU (low RAM/CPU)
Type  Before - 2.7   Before - 3.6 After - 2.7 After - 3.6
Build 19.65s 15.11s 19.63s (~) 14.79s (+2%)
Dissect  8.27s 6.24s 7.82s (+5%) 6.02s (+3%)
Build & dissect 28.40s  21.89s 27.89s (+2%) 21.52s (+1%)

PS: this will probably fail multiple times on travis

@p-l-
Copy link
Member

p-l- commented Dec 8, 2018

Great work @gpotter2! Let us know if you need help.

@codecov-io
Copy link

codecov-io commented Jan 4, 2019

Codecov Report

Merging #1735 into master will decrease coverage by 0.7%.
The diff coverage is 81.35%.

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
- Coverage   85.46%   84.76%   -0.71%     
==========================================
  Files         183      174       -9     
  Lines       42412    40829    -1583     
==========================================
- Hits        36248    34609    -1639     
- Misses       6164     6220      +56
Impacted Files Coverage Δ
scapy/contrib/lacp.py 100% <ø> (ø) ⬆️
scapy/layers/tls/record_sslv2.py 87.7% <0%> (+0.7%) ⬆️
scapy/layers/tls/handshake.py 80.75% <0%> (-0.27%) ⬇️
scapy/layers/tls/automaton_srv.py 71.81% <0%> (+0.39%) ⬆️
scapy/utils6.py 86.98% <100%> (-0.23%) ⬇️
scapy/layers/tls/crypto/h_mac.py 92.42% <100%> (+0.36%) ⬆️
scapy/compat.py 100% <100%> (+9.75%) ⬆️
scapy/layers/tls/cert.py 83.94% <100%> (ø) ⬆️
scapy/automaton.py 75.75% <100%> (-6.64%) ⬇️
scapy/layers/dns.py 91.52% <100%> (+0.01%) ⬆️
... and 74 more

@gpotter2 gpotter2 force-pushed the raw-v2 branch 5 times, most recently from 3bd1d69 to a236e17 Compare January 6, 2019 17:34
@gpotter2 gpotter2 added the cleanup Performs some code clean-up label Jan 6, 2019
@gpotter2 gpotter2 force-pushed the raw-v2 branch 7 times, most recently from 136c44c to a995cae Compare January 11, 2019 01:47
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.

Cool PR! See my comments.

scapy/autorun.py Show resolved Hide resolved
scapy/autorun.py Show resolved Hide resolved
scapy/compat.py Outdated Show resolved Hide resolved
scapy/packet.py Outdated Show resolved Hide resolved
@gpotter2 gpotter2 force-pushed the raw-v2 branch 2 times, most recently from 255e7b5 to 8dc9ec2 Compare January 12, 2019 16:32
@guedou
Copy link
Member

guedou commented Jan 15, 2019

Good to me!

@p-l- what is your call?

@gpotter2
Copy link
Member Author

gpotter2 commented Jan 26, 2019

@guedou @p-l- I added the fix to #1814 in here, as it already had some heavy cryptography changes. That will fix Travis CI tests

I would like to get this merged as soon as possible, please, it's quite hard to maintain :/ Also it's probably going to conflict every other PRs

@gpotter2
Copy link
Member Author

gpotter2 commented Jan 30, 2019

Rebased against master.

Tests will fail because of the flake8 update (see
#1823), but those will fail because of this one.

#1823 can be merged first: it’s really small

@gpotter2 gpotter2 changed the title Reduce raw() usage + compatibility workarounds Reduce raw() usage/ compatibility calls + fix cryptography Feb 1, 2019
@gpotter2 gpotter2 force-pushed the raw-v2 branch 2 times, most recently from 43544bb to 2a31424 Compare February 3, 2019 17:44
@gpotter2
Copy link
Member Author

gpotter2 commented Feb 3, 2019

@p-l- @guedou Should be ready to get merged. This + #1823 fixes unit tests

@p-l- p-l- merged commit e6b45e0 into secdev:master Feb 4, 2019
@gpotter2 gpotter2 deleted the raw-v2 branch December 20, 2019 13:13
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.

Health checks (Deprecation/Garbage) failing
4 participants