URL:
https://github.com/SSSD/sssd/pull/854
Title: #854: LDAP: Do not require START_TLS for loopback connections
simo5 commented:
"""
```
$ host localhost
Host localhost not found: 3(NXDOMAIN)
```
:-)
More seriously I would like to understand why this is needed (after all we already have
the scary hidden option to avoid TLS, why is that not sufficient ?), and why it is
safe(?), noting that in the code proposed you allow any connection to any port on *all*
localhost addresses (you are checking for IN_LOOPBACKNET not just specifically
IPADDR_LOOPBACK).
The ldapi:// case is not just about encryption but also to access control, you do not get
to interpose a socket that root set up with the right permissions on the filesystem.
In this patch though there is no way to make sure we are restricting access only to the
right peer if the port is above 1024. In a setup where root decides to set up the LDAP
server on a higher port (say because they were told to run the LDAP server as non-root to
improve security and they find it easier to just run on a higher port as that is allowed
to non-root users), an attacker that finds a way to make the ldap server crash can then
run their own ldap server on that port and feed arbitrary information to SSSD (which means
privilege escalation to root almost certainly at that point).
So I guess what I am saying is that it might be acceptable, perhaps with limiting also
ports to < 1024 ...
... except that then that is also something somewhat easy to work around in setups where
admins let containers can be spun on other local addresses and have CAP_NET_ADMIN within
the container (somewhat common in some scenarios).
I do note that using certs in this scenario is hard (which is why I guess @scabrero wants
to avoid TLS) because no sane public authority will give you a cert signed for 127.x.x.x
or the name localhost[.localdomain] as is proper. However providing a tool that root can
use to create a custom, locally trusted cert is probably not too hard and will provide a
much better proposition wrt security.
Soo ... long story short, at the moment I do not see a convincing argument to approve this
patch as is on my side, can you at least better describe the use case ? The commit and
this PR description are quite ... bare.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/854#issuecomment-514783046