URL: https://github.com/SSSD/sssd/pull/854 Author: scabrero Title: #854: LDAP: Do not require START_TLS for loopback connections Action: opened
PR body: """ If the ldap uri is resolved to a loopback address then do not require START_TLS.
Signed-off-by: Samuel Cabrero scabrero@suse.de """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/854/head:pr854 git checkout pr854
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-514695584
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
jhrozek commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-514748771
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
jhrozek commented: """ @simo5 this is one of the things I don't dare to include in the project without your blessing :-)
So me and @scabrero talked about this over e-mail initially. There are some people who would like to run an LDAP server on ldap://localhost. While we both agreed that supporting ldapi:// might be a better way, what do you think allowing non-encrypted auth towards localhost?
I was thinking about someone listening to the traffic on the localhost, but then you need either root or at least CAP_NET_RAW/CAP_NET_ADMIN.. """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-514751512
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
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
scabrero commented: """ @simo5, as you said the idea is to avoid having to set up certificates (or undocumented options) when sssd and ldap run in the same machine. I understand the potential security issues you pointed out, so would it be acceptable after adding these additional restrictions to sdap_is_loopback_address?
+ Restrict to INADDR_LOOPBACK instead the whole IN_LOOPBACKNET + Restrict to ports below 1024
IMHO it is less secure to rely on that hidden option that allows everything than the proposed changes after we agree the full list of "restrictions".
I will write a more extensive commit message if you are OK with the changes :) """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-515010382
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
simo5 commented: """ @scabrero To be honest I would prefer to write a tool that simply generates a local certificate, adds it to the local machine trust store and gives it to the LDAP server. Then there is no need for exceptions on the SSSD side, and a local attacker can't impersonate the server no matter what. Is that hard to do for some reason ? """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-515029731
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
simo5 commented: """ @scabrero @jhrozek I see no discussion ensued, just to be clear I didn't give and didn't mean my messages to be a complete NACK on the approach. But I wanted a better analysis of the situation and options before giving my blessing to any approach. HTH.
"""
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-516595230
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
mzidek-rh commented: """ Hi @scabrero ! Is this PR abandoned? Have you proceeded with the locally generated certificates as @simo5 suggested? Should we close this PR? """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-545516329
URL: https://github.com/SSSD/sssd/pull/854 Title: #854: LDAP: Do not require START_TLS for loopback connections
scabrero commented: """ Yes, we can close this.
Thanks. """
See the full comment at https://github.com/SSSD/sssd/pull/854#issuecomment-546864982
URL: https://github.com/SSSD/sssd/pull/854 Author: scabrero Title: #854: LDAP: Do not require START_TLS for loopback connections Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/854/head:pr854 git checkout pr854
sssd-devel@lists.fedorahosted.org
-
centos-ci -
jhrozek
-
mzidek-rh
-
scabrero
-
simo5