Bug#1025769: HTTP::Server::Simple vs. support for Unix domain sockets

[gregor herrmann]

On Fri, 09 Dec 2022 10:20:23 +, Ivan Shmakov wrote:

(Sorry for the late reply.)

>  > I’m afraid to say that it mostly is not actionable from the
>  > Debian packaging side.  What you propose, in my understanding,
>  > are massive changes to the code, and that’s not what we are
>  > going to do on our own in Debian, but something which needs
>  > to be discussed with upstream and needs to happen there.
>   Not all of the changes I propose are what I’d call ‘massive.’

Right, "massive" was not the best wording.

>  > I can forward your ideas upstream
> 
>   I’d appreciate that.
> 
>  > but it might be easier if you do it yourself as this will
>  > potentially require discussion:
>  > https://github.com/bestpractical/http-server-simple
> 
>   I have no Github account (and I’d rather keep it that way.)

Alright.
 
>   I have a CPAN account, though, so I can report the issues
>   to the CPAN RT instance [1], if the upstream monitor that.
> 
> [1] http://rt.cpan.org/Public/Dist/Display.html?Name=HTTP-Server-Simple

TBH, I don't know if upstream monitors CPAN RT or Github or both or
neither :)

I propose that you try to report your proposals at CPAN RT, and if
this doesn't trigger a reaction I can forward the bug report to
Github later.
 
>  > That’s something relevant for packaging; but I’m not so sure about
>  > your conclusion.  After looking around a bit I think I can say
>  > that lib/HTTP/Server/Simple/CGI.pm needs CGI.pm; libcgi-pm-perl
>  > needs to be in Build-Depends-Indep, otherwise the tests fails
>  > (which was not your point); moving libcgi-pm-perl from Depends to
>  > Recommends does not break autopkgtests so it would be ok at first
>  > sight, the question is if we want to ensure an always working
>  > HTTP::Server::Simple::CGI.  In the end I guess a Recommends would
>  > be arguable but it’s not that clear-cut IMO …
> 
>   IME it rarely is.
> 
>   The caveat here is that all the packages that depend on
>   libhttp-server-simple-perl /and/ use HTTP::Server::Simple::CGI
>   would need to be updated to depend on libcgi-pm-perl as well.

Right, and here we are getting into the question of costs vs.
benefits.

The benefits of moving libcgi-pm-perl from Depends to Recommends are
saving a bit of diskspace and installed packages; the costs are
finding where HTTP::Server::Simple::CGI is used and adding additional
dependencies there.


looks like this might be a non-trivial amount of work. For me
personally the costs outweigh the benefits, but others might come to
different conclusions.
 

Cheers,
gregor

-- 
 .''`.  https://info.comodo.priv.at -- Debian Developer https://www.debian.org
 : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D  85FA BB3A 6801 8649 AA06
 `. `'  Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe
   `-   


signature.asc
Description: Digital Signature

Bug#1025769: HTTP::Server::Simple vs. support for Unix domain sockets

[Ivan Shmakov]

> On 2022-12-09 02:11:07 +0100, gregor herrmann wrote:
> On Thu, 08 Dec 2022 20:07:38 +, Ivan Shmakov wrote:

 >> Attempting to implement Unix domain socket support by subclassing
 >> HTTP::Server::Simple uncovered several issues with the latter,
 >> which I’m going to describe in this report.  (Feel free to clone
 >> it and address them separately as you see fit.)

 > Thanks for your bug report.

 > I’m afraid to say that it mostly is not actionable from the
 > Debian packaging side.  What you propose, in my understanding,
 > are massive changes to the code, and that’s not what we are
 > going to do on our own in Debian, but something which needs
 > to be discussed with upstream and needs to happen there.

Not all of the changes I propose are what I’d call ‘massive.’
To summarize:

(peer_identify): New method, split off
(_process_request): this one.  Fixed: only call sockaddr_in on the
remote address if its family is AF_INET (was: whenever the family
is not AF_INET6.)

(listener_handle): New method, allowing for per-instance listener
sockets (was: using a single HTTPDaemon filehandle.)

(restart): Fixed: supply $^X as an indirect object to exec if $0
is not executable (was: using $0 unconditionally.)

(setup): Fixed: use (same as above) for peeraddr example value (was:
suggesting that peername can be a hostname while peeraddr cannot.)
Mention explicitly in the prose that both peername and peeraddr are
currently the same string value representing the remote IPv6 or IPv4
address, and that the code never attempts (potentially time-consuming)
reverse DNS calls.

Of the above, the first change is the most important for my
use case, and even if implemented only partially:

(_process_request): Fixed: only call sockaddr_in on the remote
address if its family is AF_INET (was: whenever the family
is not AF_INET6.)

it would still be helpful.

The second change is the most invasive.  And the rest seem
straightforward bugfixes, with the exec one being one-liner.

 > I can forward your ideas upstream

I’d appreciate that.

 > but it might be easier if you do it yourself as this will
 > potentially require discussion:
 > https://github.com/bestpractical/http-server-simple

I have no Github account (and I’d rather keep it that way.)

I have a CPAN account, though, so I can report the issues
to the CPAN RT instance [1], if the upstream monitor that.

[1] http://rt.cpan.org/Public/Dist/Display.html?Name=HTTP-Server-Simple

 >> So far as I can tell, it’s perfectly possible to subclass and
 >> use HTTP::Server::Simple without libcgi-pm-perl, except perhaps
 >> with Net::Server (I’m not familiar with the latter and won’t
 >> claim understanding of what the run method does in the case
 >> net_server is supplied.)

 >> The Recommends: relation seems like a better fit in this case:

 > That’s something relevant for packaging; but I’m not so sure about
 > your conclusion.  After looking around a bit I think I can say
 > that lib/HTTP/Server/Simple/CGI.pm needs CGI.pm; libcgi-pm-perl
 > needs to be in Build-Depends-Indep, otherwise the tests fails
 > (which was not your point); moving libcgi-pm-perl from Depends to
 > Recommends does not break autopkgtests so it would be ok at first
 > sight, the question is if we want to ensure an always working
 > HTTP::Server::Simple::CGI.  In the end I guess a Recommends would
 > be arguable but it’s not that clear-cut IMO …

IME it rarely is.

The caveat here is that all the packages that depend on
libhttp-server-simple-perl /and/ use HTTP::Server::Simple::CGI
would need to be updated to depend on libcgi-pm-perl as well.

Another alternative is to put HTTP::Server::Simple::CGI into
its own binary package, with the dependencies declared as
follows:

Package: libhttp-server-simple-perl
Depends: perl:any
Recommends: libhttp-server-simple-cgi-perl

Package: libhttp-server-simple-cgi-perl
Depends: libcgi-pm-perl, libhttp-server-simple-perl

It doesn’t avoid the need to check and possibly update all
of the depending packages, though.

-- 
FSF associate member #7257  http://am-1.org/~ivan/

Bug#1025769: HTTP::Server::Simple vs. support for Unix domain sockets

[gregor herrmann]

On Thu, 08 Dec 2022 20:07:38 +, Ivan Shmakov wrote:

>   Attempting to implement Unix domain socket support by
>   subclassing HTTP::Server::Simple uncovered several issues
>   with the latter, which I’m going to describe in this report.
>   (Feel free to clone it and address them separately as you
>   see fit.)

Thanks for your bug report.

I'm afraid to say that it mostly is not actionable from the Debian
packaging side. What you propose, in my understanding, are massive
changes to the code, and that's not what we are going to do on our
own in Debian, but something which needs to be discussed with
upstream and needs to happen there.

I can forward your ideas upstream but it might be easier if you do it
yourself as this will potentially require discussion:
https://github.com/bestpractical/http-server-simple
 
>   Lastly, the package’s absolute dependency on libcgi-pm-perl
>   is arguably at odds with Policy 7.2:
> 
> http://debian.org//doc/debian-policy/ch-relationships.html
> 
>   Depends
> 
> This declares an absolute dependency.  […]
> 
> The Depends field should be used if the depended-on package
> is required for the depending package to provide a significant
> amount of functionality.
> 
>   So far as I can tell, it’s perfectly possible to subclass
>   and use HTTP::Server::Simple without libcgi-pm-perl, except
>   perhaps with Net::Server (I’m not familiar with the latter
>   and won’t claim understanding of what the run method does
>   in the case net_server is supplied.)
> 
>   For instance, the aforementioned test suite [2] completes
>   successfully (aside of the SIGHUP issue above) even though
>   I’ve circumvented the dependency by creating an otherwise
>   empty Provides: libcgi-pm-perl package [3] with nope.sh [4]:
> 
> $ fakeroot -- nope  libcgi-pm-perl 
> 
> [3] http://am-1.org/~ivan/dist/no-libcgi-pm-perl_0.1_all.deb
> [4] http://am-1.org/~ivan/src/misc-utils-is-2020/nope.sh
> 
>   The Recommends: relation seems like a better fit in this case:
> 
>Recommends
> 
>  This declares a strong, but not absolute, dependency.
> 
>  The Recommends field should list packages that would be found
>  together with this one in all but unusual installations.

That's something relevant for packaging; but I'm not so sure about
your conclusion. After looking around a bit I think I can say that
lib/HTTP/Server/Simple/CGI.pm needs CGI.pm; libcgi-pm-perl needs to
be in Build-Depends-Indep, otherwise the tests fails (which was not
your point); moving libcgi-pm-perl from Depends to Recommends does
not break autopkgtests so it would be ok at first sight, the question
is if we want to ensure an always working HTTP::Server::Simple::CGI.
In the end I guess a Recommends would be arguable but it's not that
clear-cut IMO …


Cheers,
gregor

-- 
 .''`.  https://info.comodo.priv.at -- Debian Developer https://www.debian.org
 : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D  85FA BB3A 6801 8649 AA06
 `. `'  Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe
   `-   


signature.asc
Description: Digital Signature

Bug#1025769: HTTP::Server::Simple vs. support for Unix domain sockets

[Ivan Shmakov]

Package: libhttp-server-simple-perl
Version: 0.52-2
Severity: minor

[Please do not Cc: me, as I’m “on the list,” so to say, and
I try to reserve my inbox for private communication only.
I’d have set up Mail-Followup-To:, but there doesn’t seem
to be a way to make it point to the report being filed.]

In true multiuser setups, it’s much easier to control access
to Unix domain sockets than to INET6 and INET ones: for the
first, chmod(1) and chacl(1) can be used, while the second
is likely to require configuring nftables or iptables as root.

Attempting to implement Unix domain socket support by
subclassing HTTP::Server::Simple uncovered several issues
with the latter, which I’m going to describe in this report.
(Feel free to clone it and address them separately as you
see fit.)


As an aside, note that both Apache mod_proxy (as of 2.4.7)
and Lighttpd mod_proxy provide support for passing HTTP
traffic to a Unix domain socket; consider, e. g.:

## Lighttpd
$HTTP["url"] =~ "^/(example)(/|$)" {
  proxy.server  = ("" => (("host" => "/where/is/%1.socket")))
}

## From http://httpd.apache.org/docs/2.4/mod/mod_proxy.html

  # Unix sockets require 2.4.7 or later
  SetHandler  "proxy:unix:/path/to/app.sock|fcgi://localhost/"


Making HTTP requests via a Unix socket is supported by
curl(1) as well, e. g.:

$ curl --unix-socket ~/my.server.socket http://-/foo 


The first issue is that while most of the HTTP::Server::Simple
functionality is implemented as separate methods, easy to
override in a subclass, calls to sockaddr_in6 and sockaddr_in
are hardcoded into _process_request:

421 my $remote_sockaddr = getpeername( $self->stdio_handle );
422 my $family = sockaddr_family($remote_sockaddr);
423 
424 my ( $iport, $iaddr ) = $remote_sockaddr 
425 ? ( ($family == AF_INET6) ? 
sockaddr_in6($remote_sockaddr)
426   : 
sockaddr_in($remote_sockaddr) )
427 : (undef,undef);

I believe that this code should be split off into a separate
method (or at least the sockaddr_in call should be explicitly
guarded as sockaddr_in6 already is), like:

sub peer_identify
{
my ($self, $peer) = @_;
my $remote_sockaddr = getpeername( $self->stdio_handle );
my $family = sockaddr_family($remote_sockaddr);

my ( $iport, $iaddr )
= ( ($family == AF_INET6) ? sockaddr_in6 ($remote_sockaddr)
($family == AF_INET)  ? sockaddr_in  ($remote_sockaddr)
: (undef, undef) );
...
return ("peername" => ..., "peeraddr" => ..., "peerport" => ...);
}

This is critical to my UNIX subclass [1], as the (currently
unguarded) call to sockaddr_in on a Unix name results in an
exception, which I worked-around by overriding
Socket::unpack_sockaddr_in with a function that returns undef
if the original function throws an exception on the value
passed while unpack_sockaddr_un doesn’t (see [1].)

[1] http://am-1.org/~ivan/src/iroca-2022/lib/HTTP/Server/Simple/UNIX.pm
[2] http://am-1.org/~ivan/src/iroca-2022/test/53-un-serv.perl


Another issue is that the documentation suggests that
peername and peeraddr may be different values (presumably
the former would be an IP resolved into a hostname via rDNS,
while the latter is the address in the numeric form):

563   ITEM/METHOD   Set toExample
…
570   port Received Port 80, 8080
571   peername Remote name   "200.2.4.5", "foo.com"
572   peeraddr Remote address"200.2.4.5", "::1"
573   peerport Remote port   42424
574   localnameLocal interface   "localhost", "myhost.com"

This is not the case the way _process_request is currently
implemented:

447 my ( $file, $query_string )
448 = ( $request_uri =~ /([^?]*)(?:\?(.*))?/s );# split at ?
449 
450 $self->setup(
…
458 peername => $peeraddr,
459 peeraddr => $peeraddr,
460 peerport => $iport,
461 );

And before that, $peeraddr is obtained from Socket::getnameinfo:

429 my $loopback = ($family == AF_INET6) ? "::1" : "127.0.0.1";
430 my $peeraddr = $loopback;
431 if ($iaddr) {
432 my ($host_err,$addr, undef) = 
Socket::getnameinfo($remote_sockaddr,Socket::NI_NUMERICHOST);
433 warn ($host_err) if $host_err;
434 $peeraddr = $addr || $loopback;
435 }

AIUI, the end result is that /both/ peername and peeraddr will
be the numeric IP address (thanks to the Socket::NI_NUMERICHOST
flag used.)

This behavior (which involves no rDNS lookups) I find