rgw/beast: disable keepalive for responses before 100 Continue

resolves a long-standing category of delays/errors from boto3 for
requests like s3:PutObject that contain a request body and use the
`Expect: 100-continue` request header. if the server rejects such
requests based on their headers alone, it will respond with an http
status like `HTTP/1.1 403 Forbidden` without first sending the
`HTTP/1.1 100 Continue` status

when http keepalive is enabled, this situation creates an ambiguity for
the server when trying to read the next request from that connection -
subsequent bytes may either correspond to the current request's body or
the next request's headers

to resolve this ambiguity, disable http keepalive by sending response
header `Connection: close` instead of `Connection: keep-alive`. this
instructs the client and server to close this connection instead of
reusing it for subsequent requests

Fixes: https://tracker.ceph.com/issues/42208
Signed-off-by: Casey Bodley <cbodley@redhat.com>

diff --git a/src/rgw/rgw_asio_client.cc b/src/rgw/rgw_asio_client.cc
index 354353cde65f753595fd2b8dc86c0bd0f4243089..a8a9ad420d86c36adeeb8e8360f6198313b2a033 100644 (file)
--- a/src/rgw/rgw_asio_client.cc
+++ b/src/rgw/rgw_asio_client.cc
@@ -18,7 +18,9 @@ ClientIO::ClientIO(parser_type& parser, bool is_ssl,
   : parser(parser), is_ssl(is_ssl),
     local_endpoint(local_endpoint),
     remote_endpoint(remote_endpoint),
-    txbuf(*this)
+    txbuf(*this),
+    keepalive(parser.keep_alive()),
+    expect100continue(parser.get()[beast::http::field::expect] == "100-continue")
 {
 }
 
@@ -108,6 +110,17 @@ void ClientIO::flush()
 
 size_t ClientIO::send_status(int status, const char* status_name)
 {
+  if (expect100continue && !sent100continue) {
+    // a client expecting 100-continue is not required to wait for the
+    // '100 Continue' response before sending the request body. if we
+    // complete the request before sending 100 Continue (for example, due
+    // to an authorization error), we don't know whether any following
+    // bytes on this connection correspond to this request's body or the
+    // next request's header. so we must disable keepalive and require the
+    // client to use a new tcp connection for any subsequent requests
+    keepalive = false;
+  }
+
   static constexpr size_t STATUS_BUF_SIZE = 128;
 
   char statusbuf[STATUS_BUF_SIZE];
@@ -149,7 +162,7 @@ size_t ClientIO::complete_header()
     sent += txbuf.sputn(timestr, strlen(timestr));
   }
 
-  if (parser.keep_alive()) {
+  if (keep_alive()) {
     constexpr char CONN_KEEP_ALIVE[] = "Connection: Keep-Alive\r\n";
     sent += txbuf.sputn(CONN_KEEP_ALIVE, sizeof(CONN_KEEP_ALIVE) - 1);
   } else {

diff --git a/src/rgw/rgw_asio_client.h b/src/rgw/rgw_asio_client.h
index 8bfcacbd4ae53bf518ae9f0ce8db35b77c909818..aba1c28883354703d401f853a9ff539c8838c889 100644 (file)
--- a/src/rgw/rgw_asio_client.h
+++ b/src/rgw/rgw_asio_client.h
@@ -29,6 +29,9 @@ class ClientIO : public io::RestfulClient,
   RGWEnv env;
 
   rgw::io::StaticOutputBufferer<> txbuf;
+
+  bool keepalive = false;
+  bool expect100continue = false;
   bool sent100continue = false;
 
  public:
@@ -55,7 +58,7 @@ class ClientIO : public io::RestfulClient,
     return env;
   }
 
-  bool sent_100_continue() const { return sent100continue; }
+  bool keep_alive() const { return keepalive; }
 };
 
 } // namespace asio

diff --git a/src/rgw/rgw_asio_frontend.cc b/src/rgw/rgw_asio_frontend.cc
index 7d8aa19adc3aa7c80d5dc2db9603ca3c632a8b96..580f82ceb9ed5bcd972bc1dd499e9b69b8d35f1f 100644 (file)
--- a/src/rgw/rgw_asio_frontend.cc
+++ b/src/rgw/rgw_asio_frontend.cc
@@ -300,8 +300,6 @@ void handle_connection(boost::asio::io_context& context,
       return;
     }
 
-    bool expect_continue = (message[http::field::expect] == "100-continue");
-
     {
       auto lock = pause_mutex.async_lock_shared(yield[ec]);
       if (ec == boost::asio::error::operation_aborted) {
@@ -376,18 +374,14 @@ void handle_connection(boost::asio::io_context& context,
         return;
       }
 
-      if (real_client.sent_100_continue()) {
-        expect_continue = false;
+      if (!real_client.keep_alive()) {
+        return;
       }
     }
 
-    if (!parser.keep_alive()) {
-      return;
-    }
-
     // if we failed before reading the entire message, discard any remaining
     // bytes before reading the next
-    while (!expect_continue && !parser.is_done()) {
+    while (!parser.is_done()) {
       static std::array<char, 1024*1024> discard_buffer;
 
       auto& body = parser.get().body();