[PATCH v2 0/3] block/curl: fix S3 presigned URL support

[PATCH v2 0/3] block/curl: fix S3 presigned URL support

[Antoine Damhet]

Hi,

This series adds the support for S3 presigned URLs that only allow HTTP
GET requests. While working on this I also stumbled upon a deadlock with
concurrent I/O and slipped the fix here. Over the years there was
already an attempt to support these kinds of URLs[1] and at least one
report of a user in need of the feature[2].

Unfortunately S3 only allows a presigned URL to live up to 7 days so we
can't really put an example with a stable URL on the commit message but
here is a presigned URL for the archlinux cloud image[3] that will
expire in ~7 days.

Cheers,
Antoine

[1]: https://lore.kernel.org/qemu-devel/110120539.4133.de5ac8a5-69d1-4f59-9540-4a679771a547.open-xchange@ox.pcextreme.nl/
[2]: https://lore.kernel.org/qemu-devel/7b37cc65-1314-29f4-006f-70836bdfb4b4@linaro.org/
[3]: http://test-presigned.s3.fr-par.scw.cloud/Arch-Linux-x86_64-cloudimg.qcow2?AWSAccessKeyId=SCWDHE3XBQGZFV282QKG&Expires=1772551595&Signature=fqADX1DJvEKcC8iEeoZpssYSkTI%3D

---
Changes in v2:
- New patch (2) that refactors the http(s) QAPI types
- Specify and reword what happens when using 'force-range' with an HTTP
  server that doesn't support the feature
- Document that 'force-range' defaults to false in QAPI
- Fix a few typos
- Regen presigned URL[3]
- Link to v1: https://lore.kernel.org/qemu-devel/20260212162730.440855-1-adamhet@scaleway.com

---

Antoine Damhet (3):
  block/curl: fix concurrent completion handling
  qapi: block: Refactor HTTP(s) common arguments
  block/curl: add support for S3 presigned URLs

 block/curl.c                          | 115 ++++++++++++++++----------
 block/trace-events                    |   1 +
 docs/system/device-url-syntax.rst.inc |   6 ++
 qapi/block-core.json                  |  22 +++--
 4 files changed, 90 insertions(+), 54 deletions(-)

-- 
2.53.0

[PATCH v2 1/3] block/curl: fix concurrent completion handling

[Antoine Damhet]

curl_multi_check_completion would bail upon the first completed
transfer even if more completion messages were available thus leaving
some in flight IOs stuck.

Rework a bit the loop to make the iterations clearer and drop the breaks.

The original hang can be somewhat reproduced with the following command:

$ qemu-img convert -p -m 16 -O qcow2 -c --image-opts \
  'file.driver=https,file.url=https://scaleway.testdebit.info/10G.iso,file.readahead=1M' \
  /tmp/test.qcow2

Fixes: 1f2cead32443 ("curl: Ensure all informationals are checked for completion")
Cc: qemu-stable@nongnu.org
Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/curl.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4e77c93b46e6..6dccf002564e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -324,17 +324,11 @@ curl_find_buf(BDRVCURLState *s, uint64_t start, uint64_t len, CURLAIOCB *acb)
 static void curl_multi_check_completion(BDRVCURLState *s)
 {
     int msgs_in_queue;
+    CURLMsg *msg;
 
     /* Try to find done transfers, so we can free the easy
      * handle again. */
-    for (;;) {
-        CURLMsg *msg;
-        msg = curl_multi_info_read(s->multi, &msgs_in_queue);
-
-        /* Quit when there are no more completions */
-        if (!msg)
-            break;
-
+    while ((msg = curl_multi_info_read(s->multi, &msgs_in_queue))) {
         if (msg->msg == CURLMSG_DONE) {
             int i;
             CURLState *state = NULL;
@@ -397,7 +391,6 @@ static void curl_multi_check_completion(BDRVCURLState *s)
             }
 
             curl_clean_state(state);
-            break;
         }
     }
 }
-- 
2.53.0

[PATCH v2 2/3] qapi: block: Refactor HTTP(s) common arguments

[Antoine Damhet]

The HTTPs curl block driver is a superset of the HTTP driver, reflect
that in the QAPI.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
---
 qapi/block-core.json | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b82af7425614..a7871705fa69 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4595,23 +4595,14 @@
 # Driver specific block device options for HTTPS connections over the
 # curl backend.  URLs must start with "https://".
 #
-# @cookie: List of cookies to set; format is "name1=content1;
-#     name2=content2;" as explained by CURLOPT_COOKIE(3).  Defaults to
-#     no cookies.
-#
 # @sslverify: Whether to verify the SSL certificate's validity
 #     (defaults to true)
 #
-# @cookie-secret: ID of a QCryptoSecret object providing the cookie
-#     data in a secure way.  See @cookie for the format.  (since 2.10)
-#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttps',
-  'base': 'BlockdevOptionsCurlBase',
-  'data': { '*cookie': 'str',
-            '*sslverify': 'bool',
-            '*cookie-secret': 'str'} }
+  'base': 'BlockdevOptionsCurlHttp',
+  'data': { '*sslverify': 'bool'} }
 
 ##
 # @BlockdevOptionsCurlFtp:
-- 
2.53.0

Re: [PATCH v2 2/3] qapi: block: Refactor HTTP(s) common arguments

[Markus Armbruster]

Antoine Damhet <adamhet@scaleway.com> writes:

> The HTTPs curl block driver is a superset of the HTTP driver, reflect
> that in the QAPI.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
> ---
>  qapi/block-core.json | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b82af7425614..a7871705fa69 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4595,23 +4595,14 @@
>  # Driver specific block device options for HTTPS connections over the
>  # curl backend.  URLs must start with "https://".
>  #
> -# @cookie: List of cookies to set; format is "name1=content1;
> -#     name2=content2;" as explained by CURLOPT_COOKIE(3).  Defaults to
> -#     no cookies.
> -#
>  # @sslverify: Whether to verify the SSL certificate's validity
>  #     (defaults to true)
>  #
> -# @cookie-secret: ID of a QCryptoSecret object providing the cookie
> -#     data in a secure way.  See @cookie for the format.  (since 2.10)
> -#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttps',
> -  'base': 'BlockdevOptionsCurlBase',
> -  'data': { '*cookie': 'str',
> -            '*sslverify': 'bool',
> -            '*cookie-secret': 'str'} }
> +  'base': 'BlockdevOptionsCurlHttp',
> +  'data': { '*sslverify': 'bool'} }
>  
>  ##
>  # @BlockdevOptionsCurlFtp:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 3/3] block/curl: add support for S3 presigned URLs

[Antoine Damhet]

S3 presigned URLs are signed for a specific HTTP method (typically GET
for our use cases). The curl block driver currently issues a HEAD
request to discover the web server features and the file size, which
fails with 'HTTP 403' (forbidden).

Add a 'force-range' option that skips the HEAD request and instead
issues a minimal GET request (querying 1 byte from the server) to
extract the file size from the 'Content-Range' response header. To
achieve this the 'curl_header_cb' is redesigned to generically parse
HTTP headers.

$ $QEMU -drive driver=https,\
             'url=https://s3.example.com/some.img?X-Amz-Security-Token=XXX',
             force-range=true

Enabling the 'force-range' option without the web server specified with
@url supporting it might cause the server to respond successfully with
'HTTP 200' and attempt to send the whole file body. With the
'CURLOPT_NOBODY' option set the libcurl will skip reading after the
headers and close the connection. QEMU still gracefully detects the
missing feature. This might waste a small number of TCP packets but is
otherwise transparent to the user.

Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
---
 block/curl.c                          | 104 ++++++++++++++++++--------
 block/trace-events                    |   1 +
 docs/system/device-url-syntax.rst.inc |   6 ++
 qapi/block-core.json                  |   9 ++-
 4 files changed, 86 insertions(+), 34 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 6dccf002564e..66aecfb20ec6 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -62,10 +62,12 @@
 #define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
 #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
 #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
+#define CURL_BLOCK_OPT_FORCE_RANGE "force-range"
 
 #define CURL_BLOCK_OPT_READAHEAD_DEFAULT (256 * 1024)
 #define CURL_BLOCK_OPT_SSLVERIFY_DEFAULT true
 #define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
+#define CURL_BLOCK_OPT_FORCE_RANGE_DEFAULT false
 
 struct BDRVCURLState;
 struct CURLState;
@@ -206,27 +208,33 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
     BDRVCURLState *s = opaque;
     size_t realsize = size * nmemb;
-    const char *p = ptr;
-    const char *end = p + realsize;
-    const char *t = "accept-ranges : bytes "; /* A lowercase template */
+    g_autofree char *header = g_strstrip(g_strndup(ptr, realsize));
+    char *val = strchr(header, ':');
 
-    /* check if header matches the "t" template */
-    for (;;) {
-        if (*t == ' ') { /* space in t matches any amount of isspace in p */
-            if (p < end && g_ascii_isspace(*p)) {
-                ++p;
-            } else {
-                ++t;
-            }
-        } else if (*t && p < end && *t == g_ascii_tolower(*p)) {
-            ++p, ++t;
-        } else {
-            break;
-        }
+    if (!val) {
+        return realsize;
     }
 
-    if (!*t && p == end) { /* if we managed to reach ends of both strings */
-        s->accept_range = true;
+    *val++ = '\0';
+    g_strchomp(header);
+    while (g_ascii_isspace(*val)) {
+        ++val;
+    }
+
+    trace_curl_header_cb(header, val);
+
+    if (!g_ascii_strcasecmp(header, "accept-ranges")) {
+        if (!g_ascii_strcasecmp(val, "bytes")) {
+            s->accept_range = true;
+        }
+    } else if (!g_ascii_strcasecmp(header, "Content-Range")) {
+        /* Content-Range fmt is `bytes begin-end/full_size` */
+        val = strchr(val, '/');
+        if (val) {
+            if (qemu_strtou64(val + 1, NULL, 10, &s->len) < 0) {
+                s->len = UINT64_MAX;
+            }
+        }
     }
 
     return realsize;
@@ -668,6 +676,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "ID of secret used as password for HTTP proxy auth",
         },
+        {
+            .name = CURL_BLOCK_OPT_FORCE_RANGE,
+            .type = QEMU_OPT_BOOL,
+            .help = "Assume HTTP range requests are supported",
+        },
         { /* end of list */ }
     },
 };
@@ -690,6 +703,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
 #endif
     const char *secretid;
     const char *protocol_delimiter;
+    bool force_range;
     int ret;
 
     bdrv_graph_rdlock_main_loop();
@@ -807,35 +821,56 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->accept_range = false;
+    s->len = UINT64_MAX;
+    force_range = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_FORCE_RANGE,
+                                    CURL_BLOCK_OPT_FORCE_RANGE_DEFAULT);
+    /*
+     * When minimal CURL will be bumped to `7.83`, the header callback + manual
+     * parsing can be replaced by `curl_easy_header` calls
+     */
     if (curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1L) ||
         curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION, curl_header_cb) ||
         curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s)) {
-        pstrcpy(state->errmsg, CURL_ERROR_SIZE,
-                "curl library initialization failed.");
-        goto out;
+        goto out_init;
+    }
+    if (force_range) {
+        if (curl_easy_setopt(state->curl, CURLOPT_CUSTOMREQUEST, "GET") ||
+            curl_easy_setopt(state->curl, CURLOPT_RANGE, "0-0")) {
+            goto out_init;
+        }
     }
+
     if (curl_easy_perform(state->curl))
         goto out;
-    /* CURL 7.55.0 deprecates CURLINFO_CONTENT_LENGTH_DOWNLOAD in favour of
-     * the *_T version which returns a more sensible type for content length.
-     */
+
+    if (!force_range) {
+        /*
+         * CURL 7.55.0 deprecates CURLINFO_CONTENT_LENGTH_DOWNLOAD in favour of
+         * the *_T version which returns a more sensible type for content
+         * length.
+         */
 #if LIBCURL_VERSION_NUM >= 0x073700
-    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, &cl)) {
-        goto out;
-    }
+        if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
+                              &cl)) {
+            goto out;
+        }
 #else
-    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &cl)) {
-        goto out;
-    }
+        if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD,
+                              &cl)) {
+            goto out;
+        }
 #endif
-    if (cl < 0) {
+        if (cl >= 0) {
+            s->len = cl;
+        }
+    }
+
+    if (s->len == UINT64_MAX) {
         pstrcpy(state->errmsg, CURL_ERROR_SIZE,
                 "Server didn't report file size.");
         goto out;
     }
 
-    s->len = cl;
-
     if ((!strncasecmp(s->url, "http://", strlen("http://"))
         || !strncasecmp(s->url, "https://", strlen("https://")))
         && !s->accept_range) {
@@ -856,6 +891,9 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_opts_del(opts);
     return 0;
 
+out_init:
+    pstrcpy(state->errmsg, CURL_ERROR_SIZE,
+            "curl library initialization failed.");
 out:
     error_setg(errp, "CURL: Error opening file: %s", state->errmsg);
     curl_easy_cleanup(state->curl);
diff --git a/block/trace-events b/block/trace-events
index c9b4736ff884..d170fc96f15f 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -191,6 +191,7 @@ ssh_server_status(int status) "server status=%d"
 curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
 curl_sock_cb(int action, int fd) "sock action %d on fd %d"
 curl_read_cb(size_t realsize) "just reading %zu bytes"
+curl_header_cb(const char *key, const char *val) "looking at %s: %s"
 curl_open(const char *file) "opening %s"
 curl_open_size(uint64_t size) "size = %" PRIu64
 curl_setup_preadv(uint64_t bytes, uint64_t start, const char *range) "reading %" PRIu64 " at %" PRIu64 " (%s)"
diff --git a/docs/system/device-url-syntax.rst.inc b/docs/system/device-url-syntax.rst.inc
index aae65d138c00..445e2a0a4157 100644
--- a/docs/system/device-url-syntax.rst.inc
+++ b/docs/system/device-url-syntax.rst.inc
@@ -179,6 +179,12 @@ These are specified using a special URL syntax.
       get the size of the image to be downloaded. If not set, the
       default timeout of 5 seconds is used.
 
+   ``force-range``
+      Assume the HTTP backend supports range requests and avoid doing
+      an HTTP HEAD request to discover the feature. Typically S3
+      presigned URLs will only support one method and refuse other
+      request types.
+
    Note that when passing options to qemu explicitly, ``driver`` is the
    value of <protocol>.
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a7871705fa69..50e7078cbec0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4582,12 +4582,19 @@
 # @cookie-secret: ID of a QCryptoSecret object providing the cookie
 #     data in a secure way.  See @cookie for the format.  (since 2.10)
 #
+# @force-range: Don't issue a HEAD HTTP request to discover if the
+#     http server supports range requests and rely only on GET
+#     requests.  This is especially useful for S3 presigned URLs where
+#     HEAD requests are unauthorized.  Defaults to false.
+#     (default: false; since 11.0)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttp',
   'base': 'BlockdevOptionsCurlBase',
   'data': { '*cookie': 'str',
-            '*cookie-secret': 'str'} }
+            '*cookie-secret': 'str',
+            '*force-range': 'bool'} }
 
 ##
 # @BlockdevOptionsCurlHttps:
-- 
2.53.0

Re: [PATCH v2 3/3] block/curl: add support for S3 presigned URLs

[Markus Armbruster]

Antoine Damhet <adamhet@scaleway.com> writes:

> S3 presigned URLs are signed for a specific HTTP method (typically GET
> for our use cases). The curl block driver currently issues a HEAD
> request to discover the web server features and the file size, which
> fails with 'HTTP 403' (forbidden).
>
> Add a 'force-range' option that skips the HEAD request and instead
> issues a minimal GET request (querying 1 byte from the server) to
> extract the file size from the 'Content-Range' response header. To
> achieve this the 'curl_header_cb' is redesigned to generically parse
> HTTP headers.
>
> $ $QEMU -drive driver=https,\
>              'url=https://s3.example.com/some.img?X-Amz-Security-Token=XXX',
>              force-range=true
>
> Enabling the 'force-range' option without the web server specified with
> @url supporting it might cause the server to respond successfully with
> 'HTTP 200' and attempt to send the whole file body. With the
> 'CURLOPT_NOBODY' option set the libcurl will skip reading after the
> headers and close the connection. QEMU still gracefully detects the
> missing feature. This might waste a small number of TCP packets but is
> otherwise transparent to the user.
>
> Signed-off-by: Antoine Damhet <adamhet@scaleway.com>

[...]

> diff --git a/docs/system/device-url-syntax.rst.inc b/docs/system/device-url-syntax.rst.inc
> index aae65d138c00..445e2a0a4157 100644
> --- a/docs/system/device-url-syntax.rst.inc
> +++ b/docs/system/device-url-syntax.rst.inc
> @@ -179,6 +179,12 @@ These are specified using a special URL syntax.
>        get the size of the image to be downloaded. If not set, the
>        default timeout of 5 seconds is used.
>  
> +   ``force-range``
> +      Assume the HTTP backend supports range requests and avoid doing
> +      an HTTP HEAD request to discover the feature. Typically S3
> +      presigned URLs will only support one method and refuse other
> +      request types.
> +

Similar to the description in qapi/block-core.json.  I find the latter
clearer.  Perhaps you'd like to use it here.  Entirely up to you.

>     Note that when passing options to qemu explicitly, ``driver`` is the
>     value of <protocol>.
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a7871705fa69..50e7078cbec0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4582,12 +4582,19 @@
>  # @cookie-secret: ID of a QCryptoSecret object providing the cookie
>  #     data in a secure way.  See @cookie for the format.  (since 2.10)
>  #
> +# @force-range: Don't issue a HEAD HTTP request to discover if the
> +#     http server supports range requests and rely only on GET
> +#     requests.  This is especially useful for S3 presigned URLs where
> +#     HEAD requests are unauthorized.  Defaults to false.
> +#     (default: false; since 11.0)
> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttp',
>    'base': 'BlockdevOptionsCurlBase',
>    'data': { '*cookie': 'str',
> -            '*cookie-secret': 'str'} }
> +            '*cookie-secret': 'str',
> +            '*force-range': 'bool'} }
>  
>  ##
>  # @BlockdevOptionsCurlHttps:

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>