-
Notifications
You must be signed in to change notification settings - Fork 562
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
Comments
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). |
Thank you. We'll then pick up the master branch for development. |
@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? |
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. |
Thank you! We'll pick the 3.0.0-alpha0 for development. |
Here is the work plan for preparations for TLS 1.3 implementation, please check whether it sounds feasible.
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). |
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. |
Preparation work/refactoring for TLS 1.3 implementation will be done with following fork/branch: This is based on https://github.com/randombit/botan/releases/tag/3.0.0-alpha0 tag. |
@randombit: Few questions to agree the conventions for TLS 1.3 development:
Proposed structure draft (1 - preferable), files split into tls12/tls13 subdirectories, duplicated names allowed:
Proposed structure draft (2), files split into tls12/tls13 subdirectories, duplicated names avoided:
Proposed structure draft (3), files not split into 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? 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 :) |
@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 Currently exposed API for TLS: Refactoring to prepare for adding TLS 1.2 / Internal implementation pattern to hide the protocol-version specific code: Plan for implementing the TLS 1.3: 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.
|
@pist-eb Thanks for putting these detailed proposals and questions together.
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.
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
I think just
I think staying with single namespace is best here. Otherwise we get this situation where we have say |
Re file structure I prefer
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.
There is an emacs mode in |
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 |
Thank you for your comments @randombit! The suggestion regarding Corrected diagram for refactoring of TLS 1.2 attached below.
|
@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? |
Common representation of Cipher suites according to: https://www.iana.org/assignments/tls-parameters/tls-parameters.txt
Current abstraction for
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 For the TLS 1.3 cipher suites we can just set some fields in specific way, in particular:
Alternatively we can add separate field: @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 |
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. |
Hello @pist-eb. Thanks for all the work you put into this already! |
@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:
@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. |
@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! |
The PR is now created with short description: #2806. What was done until now: TLS 1.2 refactoring
TLS 1.3 development started
|
@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 @randombit We suggest to continue development of TLS 1.3 on the To split up the reviewing effort, lets review the refactoring pull request, and merge it into At this point we would have a reviewed TLS 1.3 development branch parallel to This would avoid upstreaming the added complexity of the refactoring into @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. |
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 Next steps from our point of view and in our order of priority:
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. |
@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 |
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 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. |
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 Expected Public API changesAs of now, the public API is stable. We merely introduced a few methods to Though, we expect some changes with the future introduction of protocol features. (EC)DH key agreement CallbacksThe 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, Callbacks
|
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 |
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:
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?
The text was updated successfully, but these errors were encountered: