[patch] more TLS configuration options for nnrpd

[Julien ÉLIE]

Hi Christian,

> I'll attach another iteration of the patch, against a clean 2.5.4 tree.

Many thanks for this new patch.
I will integrate it into INN and tell you when it has been committed.



> any comments on the defaults I
> chose for the other options? (TLSv1.x, prefer server ciphers).
> 
> One could argue
> 
> a) keep the settings as they were before, as to not change existing
>    system's behaviour
> 
> b) let the defaults be secure (the definition I use here is the one
>    from bettercrypto.org, where I'm a co-author)

We can do "both", my captain!
I would be inclined not to change existing system's behaviour
for the next STABLE 2.5.5 release, and to enforce secure settings
by default for CURRENT 2.6.0.

Anyway, if the admin needs more security, he will be able to tune it
with 2.5.5 and enforce himself more secure settings.


I think I will integrate your patch this way in STABLE 2.5.5 and
CURRENT 2.6.0; unless other



>> I also believe it should be useful to log an error if a string is
>> not recognized ("unknown SSLv4 protocol specified in tlsprotocols"
>> for instance).
> 
> OK.

Thanks for having added that in the patch.



>> In the inn.conf sample, we should put "DEFAULT" (because it matches
>> the default value, when the parameter is unset).
> 
> You mean the "tlsciphers" monster string in samples/inn.conf.in? Shall
> I just leave it empty, which is the default, which in turn leads to
> OpenSSL's default cipher list to be used?

Leaving it empty is the right thing, as you did in the new patch.



>> Isn't there something to do with SASL?  It can add security layers 
>> too...
> 
> I'm far from an expert in SASL, but nnrpd/sasl.c seems to only care
> whether we have a TLS connection, how strong (in bits) the negotiated
> cipher is, and what the client certificate CN is (if any).
> 
> So I don't think any code changes are necessary.

I was concerned with the SASL security layer it can add (depending on
the SASL mechanism used).
It seems that GSSAPI described in RFC 4752 can offer a security layer.
Other mechanisms can use TLS, as I see in RFCs listed here:
     
http://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml

In RFC 4422 (SASL), I read:

3.7. Security Layers

    SASL mechanisms may offer a wide range of services in security
    layers.  Typical services include data integrity and data
    confidentiality.  SASL mechanisms that do not provide a security
    layer are treated as negotiating no security layer.

    If use of a security layer is negotiated in the authentication
    protocol exchange, the layer is installed by the server after
    indicating the outcome of the authentication exchange and installed
    by the client upon receipt of the outcome indication.  In both cases,
    the layer is installed before transfer of further protocol data.  The
    precise position upon which the layer takes effect in the protocol
    data stream is protocol specific.
[...]

so there is probably things to do to ensure that the negotiated layer
is really secure!
Nonetheless, I do not see in Cyrus SASL documentation how we can 
parameter
that. It may be in the configuration of the SASL plugins installed, and
therefore out of scope of the application (?)

I also see that things like SASL_IPLOCALPORT and SASL_IPREMOTEPORT
can be used.



> Two interesting issues arise WRT DH parameters, however:
> 
> a) I've seen that even with 3072 bit RSA keys, the negotiation is
>    using the 1024 bit DH parameters -- I'll have a look into that.
> 
> b) ISTR that to allow ECDH, some setup of the openssl objects is
>    necessary; I'll also look into that.

In case you find anything suspicious or are aware of any improvement
that can be done, do not hesitate to tell us.



>> - TLS server_name extension should be added to nnrpd.
> 
> I suppose this should tie in with the virtualhost stuff in
> readers.conf, which I've never used myself. So if anybody wants to
> guide me WRT integration into the wider scope of things, I'd be happy
> to work on the TLS side of it.

What information would nnrpd need to use this extension?
When virtualhost is true, should we just have tlscertfile and tlskeyfile
support in readers.conf to override the value of these two parameters
from inn.conf, and then use these two values when negotiating a TLS
session with a user matching an access block with virtualhost?

Hmm... or maybe people will want different server names without
having the changes that virtualhost does.  So it would just be
independant of virtualhost, domain and pathhost.  Therefore,
it would mean using the tlscertfile and tlskeyfile parameters
from readers.conf, when overriden.

Yet, after a quick glance, I see that tls_init() is called before
looking at the user permissions in nnrpd.c and changing the order
does not seem right; so I do not see well how the TLS server_name
extension is supposed to work unless it is a global parameter
in readers.conf, not at all related to access blocks!

Do you know from your experience on that subject how that extension
is used by other programs?



>> - AUTHINFO USER/PASS exposes the user's password to eavesdropping.
>> According to RFC 4643:  "Any implementation of this command SHOULD
>> be configurable to disable it whenever a strong encryption layer
>> (such as that provided by [NNTP-TLS]) is not active, and this
>> configuration SHOULD be the default.  The server will use the 483
>> response code to indicate that the datastream is insufficiently
>> secure for the command being attempted."
> 
> I think that's already implemented, nnrpd.8:
> 
>        If compiled against the TLS/SSL libraries, an auth group with 
> the
>        require_ssl parameter set to true only applies if the incoming
>        connection is using TLS, either from the beginning if the -S 
> flag was
>        passed to nnrpd or after a successful use of STARTTLS.
> 
> From skimming the code, it seems to be implemented this way.

The case here is when you connect to nnrpd on port 119 (without any
security layer).  Then you will still see "AUTHINFO USER SASL" in 
response
to CAPABILITIES; and using AUTHINFO USER or AUTHINFO SASL with an 
unsecure
mechanism will still be allowed (PLAIN, LOGIN and EXTERNAL are the 3 
hard-coded
mechanisms in INN to disallow).

Looking at the code, it seems that just preventing to set
PERMcanauthenticatewithoutSSL to true in nnrpd/perm.c when the news
admin wants to enforce that configuration would do the trick, wouldn't 
it?


Another thing I see in TODO is:  "when using SSL, track the amount
of data that's been transferred to the client and periodically
renegotiate the session key."
Is it a recommended practice in bettercrypto too?

-- 
Julien ÉLIE