Skip to content

Conversation

@romain-perez
Copy link
Contributor

This PR adds TLS 1.3 automaton for 1-RTT handshake (both client and server) and their associated tests.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #2244 into master will increase coverage by 0.7%.
The diff coverage is 88.2%.

@@            Coverage Diff            @@
##           master    #2244     +/-   ##
=========================================
+ Coverage   85.47%   86.17%   +0.7%     
=========================================
  Files         235      235             
  Lines       49127    49257    +130     
=========================================
+ Hits        41991    42448    +457     
+ Misses       7136     6809    -327
Impacted Files Coverage Δ
scapy/layers/tls/record.py 89.59% <0%> (+0.54%) ⬆️
scapy/layers/tls/keyexchange_tls13.py 79.27% <100%> (+33.16%) ⬆️
scapy/layers/tls/crypto/cipher_aead.py 86.95% <100%> (+5.21%) ⬆️
scapy/layers/tls/crypto/suites.py 100% <100%> (ø) ⬆️
scapy/layers/tls/session.py 77.35% <100%> (+8.66%) ⬆️
scapy/layers/tls/keyexchange.py 83.29% <100%> (+0.53%) ⬆️
scapy/layers/tls/extensions.py 83% <100%> (+1.96%) ⬆️
scapy/layers/tls/handshake.py 86.37% <100%> (+16.64%) ⬆️
scapy/layers/tls/automaton_cli.py 83.24% <80%> (+6.12%) ⬆️
scapy/layers/tls/automaton.py 87.6% <85.71%> (+7.43%) ⬆️
... and 20 more

@romain-perez
Copy link
Contributor Author

There was a problem with some TLS 1.2 server tests defined in tests_tls_netaccess.uts due to a default value changed for TLSSignature. I didn't catch this error locally because my version of OpenSSL support the new value.

Which version of OpenSSL are you using for your tests with "open_ssl_client" on top ?

@guedou
Copy link
Member

guedou commented Sep 11, 2019

@romain-perez that the version installed with Ubuntu Xenial used by Travis.

You can add the following to your commits. It will display the openssl version in Travis logs.

diff --git a/.config/travis/install.sh b/.config/travis/install.sh
index 607e0c57..31b7bda1 100644
--- a/.config/travis/install.sh
+++ b/.config/travis/install.sh
@@ -31,3 +31,6 @@ python -m pip install -U tox --ignore-installed
 
 # Dump Environment (so that we can check PATH, UT_FLAGS, etc.)
 set
+
+# Display openssl version
+openssl version

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for this change? I prefer a static list created rather than that. Also, using a variable might be useless, and we can get rid of the noqa:s:

        p.ext = [
            TLS_Ext_SupportedVersion_CH(versions=["TLS 1.3"]),
            TLS_Ext_SupportedGroups(groups=supported_groups),
            TLS_Ext_KeyShare_CH(
                client_shares=[KeyShareEntry(group=self.curve)],
            ),
            TLS_Ext_SignatureAlgorithms(sig_algs=["sha256+rsaepss"]),
        ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the reason for this change is because the list of extensions can't always be static if we want to implement all the TLS 1.3 handshake functionalities. For example, the extensions will change depending we are authenticating with PSK, sending 0-RTT encrypted data...

So this change is not really particularly helpful for this PR because it implements only a "basic" 1-RTT handshake but it's mainly to prepare for the next PR where the list of extensions will change.

I will fix the noqa: and get rid of the useless variable in my next commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep a static list for now, and add dynamic changes when needed. Would that work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that in my last commit but I've made a minor modification to add the static list if there's no extension provided in self.client_hello

Copy link
Member

Choose a reason for hiding this comment

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

Please do not introduce useless changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will also fix this and the others in my next commit.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot!

Copy link
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Member

Choose a reason for hiding this comment

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

Same.

Fix failed tests and remove useless changes

add crypto_advanced flag for TLS 1.3 tests
@p-l- p-l- merged commit abb56ca into secdev:master Sep 20, 2019
@romain-perez romain-perez mentioned this pull request Oct 8, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants