commit 1d3c14aa49a6c814a0e106540d82aeb4d1473d25
author Ed Tanous <edtanous@google.com> Wed Sep 22 18:54:40 2021 -0700
committer Ed Tanous <ed@tanous.net> Fri Oct 08 23:17:04 2021 +0000
tree 3e95facf6a52ee064776f54d6af1257ceb82aabd
parent 259df359c457c9972feb24170d41dfdcbb5eef0a

Split up authenticate

This commit attempts to split up the authenticate method to make it
easier to audit, and to simplify some duplicated URL parsing code.

First, some history:
authenticate used to be token authentication middleware, then it got
promoted into http connection, because of security concerns (we needed
to effectively rate limit unauthenticated users).  Then we got rid of
middlewares entirely, then we rearranged the ownership of request such
that it owns all its data and inits later in the cycle.  This has caused
a mess, so lets try to clean it up and make the connection class
simpler.

This commit specifically breaks up authenticate into two parts, the
first, which is the same as the old authenticate, is responsible for
actually authenticating the user, and no longer carries the
authorization credentials and allowlist with it.  The allowlist, as well
as actually returning 401 is now moved into handle, where it makes more
sense, as the request is complete, and we can immediately invoke the
action, instead of having to set the isCompleted flag and wait until
later.

Because of this again, now the only calls to completeRequest are called
from handle(), which means we can remove the extra "if (req.completed)"
check we formerly had to do for authenticate, continuing to make
authenticate less of a special case.

The only possible negative to this patch, is now any allowlisted
endpoints still have to call through the authenticate code, whereas
previously they could take a fast path.  This code runs all requests
against authenticate, regardless of their allowlist status.  In theory,
this makes this slower, in practice, It seems to be an unmeasurable
impact.

Tested:
curl --insecure "https://192.168.7.2/redfish/v1"
Returns the redfish v1 resource

curl --insecure "https://192.168.7.2/redfish/v1/Systems"
Returns 401 unauthorized

curl --insecure --user root:0penBmc "https://192.168.7.2/redfish/v1/Systems"
returns the SystemsCollection

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ic9c686b8da7bb6c03b9c113a6493f0e071b5bc77

http/http_connection.hpp

diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index 3720d9d..8e53afa 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -361,6 +361,18 @@
             completeRequest();
             return;
         }
+
+        if (!crow::authorization::isOnWhitelist(req->url, req->method()) &&
+            thisReq.session == nullptr)
+        {
+            BMCWEB_LOG_WARNING << "[AuthMiddleware] authorization failed";
+            forward_unauthorized::sendUnauthorized(
+                req->url, req->getHeaderValue("User-Agent"),
+                req->getHeaderValue("Accept"), res);
+            completeRequest();
+            return;
+        }
+
         res.setCompleteRequestHandler([self(shared_from_this())] {
             boost::asio::post(self->adaptor.get_executor(),
                               [self] { self->completeRequest(); });
@@ -574,8 +586,7 @@
                 }
                 sessionIsFromTransport = false;
                 userSession = crow::authorization::authenticate(
-                    req->url, ip, res, method, parser->get().base(),
-                    userSession);
+                    ip, res, method, parser->get().base(), userSession);
                 bool loggedIn = userSession != nullptr;
                 if (loggedIn)
                 {
@@ -713,7 +724,8 @@
                     userSession = nullptr;
                 }
 
-                req.emplace(parser->release());
+                // Destroy the Request via the std::optional
+                req.reset();
                 doReadHeaders();
             });
     }

include/authorization.hpp

diff --git a/include/authorization.hpp b/include/authorization.hpp
index 7caa09a..04e0223 100644
--- a/include/authorization.hpp
+++ b/include/authorization.hpp
@@ -256,16 +256,12 @@
 }
 
 static std::shared_ptr<persistent_data::UserSession> authenticate(
-    std::string_view url, boost::asio::ip::address& ipAddress [[maybe_unused]],
-    Response& res, boost::beast::http::verb method,
+    boost::asio::ip::address& ipAddress [[maybe_unused]], 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(url, method))
-    {
-        return nullptr;
-    }
     const persistent_data::AuthConfigMethods& authMethodsConfig =
         persistent_data::SessionStore::getInstance().getAuthMethodsConfig();
 
@@ -308,10 +304,6 @@
         return sessionOut;
     }
 
-    BMCWEB_LOG_WARNING << "[AuthMiddleware] authorization failed";
-    forward_unauthorized::sendUnauthorized(url, reqHeader["User-Agent"],
-                                           reqHeader["accept"], res);
-    res.end();
     return nullptr;
 }