[Libguestfs] [PATCH nbdkit v2] curl: Implement header and cookie scripts.

[Richard W.M. Jones]

On Mon, Jul 20, 2020 at 10:20:48AM -0500, Eric Blake wrote:
> What happens if you have header-script-renew=5, but a very slow
> client that sits idle more more than 5 seconds at a time?  Are you
> burning CPU time every five seconds just to throw it away, or are
> you letting it idle until the start of each client command where you
> check whether the time since last renew is larger than the limit.
> (I guess that's implementation, not worth documenting, so I'll see
> if I spot it below)

Yes you guessed right.  There is still the opportunity for someone to
set the renew time to be too low (or for it to be set correctly but
the remote taking excessively long to renew) and they will end up
wasting time, but I'm not sure there's much we can do about it.

> >+ nbdkit curl https://service.example.com/disk.img \
> >+        header-script='
> >+          echo -n "Authorization: Bearer "
> 
> 'echo -n' is not 100% portable; better might be using 'printf'
...
> >+          curl -s -X POST https://auth.example.com/login |
> >+               jq -r .token
> >+        ' \
> >+        header-script-renew=50
> 
> Example is inconsistent with text (45 vs. 50), but it looks like you
> answered my question above, and DO let the script idle when the
> client is idle.

I'll push small updates to fix this thanks.

> >+ SERVER=esx.example.com
> >+ DCPATH=data
> >+ DS=datastore1
> >+ GUEST=guest-name
> >+ URL="https://$SERVER/folder/$GUEST/$GUEST-flat.vmdk?dcPath=$DCPATH&dsName=$DS"
> >+
> >+ nbdkit curl "$URL" \
> >+        cookie-script='
> >+            curl --head -s --insecure -u root:password "$url" |
> >+                 sed -ne '{ s/^Set-Cookie: \([^;]*\);.*/\1/ip }'
> >+        ' \
> 
> Umm, this is nesting '' inside ''.   You probably need '\'' in the sed line.

I thought I checked these ...  will check again and amend as required.
Unfortunately it's hard to add automated tests of this stuff since we
can't rely on having a real server.

> >+ nbdkit curl "$URL" \
> >+        header-script=' echo -n "Authorization: Bearer "; /tmp/auth.sh ' \
> 
> Another s/echo -n/printf/

Will fix.

> >+        header-script-renew=200 \
> >+        --filter=gzip
> >+
> >+Note that this exposes a tar file over NBD.  See also
> >+L<nbdkit-tar-filter(1)>.
> >+
> >  =head1 DEBUG FLAG
> 
> >+++ b/plugins/curl/curl.c
> >@@ -48,9 +48,11 @@
> >  #include <nbdkit-plugin.h>
> >-#include "cleanup.h"
> >  #include "ascii-ctype.h"
> >  #include "ascii-string.h"
> >+#include "cleanup.h"
> >+
> >+#include "curldefs.h"
> >  /* Macro CURL_AT_LEAST_VERSION was added in 2015 (Curl 7.43) so if the
> >   * macro isn't present then Curl is very old.
> >@@ -61,24 +63,29 @@
> >  #endif
> >  #endif
> >-static const char *url = NULL;  /* required */
> >+/* Plugin configuration. */
> >+const char *url = NULL;         /* required */
> >-static const char *cainfo = NULL;
> >-static const char *capath = NULL;
> >-static char *cookie = NULL;
> >-static struct curl_slist *headers = NULL;
> >-static char *password = NULL;
> >-static long protocols = CURLPROTO_ALL;
> >-static const char *proxy = NULL;
> >-static char *proxy_password = NULL;
> >-static const char *proxy_user = NULL;
> >-static bool sslverify = true;
> >-static bool tcp_keepalive = false;
> >-static bool tcp_nodelay = true;
> >-static uint32_t timeout = 0;
> >-static const char *unix_socket_path = NULL;
> >-static const char *user = NULL;
> >-static const char *user_agent = NULL;
> >+const char *cainfo = NULL;
> >+const char *capath = NULL;
> >+char *cookie = NULL;
> >+const char *cookie_script = NULL;
> >+unsigned cookie_script_renew = 0;
> >+struct curl_slist *headers = NULL;
> >+const char *header_script = NULL;
> >+unsigned header_script_renew = 0;
> >+char *password = NULL;
> 
> All of these variables are already guaranteed zero-initialized
> without being explicit,
> 
> >+long protocols = CURLPROTO_ALL;
> 
> although consistency with this initialization (which is not
> necessarily 0) makes sense.

Assuming it costs nothing, I think it's sometimes worth
self-documenting by having these unnecessary initializations.  Of
course if GCC is doing anything stupid like moving them out of the BSS
then that would be a problem.

> >+
> >+/* This is called with the lock held when we must run or re-run the
> >+ * header-script.
> >+ */
> >+static int
> >+run_header_script (struct curl_handle *h)
> >+{
> 
> >+
> >+/* This is called with the lock held when we must run or re-run the
> >+ * cookie-script.
> >+ */
> >+static int
> >+run_cookie_script (struct curl_handle *h)
> >+{
> >+  int fd;
> 
> These two look similar, is it worth refactoring into a common helper
> routine?

I attempted that but it's annoyingly difficult.  They are a little bit
too different :-(

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/