FIX: review comments

src/lib/tls/msg_server_hello.cpp

@@ -787,7 +787,8 @@ Server_Hello_13::Server_Hello_13(const Client_Hello_13& ch,
       const auto psk_modes = ch_exts.get<PSK_Key_Exchange_Modes>();
       BOTAN_ASSERT_NONNULL(psk_modes);
 
-      // TODO: also support non-DHE PSK Key-Exchange mode
+      // TODO: also support PSK_Key_Exchange_Mode::PSK_KE
+      //       (PSK-based handshake without an additional ephemeral key exchange)
       if(value_exists(psk_modes->modes(), PSK_Key_Exchange_Mode::PSK_DHE_KE))
          {
          if(auto server_psk = ch_exts.get<PSK>()->select_offered_psk(cs.value(), session_mgr, cb, policy))

src/lib/tls/tls12/tls_server_impl_12.cpp

@@ -485,7 +485,6 @@ void Server_Impl_12::process_client_hello_msg(const Handshake_State* active_stat
                        policy(),
                        pending_state.client_hello());
 
-
    m_next_protocol = "";
    if(pending_state.client_hello()->supports_alpn())
       {

src/lib/tls/tls13/tls_channel_impl_13.h

@@ -25,9 +25,12 @@ class Channel_Impl_13 : public Channel_Impl
    {
    protected:
       /**
-       * Helper class to coalesce handshake messages into a single TLS record of type
-       * 'Handshake'. This is used entirely internally in the Channel, Client and Server
-       * implementation.
+       * Helper class to coalesce handshake messages into a single TLS record
+       * of type 'Handshake'. This is used entirely internally in the Channel,
+       * Client and Server implementations.
+       *
+       * Note that implementations should use the derived classes that either
+       * aggregate conventional Handshake messages or Post-Handshake messages.
        */
       class AggregatedMessages
          {
@@ -59,6 +62,11 @@ class Channel_Impl_13 : public Channel_Impl
             Handshake_Layer& m_handshake_layer;
          };
 
+      /**
+       * Aggregate conventional handshake messages. This will update the given
+       * Transcript_Hash_State accordingly as individual messages are added to
+       * the aggregation.
+       */
       class AggregatedHandshakeMessages : public AggregatedMessages
          {
          public:
@@ -77,6 +85,10 @@ class Channel_Impl_13 : public Channel_Impl
             Transcript_Hash_State& m_transcript_hash;
          };
 
+      /**
+       * Aggregate post-handshake messages. In contrast to ordinary handshake
+       * messages this does not maintain a Transcript_Hash_State.
+       */
       class AggregatedPostHandshakeMessages : public AggregatedMessages
          {
          public:

src/lib/tls/tls13/tls_client_impl_13.cpp

@@ -130,6 +130,21 @@ bool Client_Impl_13::handshake_finished() const
 
 std::optional<Session_with_Handle> Client_Impl_13::find_session_for_resumption()
    {
+   // RFC 8446 4.6.1
+   //    Clients MUST only resume if the new SNI value is valid for the
+   //    server certificate presented in the original session and SHOULD only
+   //    resume if the SNI value matches the one used in the original session.
+   //
+   // Below we search sessions based on their SNI value. Assuming that a
+   // 3rd party session manager does not lie to the implementation, we don't
+   // explicitly re-check that the SNI values match.
+   //
+   // Also, the default implementation did verify that very same SNI information
+   // against the server's certificate (via Callbacks::tls_verify_cert_chain())
+   // before storing it in the session.
+   //
+   // We therefore assume that the session returned by the `Session_Manager` is
+   // suitable for resumption in this context.
    auto sessions = session_manager().find(m_info, callbacks(), policy());
    if(sessions.empty())
       { return std::nullopt; }
@@ -139,27 +154,6 @@ std::optional<Session_with_Handle> Client_Impl_13::find_session_for_resumption()
    //       does imply otherwise with its API, though.
    auto& session_to_resume = sessions.front();
 
-   // RFC 8446 4.6.1
-   //    Clients MUST only resume if the new SNI value is valid for the
-   //    server certificate presented in the original session and SHOULD only
-   //    resume if the SNI value matches the one used in the original session.
-   //
-   // ... i.e. we do not attempt a resumption if the session's certificate does
-   // not withstand basic validity checks or -- if no server certificate is
-   // available -- at least the session's host name matches the expectations.
-   const auto& cert_chain = session_to_resume.session.peer_certs();
-   if(!cert_chain.empty())
-      {
-      const auto& server_cert = cert_chain.front();
-      if(!server_cert.is_self_signed() &&
-         (!server_cert.matches_dns_name(m_info.hostname()) ||
-          X509_Time(callbacks().tls_current_timestamp()) > server_cert.not_after()))
-         { return std::nullopt; }
-      }
-   else if(!session_to_resume.session.server_info().empty() &&
-           session_to_resume.session.server_info().hostname() != m_info.hostname())
-      { return std::nullopt; }
-
    return std::move(session_to_resume);
    }
 

src/lib/tls/tls_extensions.h

@@ -670,6 +670,16 @@ class BOTAN_UNSTABLE_API PSK final : public Extension
 
       PSK(TLS_Data_Reader& reader, uint16_t extension_size, Handshake_Type message_type);
 
+      /**
+       * Creates a PSK extension with a TLS 1.3 session object containing a
+       * master_secret. Note that it will extract that secret from the session,
+       * and won't create a copy of it.
+       *
+       * @param session_to_resume  the session to be resumed; note that the
+       *                           master secret will be taken away from the
+       *                           session object.
+       * @param callbacks          the application's callbacks
+       */
       PSK(Session_with_Handle& session_to_resume, Callbacks& callbacks);
 
       ~PSK();

src/lib/tls/tls_messages.h

@@ -17,6 +17,7 @@
 #include <variant>
 #include <chrono>
 
+#include <botan/strong_type.h>
 #include <botan/tls_extensions.h>
 #include <botan/tls_handshake_msg.h>
 #include <botan/tls_session.h>
@@ -936,8 +937,6 @@ class BOTAN_UNSTABLE_API New_Session_Ticket_12 final : public Handshake_Message
 
 #if defined(BOTAN_HAS_TLS_13)
 
-#include <botan/strong_type.h>
-
 /// @brief Used to derive the ticket's PSK from the resumption_master_secret
 using Ticket_Nonce = Strong<std::vector<uint8_t>, struct Ticket_Nonce_>;
 

src/lib/tls/tls_session_manager.h

@@ -245,6 +245,12 @@ class BOTAN_PUBLIC_API(3, 0) Session_Manager
        * Applications that wish to implement their own Session_Manager will
        * have to provide an implementation for it.
        *
+       * Note that the TLS client implementations do not perform any checks on
+       * the validity of the session for a given @p info. Particularly, it is
+       * the Session_Manager's responsibility to ensure the restrictions posed
+       * in RFC 8446 4.6.1 regarding server certificate validity for the given
+       * @p info.
+       *
        * This is called for TLS clients only.
        *
        * @param info the information about the server