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

Support for the TLS 1.3 - development #2714

Closed
pist-eb opened this issue Apr 16, 2021 · 29 comments
Closed

Support for the TLS 1.3 - development #2714

pist-eb opened this issue Apr 16, 2021 · 29 comments

Comments

@pist-eb
Copy link

pist-eb commented Apr 16, 2021

We'd like to contribute to Botan by starting the implementation of the TLS 1.3 protocol support.
The motivation is that our use-cases require the TLS 1.3 from the client side and therefore we could support the development in that topic. The plan is to start in May (or even earlier with rough design and development plan).

The rough plan:

  • design draft, to agree the direction of the development with the community,
  • refactoring of the existing TLS code, i.e. splitting the common code and the protocol version specific, introducing pimpl idiom to have generic interfaces and version specific implementation, etc.,
  • verification of refactored code to keep the quality on high level and avoid any regression,
  • start the development of TLS 1.3 protocol,
  • next steps...

The important thing would be to select proper start point (version) for the development. It seems that the release 2 (with the latest 2.18) is already mature and development on master branch is probably meant for upcoming 3.x release.
Is there any rough plan, when first 3.x version will be released, as we would like to pick up some 'official tag' for the development?

Please, comment whether this topic sounds interesting and implementing the TLS 1.3 support would be valuable here?

@randombit
Copy link
Owner

Yes I'd be very happy to accept patches adding TLS 1.3 support and would work with you to make this happen.

This work should happen on master branch for 3.0 release. 2.x at this point is effectively bug fix only, or at most backporting small features that are merged first onto master. Adding 1.3 support to master would probably be a bit simpler as we've shed a number of obsolete or obscure features (TLS 1.0, SRP, DSA support, etc) in master, as compared to 2.x which still retains them. There isn't a particular tag to pick up but, modulo adding TLS 1.3 support, I don't anticipate further major changes to the TLS code at this point, as the feature removals that are going to happen for 3.0 have already occurred on master.

There is no explicit schedule for 3.0.0 final release. I am planning on doing an initial alpha for community preview purposes in the next month or so. The final release will happen when it is ready (but I would certainly hope sometime this year).

cc @reneme @securitykernel

@pist-eb
Copy link
Author

pist-eb commented Apr 19, 2021

Thank you. We'll then pick up the master branch for development.
I'll share some design concepts, plans here when we start.

@pist-eb
Copy link
Author

pist-eb commented May 5, 2021

@randombit: We need to pick up the Botan version to start working on the TLS 1.3 protocol implementation. As agreed, master branch shall be used. Due to the formal reasons, it would be good to have any official tag to start with and to have some reference for that starting point later. When do you expect to release mentioned 3.0.0 preview/alpha version?

Another question: What is the stability of current development (master) branch comparing to the latest 2.18.0 release? Are there any known issues, which could prevent from experimental usage of the master branch already now?

@randombit
Copy link
Owner

There are no known bugs or stability issues on master. The majority of the reason for the major version bump is due to removing deprecated features or interfaces.

I suppose there is nothing stopping doing an alpha0 preview release in the very near future. It would be good for users to have something to test their application vs the modified APIs. I expect most of the changes from here on out (between now and 3.0.0 final release) will be additive, rather than modifications of existing APIs - though TLS 1.3 might be an exception to that. I'll tag and release something in the next day or so.

@randombit
Copy link
Owner

@pist-eb I've tagged 3.0.0-alpha0 at edafe58

@pist-eb
Copy link
Author

pist-eb commented May 10, 2021

Thank you! We'll pick the 3.0.0-alpha0 for development.

@pist-eb
Copy link
Author

pist-eb commented May 10, 2021

Here is the work plan for preparations for TLS 1.3 implementation, please check whether it sounds feasible.

  1. Removing of the TLS 1.0 and TLS 1.1 support from Botan (DONE already in master)
  2. Moving the TLS 1.2 specific code to tls12 subdirectory
    • this might be postponed to a later phase, as splitting protocol-specific from common code might be problematic at this stage
  3. Refactoring of TLS::Channel to be a pure interface
    • pimpl idiom to instantiate proper implementation depending on protocol version
    • UT adjustments
    • regression checks
  4. Refactoring of TLS::Client
    • pimpl idiom to instantiate proper implementation depending on protocol version
    • UT adjustments
    • regression checks
  5. Refactoring of TLS::Server
    • pimpl idiom to instantiate proper implementation depending on protocol version
    • UT adjustments
    • regression checks
  6. (optional) Factory methods for instantiating the specific implementation of Channel, Client, Server
    • it could be nice to have some factory to instantiate the Client/Server and omit the direct calls of ctors.
  7. Verification of refactored code

The purpose of the preparation step is mainly clear separation of TLS 1.2 from TLS 1.3 (the idea is to have the build flags as it was for TLS 1.0/1.1 and DTLS 1.1 in releases 2.x).
The approach would be to deliver patches step-by-step to have the Community review and prevent from single, huge, and unreviewable patch in the end.

@winterland1989
Copy link

Are there any possibilities to add FFI code while refractory TLS code? The current C++ API has some corner cases for writing C bindings, maybe it's better to write a C API at the same time.

@pist-eb
Copy link
Author

pist-eb commented May 14, 2021

Are there any possibilities to add FFI code while refractory TLS code? The current C++ API has some corner cases for writing C bindings, maybe it's better to write a C API at the same time.

@winterland1989: At least at the moment it is not planned in scope of the TLS 1.3 implementation, the small refactoring is only planned to prepare for TLS 1.3.

@pist-eb
Copy link
Author

pist-eb commented May 27, 2021

Preparation work/refactoring for TLS 1.3 implementation will be done with following fork/branch:
https://github.com/pist-eb/botan/tree/tls1.3_refactoring

This is based on https://github.com/randombit/botan/releases/tag/3.0.0-alpha0 tag.

@pist-eb
Copy link
Author

pist-eb commented May 31, 2021

@randombit: Few questions to agree the conventions for TLS 1.3 development:

  • Separate directories for (D)TLS 1.2 and TLS 1.3 specific code: src/lib/tls/tls12 and src/lib/tls/tls13, while shared code/interfaces will stay in src/lib/tls

  • File names: It will be hard to find distinct names, i.e. tls_handshake_state.* and we might end up with same file names in differed subdirs; would it be fine to use tls_13 prefix for TLS 1.3 specific files? Or we shall just rely on proper file location, i.e. #include <botan/tls/tls12/tls_handshake_state.h>, distinct include guards and set up the build system to avoid obj files collision? Any other proposal here?

  • Namespaces: TLS for common/generic code, TLS/v13 for TLS 1.3? TLS/v12 for TLS 1.2? Or better just leave TLS and rely on file names/structure?

  • Class names: in one of the plans you mentioned: TLS::Channel_Impl_12, TLS::Client_Impl_12, etc., other idea would be to use the same names but in different namespaces?

Proposed structure draft (1 - preferable), files split into tls12/tls13 subdirectories, duplicated names allowed:

.
└── lib
    └── tls
        ├── tls12
        │   ├── tls_channel_impl.cpp
        │   ├── tls_channel_impl.h
        │   ├── tls_client_impl.cpp
        │   ├── tls_client_impl.h
        │   ├── tls_server_impl.cpp
        │   └── tls_server_impl.h
        ├── tls13
        │   ├── tls_channel_impl.cpp
        │   ├── tls_channel_impl.h
        │   ├── tls_client_impl.cpp
        │   ├── tls_client_impl.h
        │   ├── tls_server_impl.cpp
        │   └── tls_server_impl.h
        ├── tls_callbacks.cpp
        ├── tls_callbacks.h
        ├── tls_channel.cpp
        ├── tls_channel.h
        ├── tls_client.cpp
        ├── tls_client.h
        ├── tls_policy.cpp
        ├── tls_policy.h
        ├── tls_server.cpp
        └── tls_server.h

Proposed structure draft (2), files split into tls12/tls13 subdirectories, duplicated names avoided:

.
└── lib
    └── tls
        ├── tls12
        │   ├── tls_12_channel_impl.cpp
        │   ├── tls_12_channel_impl.h
        │   ├── tls_12_client_impl.cpp
        │   ├── tls_12_client_impl.h
        │   ├── tls_12_server_impl.cpp
        │   └── tls_12_server_impl.h
        ├── tls13
        │   ├── tls_13_channel_impl.cpp
        │   ├── tls_13_channel_impl.h
        │   ├── tls_13_client_impl.cpp
        │   ├── tls_13_client_impl.h
        │   ├── tls_13_server_impl.cpp
        │   └── tls_13_server_impl.h
        ├── tls_callbacks.cpp
        ├── tls_callbacks.h
        ├── tls_channel.cpp
        ├── tls_channel.h
        ├── tls_client.cpp
        ├── tls_client.h
        ├── tls_policy.cpp
        ├── tls_policy.h
        ├── tls_server.cpp
        └── tls_server.h

Proposed structure draft (3), files not split into subdirectories, duplicated names avoided:

.
└── lib
    └── tls
        ├── tls_12_channel_impl.cpp
        ├── tls_12_channel_impl.h
        ├── tls_12_client_impl.cpp
        ├── tls_12_client_impl.h
        ├── tls_12_server_impl.cpp
        ├── tls_12_server_impl.h
        ├── tls_13_channel_impl.cpp
        ├── tls_13_channel_impl.h
        ├── tls_13_client_impl.cpp
        ├── tls_13_client_impl.h
        ├── tls_13_server_impl.cpp
        ├── tls_13_server_impl.h
        ├── tls_callbacks.cpp
        ├── tls_callbacks.h
        ├── tls_channel.cpp
        ├── tls_channel.h
        ├── tls_client.cpp
        ├── tls_client.h
        ├── tls_policy.cpp
        ├── tls_policy.h
        ├── tls_server.cpp
        └── tls_server.h

Depending on the answers to above questions: if we use conventions for filenames/namespaces for TLS 1.3, in my opinion it would be good to have the same for TLS 1.2, but maybe it is unnecessary waste of time and we shall not touch TLS 1.2 filenames/namespaces?

UPDATE: Sharing any formatting style configuration (i.e. clang-format) would be appreciated. The most important coding convention guideline would be to follow existing one :)

@pist-eb
Copy link
Author

pist-eb commented Jun 14, 2021

@randombit, All: The draft of design for TLS refactoring step and TLS 1.3 implementation can be checked in below diagrams. Please note, that this just includes the Channel, Client, Server. The common code or other version-specific classes will be considered in later steps.

Currently exposed API for TLS:
botan_tls_view_tls12_original_1 0d

Refactoring to prepare for adding TLS 1.2 / Internal implementation pattern to hide the protocol-version specific code:
botan_tls_view_tls12_refactored_1 0d

Plan for implementing the TLS 1.3:
botan_tls_view_tls13_implementation_1 0d

If the Client is supporting both: TLS 1.3 and TLS 1.2 (configured in policy), then it will handle the Server_Hello message to check the protocol version (legacy version + supported_versions extension) and either continue with TLS 1.3 client implementation, or switch to TLS 1.2 client implementation. If both TLS versions are supported on the server side, the instantiation of version specific impl can be delayed until Client_Hello is received and exact version of TLS proposed by the client is known.

Creative Commons License
This work is licensed under a Creative Commons Attribution-ShareAlike 4.0 International License.

@pist-eb pist-eb changed the title Support for the TLS 1.3 - development planned Support for the TLS 1.3 - development Jun 15, 2021
@randombit
Copy link
Owner

@pist-eb Thanks for putting these detailed proposals and questions together.

Separate directories for (D)TLS 1.2 and TLS 1.3 specific code: src/lib/tls/tls12 and src/lib/tls/tls13, while shared code/interfaces will stay in src/lib/tls

Sounds good. This will also allow us to enable or disable TLS 1.2 or 1.3 selectively at build time. Eg for envs that only want 1.3 support, or that only need 1.2 for whatever reason. So useful for those concerned about code size or highest security.

File names: It will be hard to find distinct names, i.e. tls_handshake_state.* and we might end up with same file names in differed subdirs; would it be fine to use tls_13 prefix for TLS 1.3 specific files?

Right now all files within the source tree are unique (there are some dups but they are dup'ed between the library and the cli which are distinct projects). Off the top of my head I'm not sure if same filename in different dirs works or not, I think for source files it works because everything is namespaced in the build objects. However for headers it does not, everything is squashed down into a single directory. Changing that would have some dramatic changes to the public API and also across the whole codebase. [It would not be necessarily a bad change! Not doing so from the start was probably a mistake on my part. But let's not add more work for ourselves] So lets use tls_13 filename prefix and also tls_12 prefix where appropriate.

Namespaces: TLS for common/generic code, TLS/v13 for TLS 1.3? TLS/v12 for TLS 1.2? Or better just leave TLS and rely on file names/structure?

I think just TLS is fine for everything and we can use eg 'v13` in class names or functions where they are 1.3 specific. Otherwise it gets quite noisy in the code IMO.

Class names: in one of the plans you mentioned: TLS::Channel_Impl_12, TLS::Client_Impl_12, etc., other idea would be to use the same names but in different namespaces?

I think staying with single namespace is best here. Otherwise we get this situation where we have say TLS::Channel_Impl (some generic interface), TLS::v13::Channel_Impl and TLS::v12::Channel_Impl and then you have to either wear out your : key or else it becomes confusing as it will be unclear which Channel_Impl is in the current scope.

@randombit
Copy link
Owner

Re file structure I prefer

Proposed structure draft (2), files split into tls12/tls13 subdirectories, duplicated names avoided:

Depending on the answers to above questions: if we use conventions for filenames/namespaces for TLS 1.3, in my opinion it would be good to have the same for TLS 1.2, but maybe it is unnecessary waste of time and we shall not touch TLS 1.2 filenames/namespaces?

IMO it's best to move everything so that 1.2 and 1.3 are treated consistently. It will make more noise in the short term but will make comprehension easier overall.

UPDATE: Sharing any formatting style configuration (i.e. clang-format) would be appreciated. The most important coding convention guideline would be to follow existing one :)

There is an emacs mode in src/configs/indent.el and an astyle that is pretty close in src/configs/astyle.rc we are going to do clang-format "Real Soon Now" the main blocker there is that clang-format cannot even plausibly match the current convention (which is admittedly a fairly unusual one, and with lots of very specific manual formatting in the ASN.1 code) so applying clang-format also means making diffs for backports etc excessively difficult. :(

@randombit
Copy link
Owner

You're designs look about right. One concern I'd have is virtual inheritence which has caused a lot of weird ABI problems for us in the public key code. If we can avoid introducing that here I'd really prefer it. Is there a reason why Channel_Impl should derive from Channel? IMO we should treat Channel as a pure interface which is implemented by Client and Server. They implement the Channel interface by delegating calls to a Channel_Impl. But in principle Channel_Impl could look completely different from Channel, and Client and Server mediate that. WDYT?

@pist-eb
Copy link
Author

pist-eb commented Jun 21, 2021

Thank you for your comments @randombit!

The suggestion regarding Channel_Impl and Channel relation sounds reasonable, we'll apply it.
The agreed conventions, file/directory structure, file names, etc. will be used.

Corrected diagram for refactoring of TLS 1.2 attached below.
botan_tls_view_tls12_refactored_1 1d_signed

Creative Commons License
This work is licensed under a Creative Commons Attribution-ShareAlike 4.0 International License.

@pist-eb
Copy link
Author

pist-eb commented Jun 23, 2021

@randombit: We'd like to provide first patches with TLS 1.2 refactoring as a preparation for TLS 1.3 implementation. The plan is to use the fork/branch: https://github.com/pist-eb/botan/tree/tls1.3_refactoring

We'd like to agree on how to provide the changes to the Botan / master? Will you prefer that we work on fork/branch and only create the PR to Botan master when the TLS 1.3 is implemented? Or maybe to use a step-wise approach, including initial refactoring & preparation steps?

@pist-eb
Copy link
Author

pist-eb commented Jun 24, 2021

Common representation of Cipher suites according to: https://www.iana.org/assignments/tls-parameters/tls-parameters.txt

    0x13,0x01   TLS_AES_128_GCM_SHA256                                       Y       Y           [RFC8446]
    0x13,0x02   TLS_AES_256_GCM_SHA384                                       Y       Y           [RFC8446]
    0x13,0x03   TLS_CHACHA20_POLY1305_SHA256                                 Y       Y           [RFC8446]
    0x13,0x04   TLS_AES_128_CCM_SHA256                                       Y       Y           [RFC8446]

Current abstraction for Ciphersuite:

Ciphersuite(uint16_t ciphersuite_code,
                  const char* iana_id,
                  Auth_Method auth_method,
                  Kex_Algo kex_algo,
                  const char* cipher_algo,
                  size_t cipher_keylen,
                  const char* mac_algo,
                  size_t mac_keylen,
                  KDF_Algo prf_algo,
                  Nonce_Format nonce_format) :
         m_ciphersuite_code(ciphersuite_code),
         m_iana_id(iana_id),
         m_auth_method(auth_method),
         m_kex_algo(kex_algo),
         m_prf_algo(prf_algo),
         m_nonce_format(nonce_format),
         m_cipher_algo(cipher_algo),
         m_mac_algo(mac_algo),
         m_cipher_keylen(cipher_keylen),
         m_mac_keylen(mac_keylen)
         {
         m_usable = is_usable();
         }

Initially we thought about creating some additional abstraction, but in the end (to keep compatibility, allow mixing of TLS 1.2 and TLS 1.3 cipher suites) it seems that current Ciphersuite class could be commonly used.

For the TLS 1.3 cipher suites we can just set some fields in specific way, in particular:

  • ciphersuite_code : iana code specific to TLS 1.3 cipher suite
  • iana_id : iana id specific to TLS 1.3 cipher suite
  • auth_method : Auth_Method::IMPLICIT as this is no longer part of cipher suite
  • kex_algo : Kex_Algo::IMPLICIT as this is no longer part of cipher suite
  • cipher_algo : AEAD cipher mode, i.e.: AES-128/GCM
  • cipher_keylen : key length
  • mac_algo : AEAD
  • mac_keylen : 0
  • prf_algo : we can use this field to set HKDF Hash Algorithm, i.e.: KDF_Algo::SHA_256
  • nonce_format : AEAD_IMPLICIT_4 (AES/GCM) or AEAD_XOR_12 (ChaCha20Poly1305)

Alternatively we can add separate field: hkdf_algo, while setting the prf_algo to KDF_Algo::NONE for TLS 1.3 cipher suites and accordingly the hkdf_algo to HKDF_Algo::NONE for TLS 1.2 cipher suites.

@randombit, others: WDYT about such an approach?

If this is fine, then we'll update the tls_suite_info.py script to add the TLS 1.3 cipher suites. Maybe it would also be worth to include TLS 1.3 cipher suites conditionally, under BOTAN_HAS_TLS_13 define?

@pist-eb
Copy link
Author

pist-eb commented Sep 10, 2021

Hi @randombit, All.

The refactored TLS 1.2 code and parts of TLS 1.3 implementation is available in fork: https://github.com/pist-eb/botan, branch: tls1.3_development (https://github.com/pist-eb/botan/tree/tls1.3_development).

We can do a PR to some development branch of Botan, if you wish. We appreciate any support and help given until now - thank you for that!

Unfortunately we had to stop TLS 1.3 development recently due to commercial reasons. We are still available for some questions and adjustments of provided code if required.

Hopefully our contribution helps the community to get the TLS 1.3 feature completely implemented in the future.

@reneme
Copy link
Collaborator

reneme commented Sep 15, 2021

Hello @pist-eb. Thanks for all the work you put into this already!
We are trying to get an overview of the progress you made so far and what is left to be done. Could you provide some pointers and a rough ToDo-list to give us some handle into your work?

@hrantzsch
Copy link
Collaborator

@pist-eb @randombit, we think the work that has been done so far looks great. Tests and Bogo-Shim run fine, our asio stream is also fine and example client and server implementations work as expected.

It would probably be a good idea to start by merging the refactored TLS 1.2 code. We see the following steps necessary for this:

  • cleanup commit history (or squash during merge)
  • remove TLS 1.3 specific code for now
  • decide if TLS 1.0 and 1.1 support should be re-added
  • thorough code review

@pist-eb could you open a PR to the branch dev/tls-13? Also, would you like to squash the commits yourself to preserve the authorship information of you and your colleagues?

We can't promise that we'll pick up the work on TLS 1.3, but the refactoring on its own seems worth integrating.

@pist-eb
Copy link
Author

pist-eb commented Sep 23, 2021

@hrantzsch: I'll prepare the PR next week, when back from holidays.

@reneme: I'll also share then a short summary what was done till now and the original plan.

Thank you for looking at the code!

@pist-eb
Copy link
Author

pist-eb commented Sep 28, 2021

The PR is now created with short description: #2806.

What was done until now:

TLS 1.2 refactoring

  1. Using private implementation for TLS Channel, Client and Server. Motivation:
  • Common interface for TLS 1.2 and TLS 1.3 classes,
  • Protocol-version specific internal implementation,
  • Clear separation of TLS 1.2 and TLS 1.3 implementation,
  • Ability to switch the internal implementation during the handshake, i.e. TLS client proposes TLS 1.3 and TLS 1.2, while TLS server selects the TLS 1.2.
  1. Using private implementation for TLS handshake messages. Motivation:
  • Handshake messages which appear in both versions of protocol shall have common interface,
  • Internal implementation and interpretation of data shall be protocol-version specific.
  1. Creating the factories to instantiate proper versions of internal implementation. Motivation:
  • Unifying the way of creating an internal implementation depending on selected protocol version.

TLS 1.3 development started

  1. Adding new module for Botan: tls13,
  2. Extending the Protocol_Version structure with TLS_V13,
  3. Extending the Policy with TLS_V13 support,
  4. Implementing/testing new extensions:
  • Cookie (44),
  • Key_share (51),
  • Signature_algorithms_cert (50),
  • Supported_versions (43),
  • Supported_groups (10),
  • Signature_algorithms (13).
  1. Implementing the skeleton of Channel and Client for TLS 1.3,
  2. Implementing the skeleton of Client_Hello for TLS 1.3.

@reneme
Copy link
Collaborator

reneme commented Oct 1, 2021

@pist-eb et al. Thanks for all the extra work you put in to squash your commits and sort them into "TLS 1.2 refactoring" and "start of TLS 1.3 development".

@hrantzsch opened another pull request against dev/tls13 (#2808) containing only your refactoring commit (+ a small GCC-related fix).

@randombit We suggest to continue development of TLS 1.3 on the dev/tls13 branch parallel to master.

To split up the reviewing effort, lets review the refactoring pull request, and merge it into dev/tls13 first. Then, lets rebase @pist-eb's TLS 1.3 first steps onto the dev/tls13 branch and review/merge them as well.

At this point we would have a reviewed TLS 1.3 development branch parallel to master in which TLS 1.3 development could move forward without applying changes to botan upstream, yet. Eventually, both refactoring and a finished implementation of TLS 1.3 could be merged back into master.

This would avoid upstreaming the added complexity of the refactoring into master before it is actually needed. Assuming that things don't diverge a lot on master in the mean time, the extra work of maintaining the two branches should be manageable.

@randombit Do you agree with this plan?

@hrantzsch and I would gladly help with the review to get an overview of the current development state. Also, it would allow us to assess the effort required to finish the TLS 1.3 development and evaluate whether our company can shell out enough of our dev-time to move this further along.

@reneme
Copy link
Collaborator

reneme commented Feb 7, 2022

We feel its time to provide some progress update on this:

Based on the work of @pist-eb et.al. we now managed to implement a rudimentary TLS 1.3 handshake and application data exchange. Basically a full rundown of RFC 8448 Section 3, minus server certificate validation. There are obviously many loose ends, but google.com:443 answered our call successfully.

Major new building blocks are an implementation of the TLS 1.3 Record Layer, TLS 1.3's (traffic) key derivation, the Key_Share extension encapsulating the Key Exchange, some TLS_Policy additions (most notably key_exchange_groups_to_offer()) and an integration test based on RFC 8448 Section 3 (client side). Furthermore, Channel_Impl_13 and Client_Impl_13 act as composition root and steer the new components.

Next steps from our point of view and in our order of priority:

  • Minimal Viable Product

    • server certificate validation
      • OCSP stapling
    • Hello Retry Request
    • handle CLOSE_NOTIFY properly (difference between TLS 1.2 and 1.3)
    • bogo test suite integration
    • Session Resumption (PSK w/o 0-RTT)
    • Key Update
    • Key Material Export (RFC 8446 7.5 / RFC 5705)
    • configure ClientHello settings (e.g. ALPN)
  • Towards a full implementation

    • server side implementation
    • record size limit RFC8449
    • 0-RTT
    • Client Authentication
    • protocol version downgrade
    • separate TLS 1.3 cipher suites from 1.2

A field of open dabate is the possibility to negotiate TLS 1.2 based on the TLS 1.3 implementation.

@randombit Let's talk about a rough roadmap to (iteratively) integrate those changes. Currently, we hope to have a working MVP by the end of March. That said: we would appreciate to establish a more-or-less regular code review cycle with you, as the current draft PR is getting quite large and doesn't even include the refactorings of last year. We'd be happy to walk you through the code base, of course.

@randombit
Copy link
Owner

@reneme It's great you have been able to get this far. I think a key step will be, to the extent possible, incrementally landing some of these changes onto master. I'd rather avoid a single huge merge if we can help it. (If it is too difficult to disentangle then so be it). I'll start reviewing your current work this week or over this weekend at latest.

@reneme
Copy link
Collaborator

reneme commented Feb 17, 2022

Thanks for getting back to us on that. I couldn't agree more; the code changes are pretty substantial by now. We're currently in the process of disentangling TLS 1.2 and 1.3 specific code in the handshake messages and the handling of those in the TLS 1.3 Channel and Client.

This refactoring will be the basis for moving more of the version-specific stuff into tls12 and tls13 modules. Hopefully, this will make it easier to split the PR into smaller pieces. Say (roughly): utilities (e.g. additions to the test framework), pimpl-refactoring (mostly done by @pist-eb et. al.), moving of TLS 1.2 specifics into a tls12 module and eventually introduction of the TLS 1.3 MVP code as outlined above. All of those will likely be fairly large chunks of changes, though.

Once all of this is merged to master, the rest of the protocol implementation should be fairly straight-forward to split into smaller self-contained changesets.

Nevertheless, we appreciate any feedback on the current state of the implementation.

@reneme
Copy link
Collaborator

reneme commented Mar 2, 2022

We're done with the announced refactorings that removed most of the TLS 1.2 specific code from the TLS 1.3 implementation. As a result, the Pimpl-constructions in the TLS handshake message classes were reverted. Those are all internal (or at least BOTAN_UNSTABLE) interfaces, we didn't see a benefit of trying to unify APIs of TLS 1.2 and 1.3 handshake message classes.

Expected Public API changes

As of now, the public API is stable. We merely introduced a few methods to TLS::Policy. E.g. to define what DH-groups should be offered in the initial client hello (key_exchange_groups_to_offer). The Pimpl-constructions for user-facing classes (Channel, Client, Server) are still there and will stay to keep the API for 1.3 and 1.2 compatible.

Though, we expect some changes with the future introduction of protocol features.

(EC)DH key agreement Callbacks

The key agreement mechanics changed ever so slightly from TLS 1.2 to TLS 1.3 and hence reusing the default TLS 1.2 key agreement code in those callbacks is not an option. The callbacks would need to become aware of the negotiated protocol version at least. We, however, suggest to deprecate said callbacks in Botan 3.0 and (if needed) think about a more generic solution. Similarly, tls_decode_group_param might be worth reconsidering as well.

Callbacks tls_examine_extensions and tls_modify_extensions

TLS 1.3 introduces quite a few more places where Extensions are (optionally) added to handshake messages and even their sub-structures. For instance, most Server Hello extensions are now in an Encrypted Extensions message, OCSP stapling and SCT are implemented via extensions in certificate entries in a Certificate handshake message and so forth. Even New Session Ticket which is a Post-Handshake Message can have extensions.

I.e. the above-mentioned callbacks should have a way to communicate the context in which the extensions to modify/examine appear. Alternatively, one could deprecate those callbacks and request the library user to use tls_inspect_handshake_message instead (+ add a tls_modify_handshake_message) for that purpose.

Implementation: We added a "Handshake Message Type" context. Technically, that might still be lacking some context in case of the extensions in individual certificate entries. Here, we just invoke the callback multiple times.

TLS 1.3 Session Resumption

From a first glance, the PSK mechanism of TLS 1.3 won't easily fit the existing TLS::Session API (e.g. Callbacks::session_established doesn't make sense in TLS 1.3). For example, TLS 1.3 may provide session tickets at any time after a successful handshake.

0-RTT and early data

If Botan chooses to support those, we might need some additional public APIs to let applications communicate the "early data" they want to transmit. Obviously, 0-RTT has a dependency on PSK.

Signature Algorithms Cert Extension

TLS 1.3 introduces the "signature_algortihms_cert" extension alongside TLS 1.2's "signature_algorithms" extension. This allows clients to specify distinct signature algorithm requirements for signatures in certificates and signatures in CertificateVerify messages. I.e. a certificate can be used for server authentication only iff its public key and its CA signature comply with the (possibly different) signature algorithm requirements.

To support that we will need a new virtual method in TLS::CredentialsMananger that allows specifying both restrictions.

Next Steps

It seems quite challenging to split the pull request into managable chunks, to be honest. As an alternative, we propose to provide some documentation of our design choices and architecture diagrams.

@randombit Can we schedule a "workshop" in the near future? There we could give you a high-level walkthrough of the changes and discuss next steps.

@reneme
Copy link
Collaborator

reneme commented Jun 9, 2022

With basic TLS 1.3 client support now on the finish line, the time has come to retire this issue. Thanks a lot everyone for the time and sweat that went into this development! @pist-eb (et.al.), @hrantzsch, @randombit.

Further development of the TLS 1.3 implementation will be tracked here: #2990

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

No branches or pull requests

5 participants