review: low hanging fruits

5 files changed, 30 insertions, 26 deletions

diff --git a/src/lib/tls/asio/asio_async_base.h b/src/lib/tls/asio/asio_async_base.h
index 227f2bf82..6e344ec9b 100644
--- a/src/lib/tls/asio/asio_async_base.h
+++ b/src/lib/tls/asio/asio_async_base.h
@@ -16,7 +16,6 @@
 #include <boost/version.hpp>
 #if BOOST_VERSION >= 106600
 
-#include <boost/asio/coroutine.hpp>
 #include <botan/internal/asio_includes.h>
 
 namespace Botan {
@@ -56,6 +55,10 @@ namespace TLS {
  * as `async_read_some`, with and empty buffer, set the object itself as the handler, and `yield`. As a result, the call
  * operator will be invoked again, this time as a continuation, and will jump to the location where it yielded before
  * using `reenter`. It is now safe to call the handler function via `complete_now`.
+ *
+ * \tparam Handler Type of the completion handler
+ * \tparam Executor1 Type of the asio executor (usually derived from the lower layer)
+ * \tparam Allocator Type of the allocator to be used
  */
 template <class Handler, class Executor1, class Allocator>
 struct AsyncBase : boost::asio::coroutine
diff --git a/src/lib/tls/asio/asio_async_handshake_op.h b/src/lib/tls/asio/asio_async_handshake_op.h
index c23e654ff..f3949a404 100644
--- a/src/lib/tls/asio/asio_async_handshake_op.h
+++ b/src/lib/tls/asio/asio_async_handshake_op.h
@@ -59,9 +59,9 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
          {
          reenter(this)
             {
-            // Provide encrypted TLS data received from the network to TLS::Channel for decryption
             if(bytesTransferred > 0 && !ec)
                {
+               // Provide encrypted TLS data received from the network to TLS::Channel for decryption
                boost::asio::const_buffer read_buffer {m_core.input_buffer.data(), bytesTransferred};
                try
                   {
@@ -73,9 +73,10 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
                   }
                }
 
-            // Write encrypted TLS data provided by the TLS::Channel on the wire
             if(m_core.hasDataToSend() && !ec)
                {
+               // Write encrypted TLS data provided by the TLS::Channel on the wire
+
                // Note: we construct `AsyncWriteOperation` with 0 as its last parameter (`plainBytesTransferred`).
                // This operation will eventually call `*this` as its own handler, passing the 0 back to this call
                // operator. This is necessary because the check of `bytesTransferred > 0` assumes that
@@ -88,9 +89,9 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
                return;
                }
 
-            // Read more encrypted TLS data from the network
             if(!m_stream.native_handle()->is_active() && !ec)
                {
+               // Read more encrypted TLS data from the network
                m_stream.next_layer().async_read_some(m_core.input_buffer, std::move(*this));
                return;
                }
diff --git a/src/lib/tls/asio/asio_includes.h b/src/lib/tls/asio/asio_includes.h
index 63beccad5..494233d55 100644
--- a/src/lib/tls/asio/asio_includes.h
+++ b/src/lib/tls/asio/asio_includes.h
@@ -18,6 +18,7 @@
    #define BOOST_ASIO_DISABLE_SERIAL_PORT
    #include <boost/asio.hpp>
    #include <boost/asio/buffer.hpp>
+   #include <boost/asio/coroutine.hpp>
    #include <boost/asio/ip/tcp.hpp>
 
 #endif // BOTAN_HAS_BOOST_ASIO
diff --git a/src/lib/tls/asio/asio_stream.h b/src/lib/tls/asio/asio_stream.h
index 6d69d5392..4a30ea421 100644
--- a/src/lib/tls/asio/asio_stream.h
+++ b/src/lib/tls/asio/asio_stream.h
@@ -13,6 +13,7 @@
 
 #if defined(BOTAN_HAS_TLS) && defined(BOTAN_HAS_BOOST_ASIO)
 
+// first version to be compatible with Networking TS (N4656) and boost::beast
 #include <boost/version.hpp>
 #if BOOST_VERSION >= 106600
 
@@ -30,14 +31,6 @@
 #include <thread>
 #include <type_traits>
 
-namespace boost {
-namespace asio {
-namespace ssl {
-class context;
-}
-}
-}
-
 namespace Botan {
 
 namespace TLS {
@@ -95,8 +88,7 @@ class Stream : public StreamBase<Channel>
       /**
        * @throws Not_Implemented
        */
-      template<
-         typename VerifyCallback>
+      template<typename VerifyCallback>
       void set_verify_callback(VerifyCallback callback)
          {
          BOTAN_UNUSED(callback);
@@ -107,8 +99,7 @@ class Stream : public StreamBase<Channel>
        * Not Implemented.
        * @param ec Will be set to `Botan::TLS::error::not_implemented`
        */
-      template<
-         typename VerifyCallback>
+      template<typename VerifyCallback>
       void set_verify_callback(VerifyCallback callback,
                                boost::system::error_code& ec)
          {
@@ -183,7 +174,7 @@ class Stream : public StreamBase<Channel>
          {
          while(!native_handle()->is_active())
             {
-            writePendingTlsData(ec);
+            sendPendingEncryptedData(ec);
             if(ec)
                { return; }
 
@@ -207,7 +198,7 @@ class Stream : public StreamBase<Channel>
                return;
                }
 
-            writePendingTlsData(ec);
+            sendPendingEncryptedData(ec);
             }
          }
 
@@ -344,7 +335,7 @@ class Stream : public StreamBase<Channel>
             ec = Botan::TLS::convertException();
             return;
             }
-         writePendingTlsData(ec);
+         sendPendingEncryptedData(ec);
          }
 
       /**
@@ -371,6 +362,8 @@ class Stream : public StreamBase<Channel>
          BOOST_ASIO_HANDSHAKE_HANDLER_CHECK(ShutdownHandler, handler) type_check;
          BOTAN_UNUSED(handler);
          throw Not_Implemented("async shutdown is not implemented");
+         // TODO: Implement a subclass of AsyncBase that calls native_handle()->close() and writes pending data from
+         // the core to the network, e.g. using AsyncWriteOperation.
          }
 
       //
@@ -391,7 +384,7 @@ class Stream : public StreamBase<Channel>
          if(this->m_core.hasReceivedData())
             { return this->m_core.copyReceivedData(buffers); }
 
-         tls_decrypt_some(ec);
+         tls_receive_some(ec);
          if(ec)
             { return 0; }
 
@@ -401,6 +394,7 @@ class Stream : public StreamBase<Channel>
       /**
        * Read some data from the stream. The function call will block until one or more bytes of data has
        * been read successfully, or until an error occurs.
+       *
        * @param buffers The buffers into which the data will be read.
        * @return The number of bytes read. Returns 0 if an error occurred.
        * @throws boost::system::system_error if error occured
@@ -417,6 +411,7 @@ class Stream : public StreamBase<Channel>
       /**
        * Write some data to the stream. The function call will block until one or more bytes of data has been written
        * successfully, or until an error occurs.
+       *
        * @param buffers The data to be written.
        * @param ec Set to indicate what error occurred, if any.
        * @return The number of bytes written.
@@ -429,7 +424,7 @@ class Stream : public StreamBase<Channel>
          if(ec)
             { return 0; }
 
-         writePendingTlsData(ec);
+         sendPendingEncryptedData(ec);
          if(ec)
             { return 0; }
 
@@ -513,7 +508,7 @@ class Stream : public StreamBase<Channel>
          }
 
    protected:
-      size_t writePendingTlsData(boost::system::error_code& ec)
+      size_t sendPendingEncryptedData(boost::system::error_code& ec)
          {
          auto writtenBytes = boost::asio::write(m_nextLayer, this->m_core.sendBuffer(), ec);
 
@@ -521,7 +516,7 @@ class Stream : public StreamBase<Channel>
          return writtenBytes;
          }
 
-      void tls_decrypt_some(boost::system::error_code& ec)
+      void tls_receive_some(boost::system::error_code& ec)
          {
          boost::asio::const_buffer read_buffer =
             {
diff --git a/src/lib/tls/asio/asio_stream_core.h b/src/lib/tls/asio/asio_stream_core.h
index c5cba8a1a..e15632152 100644
--- a/src/lib/tls/asio/asio_stream_core.h
+++ b/src/lib/tls/asio/asio_stream_core.h
@@ -19,6 +19,7 @@
 #include <boost/beast/core/flat_buffer.hpp>
 #include <botan/internal/asio_includes.h>
 #include <botan/tls_callbacks.h>
+#include <botan/tls_magic.h>
 #include <mutex>
 #include <vector>
 
@@ -32,21 +33,23 @@ namespace TLS {
 struct StreamCore : public Botan::TLS::Callbacks
    {
       StreamCore()
-         : m_input_buffer_space(17 * 1024, '\0'), // enough for a TLS Datagram
+         : m_input_buffer_space(MAX_CIPHERTEXT_SIZE, '\0'),
            input_buffer(m_input_buffer_space.data(), m_input_buffer_space.size()) {}
 
       virtual ~StreamCore() = default;
 
       void tls_emit_data(const uint8_t data[], std::size_t size) override
          {
+         // Provide the encrypted TLS data in the sendBuffer. Actually sending the data is done
+         // using (async_)write_some either in the stream or in an async operation.
          m_send_buffer.commit(
             boost::asio::buffer_copy(m_send_buffer.prepare(size), boost::asio::buffer(data, size)));
          }
 
       void tls_record_received(uint64_t, const uint8_t data[], std::size_t size) override
          {
-         // TODO: It would be nice to avoid this buffer copy. However, we need to deal with the case that the receive
-         // buffer provided by the caller is smaller than the decrypted record.
+         // TODO: It would be nice to avoid this buffer copy. However, we need to deal with the case
+         // that the receive buffer provided by the caller is smaller than the decrypted record.
          auto buffer = m_receive_buffer.prepare(size);
          auto copySize =
             boost::asio::buffer_copy(buffer, boost::asio::const_buffer(data, size));
@@ -68,6 +71,7 @@ struct StreamCore : public Botan::TLS::Callbacks
 
       bool tls_session_established(const Botan::TLS::Session&) override
          {
+         // TODO: it should be possible to configure this in the using application (via callback?)
          return true;
          }