[sssd PR#854][opened] LDAP: Do not require START_TLS for loopback connections

[scabrero]

   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(a)suse.de&gt;
"""

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 PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[centos-ci]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[jhrozek]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[jhrozek]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[simo5]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[scabrero]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[simo5]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[simo5]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[mzidek-rh]

  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

[sssd PR#854][comment] LDAP: Do not require START_TLS for loopback connections

[scabrero]

  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

[sssd PR#854][closed] LDAP: Do not require START_TLS for loopback connections

[scabrero]

   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