Hi Michael,
On Thu, 2025-03-13 at 12:05 +0100, Michael Trapp wrote:
> Use MHD_OPTION_SOCK_ADDR to bind the http listen socket to a single address.
> The address can be any IPv4 or IPv6 address configured on the system:
> --http-addr=127.0.0.1
> --http-addr=::1
> --http-addr='ANY_ACTIVE_LOCAL_IP_ADDRESS'
> As debuginfod does not include any security features, a listen on the
> localhost address is sufficient for a HTTP/HTTPS reverse-proxy setup.
I like the idea behind this patch. But is the option name clear enough?
Should it maybe be --bind-address= or --listen-address= simply --addr=
like it is --port=?
Or even combine it as --listen=addr:port ?
Does it make sense to allow multiple local addresses to bind to? Like
both 127.0.01 and ::1 ? I guess maybe not because if you are using this
you probably are behind a proxy anyway. So maybe it could simply be --
listen-local and debuginfod figures out whether that is ipv4
(127.0.0.1) and/or ipv6 (::1) ?
I don't know what other programs do, can we follow some semi-standard?
The code itself does look ok, although I think it could be simplified a
little if we go for something like --listen-local only (assuming that
makes sense).
Cheers,
Mark
> ---
> debuginfod/debuginfod.cxx | 115 ++++++++++++++++++++++++++------------
> doc/debuginfod.8 | 5 ++
> 2 files changed, 84 insertions(+), 36 deletions(-)
>
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 0edd57cb..30916093 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -81,6 +81,7 @@ extern "C" {
> #include <math.h>
> #include <float.h>
> #include <fnmatch.h>
> +#include <arpa/inet.h>
>
>
> /* If fts.h is included before config.h, its indirect inclusions may not
> @@ -481,6 +482,8 @@ static const struct argp_option options[] =
> #define ARGP_KEY_METADATA_MAXTIME 0x100C
> { "metadata-maxtime", ARGP_KEY_METADATA_MAXTIME, "SECONDS", 0,
> "Number of seconds to limit metadata query run time, 0=unlimited.", 0 },
> +#define ARGP_KEY_HTTP_ADDR 0x100D
> + { "http-addr", ARGP_KEY_HTTP_ADDR, "ADDR", 0, "HTTP address to listen
> on.", 0 },
> { NULL, 0, NULL, 0, NULL, 0 },
> };
>
> @@ -512,6 +515,8 @@ static volatile sig_atomic_t sigusr1 = 0;
> static volatile sig_atomic_t forced_groom_count = 0;
> static volatile sig_atomic_t sigusr2 = 0;
> static unsigned http_port = 8002;
> +static struct sockaddr_in6 http_sockaddr;
> +static string addr_info = "";
> static bool webapi_cors = false;
> static unsigned rescan_s = 300;
> static unsigned groom_s = 86400;
> @@ -753,6 +758,16 @@ parse_opt (int key, char *arg,
> requires_koji_sigcache_mapping = true;
> break;
> #endif
> + case ARGP_KEY_HTTP_ADDR:
> + if (inet_pton(AF_INET, arg,
> &(((sockaddr_in*)&http_sockaddr)->sin_addr)) == 1)
> + http_sockaddr.sin6_family = AF_INET;
> + else
> + if (inet_pton(AF_INET6, arg, &http_sockaddr.sin6_addr) == 1)
> + http_sockaddr.sin6_family = AF_INET6;
> + else
> + argp_failure(state, 1, EINVAL, "HTTP address");
> + addr_info = arg;
> + break;
> // case 'h': argp_state_help (state, stderr,
> ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
> default: return ARGP_ERR_UNKNOWN;
> }
> @@ -5596,6 +5611,8 @@ main (int argc, char *argv[])
> fdcache_prefetch = 64; // guesstimate storage is this much less costly
> than re-decompression
>
> /* Parse and process arguments. */
> + memset(&http_sockaddr, 0, sizeof(http_sockaddr));
> + http_sockaddr.sin6_family = AF_UNSPEC;
> int remaining;
> (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
> if (remaining != argc)
> @@ -5702,50 +5719,75 @@ main (int argc, char *argv[])
> #endif
> | MHD_USE_DEBUG); /* report errors to stderr */
>
> - // Start httpd server threads. Use a single dual-homed pool.
> - MHD_Daemon *d46 = MHD_start_daemon (mhd_flags, http_port,
> - NULL, NULL, /* default accept policy */
> - handler_cb, NULL, /* handler callback */
> - MHD_OPTION_EXTERNAL_LOGGER,
> - error_cb, NULL,
> - MHD_OPTION_THREAD_POOL_SIZE,
> - (int)connection_pool,
> - MHD_OPTION_END);
> -
> - MHD_Daemon *d4 = NULL;
> - if (d46 == NULL)
> + MHD_Daemon *dsa = NULL,
> + *d4 = NULL,
> + *d46 = NULL;
> +
> + if (http_sockaddr.sin6_family != AF_UNSPEC)
> {
> - // Cannot use dual_stack, use ipv4 only
> - mhd_flags &= ~(MHD_USE_DUAL_STACK);
> - d4 = MHD_start_daemon (mhd_flags, http_port,
> - NULL, NULL, /* default accept policy */
> + if (http_sockaddr.sin6_family == AF_INET)
> + ((sockaddr_in*)&http_sockaddr)->sin_port = htons(http_port);
> + if (http_sockaddr.sin6_family == AF_INET6)
> + http_sockaddr.sin6_port = htons(http_port);
> + // Start httpd server threads on socket addr:port.
> + dsa = MHD_start_daemon (mhd_flags & ~MHD_USE_DUAL_STACK, http_port,
> + NULL, NULL, /* default accept policy */
> handler_cb, NULL, /* handler callback */
> MHD_OPTION_EXTERNAL_LOGGER,
> error_cb, NULL,
> - (connection_pool
> - ? MHD_OPTION_THREAD_POOL_SIZE
> - : MHD_OPTION_END),
> - (connection_pool
> - ? (int)connection_pool
> - : MHD_OPTION_END),
> + MHD_OPTION_SOCK_ADDR,
> + (struct sockaddr *) &http_sockaddr,
> + MHD_OPTION_THREAD_POOL_SIZE,
> + (int)connection_pool,
> MHD_OPTION_END);
> - if (d4 == NULL)
> + }
> + else
> + {
> + // Start httpd server threads. Use a single dual-homed pool.
> + d46 = MHD_start_daemon (mhd_flags, http_port,
> + NULL, NULL, /* default accept policy */
> + handler_cb, NULL, /* handler callback */
> + MHD_OPTION_EXTERNAL_LOGGER,
> + error_cb, NULL,
> + MHD_OPTION_THREAD_POOL_SIZE,
> + (int)connection_pool,
> + MHD_OPTION_END);
> + addr_info = "IPv4 IPv6";
> + if (d46 == NULL)
> {
> - sqlite3 *database = db;
> - sqlite3 *databaseq = dbq;
> - db = dbq = 0; // for signal_handler not to freak
> - sqlite3_close (databaseq);
> - sqlite3_close (database);
> - error (EXIT_FAILURE, 0, "cannot start http server at port %d",
> - http_port);
> + // Cannot use dual_stack, use ipv4 only
> + mhd_flags &= ~(MHD_USE_DUAL_STACK);
> + d4 = MHD_start_daemon (mhd_flags, http_port,
> + NULL, NULL, /* default accept policy */
> + handler_cb, NULL, /* handler callback */
> + MHD_OPTION_EXTERNAL_LOGGER,
> + error_cb, NULL,
> + (connection_pool
> + ? MHD_OPTION_THREAD_POOL_SIZE
> + : MHD_OPTION_END),
> + (connection_pool
> + ? (int)connection_pool
> + : MHD_OPTION_END),
> + MHD_OPTION_END);
> + addr_info = "IPv4";
> }
> -
> }
> - obatched(clog) << "started http server on"
> - << (d4 != NULL ? " IPv4 " : " IPv4 IPv6 ")
> - << "port=" << http_port
> - << (webapi_cors ? " with cors" : "")
> - << endl;
> + if (d4 == NULL && d46 == NULL && dsa == NULL)
> + {
> + sqlite3 *database = db;
> + sqlite3 *databaseq = dbq;
> + db = dbq = 0; // for signal_handler not to freak
> + sqlite3_close (databaseq);
> + sqlite3_close (database);
> + error (EXIT_FAILURE, 0, "cannot start http server on %s port %d",
> + addr_info.c_str(), http_port);
> + }
> +
> + obatched(clog) << "started http server on "
> + << addr_info
> + << " port=" << http_port
> + << (webapi_cors ? " with cors" : "")
> + << endl;
>
> // add maxigroom sql if -G given
> if (maxigroom)
> @@ -5869,6 +5911,7 @@ main (int argc, char *argv[])
> pthread_join (it, NULL);
>
> /* Stop all the web service threads. */
> + if (dsa) MHD_stop_daemon (dsa);
> if (d46) MHD_stop_daemon (d46);
> if (d4) MHD_stop_daemon (d4);
>
> diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
> index 1cf9a18f..6774f8c8 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -154,6 +154,11 @@ listen, to service HTTP requests. Both IPv4 and IPV6
> sockets are
> opened, if possible. The webapi is documented below. The default
> port number is 8002.
>
> +.TP
> +.B "\-\-http\-addr=ADDR"
> +Set the IP address (any local IPv4/IPv6 address of the system) on
> +which debuginfod should listen, to service HTTP requests.
> +
> .TP
> .B "\-\-cors"
> Add CORS-related response headers and OPTIONS method processing.
