commit b4963077e81b23b3745293e90fbc4840a90b7d0a
author Boleslaw Ogonczyk Makowski <boleslawx.ogonczyk-makowski@intel.com> Mon Feb 06 09:59:58 2023 +0100
committer Ed Tanous <ed@tanous.net> Fri Feb 10 23:32:36 2023 +0000
tree a6c5da714c950d6c07cea41ba93ac299ba11f2ac
parent c6830d5f5ae53ea894fd034b0c4fa2e598fbf485

Fix overwriting mTLS session

sessionIsFromTransport variable was always being overwritten to false
[1], it caused userSession to get cleared [2] while it was also being
used for mTLS session which prevented authentication in next requests
and made cleanup code inaccessible [3]. Using the same variable for
session from request and session created by mTLS broke single
responsibility principle.

Tested:
Follow the guide in [4] to create a valid certificate for a user that
can access some resource (for example /redfish/v1/Chassis) and create a
file containing the address to it more than once in following format:
url=https://BMC_IP/redfish/v1/Chassis
url=https://BMC_IP/redfish/v1/Chassis

curl --cert client-cert.pem --key client-key.pem -vvv --cacert
CA-cert.pem -K addr_file -H "Connection: keep-alive"

Before this change requests after first would fail with "401
Unauthorized"

[1]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L468
[2]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L555
[3]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L283
[4]: https://github.com/openbmc/docs/blob/f4febd002df578bad816239b70950f84ea4567e8/security/TLS-configuration.md

Change-Id: I4cf70ceb23c7a9b2668b2fcb44566f9971ac9ad4
Signed-off-by: Karol Niczyj <karol.niczyj@intel.com>
Signed-off-by: Boleslaw Ogonczyk Makowski <boleslawx.ogonczyk-makowski@intel.com>

http/http_connection.hpp

diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index b2096d4..028222e 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -102,10 +102,12 @@
         // don't require auth
         if (preverified)
         {
-            userSession = verifyMtlsUser(req->ipAddress, ctx);
-            if (userSession)
+            mtlsSession = verifyMtlsUser(req->ipAddress, ctx);
+            if (mtlsSession)
             {
-                sessionIsFromTransport = true;
+                BMCWEB_LOG_DEBUG
+                    << this
+                    << " Generating TLS session: " << mtlsSession->uniqueId;
             }
         }
         return true;
@@ -280,13 +282,13 @@
                                          boost::asio::ip::tcp::socket>>)
         {
             adaptor.next_layer().close();
-            if (sessionIsFromTransport && userSession != nullptr)
+            if (mtlsSession != nullptr)
             {
                 BMCWEB_LOG_DEBUG
                     << this
-                    << " Removing TLS session: " << userSession->uniqueId;
+                    << " Removing TLS session: " << mtlsSession->uniqueId;
                 persistent_data::SessionStore::getInstance().removeSession(
-                    userSession);
+                    mtlsSession);
             }
         }
         else
@@ -465,11 +467,10 @@
             {
                 BMCWEB_LOG_DEBUG << "Unable to get client IP";
             }
-            sessionIsFromTransport = false;
 #ifndef BMCWEB_INSECURE_DISABLE_AUTHX
             boost::beast::http::verb method = parser->get().method();
             userSession = crow::authentication::authenticate(
-                ip, res, method, parser->get().base(), userSession);
+                ip, res, method, parser->get().base(), mtlsSession);
 
             bool loggedIn = userSession != nullptr;
             if (!loggedIn)
@@ -550,12 +551,7 @@
                                                   // newly created parser
             buffer.consume(buffer.size());
 
-            // If the session was built from the transport, we don't need to
-            // clear it.  All other sessions are generated per request.
-            if (!sessionIsFromTransport)
-            {
-                userSession = nullptr;
-            }
+            userSession = nullptr;
 
             // Destroy the Request via the std::optional
             req.reset();
@@ -628,8 +624,8 @@
     std::optional<crow::Request> req;
     crow::Response res;
 
-    bool sessionIsFromTransport = false;
     std::shared_ptr<persistent_data::UserSession> userSession;
+    std::shared_ptr<persistent_data::UserSession> mtlsSession;
 
     boost::asio::steady_timer timer;