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>
diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index a1a7045..a4723f1 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -69,7 +69,6 @@
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 @@
}
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 @@
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 @@
}
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 @@
{
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 @@
}
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 @@
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 @@
// 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 @@
{
// 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 @@
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 @@
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 @@
if (isAlive())
{
cancelDeadlineTimer();
- bool loggedIn = req && req->session;
- if (loggedIn)
+ if (userSession != nullptr)
{
startDeadline(loggedInAttempts);
}
@@ -692,7 +712,7 @@
// newly created parser
buffer.consume(buffer.size());
- req.emplace(parser->get());
+ req.emplace(parser->release());
doReadHeaders();
});
}
@@ -762,7 +782,6 @@
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 @@
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;