* Re: [PATCH] debuginfod-client: Add DEBUGINFOD_HEADERS_FILE.
2022-10-18 21:21 [PATCH] debuginfod-client: Add DEBUGINFOD_HEADERS_FILE Daniel Thornburgh
@ 2022-10-28 10:10 ` Mark Wielaard
0 siblings, 0 replies; 2+ messages in thread
From: Mark Wielaard @ 2022-10-28 10:10 UTC (permalink / raw)
To: Daniel Thornburgh, elfutils-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread