TLS 1.2 Refactoring #2808

hrantzsch · 2021-10-01T08:40:03Z

The refactoring of the TLS 1.2 code done by @pist-eb and his colleagues in #2806 only. This way we can review and merge into the TLS 1.3 dev branch independently of the added TLS 1.3 code.

src/lib/tls/tls_messages.h

reneme · 2021-10-11T14:09:07Z

In the upstream implementation, concrete implementations of Handshake_Message::serialize() were private. This refactoring exposes them as part of the public class interface. Leaving that here for later investigation.

reneme

In a first pass over the changes I focussed on the refactoring of the TLS message implementations. Mainly sanity checking the PIMPL-scheme layout. Furthermore, I double-checked the upstream protocol implementation code and the associated relocated _impl_12 classes.

No real surprises, but I'd like to discuss whether we should revert the splitting of the Server_Hello parsing code into the abstract _Impl and concrete _Impl_12 classes. I feel it would be better to do those (logic altering) changes in the next step. That said: I didn't find obvious issues with the separation either. @pist-eb, what do you think?

Anyway! Great to see the protocol version abstraction falling into place. 👍

reneme · 2021-10-11T13:39:31Z

src/lib/tls/msg_cert_req_impl.cpp

+#include <botan/internal/tls_handshake_hash.h>
+#include <botan/internal/msg_cert_req_impl.h>
+#include <botan/der_enc.h>
+#include <botan/ber_dec.h>


Remove superfluous includes

src/lib/tls/msg_cert_req_impl.cpp

src/lib/tls/msg_cert_req_impl.h

src/lib/tls/msg_cert_req.cpp

src/lib/tls/msg_cert_verify_impl.h

src/lib/tls/msg_server_hello_impl.cpp

reneme · 2021-10-11T14:55:59Z

src/lib/tls/msg_server_hello_impl.cpp

+                                     RandomNumberGenerator& rng,
+                                     const Client_Hello& client_hello,
+                                     const Server_Hello::Settings& server_settings,
+                                     const std::string next_protocol) :


This looks different from the upstream Server_Hello constructor. Note to self: need to look deeper into it.

I guess this prepares the Server_Hello_Impl base class for code sharing between 1.2 and 1.3. Frankly, I feel like we should refrain from doing this in this refactoring step and rather do those logic changes with the integration of 1.3.

Yes, indeed the _Impl base classes for messages were prepared to share the common logic of TLS 1.2 and TLS 1.3.

reneme · 2021-10-11T15:08:26Z

src/lib/tls/tls13/msg_client_hello_impl_13.h

+   public:
+      explicit Client_Hello_Impl_13()
+         {
+         // TODO throw std::runtime_error("Implemenation for TLSv1.3 not ready yet. You are welcome to implement it.");


why not actually throw?

I cannot recall now, but either BoGo_shim was failing with that throw, or one of our custom test scenarios.

src/lib/tls/tls_message_factory.h

src/lib/tls/tls_messages.h

pist-eb · 2021-10-12T05:14:28Z

In a first pass over the changes I focussed on the refactoring of the TLS message implementations. Mainly sanity checking the PIMPL-scheme layout. Furthermore, I double-checked the upstream protocol implementation code and the associated relocated _impl_12 classes.

No real surprises, but I'd like to discuss whether we should revert the splitting of the Server_Hello parsing code into the abstract _Impl and concrete _Impl_12 classes. I feel it would be better to do those (logic altering) changes in the next step. That said: I didn't find obvious issues with the separation either. @pist-eb, what do you think?

Anyway! Great to see the protocol version abstraction falling into place. 👍

Honestly, the border between the 'refactoring of TLS 1.2' and 'implementation of TLS 1.3' was quite thin and maybe we partially started some TLS 1.3 logic changes already during refactoring. Please, feel free to adapt refactored code to your needs and ideas :)

reneme

Things look good so far. Still missing tls_channel_impl_12 and tls_client_impl_12.

src/lib/tls/tls_client.cpp

src/lib/tls/tls_channel_impl.h

src/lib/tls/tls12/tls_server_impl_12.cpp

reneme

This was the final review pass going through the remaining implementation files (mainly channel_impl_12 and client_impl_12. Unsurprisingly, 99% of these files are just copy & pasted code from the previous (non-pimpl-ed) locations.

This concludes comparing the implementations side-by-side with the respective implementation on the current master. Cumbersome work, but that also gave me a good overview of the TLS 1.2 implementation code base and the newly added layer of abstraction.

src/lib/tls/tls12/tls_channel_impl_12.cpp

reneme · 2021-10-22T11:37:55Z

So I went through the refactoring code and compared it to the respective implementations in latest master. Looks good to me. No real surprises, just lot's of code moving around.

@pist-eb We don't expect you to tackle the nitpicks and comments I attached in the last two weeks. @hrantzsch and I are going to resolve those next week.

After that, from our perspective it should be fine to merge this to dev/tls-13. We'll continue as described in GH #2714, except you have any objections to this plan, @randombit?

In any case, dev/tls-13 will need a rebase soon, to bring in the recent CI fixes and make sure everything is green. We'll do that after we tackled the nitpicks.

pist-eb · 2021-10-22T11:53:26Z

Please note, that in our refactoring we still had some doubts, whether there are more classes that could be 'protocol version specific' in the end. Unfortunately, we couldn't determine all of them at refactoring phase, probably everything will become clear when implementing TLS 1.3 :)

Co-authored-by: Marek Kocik <extern.Marek.Kocik@elektrobit.com> Co-authored-by: Grzegorz Dulewicz <extern.Grzegorz.Dulewicz@elektrobit.com> Co-authored-by: Pawel Bazelewski <extern.pawel.bazelewski@elektrobit.com> Co-authored-by: Pawel Jarosz <extern.pawel.jarosz@elektrobit.com>

reneme · 2021-10-25T12:20:11Z

Rebased to latest master (to pick up recent CI fixes)

Co-authored-by: René Meusel <rene.meusel@nexenio.com>

reneme

Good for now. I'm sure we'll find many more things while getting our feet wet when implementing TLS 1.3.

reneme mentioned this pull request Oct 1, 2021

Support for the TLS 1.3 - development #2714

Closed

hrantzsch force-pushed the dev/tls-13-refactoring branch from d9abf34 to e6bbf61 Compare October 1, 2021 10:16

hrantzsch commented Oct 8, 2021

View reviewed changes

src/lib/tls/tls_messages.h Outdated Show resolved Hide resolved

reneme reviewed Oct 11, 2021

View reviewed changes

hrantzsch force-pushed the dev/tls-13-refactoring branch from 775610b to c11a308 Compare October 12, 2021 11:37

reneme reviewed Oct 14, 2021

View reviewed changes

reneme reviewed Oct 22, 2021

View reviewed changes

src/lib/tls/tls12/tls_channel_impl_12.cpp Outdated Show resolved Hide resolved

src/lib/tls/tls12/tls_channel_impl_12.cpp Outdated Show resolved Hide resolved

reneme force-pushed the dev/tls-13-refactoring branch from bf441e8 to 7de394f Compare October 25, 2021 12:18

reneme force-pushed the dev/tls-13 branch from 8c00cca to e43f1ce Compare October 25, 2021 12:19

hrantzsch mentioned this pull request Oct 25, 2021

TLS 1.2 refactoring and TLS 1.3 development started #2806

Closed

TLS 1.2 refactoring and cleanup

05e7e7c

Co-authored-by: René Meusel <rene.meusel@nexenio.com>

hrantzsch force-pushed the dev/tls-13-refactoring branch from 7de394f to 05e7e7c Compare December 8, 2021 12:47

hrantzsch requested a review from reneme December 8, 2021 13:45

reneme approved these changes Dec 8, 2021

View reviewed changes

hrantzsch merged commit 08140e7 into dev/tls-13 Dec 8, 2021

reneme mentioned this pull request Apr 4, 2022

[TLS 1.3] Preparations for addition of TLS 1.3 #2946

Merged

randombit deleted the dev/tls-13-refactoring branch December 1, 2022 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS 1.2 Refactoring #2808

TLS 1.2 Refactoring #2808

hrantzsch commented Oct 1, 2021 •

edited

Loading

reneme commented Oct 11, 2021

reneme left a comment

reneme Oct 11, 2021

reneme Oct 11, 2021

reneme Oct 11, 2021

pist-eb Oct 12, 2021 •

edited

Loading

reneme Oct 11, 2021

pist-eb Oct 12, 2021

pist-eb commented Oct 12, 2021

reneme left a comment

reneme left a comment

reneme commented Oct 22, 2021

pist-eb commented Oct 22, 2021

reneme commented Oct 25, 2021

reneme left a comment

TLS 1.2 Refactoring #2808

TLS 1.2 Refactoring #2808

Conversation

hrantzsch commented Oct 1, 2021 • edited Loading

reneme commented Oct 11, 2021

reneme left a comment

Choose a reason for hiding this comment

reneme Oct 11, 2021

Choose a reason for hiding this comment

reneme Oct 11, 2021

Choose a reason for hiding this comment

reneme Oct 11, 2021

Choose a reason for hiding this comment

pist-eb Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

reneme Oct 11, 2021

Choose a reason for hiding this comment

pist-eb Oct 12, 2021

Choose a reason for hiding this comment

pist-eb commented Oct 12, 2021

reneme left a comment

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

reneme commented Oct 22, 2021

pist-eb commented Oct 22, 2021

reneme commented Oct 25, 2021

reneme left a comment

Choose a reason for hiding this comment

hrantzsch commented Oct 1, 2021 •

edited

Loading

pist-eb Oct 12, 2021 •

edited

Loading