Fix large content error codes

When pushing multi-part payloads, it's quite helpful if the server
supports the header field of "Expect: 100-Continue".  What this does, is
on a large file push, allows the server to possibly reject a request
before the payload is actually sent, thereby saving bandwidth, and
giving the user more information.

Bmcweb, since commit 3909dc82a003893812f598434d6c4558107afa28 by James
(merged July 2020) has simply closed the connection if a user attempts
to send too much data, thereby making the bmcweb implementation simpler.

Unfortunately, to a security tester, this has the appearance on the
network as a crash, which will likely then get filed as a "verify this
isn't failing" bug.

In addition, the default args on curl multipart upload enable the
Expect: 100-Continue behavior, so folks testing must've just been
disabling that behavior.

Bmcweb should just support the right thing here.  Unfortunately, closing
a connection uncleanly is easy.  Closing a connection cleanly is
difficult.  This requires a pretty large refactor of the http connection
class to accomplish.

Tested:
Create files of various size and try to send them (Note, default body
limit is 30 MB) and upload them with an without a username.

```
dd if=/dev/zero of=zeros-file bs=1048576 count=16 of=16mb.txt

curl -k --location POST https://192.168.7.2/redfish/v1/UpdateService/update -F 'UpdateParameters={"Targets":["/redfish/v1/Managers/bmc"]} ;type=application/json'  -F UpdateFile=@32mb.txt -v
```

No Username:
32MB returns < HTTP/1.1 413 Payload Too Large
16MB returns < HTTP/1.1 401 Unauthorized

With Username
32MB returns < HTTP/1.1 413 Payload Too Large
16MB returns < HTTP/1.1 400 Bad Request

Note, in all cases except the last one, the payload is never sent from
curl.

Redfish protocol validator fails no new tests (SSE failure still
present).

Redfish service validator passes.

Change-Id: I72bc8bbc49a05555c31dc7209292f846ec411d43
Signed-off-by: Ed Tanous <ed@tanous.net>

1 files changed, 203 insertions, 88 deletions

diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index bd8cf3190e..d0aa5d5be4 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -42,9 +42,9 @@ static int connectionCount = 0;
 constexpr uint64_t httpReqBodyLimit = 1024UL * 1024UL *
                                       bmcwebHttpReqBodyLimitMb;
 
-constexpr uint64_t loggedOutPostBodyLimit = 4096;
+constexpr uint64_t loggedOutPostBodyLimit = 4096U;
 
-constexpr uint32_t httpHeaderLimit = 8192;
+constexpr uint32_t httpHeaderLimit = 8192U;
 
 template <typename>
 struct IsTls : std::false_type
@@ -68,9 +68,7 @@ class Connection :
         handler(handlerIn), timer(std::move(timerIn)),
         getCachedDateStr(getCachedDateStrF)
     {
-        parser.emplace(std::piecewise_construct, std::make_tuple());
-        parser->body_limit(httpReqBodyLimit);
-        parser->header_limit(httpHeaderLimit);
+        initParser();
 
 #ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
         prepareMutualTls();
@@ -78,13 +76,13 @@ class Connection :
 
         connectionCount++;
 
-        BMCWEB_LOG_DEBUG("{} Connection open, total {}", logPtr(this),
+        BMCWEB_LOG_DEBUG("{} Connection created, total {}", logPtr(this),
                          connectionCount);
     }
 
     ~Connection()
     {
-        res.setCompleteRequestHandler(nullptr);
+        res.releaseCompleteRequestHandler();
         cancelDeadlineTimer();
 
         connectionCount--;
@@ -104,13 +102,7 @@ class Connection :
         // don't require auth
         if (preverified)
         {
-            boost::asio::ip::address ipAddress;
-            if (getClientIp(ipAddress))
-            {
-                return true;
-            }
-
-            mtlsSession = verifyMtlsUser(ipAddress, ctx);
+            mtlsSession = verifyMtlsUser(ip, ctx);
             if (mtlsSession)
             {
                 BMCWEB_LOG_DEBUG("{} Generating TLS session: {}", logPtr(this),
@@ -159,15 +151,19 @@ class Connection :
 
     void start()
     {
+        BMCWEB_LOG_DEBUG("{} Connection started, total {}", logPtr(this),
+                         connectionCount);
         if (connectionCount >= 200)
         {
-            BMCWEB_LOG_CRITICAL("{}Max connection count exceeded.",
+            BMCWEB_LOG_CRITICAL("{} Max connection count exceeded.",
                                 logPtr(this));
             return;
         }
 
         startDeadline();
 
+        readClientIp();
+
         // TODO(ed) Abstract this to a more clever class with the idea of an
         // asynchronous "start"
         if constexpr (IsTls<Adaptor>::value)
@@ -216,6 +212,19 @@ class Connection :
         doReadHeaders();
     }
 
+    void initParser()
+    {
+        boost::beast::http::request_parser<bmcweb::HttpBody>& instance =
+            parser.emplace(std::piecewise_construct, std::make_tuple());
+
+        // reset header limit for newly created parser
+        instance.header_limit(httpHeaderLimit);
+
+        // Initially set no body limit. We don't yet know if the user is
+        // authenticated.
+        instance.body_limit(boost::none);
+    }
+
     void handle()
     {
         std::error_code reqEc;
@@ -234,7 +243,7 @@ class Connection :
         req.session = userSession;
 
         // Fetch the client IP address
-        readClientIp();
+        req.ipAddress = ip;
 
         // Check for HTTP version 1.1.
         if (req.version() == 11)
@@ -319,22 +328,41 @@ class Connection :
         handler->handle(req, asyncResp);
     }
 
-    void close()
+    void hardClose()
     {
+        BMCWEB_LOG_DEBUG("{} Closing socket", logPtr(this));
+        boost::beast::get_lowest_layer(adaptor).close();
+    }
+
+    void tlsShutdownComplete(const std::shared_ptr<self_type>& self,
+                             const boost::system::error_code& ec)
+    {
+        if (ec)
+        {
+            BMCWEB_LOG_WARNING("{} Failed to shut down TLS cleanly {}",
+                               logPtr(self.get()), ec);
+        }
+        self->hardClose();
+    }
+
+    void gracefulClose()
+    {
+        BMCWEB_LOG_DEBUG("{} Socket close requested", logPtr(this));
+        if (mtlsSession != nullptr)
+        {
+            BMCWEB_LOG_DEBUG("{} Removing TLS session: {}", logPtr(this),
+                             mtlsSession->uniqueId);
+            persistent_data::SessionStore::getInstance().removeSession(
+                mtlsSession);
+        }
         if constexpr (IsTls<Adaptor>::value)
         {
-            adaptor.next_layer().close();
-            if (mtlsSession != nullptr)
-            {
-                BMCWEB_LOG_DEBUG("{} Removing TLS session: {}", logPtr(this),
-                                 mtlsSession->uniqueId);
-                persistent_data::SessionStore::getInstance().removeSession(
-                    mtlsSession);
-            }
+            adaptor.async_shutdown(std::bind_front(
+                &self_type::tlsShutdownComplete, this, shared_from_this()));
         }
         else
         {
-            adaptor.close();
+            hardClose();
         }
     }
 
@@ -355,19 +383,7 @@ class Connection :
 
     void readClientIp()
     {
-        boost::asio::ip::address ip;
-        boost::system::error_code ec = getClientIp(ip);
-        if (ec)
-        {
-            return;
-        }
-        req.ipAddress = ip;
-    }
-
-    boost::system::error_code getClientIp(boost::asio::ip::address& ip)
-    {
         boost::system::error_code ec;
-        BMCWEB_LOG_DEBUG("Fetch the client IP address");
 
         if constexpr (!std::is_same_v<Adaptor, boost::beast::test::stream>)
         {
@@ -380,19 +396,83 @@ class Connection :
                 // will be empty.
                 BMCWEB_LOG_ERROR(
                     "Failed to get the client's IP Address. ec : {}", ec);
-                return ec;
+                return;
             }
             ip = endpoint.address();
         }
-        return ec;
     }
 
   private:
+    uint64_t getContentLengthLimit()
+    {
+#ifndef BMCWEB_INSECURE_DISABLE_AUTHX
+        if (userSession == nullptr)
+        {
+            return loggedOutPostBodyLimit;
+        }
+#endif
+
+        return httpReqBodyLimit;
+    }
+
+    // Returns true if content length was within limits
+    // Returns false if content length error has been returned
+    bool handleContentLengthError()
+    {
+        if (!parser)
+        {
+            BMCWEB_LOG_CRITICAL("Paser was null");
+            return false;
+        }
+        const boost::optional<uint64_t> contentLength =
+            parser->content_length();
+        if (!contentLength)
+        {
+            BMCWEB_LOG_DEBUG("{} No content length available", logPtr(this));
+            return true;
+        }
+
+        uint64_t maxAllowedContentLength = getContentLengthLimit();
+
+        if (*contentLength > maxAllowedContentLength)
+        {
+            // If the users content limit is between the logged in
+            // and logged out limits They probably just didn't log
+            // in
+            if (*contentLength > loggedOutPostBodyLimit &&
+                *contentLength < httpReqBodyLimit)
+            {
+                BMCWEB_LOG_DEBUG(
+                    "{} Content length {} valid, but greater than logged out"
+                    " limit of {}. Setting unauthorized",
+                    logPtr(this), *contentLength, loggedOutPostBodyLimit);
+                res.result(boost::beast::http::status::unauthorized);
+            }
+            else
+            {
+                // Otherwise they're over both limits, so inform
+                // them
+                BMCWEB_LOG_DEBUG(
+                    "{} Content length {} was greater than global limit {}."
+                    " Setting payload too large",
+                    logPtr(this), *contentLength, httpReqBodyLimit);
+                res.result(boost::beast::http::status::payload_too_large);
+            }
+
+            keepAlive = false;
+            doWrite();
+            return false;
+        }
+
+        return true;
+    }
+
     void doReadHeaders()
     {
         BMCWEB_LOG_DEBUG("{} doReadHeaders", logPtr(this));
         if (!parser)
         {
+            BMCWEB_LOG_CRITICAL("Parser was not initialized.");
             return;
         }
         // Clean up any previous Connection.
@@ -408,54 +488,57 @@ class Connection :
             {
                 cancelDeadlineTimer();
 
-                if (ec == boost::beast::http::error::end_of_stream)
+                if (ec == boost::beast::http::error::header_limit)
                 {
-                    BMCWEB_LOG_WARNING("{} Error while reading: {}",
-                                       logPtr(this), ec.message());
+                    BMCWEB_LOG_ERROR("{} Header field too large, closing",
+                                     logPtr(this), ec.message());
+
+                    res.result(boost::beast::http::status::
+                                   request_header_fields_too_large);
+                    keepAlive = false;
+                    doWrite();
+                    return;
                 }
-                else
+                if (ec == boost::beast::http::error::end_of_stream)
                 {
-                    BMCWEB_LOG_ERROR("{} Error while reading: {}", logPtr(this),
-                                     ec.message());
+                    BMCWEB_LOG_WARNING("{} End of stream, closing {}",
+                                       logPtr(this), ec);
+                    hardClose();
+                    return;
                 }
-                close();
-                BMCWEB_LOG_DEBUG("{} from read(1)", logPtr(this));
+
+                BMCWEB_LOG_DEBUG("{} Closing socket due to read error {}",
+                                 logPtr(this), ec.message());
+                gracefulClose();
+
                 return;
             }
 
-            readClientIp();
-
-            boost::asio::ip::address ip;
-            if (getClientIp(ip))
+            std::string_view expect =
+                req.getHeaderValue(boost::beast::http::field::expect);
+            if (bmcweb::asciiIEquals(expect, "100-continue"))
             {
-                BMCWEB_LOG_DEBUG("Unable to get client IP");
+                res.result(boost::beast::http::status::continue_);
+                doWrite();
+                return;
             }
+
             if constexpr (!std::is_same_v<Adaptor, boost::beast::test::stream>)
             {
 #ifndef BMCWEB_INSECURE_DISABLE_AUTHX
                 boost::beast::http::verb method = parser->get().method();
                 userSession = crow::authentication::authenticate(
                     ip, res, method, parser->get().base(), mtlsSession);
-
-                bool loggedIn = userSession != nullptr;
-                if (!loggedIn)
-                {
-                    const boost::optional<uint64_t> contentLength =
-                        parser->content_length();
-                    if (contentLength &&
-                        *contentLength > loggedOutPostBodyLimit)
-                    {
-                        BMCWEB_LOG_DEBUG("Content length greater than limit {}",
-                                         *contentLength);
-                        close();
-                        return;
-                    }
-
-                    BMCWEB_LOG_DEBUG("Starting quick deadline");
-                }
 #endif // BMCWEB_INSECURE_DISABLE_AUTHX
             }
 
+            if (!handleContentLengthError())
+            {
+                return;
+            }
+
+            parser->body_limit(getContentLengthLimit());
+
             if (parser->is_done())
             {
                 handle();
@@ -486,8 +569,22 @@ class Connection :
             {
                 BMCWEB_LOG_ERROR("{} Error while reading: {}", logPtr(this),
                                  ec.message());
-                close();
-                BMCWEB_LOG_DEBUG("{} from read(1)", logPtr(this));
+                if (ec == boost::beast::http::error::body_limit)
+                {
+                    if (handleContentLengthError())
+                    {
+                        BMCWEB_LOG_CRITICAL("Body length limit reached, "
+                                            "but no content-length "
+                                            "available?  Should never happen");
+                        res.result(
+                            boost::beast::http::status::internal_server_error);
+                        keepAlive = false;
+                        doWrite();
+                    }
+                    return;
+                }
+
+                gracefulClose();
                 return;
             }
 
@@ -513,8 +610,8 @@ class Connection :
                       const boost::system::error_code& ec,
                       std::size_t bytesTransferred)
     {
-        BMCWEB_LOG_DEBUG("{} async_write {} bytes", logPtr(this),
-                         bytesTransferred);
+        BMCWEB_LOG_DEBUG("{} async_write wrote {} bytes, ec=", logPtr(this),
+                         bytesTransferred, ec);
 
         cancelDeadlineTimer();
 
@@ -523,19 +620,27 @@ class Connection :
             BMCWEB_LOG_DEBUG("{} from write(2)", logPtr(this));
             return;
         }
+
+        if (res.result() == boost::beast::http::status::continue_)
+        {
+            // Reset the result to ok
+            res.result(boost::beast::http::status::ok);
+            doRead();
+            return;
+        }
+
         if (!keepAlive)
         {
-            close();
-            BMCWEB_LOG_DEBUG("{} from write(1)", logPtr(this));
+            BMCWEB_LOG_DEBUG("{} keepalive not set.  Closing socket",
+                             logPtr(this));
+
+            gracefulClose();
             return;
         }
 
         BMCWEB_LOG_DEBUG("{} Clearing response", logPtr(this));
         res.clear();
-        parser.emplace(std::piecewise_construct, std::make_tuple());
-        parser->body_limit(httpReqBodyLimit); // reset body limit for
-                                              // newly created parser
-        buffer.consume(buffer.size());
+        initParser();
 
         userSession = nullptr;
 
@@ -581,6 +686,13 @@ class Connection :
                 weakSelf.lock();
             if (!self)
             {
+                if (ec == boost::asio::error::operation_aborted)
+                {
+                    BMCWEB_LOG_DEBUG(
+                        "{} Timer canceled on connection being destroyed",
+                        logPtr(self.get()));
+                    return;
+                }
                 BMCWEB_LOG_CRITICAL("{} Failed to capture connection",
                                     logPtr(self.get()));
                 return;
@@ -588,21 +700,21 @@ class Connection :
 
             self->timerStarted = false;
 
-            if (ec == boost::asio::error::operation_aborted)
-            {
-                // Canceled wait means the path succeeded.
-                return;
-            }
             if (ec)
             {
-                BMCWEB_LOG_CRITICAL("{} timer failed {}", logPtr(self.get()),
+                if (ec == boost::asio::error::operation_aborted)
+                {
+                    BMCWEB_LOG_DEBUG("{} Timer canceled", logPtr(self.get()));
+                    return;
+                }
+                BMCWEB_LOG_CRITICAL("{} Timer failed {}", logPtr(self.get()),
                                     ec);
             }
 
-            BMCWEB_LOG_WARNING("{}Connection timed out, closing",
+            BMCWEB_LOG_WARNING("{} Connection timed out, hard closing",
                                logPtr(self.get()));
 
-            self->close();
+            self->hardClose();
         });
 
         timerStarted = true;
@@ -611,6 +723,9 @@ class Connection :
 
     Adaptor adaptor;
     Handler* handler;
+
+    boost::asio::ip::address ip;
+
     // Making this a std::optional allows it to be efficiently destroyed and
     // re-created on Connection reset
     std::optional<boost::beast::http::request_parser<bmcweb::HttpBody>> parser;