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

added support for more DHCPv6 options #1945

Merged
merged 9 commits into from
Mar 30, 2019
Merged

added support for more DHCPv6 options #1945

merged 9 commits into from
Mar 30, 2019

Conversation

godfryd
Copy link
Contributor

@godfryd godfryd commented Mar 27, 2019

This PR improves several options in DHCPv6 and adds some new.

Added options:

  • DHCP6OptPanaAuthAgent
  • DHCP6OptNewPOSIXTimeZone
  • DHCP6OptNewTZDBTimeZone
  • DHCP6OptLQClientLink
  • DHCP6OptBootFileUrl
  • DHCP6OptClientArchType
  • DHCP6OptClientNetworkInterId
  • DHCP6OptERPDomain
  • DHCP6OptRelaySuppliedOpt

Improved DHCP6OptIA_NA, DHCP6OptIA_TA, DHCP6OptIA_PD to convey correctly different types of options (not only DHCP6OptIAPrefix as it was before).

Fixed parsing and detecting suboptions using guess_payload_class_and_make_instance.
Previously there was inserted in-between empty Generic Message.

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.

Looking good. It’s a great practice to update older custom fields and use PacketListField & co instead. Thanks for that !

@@ -413,8 +414,8 @@ class DHCP6OptIA_TA(_DHCP6OptGuessPayload): # RFC sect 22.5
FieldLenField("optlen", None, length_of="iataopts",
fmt="!H", adjust=lambda pkt, x: x + 4),
XIntField("iaid", None),
_IATAOptField("iataopts", [], DHCP6OptIAAddress,
length_from=lambda pkt: pkt.optlen - 4)]
PacketListField("iataopts", [], guess_payload_class_and_make_instance,
Copy link
Member

@gpotter2 gpotter2 Mar 27, 2019

Choose a reason for hiding this comment

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

Instead of this, you could set _DHCP6OptGuessPayload and add a dispatch_hook handler to the latter. That would be super clean.

See p-l-‘s post on: https://stackoverflow.com/a/28025620/5459467 for more infos (we should add this in the doc..)

@gpotter2
Copy link
Member

gpotter2 commented Mar 27, 2019

Flake8 report:

flake8 runtests: commands[0] | flake8 scapy/
scapy/layers/dhcp6.py:24:1: F811 redefinition of unused 'ShortField' from line 24
scapy/layers/dhcp6.py:166:80: E501 line too long (83 > 79 characters)
scapy/layers/dhcp6.py:407:80: E501 line too long (89 > 79 characters)
scapy/layers/dhcp6.py:417:80: E501 line too long (89 > 79 characters)
scapy/layers/dhcp6.py:781:80: E501 line too long (88 > 79 characters)
scapy/layers/dhcp6.py:971:80: E501 line too long (82 > 79 characters)
scapy/layers/dhcp6.py:981:1: W191 indentation contains tabs
scapy/layers/dhcp6.py:981:1: E101 indentation contains mixed spaces and tabs
scapy/layers/dhcp6.py:981:6: E128 continuation line under-indented for visual indent
scapy/layers/dhcp6.py:982:1: W191 indentation contains tabs
scapy/layers/dhcp6.py:982:3: E101 indentation contains mixed spaces and tabs
scapy/layers/dhcp6.py:982:6: E126 continuation line over-indented for hanging indent
scapy/layers/dhcp6.py:983:1: W191 indentation contains tabs
scapy/layers/dhcp6.py:983:3: E101 indentation contains mixed spaces and tabs
scapy/layers/dhcp6.py:987:1: E101 indentation contains mixed spaces and tabs
scapy/layers/dhcp6.py:997:80: E501 line too long (86 > 79 characters)
scapy/layers/dhcp6.py:998:80: E501 line too long (94 > 79 characters)
scapy/layers/dhcp6.py:1002:1: E302 expected 2 blank lines, found 1
scapy/layers/dhcp6.py:1028:1: E305 expected 2 blank lines after class or function definition, found 1
ERROR: InvocationError for command '/home/travis/build/secdev/scapy/.tox/flake8/bin/flake8 scapy/' (exited with code 1)

Tabs/spaces inconsistency are critical in Python 3+. Sorry for the annoyance, we enforce the PEP8 rules to all new code

Codespell report:


test/regression.uts:4573: diferent  ==> different
test/regression.uts:4595: diferent  ==> different
test/regression.uts:4978: diferent  ==> different
test/regression.uts:5371: archtypes  ==> archetypes
test/regression.uts:5374: archtypes  ==> archetypes
test/regression.uts:5377: archtypes  ==> archetypes
test/regression.uts:5381: archtypes  ==> archetypes
test/regression.uts:5384: archtypes  ==> archetypes
test/regression.uts:5388: archtypes  ==> archetypes

Please add the archtypes exception in https://github.com/secdev/scapy/blob/master/.travis/codespell_ignore.txt

@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #1945 into master will decrease coverage by 4.55%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1945      +/-   ##
=========================================
- Coverage   85.86%   81.3%   -4.56%     
=========================================
  Files         187     129      -58     
  Lines       42775   30505   -12270     
=========================================
- Hits        36729   24803   -11926     
+ Misses       6046    5702     -344
Impacted Files Coverage Δ
scapy/layers/dhcp6.py 63.32% <100%> (-21.48%) ⬇️
scapy/arch/bpf/supersocket.py 27.6% <0%> (-49.33%) ⬇️
scapy/arch/bpf/core.py 56.7% <0%> (-24.75%) ⬇️
scapy/consts.py 70.83% <0%> (-20.84%) ⬇️
scapy/sendrecv.py 67.62% <0%> (-17.8%) ⬇️
scapy/contrib/gtp.py 75.17% <0%> (-13.64%) ⬇️
scapy/layers/tls/crypto/compression.py 78.26% <0%> (-13.05%) ⬇️
scapy/supersocket.py 60.93% <0%> (-11.72%) ⬇️
scapy/arch/__init__.py 84.44% <0%> (-11.12%) ⬇️
... and 95 more

@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #1945 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1945      +/-   ##
==========================================
+ Coverage   85.86%   85.89%   +0.03%     
==========================================
  Files         187      187              
  Lines       42775    42848      +73     
==========================================
+ Hits        36729    36806      +77     
+ Misses       6046     6042       -4
Impacted Files Coverage Δ
scapy/layers/dhcp6.py 85.13% <100%> (+0.33%) ⬆️
scapy/layers/tls/handshake_sslv2.py 91.32% <0%> (-1.14%) ⬇️
scapy/themes.py 79.15% <0%> (-0.78%) ⬇️
scapy/layers/tls/basefields.py 79.86% <0%> (-0.68%) ⬇️
scapy/arch/windows/__init__.py 71.81% <0%> (-0.19%) ⬇️
scapy/sendrecv.py 85.25% <0%> (-0.17%) ⬇️
scapy/layers/x509.py 91.99% <0%> (ø) ⬆️
scapy/arch/pcapdnet.py 57.03% <0%> (ø) ⬆️
scapy/layers/inet.py 70.57% <0%> (+0.14%) ⬆️
scapy/volatile.py 78.5% <0%> (+0.15%) ⬆️
... and 5 more

@@ -1825,7 +1825,7 @@ class ATMT5(Automaton):
@ATMT.condition(BEGIN, prio=-1)
def tr3(self):
self.res += "u"

Copy link
Member

Choose a reason for hiding this comment

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

The spacing changes in the automaton classes break them. You could probably delete those spacing lines.

@gpotter2
Copy link
Member

gpotter2 commented Mar 27, 2019

Thanks for the update ! A last comment and that will be it

@@ -1437,11 +1437,11 @@ def _test():
conf.debug_dissector = False
ans,unans=sr(IP(dst="www.google.com/30")/TCP(dport=[80,443]),timeout=2)
conf.debug_dissector = old_debug_dissector

Copy link
Member

Choose a reason for hiding this comment

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

This one is failing to. It should be the last one

@guedou
Copy link
Member

guedou commented Mar 30, 2019

@godfryd it looks good to me, however before this PR can be merged could you revert the unnecessary change to regression.uts ?

@gpotter2
Copy link
Member

@guedou Do we really care about the unit tests history ?

@guedou
Copy link
Member

guedou commented Mar 30, 2019 via email

@godfryd
Copy link
Contributor Author

godfryd commented Mar 30, 2019

Do you mean I should bring back trailing whitespaces in regression.uts or something else?

@guedou
Copy link
Member

guedou commented Mar 30, 2019 via email

@godfryd
Copy link
Contributor Author

godfryd commented Mar 30, 2019

What next?

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.

@guedou guedou merged commit d3ef08b into secdev:master Mar 30, 2019
@guedou guedou mentioned this pull request Mar 30, 2019
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.

3 participants