Change ownership of boost::req to crow::req

req is being created later, in the connection life cycle. req was
holding many important values when it was passed to authenticate, so the
authenticate call had to be refactored to includes all the data req was
holding.

Also uses of req before handle have been changed to direct calls to
boot::parse

Tested:
Made a request that did not require authentication
$ curl -vvvv --insecure "https://192.168.7.2:18080/redfish/v1"
Got correct service root

Made a unauthenticated request (Chassis)
$ curl -c cjar -b cjar -k -H "Content-Type: application/json" -X GET https://192.168.7.2:18080/redfish/v1/Chassis
Unauthenticated

Made a log-in request
$ curl -c cjar -b cjar -k -H "Content-Type: application/json" -X POST https://192.168.7.2:18080/login -d "{\"data\": [ \"root\", \"0penBmc\" ] }"

Made (same) Chassis request
$ curl -c cjar -b cjar -k -H "Content-Type: application/json" -X GET https://192.168.7.2:18080/redfish/v1/Chassis

Tested the websockets using scripts/websocket_test.py
Websockets continued to work after this change.

Followed the mTLS instructions here https://github.com/openbmc/docs/blob/master/security/TLS-configuration.md
mTLS continues to work after this change.

Change-Id: I78f78063be0331be00b66349d5d184847add1708
Signed-off-by: John Edward Broadbent <jebr@google.com>

5 files changed, 144 insertions, 121 deletions

diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index a1a7045c03..a4723f121a 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -69,7 +69,6 @@ class Connection :
         parser.emplace(std::piecewise_construct, std::make_tuple());
         parser->body_limit(httpReqBodyLimit);
         parser->header_limit(httpHeaderLimit);
-        req.emplace(parser->get());
 
 #ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
         prepareMutualTls();
@@ -263,15 +262,16 @@ class Connection :
             }
             sslUser.resize(lastChar);
             std::string unsupportedClientId = "";
-            session = persistent_data::SessionStore::getInstance()
-                          .generateUserSession(
-                              sslUser, req->ipAddress.to_string(),
-                              unsupportedClientId,
-                              persistent_data::PersistenceType::TIMEOUT);
-            if (auto sp = session.lock())
+            userSession = persistent_data::SessionStore::getInstance()
+                              .generateUserSession(
+                                  sslUser, req->ipAddress.to_string(),
+                                  unsupportedClientId,
+                                  persistent_data::PersistenceType::TIMEOUT);
+            if (userSession != nullptr)
             {
-                BMCWEB_LOG_DEBUG << this
-                                 << " Generating TLS session: " << sp->uniqueId;
+                BMCWEB_LOG_DEBUG
+                    << this
+                    << " Generating TLS session: " << userSession->uniqueId;
             }
             return true;
         });
@@ -319,9 +319,9 @@ class Connection :
         bool isInvalidRequest = false;
 
         // Check for HTTP version 1.1.
-        if (req->version() == 11)
+        if (parser->get().version() == 11)
         {
-            if (req->getHeaderValue(boost::beast::http::field::host).empty())
+            if (parser->get()[boost::beast::http::field::host].empty())
             {
                 isInvalidRequest = true;
                 res.result(boost::beast::http::status::bad_request);
@@ -329,9 +329,23 @@ class Connection :
         }
 
         BMCWEB_LOG_INFO << "Request: "
-                        << " " << this << " HTTP/" << req->version() / 10 << "."
-                        << req->version() % 10 << ' ' << req->methodString()
-                        << " " << req->target() << " " << req->ipAddress;
+                        << " " << this << " HTTP/"
+                        << parser->get().version() / 10 << "."
+                        << parser->get().version() % 10 << ' '
+                        << parser->get().method_string() << " "
+                        << parser->get().target() << " " << req->ipAddress;
+        req.emplace(parser->release());
+        req->session = userSession;
+        try
+        {
+            // causes life time issue
+            req->urlView = boost::urls::url_view(req->target());
+            req->url = req->urlView.encoded_path();
+        }
+        catch (std::exception& p)
+        {
+            BMCWEB_LOG_ERROR << p.what();
+        }
 
         needToCallAfterHandlers = false;
 
@@ -397,11 +411,13 @@ class Connection :
         {
             adaptor.next_layer().close();
 #ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
-            if (auto sp = session.lock())
+            if (userSession != nullptr)
             {
-                BMCWEB_LOG_DEBUG << this
-                                 << " Removing TLS session: " << sp->uniqueId;
-                persistent_data::SessionStore::getInstance().removeSession(sp);
+                BMCWEB_LOG_DEBUG
+                    << this
+                    << " Removing TLS session: " << userSession->uniqueId;
+                persistent_data::SessionStore::getInstance().removeSession(
+                    userSession);
             }
 #endif // BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
         }
@@ -437,7 +453,7 @@ class Connection :
         }
         if (res.body().empty() && !res.jsonValue.empty())
         {
-            if (http_helpers::requestPrefersHtml(*req))
+            if (http_helpers::requestPrefersHtml(req->getHeaderValue("Accept")))
             {
                 prettyPrintJson(res);
             }
@@ -477,6 +493,17 @@ 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";
         boost::asio::ip::tcp::endpoint endpoint =
@@ -488,11 +515,10 @@ class Connection :
             // will be empty.
             BMCWEB_LOG_ERROR << "Failed to get the client's IP Address. ec : "
                              << ec;
+            return ec;
         }
-        else
-        {
-            req->ipAddress = endpoint.address();
-        }
+        ip = endpoint.address();
+        return ec;
     }
 
   private:
@@ -519,7 +545,8 @@ class Connection :
                 {
                     // if the adaptor isn't open anymore, and wasn't handed to a
                     // websocket, treat as an error
-                    if (!isAlive() && !req->isUpgrade())
+                    if (!isAlive() &&
+                        !boost::beast::websocket::is_upgrade(parser->get()))
                     {
                         errorWhileReading = true;
                     }
@@ -534,24 +561,12 @@ class Connection :
                     return;
                 }
 
-                if (!req)
-                {
-                    close();
-                    return;
-                }
-
-                // Note, despite the bmcweb coding policy on use of exceptions
-                // for error handling, this one particular use of exceptions is
-                // deemed acceptible, as it solved a significant error handling
-                // problem that resulted in seg faults, the exact thing that the
-                // exceptions rule is trying to avoid. If at some point,
-                // boost::urls makes the parser object public (or we port it
-                // into bmcweb locally) this will be replaced with
-                // parser::parse, which returns a status code
-
+                boost::beast::http::verb method = parser->get().method();
+                readClientIp();
                 try
                 {
-                    req->urlView = boost::urls::url_view(req->target());
+                    req->urlView =
+                        boost::urls::url_view(parser->get().target());
                     req->url = req->urlView.encoded_path();
                 }
                 catch (std::exception& p)
@@ -559,9 +574,15 @@ class Connection :
                     BMCWEB_LOG_ERROR << p.what();
                 }
 
-                crow::authorization::authenticate(*req, res, session);
-
-                bool loggedIn = req && req->session;
+                boost::asio::ip::address ip;
+                if (getClientIp(ip))
+                {
+                    BMCWEB_LOG_DEBUG << "Unable to get client IP";
+                }
+                userSession = crow::authorization::authenticate(
+                    req->url, ip, res, method, parser->get().base(),
+                    userSession);
+                bool loggedIn = userSession != nullptr;
                 if (loggedIn)
                 {
                     startDeadline(loggedInAttempts);
@@ -622,8 +643,7 @@ class Connection :
                     if (isAlive())
                     {
                         cancelDeadlineTimer();
-                        bool loggedIn = req && req->session;
-                        if (loggedIn)
+                        if (userSession != nullptr)
                         {
                             startDeadline(loggedInAttempts);
                         }
@@ -692,7 +712,7 @@ class Connection :
                                                       // newly created parser
                 buffer.consume(buffer.size());
 
-                req.emplace(parser->get());
+                req.emplace(parser->release());
                 doReadHeaders();
             });
     }
@@ -762,7 +782,6 @@ class Connection :
   private:
     Adaptor adaptor;
     Handler* handler;
-
     // Making this a std::optional allows it to be efficiently destroyed and
     // re-created on Connection reset
     std::optional<
@@ -778,7 +797,7 @@ class Connection :
     std::optional<crow::Request> req;
     crow::Response res;
 
-    std::weak_ptr<persistent_data::UserSession> session;
+    std::shared_ptr<persistent_data::UserSession> userSession;
 
     std::optional<size_t> timerCancelKey;
 
diff --git a/http/http_request.hpp b/http/http_request.hpp
index 44ed2e30f5..fecb9de3fb 100644
--- a/http/http_request.hpp
+++ b/http/http_request.hpp
@@ -18,7 +18,7 @@ namespace crow
 
 struct Request
 {
-    boost::beast::http::request<boost::beast::http::string_body>& req;
+    boost::beast::http::request<boost::beast::http::string_body> req;
     boost::beast::http::fields& fields;
     std::string_view url{};
     boost::urls::url_view urlView{};
@@ -34,9 +34,9 @@ struct Request
 
     std::string userRole{};
     Request(
-        boost::beast::http::request<boost::beast::http::string_body>& reqIn) :
-        req(reqIn),
-        fields(reqIn.base()), body(reqIn.body())
+        boost::beast::http::request<boost::beast::http::string_body> reqIn) :
+        req(std::move(reqIn)),
+        fields(req.base()), body(req.body())
     {}
 
     boost::beast::http::verb method() const
diff --git a/include/authorization.hpp b/include/authorization.hpp
index b1d1097068..ecbdca04c0 100644
--- a/include/authorization.hpp
+++ b/include/authorization.hpp
@@ -93,36 +93,37 @@ static std::shared_ptr<persistent_data::UserSession>
     BMCWEB_LOG_DEBUG << "[AuthMiddleware] Token authentication";
 
     std::string_view token = authHeader.substr(strlen("Token "));
-    auto session =
+    auto sessionOut =
         persistent_data::SessionStore::getInstance().loginSessionByToken(token);
-    return session;
+    return sessionOut;
 }
 #endif
 
 #ifdef BMCWEB_ENABLE_XTOKEN_AUTHENTICATION
 static std::shared_ptr<persistent_data::UserSession>
-    performXtokenAuth(const crow::Request& req)
+    performXtokenAuth(const boost::beast::http::header<true>& reqHeader)
 {
     BMCWEB_LOG_DEBUG << "[AuthMiddleware] X-Auth-Token authentication";
 
-    std::string_view token = req.getHeaderValue("X-Auth-Token");
+    std::string_view token = reqHeader["X-Auth-Token"];
     if (token.empty())
     {
         return nullptr;
     }
-    auto session =
+    auto sessionOut =
         persistent_data::SessionStore::getInstance().loginSessionByToken(token);
-    return session;
+    return sessionOut;
 }
 #endif
 
 #ifdef BMCWEB_ENABLE_COOKIE_AUTHENTICATION
 static std::shared_ptr<persistent_data::UserSession>
-    performCookieAuth(const crow::Request& req)
+    performCookieAuth(boost::beast::http::verb method,
+                      const boost::beast::http::header<true>& reqHeader)
 {
     BMCWEB_LOG_DEBUG << "[AuthMiddleware] Cookie authentication";
 
-    std::string_view cookieValue = req.getHeaderValue("Cookie");
+    std::string_view cookieValue = reqHeader["Cookie"];
     if (cookieValue.empty())
     {
         return nullptr;
@@ -142,20 +143,20 @@ static std::shared_ptr<persistent_data::UserSession>
     std::string_view authKey =
         cookieValue.substr(startIndex, endIndex - startIndex);
 
-    std::shared_ptr<persistent_data::UserSession> session =
+    std::shared_ptr<persistent_data::UserSession> sessionOut =
         persistent_data::SessionStore::getInstance().loginSessionByToken(
             authKey);
-    if (session == nullptr)
+    if (sessionOut == nullptr)
     {
         return nullptr;
     }
 #ifndef BMCWEB_INSECURE_DISABLE_CSRF_PREVENTION
     // RFC7231 defines methods that need csrf protection
-    if (req.method() != boost::beast::http::verb::get)
+    if (method != boost::beast::http::verb::get)
     {
-        std::string_view csrf = req.getHeaderValue("X-XSRF-TOKEN");
+        std::string_view csrf = reqHeader["X-XSRF-TOKEN"];
         // Make sure both tokens are filled
-        if (csrf.empty() || session->csrfToken.empty())
+        if (csrf.empty() || sessionOut->csrfToken.empty())
         {
             return nullptr;
         }
@@ -165,31 +166,33 @@ static std::shared_ptr<persistent_data::UserSession>
             return nullptr;
         }
         // Reject if csrf token not available
-        if (!crow::utility::constantTimeStringCompare(csrf, session->csrfToken))
+        if (!crow::utility::constantTimeStringCompare(csrf,
+                                                      sessionOut->csrfToken))
         {
             return nullptr;
         }
     }
 #endif
-    return session;
+    return sessionOut;
 }
 #endif
 
 #ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
 static std::shared_ptr<persistent_data::UserSession>
-    performTLSAuth(const crow::Request& req, Response& res,
+    performTLSAuth(Response& res,
+                   const boost::beast::http::header<true>& reqHeader,
                    const std::weak_ptr<persistent_data::UserSession>& session)
 {
     if (auto sp = session.lock())
     {
         // set cookie only if this is req from the browser.
-        if (req.getHeaderValue("User-Agent").empty())
+        if (reqHeader["User-Agent"].empty())
         {
             BMCWEB_LOG_DEBUG << " TLS session: " << sp->uniqueId
                              << " will be used for this request.";
             return sp;
         }
-        std::string_view cookieValue = req.getHeaderValue("Cookie");
+        std::string_view cookieValue = reqHeader["Cookie"];
         if (cookieValue.empty() ||
             cookieValue.find("SESSION=") == std::string::npos)
         {
@@ -211,18 +214,17 @@ static std::shared_ptr<persistent_data::UserSession>
 #endif
 
 // checks if request can be forwarded without authentication
-static bool isOnWhitelist(const crow::Request& req)
+static bool isOnWhitelist(std::string_view url, boost::beast::http::verb method)
 {
-    // it's allowed to GET root node without authentication
-    if (boost::beast::http::verb::get == req.method())
+    if (boost::beast::http::verb::get == method)
     {
-        if (req.url == "/redfish/v1" || req.url == "/redfish/v1/" ||
-            req.url == "/redfish" || req.url == "/redfish/" ||
-            req.url == "/redfish/v1/odata" || req.url == "/redfish/v1/odata/")
+        if (url == "/redfish/v1" || url == "/redfish/v1/" ||
+            url == "/redfish" || url == "/redfish/" ||
+            url == "/redfish/v1/odata" || url == "/redfish/v1/odata/")
         {
             return true;
         }
-        if (crow::webroutes::routes.find(std::string(req.url)) !=
+        if (crow::webroutes::routes.find(std::string(url)) !=
             crow::webroutes::routes.end())
         {
             return true;
@@ -231,11 +233,11 @@ static bool isOnWhitelist(const crow::Request& req)
 
     // it's allowed to POST on session collection & login without
     // authentication
-    if (boost::beast::http::verb::post == req.method())
+    if (boost::beast::http::verb::post == method)
     {
-        if ((req.url == "/redfish/v1/SessionService/Sessions") ||
-            (req.url == "/redfish/v1/SessionService/Sessions/") ||
-            (req.url == "/login"))
+        if ((url == "/redfish/v1/SessionService/Sessions") ||
+            (url == "/redfish/v1/SessionService/Sessions/") ||
+            (url == "/login"))
         {
             return true;
         }
@@ -244,66 +246,68 @@ static bool isOnWhitelist(const crow::Request& req)
     return false;
 }
 
-static void authenticate(
-    crow::Request& req, Response& res,
-    [[maybe_unused]] const std::weak_ptr<persistent_data::UserSession>& session)
+static std::shared_ptr<persistent_data::UserSession> authenticate(
+    std::string_view url, boost::asio::ip::address& ipAddress, Response& res,
+    boost::beast::http::verb method,
+    const boost::beast::http::header<true>& reqHeader,
+    [[maybe_unused]] const std::shared_ptr<persistent_data::UserSession>&
+        session)
 {
-    if (isOnWhitelist(req))
+    if (isOnWhitelist(url, method))
     {
-        return;
+        return nullptr;
     }
-
     const persistent_data::AuthConfigMethods& authMethodsConfig =
         persistent_data::SessionStore::getInstance().getAuthMethodsConfig();
 
+    std::shared_ptr<persistent_data::UserSession> sessionOut = nullptr;
 #ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
-    if (req.session == nullptr && authMethodsConfig.tls)
+    if (authMethodsConfig.tls)
     {
-        req.session = performTLSAuth(req, res, session);
+        sessionOut = performTLSAuth(res, reqHeader, session);
     }
 #endif
 #ifdef BMCWEB_ENABLE_XTOKEN_AUTHENTICATION
-    if (req.session == nullptr && authMethodsConfig.xtoken)
+    if (sessionOut == nullptr && authMethodsConfig.xtoken)
     {
-        req.session = performXtokenAuth(req);
+        sessionOut = performXtokenAuth(reqHeader);
     }
 #endif
 #ifdef BMCWEB_ENABLE_COOKIE_AUTHENTICATION
-    if (req.session == nullptr && authMethodsConfig.cookie)
+    if (sessionOut == nullptr && authMethodsConfig.cookie)
     {
-        req.session = performCookieAuth(req);
+        sessionOut = performCookieAuth(method, reqHeader);
     }
 #endif
-    if (req.session == nullptr)
+    std::string_view authHeader = reqHeader["Authorization"];
+
+    if (!authHeader.empty())
     {
-        std::string_view authHeader = req.getHeaderValue("Authorization");
-        if (!authHeader.empty())
+        // Reject any kind of auth other than basic or token
+        if (boost::starts_with(authHeader, "Token ") &&
+            authMethodsConfig.sessionToken)
         {
-            // Reject any kind of auth other than basic or token
-            if (boost::starts_with(authHeader, "Token ") &&
-                authMethodsConfig.sessionToken)
-            {
 #ifdef BMCWEB_ENABLE_SESSION_AUTHENTICATION
-                req.session = performTokenAuth(authHeader);
+            sessionOut = performTokenAuth(authHeader);
 #endif
-            }
-            else if (boost::starts_with(authHeader, "Basic ") &&
-                     authMethodsConfig.basic)
-            {
+        }
+        else if (boost::starts_with(authHeader, "Basic ") &&
+                 authMethodsConfig.basic)
+        {
 #ifdef BMCWEB_ENABLE_BASIC_AUTHENTICATION
-                req.session = performBasicAuth(req.ipAddress, authHeader);
+            sessionOut = performBasicAuth(ipAddress, authHeader);
 #endif
-            }
+        }
+        if (sessionOut != nullptr)
+        {
+            return sessionOut;
         }
     }
-
-    if (req.session == nullptr)
-    {
-        BMCWEB_LOG_WARNING << "[AuthMiddleware] authorization failed";
-        forward_unauthorized::sendUnauthorized(req, res);
-        res.end();
-        return;
-    }
+    BMCWEB_LOG_WARNING << "[AuthMiddleware] authorization failed";
+    forward_unauthorized::sendUnauthorized(url, reqHeader["User-Agent"],
+                                           reqHeader["accept"], res);
+    res.end();
+    return nullptr;
 }
 
 } // namespace authorization
diff --git a/include/forward_unauthorized.hpp b/include/forward_unauthorized.hpp
index 46a25d4b42..29fef337f0 100644
--- a/include/forward_unauthorized.hpp
+++ b/include/forward_unauthorized.hpp
@@ -8,18 +8,19 @@ namespace forward_unauthorized
 
 static bool hasWebuiRoute = false;
 
-inline void sendUnauthorized(const crow::Request& req, crow::Response& res)
+inline void sendUnauthorized(std::string_view url, std::string_view userAgent,
+                             std::string_view accept, crow::Response& res)
 {
     // If it's a browser connecting, don't send the HTTP authenticate
     // header, to avoid possible CSRF attacks with basic auth
-    if (http_helpers::requestPrefersHtml(req))
+    if (http_helpers::requestPrefersHtml(accept))
     {
         // If we have a webui installed, redirect to that login page
         if (hasWebuiRoute)
         {
             res.result(boost::beast::http::status::temporary_redirect);
             res.addHeader("Location",
-                          "/#/login?next=" + http_helpers::urlEncode(req.url));
+                          "/#/login?next=" + http_helpers::urlEncode(url));
         }
         else
         {
@@ -35,7 +36,7 @@ inline void sendUnauthorized(const crow::Request& req, crow::Response& res)
         // only send the WWW-authenticate header if this isn't a xhr
         // from the browser.  Most scripts, tend to not set a user-agent header.
         // So key off that to know whether or not we need to suggest basic auth
-        if (req.getHeaderValue("User-Agent").empty())
+        if (userAgent.empty())
         {
             res.addHeader("WWW-Authenticate", "Basic");
         }
diff --git a/include/http_utility.hpp b/include/http_utility.hpp
index 119a1eefb3..ef65e23419 100644
--- a/include/http_utility.hpp
+++ b/include/http_utility.hpp
@@ -5,9 +5,8 @@
 
 namespace http_helpers
 {
-inline bool requestPrefersHtml(const crow::Request& req)
+inline bool requestPrefersHtml(std::string_view header)
 {
-    std::string_view header = req.getHeaderValue("accept");
     std::vector<std::string> encodings;
     // chrome currently sends 6 accepts headers, firefox sends 4.
     encodings.reserve(6);