On Fri, Apr 15, 2011 at 12:16:26PM +0200, Toni Mueller wrote:
> On Fri, 15.04.2011 at 09:43:33 +0000, Ralf Schlatterbeck <issues@roundup-tracker.org> wrote:
> > As far as I understand the first part of this patch, this changes the
> > listening behaviour of roundup-server to per default listen only on
> > localhost.
> 
> the documentation in admin_guide.txt always said that roundup will, by
> default, listen only on localhost, but the code listened on all
> interfaces. Also, the guide said that the option was named 'hostname',
> when, in fact, it is named 'host'. So, users were misled to configure
> a non-existent 'hostname' option, that was subsequently being ignored.

OK, so the documentation was never doing what the implementation did.
And the name of the config-option never matched the implementation. But
the help-text of the config-option in the generated .ini file did
correctly describe the behavior.

> > So I don't think I want to apply this in the current form. It
> > should still be easy to listen on all interfaces (thats what many people
> > want) and we can discuss if 'localhost' is a sane default (I don't think
> > the current main use-case of roundup is that all clients access the
> > server on localhost!). At least that means that almost all users will
> > either have to add an explicit option to roundup-server or add (and
> > configure) a server.ini file.
> 
> Yes, it means that all users will have to have a clause in their
> roundup-server.ini. I also agree that the typical roundup user will
> probably not want to listen only on localhost, but then, there may be
> all sorts of front-ending that makes it unneccessary and/or undesirable
> to have roundup listen on all interfaces, anyway. Eg. if you want to
> have SSL and have a reverse proxy to do that for you.

Right. I was arguing for the users that currently have a working
configuration that could break with the change: I've interpreted the
comment in your patch that an empty space in the config-file will now
mean something different (localhost). So we would break lots of working
configurations out there.

But I was misunderstanding the semantics. After your patch an empty
space, i.e., an explicit

host =

will still mean "bind to all IPs". Whereas a *missing* configuration,
i.e.,

#host = 

(or no config-file at all) will then default to localhost.
So my fear that this would break existing configurations is only true
for installations that do not use a .ini file for the server. These will
need an additional "-n ''" parameter to listen on all interfaces
explicitly.

> > And it breaks the documentation for the option of listening on *all*
> > network interfaces (which is the current default).
> 
> I can't seem to find that documentation. In the origiginal
> admin_guide.txt, it says:
> 
> **hostname**
>   Defines the local hostname to listen for clients on. Only required if
>   "localhost" is not sufficient.

roundup-server -S
produces a config.ini which contains:
# Host name of the Roundup web server instance.
# If empty, listen on all network interfaces.
# Default:
host =

Which would change with the proposed patch to:
[...]
If empty, listen on localhost only.
[...]
Which is definitely wrong. If left *unconfigured* it will listen on
localhost. If explicitly configured to empty, it will still listen on
all interfaces. This is the default behaviour of python's socket module.
So I suggest to *not* change the documentation on that config-item, I'm
open for changing the default to localhost, though (and maybe add
0.0.0.0 as suggested by you, the -n option could also use some more
documentation)

> I understand this to mean that the user needs to configure something,
> anyway, if he wants the server to listen on more than localhost.

Currently not.

Umm I was wrong there. See above.

> In one of my roundup-server.ini files, I have the standard
> 
> host = 0.0.0.0
> 
> clause to have the server listen on all interfaces. I should just add
> that as a commented alternative to the shipped .ini file.

Ah, I've never used that alternative. Yes we should document that.

> > So I suggest that Debian ship their default configuration of
> > roundup-server with this option set to 'localhost' in a debian-specific
> > server.ini file for roundup-server. The /etc/init.d script for
> > roundup-server should probably use the -C option to specify a
> > config-file for roundup-server. This file should probably be located in
> > /etc/default/roundup-server or so.
> 
> Nope to all of these. How deep are you into Debian packaging, and why
> do you vent such hostility?

Um, What did you find hostile in that sentence? I can understand if
you've understood my comment about the "(sometimes annoying) behaviour"
of Debian a little hostile. But thats my personal opinion. I'm always
struggling with each new service I install to find out how to get that
particular service to listen to the network. This is unfortunately
different for each service. But let me state that this does not
constitute a hostility to Debian. And being secure by default is still a
good thing. The paragraph above was meant as a suggestion how to proceed
if the patch doesn't not go in as-is. I didn't want to criticise your
work.

Yes. Probably. I did understand the comment to your patch in a way that
suggested that leaving the config-option empty will no longer listen to
the network but will listen to localhost. Thats wrong (see above), so we
won't break existing config files. We will *still* break users having
written their own init.d script or similar that uses no '-n' option. So
the changes should be documented in the upgrading.txt documentation.

Also note that if Debian shipped the release with your original patch,
users would be insecure because they could asume that an empty option
would listen to localhost. That's wrong.

Proposed patch attached.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
osAlliance member                       email: rsc@osalliance.com