Re: [PATCH] debuginfod-client: Add DEBUGINFOD_HEADERS_FILE.

[Mark Wielaard <mark@klomp.org>]

Hi Daniel,

On Tue, 2022-10-18 at 14:21 -0700, Daniel Thornburgh via Elfutils-devel wrote:
> This DEBUGINFOD_HEADERS_FILE environment variable names a file to supply
> HTTP headers to outgoing requests. Notably, this allows for
> Authorization headers to be added from a file under OS access control.
> 
> Signed-off-by: Daniel Thornburgh <dthorn@google.com>
> ---
>  NEWS                            |  3 +++
>  debuginfod/ChangeLog            |  5 ++++
>  debuginfod/debuginfod-client.c  | 44 +++++++++++++++++++++++++++++++++
>  debuginfod/debuginfod.h.in      |  1 +
>  doc/ChangeLog                   |  4 +++
>  doc/debuginfod-client-config.7  |  7 ++++++
>  doc/debuginfod_find_debuginfo.3 | 12 ++++++---
>  7 files changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 6ebd172c..3df652e3 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,9 @@ Version 0.188 some time after 0.187
>  
>  readelf: Add -D, --use-dynamic option.
>  
> +debuginfod-client: Add $DEBUGINFOD_HEADERS_FILE setting to supply outgoing
> +                   HTTP headers.
> +
>  debuginfod: Add --disable-source-scan option.

Thanks for the NEWS entry.

>  libdwfl: Add new function dwfl_get_debuginfod_client.
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 59d50df1..1df903fe 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-10-18  Daniel Thornburgh <dthorn@google.com>
> +
> +  * debuginfod-client.c (debuginfod_query_server): Add DEBUGINFOD_HEADERS_FILE
> +  setting to supply outgoing HTTP headers.
> +
>  2022-10-17  Frank Ch. Eigler  <fche@redhat.com>
>  
>  	* debuginfod.cxx (main): Report libmicrohttpd version.
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index 2a14d9d9..549520c1 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -42,6 +42,7 @@
>  #include "config.h"
>  #include "debuginfod.h"
>  #include "system.h"
> +#include <ctype.h>
>  #include <errno.h>
>  #include <stdlib.h>

OK, ctype.h added for isspace.
 
> @@ -447,6 +448,44 @@ add_default_headers(debuginfod_client *client)
>    free (utspart);
>  }
>  
> +/* Add HTTP headers found in the given file, one per line. Blank lines or invalid
> + * headers are ignored.
> + */
> +static void
> +add_headers_from_file(debuginfod_client *client, const char* filename)
> +{
> +  int vds = client->verbose_fd;
> +  FILE *f = fopen (filename, "r");
> +  if (f == NULL)
> +    {
> +      dprintf(vds, "header file %s: %s\n", filename, strerror(errno));
> +      return;
> +    }

Since verbose_fd can be negative it seems better to guard the dprintf
with if (vds >= 0) like other calls that use client->verbose_fd.
dprintf will likely simply silently fail, but better be consistent.

> +  while (1)
> +    {
> +      char buf[8192];

OK, 8K should be enough for everybody. It looks like some servers limit
the total header size to 8K. So this seems a fair limit. 

> +      char *s = &buf[0];
> +      if (feof(f))
> +        break;
> +      if (fgets (s, sizeof(buf), f) == NULL)
> +        break;
> +      for (char *c = s; *c != '\0'; ++c)
> +        if (!isspace(*c))
> +          goto nonempty;
> +      continue;
> +    nonempty:
> +      ;
> +      size_t last = strlen(s)-1;
> +      if (s[last] == '\n')
> +        s[last] = '\0';
> +      int rc = debuginfod_add_http_header(client, s);

OK, this checks there is at least a space between header and value, and
that an ending \n is chopped off. It doesn't check for having a ':',
but debuginfod_add_http_header will do further sanity checks.

> +      if (rc < 0)
> +        dprintf(vds, "skipping bad header: %s\n", strerror(-rc));

Like above, guard with if (rc < 0 && vds >= 0).

> +    }
> +  fclose (f);
> +}
> +

OK, we will always get here, so f will be closed unless it couldn't be
opened.
 
>  #define xalloc_str(p, fmt, args...)        \
>    do                                       \
> @@ -610,6 +649,11 @@ debuginfod_query_server (debuginfod_client *c,
>    if (maxtime && vfd >= 0)
>      dprintf(vfd, "using max time %lds\n", maxtime);
>  
> +  const char *headers_file_envvar;
> +  headers_file_envvar = getenv(DEBUGINFOD_HEADERS_FILE_ENV_VAR);
> +  if (headers_file_envvar != NULL)
> +    add_headers_from_file(c, headers_file_envvar);
> +
>    /* Maxsize is valid*/
>    if (maxsize > 0)
>      {

This sets the headers every time debuginfod_query_server is called.
That is correct because at the end of debuginfod_query_server all
headers are cleared. So if the client handle is reused the headers need
to be set again.

> diff --git a/debuginfod/debuginfod.h.in b/debuginfod/debuginfod.h.in
> index 40b1ea00..7d8e4972 100644
> --- a/debuginfod/debuginfod.h.in
> +++ b/debuginfod/debuginfod.h.in
> @@ -38,6 +38,7 @@
>  #define DEBUGINFOD_RETRY_LIMIT_ENV_VAR "DEBUGINFOD_RETRY_LIMIT"
>  #define DEBUGINFOD_MAXSIZE_ENV_VAR "DEBUGINFOD_MAXSIZE"
>  #define DEBUGINFOD_MAXTIME_ENV_VAR "DEBUGINFOD_MAXTIME"
> +#define DEBUGINFOD_HEADERS_FILE_ENV_VAR "DEBUGINFOD_HEADERS_FILE"
>  
>  /* The libdebuginfod soname.  */
>  #define DEBUGINFOD_SONAME "@LIBDEBUGINFOD_SONAME@"
> diff --git a/doc/ChangeLog b/doc/ChangeLog
> index b2bb4890..073023b4 100644
> --- a/doc/ChangeLog
> +++ b/doc/ChangeLog
> @@ -1,3 +1,7 @@
> +2022-10-18  Daniel Thornburgh  <dthorn@google.com>
> +
> +	* debuginfod_find_debuginfo.3: Document DEBUGINFOD_HEADERS_FILE.
> +
>  2022-09-02  Frank Ch. Eigler  <fche@redhat.com>
>  
>  	* debuginfod_find_debuginfo.3: Tweaked debuginfod_get_headers docs.
> diff --git a/doc/debuginfod-client-config.7 b/doc/debuginfod-client-config.7
> index fecc6038..53d82806 100644
> --- a/doc/debuginfod-client-config.7
> +++ b/doc/debuginfod-client-config.7
> @@ -75,6 +75,13 @@ only small files are downloaded. A value of 0 causes no consideration
>  for size, and the client may attempt to download a file of any size.
>  The default is 0 (infinite size).
>  
> +.TP
> +.B $DEBUGINFOD_HEADERS_FILE
> +This environment variable points to a file that supplies headers to
> +outbound HTTP requests, one per line. The header lines shouldn't end with
> +CRLF, unless that's the system newline convention. Whitespace-only lines
> +are skipped.
> +
>  .SH CACHE
>  
>  Before each query, the debuginfod client library checks for a need to
> diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
> index aebbec3f..3dd83240 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -188,12 +188,18 @@ indicates success, but out-of-memory conditions may result in
>  a non-zero \fI-ENOMEM\fP. If the string is in the wrong form
>  \fI-EINVAL\fP will be returned.
>  
> +\fI$DEBUGINFOD_HEADERS_FILE\fP specifies a file to supply headers to
> +outgoing requests. Each non-whitespace line of this file is handled
> +as if
> +.BR \%debuginfod_add_http_header ()
> +were called on the contents.
> +
>  Note that the current debuginfod-client library implementation uses
>  libcurl, but you shouldn't rely on that fact. Don't use this function
>  for replacing any standard headers, except for the User-Agent mentioned
> -below. The only supported usage of this function is for adding an
> -optional header which might or might not be passed through to the
> -server for logging purposes only.
> +below. You can use this function to add authorization information for
> +access control, or to provide optional headers to the server for
> +logging purposes.
>  
>  By default, the library adds a descriptive \fIUser-Agent:\fP
>  header to outgoing requests.  If the client application adds

Thanks for the documentation, which looks good.

I have pushed this with the two extra vfd >= 0 guards mentioned above.

Note that it would be really good to have a testcase for this so it
doesn't accidentally breaks. Since it might be that other developers
won't use this functionality.

Thanks,

Mark