Skip to content

Conversation

@romain-perez
Copy link
Contributor

@romain-perez romain-perez commented Sep 23, 2019

This pull request adds :

  • Support for HelloRetryRequest message in automaton (client and server)
  • Tests from RFC8448 for HelloRetryRequest

Edits by gpotter2:

  • Drops support for cryptography < 2.0.0 (2017)
  • fix PR according to review
  • rebase

@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #2260 into master will decrease coverage by 0.07%.
The diff coverage is 51.58%.

@@            Coverage Diff             @@
##           master    #2260      +/-   ##
==========================================
- Coverage   87.76%   87.69%   -0.08%     
==========================================
  Files         242      242              
  Lines       50553    50639      +86     
==========================================
+ Hits        44369    44406      +37     
- Misses       6184     6233      +49
Impacted Files Coverage Δ
scapy/layers/tls/record.py 90.46% <0%> (ø) ⬆️
scapy/layers/tls/keyexchange_tls13.py 79.59% <100%> (+0.1%) ⬆️
scapy/config.py 84.13% <100%> (+0.36%) ⬆️
scapy/layers/tls/automaton_cli.py 83.76% <25%> (-2.38%) ⬇️
scapy/layers/tls/cert.py 86.17% <27.27%> (+1.03%) ⬆️
scapy/layers/tls/automaton_srv.py 78.15% <43.9%> (-2.04%) ⬇️
scapy/layers/tls/extensions.py 82.22% <63.63%> (-0.79%) ⬇️
scapy/layers/tls/handshake.py 86.53% <87.5%> (+0.04%) ⬆️
scapy/layers/tls/crypto/groups.py 88.76% <0%> (-8.99%) ⬇️
scapy/layers/tftp.py 89.36% <0%> (-0.29%) ⬇️
... and 3 more

@p-l-
Copy link
Member

p-l- commented Sep 24, 2019

Hi,

Thanks for this PR. You have two failed tests. Could you have a look?

For the second one, you may need to add ~ crypto_advanced on some of your tests.

@romain-perez
Copy link
Contributor Author

romain-perez commented Sep 24, 2019

The first failed test is a bit unclear to me because it failed when generating the doc and I didn't change this part. I am still looking at it.

For the second one, I didn't used ~ crypto_advanced flag on purpose because I thought it was necessary only for key exchange with curve x25519, but the curve used for this test is secp256r1. But I think this flag is also necessary because of the symmetric cipher, so I will add it.

@romain-perez
Copy link
Contributor Author

Hi,

I have been able to reproduce the first failed test and I found a way to fix it but I am not sure if it's the right way to fix it or not.

The error is caused by a missing module 'cryptography' when importing module 'all' from module 'scapy.layers.tls'. So, if I add the module 'cryptography' in deps for [testenv:docs] defined in the tox.ini as follow, the test is not failing anymore :

[testenv:docs]
skip_install = true
changedir = doc/scapy
deps = sphinx
       sphinx_rtd_theme
       cryptography
commands =
  sphinx-build -q -W -b html . _build/html

But I can't really see why it was failing only in this pull request and not in my previous one. Could it be possible that the module 'scapy.layers.tls' was not imported previously ? Do you have any ideas on this ?

@gpotter2
Copy link
Member

gpotter2 commented Oct 8, 2019

TLS files are using a safe check to be able to be imported even without cryptography:

if conf.crypto_valid:

Please continue to use those, that way it won't crash :p I must say I am not sure why it was done this way, I don't know if many things work without cryptography..

Side note: I think you could hardcore the RetryRequest bytes marker with a comment explaining how it got generated.

@romain-perez romain-perez mentioned this pull request Oct 8, 2019
9 tasks
@ria4
Copy link
Contributor

ria4 commented Oct 17, 2019

Hi,

Regarding crypto_valid, I introduced it in order for Scapy not to have a cryptography dependency. If this condition was not met, the TLS 1.3 module would still be able to parse/encode packets -even though you wouldn't be able to decrypt/verify protected contents.

I believe it's still the intention of the maintainers not to depend upon cryptography, so you should keep this validation.

@gpotter2
Copy link
Member

@guedou @p-l- ready for review

@gpotter2
Copy link
Member

Let's move on. This has staled for too long already

@gpotter2 gpotter2 merged commit 75c0d2a into secdev:master Dec 31, 2019
@gpotter2
Copy link
Member

@romain-perez Sorry for the delay, feel free to create PRs out of your other branches.

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