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

Session resumption #427

Merged
merged 10 commits into from Jul 25, 2018
Merged

Session resumption #427

merged 10 commits into from Jul 25, 2018

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Jul 18, 2018

Description

Implement support for session resumption in TLS 1.3

Motivation and Context

fixes #184

Checklist

  • I have read the CONTRIBUTING.md document and my PR follows change requirements therein
  • the changes are also reflected in documentation and code comments
  • all new and existing tests pass (see Travis CI results)
  • test script checklist was followed for new scripts
  • new test script added to tlslite-ng.json and tlslite-ng-random-subset.json
  • new and modified scripts were ran against popular TLS implementations:
    • OpenSSL
    • NSS
    • GnuTLS
  • required version of tlslite-ng updated in requirements.txt and README.md

This change is Reviewable

@tomato42 tomato42 added the enhancement new feature to be implemented label Jul 18, 2018
@tomato42 tomato42 self-assigned this Jul 18, 2018
@tomato42 tomato42 requested a review from Jakuje July 19, 2018 11:03
@tomato42 tomato42 force-pushed the session-resumption branch 2 times, most recently from d36c634 to 195afa0 Compare July 19, 2018 14:18
@tomato42 tomato42 added the new test script will require creation of a new connection script label Jul 19, 2018
there is a method to get the last message of a given type so use it
as we keep the list of exchanged messages in session resumption,
the test unnecessairly triggers in session resumption

move the responsibility to expect just one SH and HRR message to
the person writing test case
to actually use the ticket, we need to know how old it is, so save
the time it was received
as we don't know the identity of the ticket that will be provided
by server when the conversation is set up, we need to have a dynamic
method that uses current connection state to create the PSK extension

also, as the ticket may be used for 0-rtt data, it needs to be placed
first for the 0-rtt data to be readable (not used now, as 0-rtt is not
supported, but will be needed in the future)
the return value is an actual extension, the generators are
defined in methods below
Jakuje
Jakuje previously approved these changes Jul 25, 2018
Copy link
Collaborator

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

Except for the (probably) bogus imports in the test script, the changes look good for me.


from tlsfuzzer.runner import Runner
from tlsfuzzer.messages import Connect, ClientHelloGenerator, \
ClientKeyExchangeGenerator, ChangeCipherSpecGenerator, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I see right, the ChangeCipherSpecGenerator is unused in this file

from tlslite.constants import CipherSuite, AlertLevel, AlertDescription, \
TLS_1_3_DRAFT, GroupName, ExtensionType, SignatureScheme, \
PskKeyExchangeMode
from tlslite.keyexchange import ECDHKeyExchange
Copy link
Collaborator

Choose a reason for hiding this comment

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

ECDHKeyExchange is not use din the code either.

from tlslite.keyexchange import ECDHKeyExchange
from tlsfuzzer.utils.lists import natural_sort_keys
from tlsfuzzer.utils.ordered_dict import OrderedDict
from tlslite.extensions import KeyShareEntry, ClientKeyShareExtension, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused KeyShareEntry.

as the ExpectServerHello doesn't explicitly reset the keys,
but uses them if they are, or uses protocol specified defaults,
if they aren't there, the calculated keys can be wrong in case the
resumption failed or when connection switched from psk_ke to psk_dhe_ke
@tomato42
Copy link
Member Author

@Jakuje imports fixed

@Jakuje
Copy link
Collaborator

Jakuje commented Jul 25, 2018

I am not sure if the travis failure is related to the change or not (on specific python versions), but if not, I will add my ack.

@tomato42
Copy link
Member Author

both failures were unrelated; scripts/test-tls13-finished.py with the large pad is known to be problematic (#431 among others), rescheduled

@tomato42 tomato42 merged commit 77fa080 into master Jul 25, 2018
@tomato42
Copy link
Member Author

Thanks for the review!

@tomato42 tomato42 deleted the session-resumption branch July 25, 2018 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature to be implemented new test script will require creation of a new connection script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS 1.3 - Session resumption
2 participants