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