Hi Mark, > Am 19.03.2025 um 13:35 schrieb Mark Wielaard <m...@klomp.org>: > > 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 ?
that’s interesting, initially I wanted to use ‘--addr=‘ because of the already available ‘-—port=‘ option. This would be more consistent but less descriptive. If this is an option, I would prefer anything like http-addr/listen-addr/bind-addr. > 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) ? There might be use cases but I’m not aware of any and for my use case, debuginfod behind a reverse proxy, a listen on a localhost address is sufficient. Either IPv4 or IPv6 would work, more important is the fact, that the listen is not exposed and there is no need for a ruleset on a firewall. The change uses the MHD_OPTION_SOCK_ADDR of libmicrohttpd, which can be used for a single sockaddr. I guess multiple listen socket must be handled in the application and connections have to be send to libmicrohttpd with MHD_add_connection. From https://www.gnu.org/software/libmicrohttpd/manual/libmicrohttpd.html MHD_USE_NO_LISTEN_SOCKETRun the HTTP server without any listen socket. This option only makes sense if MHD_add_connection is going to be used exclusively to connect HTTP clients to the HTTP server. This option is incompatible with using a thread pool; if it is used, MHD_OPTION_THREAD_POOL_SIZE is ignored. This is a different concept and out of scope of this change. The initial intention was a bind to 127.0.0.1 or ::1 and therefore ‘—-listen-local4’ and ‘—-listen-local6’ is sufficient. I would prefer these explicit options over a generic ‘—-listen-local’ because a reverse proxy setup for a local service should have a proxy config like 'proxy_pass http://127.0.0.1:8002;’ and from a security perspective the configuration should be as strict as possible. If you want to setup the service on 127.0.0.1 this should be possible without any need for customisation of the name resolution. Binding any other local address does not need additional code and therefore I decided to provide the generic option ‘—-http-addr=‘ which supports any active IPv4/IPv6 address. I see no need for this generic version in my setup, but it’s available and might be interesting for other setups. > > 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). In summary, I would use the generic option, as you’ve suggested with, '—-listen-address=_IPv4/IPv6_ADDRESS_’ because it covers all possible addresses. The more restrictive option for localhost addresses could be '—-listen-local4’ to bind to 127.0.0.1 or '—-listen-local6’ for a bind to ::1. What do you think about that? Thanks, Michael > > 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. >
