From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id C25BD3858C2B for ; Fri, 28 Oct 2022 10:10:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C25BD3858C2B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 5CDC0330CD41; Fri, 28 Oct 2022 12:10:14 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 7A237413CD0E; Fri, 28 Oct 2022 12:10:13 +0200 (CEST) Message-ID: <4acb358d00935eb62b0a532ffe8ae772ec361125.camel@klomp.org> Subject: Re: [PATCH] debuginfod-client: Add DEBUGINFOD_HEADERS_FILE. From: Mark Wielaard To: Daniel Thornburgh , elfutils-devel@sourceware.org Date: Fri, 28 Oct 2022 12:10:13 +0200 In-Reply-To: <20221018212132.53883-1-dthorn@google.com> References: <20221018212132.53883-1-dthorn@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-3039.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Daniel, On Tue, 2022-10-18 at 14:21 -0700, Daniel Thornburgh via Elfutils-devel wro= te: > 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. >=20 > Signed-off-by: Daniel Thornburgh > --- > 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(-) >=20 > 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 > =20 > readelf: Add -D, --use-dynamic option. > =20 > +debuginfod-client: Add $DEBUGINFOD_HEADERS_FILE setting to supply outgoi= ng > + 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 > + > + * debuginfod-client.c (debuginfod_query_server): Add DEBUGINFOD_HEADER= S_FILE > + setting to supply outgoing HTTP headers. > + > 2022-10-17 Frank Ch. Eigler > =20 > * debuginfod.cxx (main): Report libmicrohttpd version. > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-clien= t.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 > #include > #include OK, ctype.h added for isspace. =20 > @@ -447,6 +448,44 @@ add_default_headers(debuginfod_client *client) > free (utspart); > } > =20 > +/* Add HTTP headers found in the given file, one per line. Blank lines o= r invalid > + * headers are ignored. > + */ > +static void > +add_headers_from_file(debuginfod_client *client, const char* filename) > +{ > + int vds =3D client->verbose_fd; > + FILE *f =3D fopen (filename, "r"); > + if (f =3D=3D 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 >=3D 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.=20 > + char *s =3D &buf[0]; > + if (feof(f)) > + break; > + if (fgets (s, sizeof(buf), f) =3D=3D NULL) > + break; > + for (char *c =3D s; *c !=3D '\0'; ++c) > + if (!isspace(*c)) > + goto nonempty; > + continue; > + nonempty: > + ; > + size_t last =3D strlen(s)-1; > + if (s[last] =3D=3D '\n') > + s[last] =3D '\0'; > + int rc =3D 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 >=3D 0). > + } > + fclose (f); > +} > + OK, we will always get here, so f will be closed unless it couldn't be opened. =20 > #define xalloc_str(p, fmt, args...) \ > do \ > @@ -610,6 +649,11 @@ debuginfod_query_server (debuginfod_client *c, > if (maxtime && vfd >=3D 0) > dprintf(vfd, "using max time %lds\n", maxtime); > =20 > + const char *headers_file_envvar; > + headers_file_envvar =3D getenv(DEBUGINFOD_HEADERS_FILE_ENV_VAR); > + if (headers_file_envvar !=3D 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" > =20 > /* 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 > + > + * debuginfod_find_debuginfo.3: Document DEBUGINFOD_HEADERS_FILE. > + > 2022-09-02 Frank Ch. Eigler > =20 > * debuginfod_find_debuginfo.3: Tweaked debuginfod_get_headers docs. > diff --git a/doc/debuginfod-client-config.7 b/doc/debuginfod-client-confi= g.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 n= o consideration > for size, and the client may attempt to download a file of any size. > The default is 0 (infinite size). > =20 > +.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 wit= h > +CRLF, unless that's the system newline convention. Whitespace-only lines > +are skipped. > + > .SH CACHE > =20 > Before each query, the debuginfod client library checks for a need to > diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debugi= nfo.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. > =20 > +\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. > =20 > 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 >=3D 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