Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ldapi:// URLs #6484

Closed
wants to merge 6 commits into from
Closed

Add support for ldapi:// URLs #6484

wants to merge 6 commits into from

Conversation

minfrin
Copy link
Contributor

@minfrin minfrin commented Dec 9, 2022

Make sssd aware of unix domain sockets, allowing connections to local LDAP servers.

Make use of struct sockaddr and socklen_t consistent across the code.

Resolves: #1651

:feature: Add support for ldapi:// URLs

Make sssd aware of unix domain sockets, allowing connections to local
LDAP servers.

Make use of struct sockaddr and socklen_t consistent across the code.
@alexey-tikhonov alexey-tikhonov added Waiting for review no-backport This should go to target branch only. labels Dec 9, 2022
@minfrin
Copy link
Contributor Author

minfrin commented Dec 9, 2022

This was developed for, and backports cleanly to, the v2.7 branch.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 9, 2022

This was developed for, and backports cleanly to, the v2.7 branch.

At this point RFEs target v2.9

@alexey-tikhonov
Copy link
Member

@sumit-bose
Copy link
Contributor

Hi,

thank you for your patience. Please have a look at the Details of the failed ci / build (pull_request) check. It looks somthing like

diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c
index bdfbcf245..1ef5a9019 100644
--- a/src/tests/cmocka/test_dyndns.c
+++ b/src/tests/cmocka/test_dyndns.c
@@ -205,7 +205,7 @@ void will_return_getifaddrs(const char *ifname, const char *straddr,
 void dyndns_test_sss_iface_addr_get_misc(void **state)
 {
     struct sss_iface_addr addrs[3];
-    struct sockaddr_storage ss[3];
+    struct sockaddr ss[3];
 
     addrs[0].prev = NULL;
     addrs[0].next = &addrs[1];

is missing.

In general I'm fine with the removal of struct sockaddr_storage but I wonder if it is really needed to carry around the sockaddr_len. If I haven't miss anything the value is only used in your patch when calling sdap_connect_send(). Here it replaces sizeof(struct sockaddr_storage), which is just the maximum of the sizes of the different struct sockaddr_*. If I follow the variable it ends up in the call connect(fd, addr, addr_len); in sssd_async_connect_send(). Since it was working before and you added the support for UNIX domain sockets with struct sockaddr_un I wonder if the size of struct sockaddr_un is really important for the call to connect() or if it would also work with the size of struct sockaddr_storage?

And if the size is important it still might be easier to move the switch-block from resolv_get_sockaddr_address_index() to sssd_async_connect_send() and determing the length where it is needed.

bye,
Sumit

@minfrin
Copy link
Contributor Author

minfrin commented Jan 16, 2023

thank you for your patience. Please have a look at the Details of the failed ci / build (pull_request) check. It looks somthing like

diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c
index bdfbcf245..1ef5a9019 100644
--- a/src/tests/cmocka/test_dyndns.c
+++ b/src/tests/cmocka/test_dyndns.c
@@ -205,7 +205,7 @@ void will_return_getifaddrs(const char *ifname, const char *straddr,
 void dyndns_test_sss_iface_addr_get_misc(void **state)
 {
     struct sss_iface_addr addrs[3];
-    struct sockaddr_storage ss[3];
+    struct sockaddr ss[3];
 
     addrs[0].prev = NULL;
     addrs[0].next = &addrs[1];

is missing.

Thank you for catching this. I've just submitted the fix and it appears to have worked.

In general I'm fine with the removal of struct sockaddr_storage but I wonder if it is really needed to carry around the sockaddr_len.

Alas the length is important, as the length indicates the type of unix domain socket. Different lengths indicate whether the socket is anonymous, a file, etc. You need to keep the length it gives you, otherwise you get all sorts of headaches.

@alexey-tikhonov
Copy link
Member

In general, looks good to me.

@sumit-bose
Copy link
Contributor

Hi,

if I understand it correctly the length is only important for anonymous/unnamed and abstract (Linux only) sockets which both are most probably not relevant here. But I'm fine with keeping the length for completeness.

Can you enhance the sdap_get_server_ip_str() function so that the socket path is returned. Currently only localhost is printed in the logs but having the actual path would be better imo.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Hi @minfrin,

besides addressing recent comment from Sumit, could you please also add a release note to the commit message, following template:

# :feature: New feature description.

Those are used to automatically generate upstream release notes.

And there seems to be another one issue.
When SSSD is configured to use 'ldapi://' and 'auth_provider = ldap' with this patch, it fails on auth attempt.
In this case SSSD attempts 'START-TLS' over insecure connection before attempting to bind with user credentials. And this fails at least with 389ds:

conn=4 fd=65 slot=65 connection from local to /run/slapd-example1.socket
conn=4 op=0 EXT oid="1.3.6.1.4.1.1466.20037" name="start_tls_plugin"
conn=4 op=0 RESULT err=0 tag=120 nentries=0 wtime=0.000065976 optime=0.000030469 etime=0.000094887
conn=4 op=-1 fd=65 Disconnect - Network address type not supported.

ERR - configure_pr_socket - PR_SetSocketOption(PR_SockOpt_NoDelay) failed, Netscape Portable Runtime error -5968 (TCP-specific function attempted on a non-TCP file descriptor.)
ERR - configure_pr_socket - setsockopt(TCP_LINGER2) failed, error 95 (Operation not supported)
ERR - configure_pr_socket - setsockopt(TCP_KEEPIDLE) failed, error 95 (Operation not supported)

(we didn't check other LDAP servers but quick search hints this also might be the same with OpenLDAP).

There is undocumented SSSD config options ldap_auth_disable_tls_never_use_in_production, setting it to true works around the issue. But we clearly can't release this feature with recommendation to use ..._never_use_in_production config option :)
Probably we should have disable_tls = true by default for 'ldapi://'... But this is something to discuss.

@alexey-tikhonov
Copy link
Member

@minfrin, hi, do you plan any additional work on this?

@minfrin
Copy link
Contributor Author

minfrin commented Mar 17, 2023

Yes - been a little snowed under though.

@alexey-tikhonov
Copy link
Member

Hi @minfrin,

JFYI: we plan 2.9.0 upstream release "soon". After 2.9.0 is out, RFEs will target 2.10.0

@minfrin
Copy link
Contributor Author

minfrin commented Mar 28, 2023

This week is mad, let me look at this next, maybe over this weekend.

@minfrin
Copy link
Contributor Author

minfrin commented Apr 2, 2023

if I understand it correctly the length is only important for anonymous/unnamed and abstract (Linux only) sockets which both are most probably not relevant here. But I'm fine with keeping the length for completeness.

I spent quite a while trying to find a constant that could be reliably used to avoid passing around the length of the sockaddr, and eventually concluded there isn't a safe one. You need to use what was given to you, or it doesn't work.

Can you enhance the sdap_get_server_ip_str() function so that the socket path is returned. Currently only localhost is printed in the logs but having the actual path would be better imo.

Done in 5818ac0

@minfrin
Copy link
Contributor Author

minfrin commented Apr 2, 2023

besides addressing recent comment from Sumit, could you please also add a release note to the commit message, following template:

# :feature: New feature description.

Those are used to automatically generate upstream release notes.

Done.

And there seems to be another one issue. When SSSD is configured to use 'ldapi://' and 'auth_provider = ldap' with this patch, it fails on auth attempt. In this case SSSD attempts 'START-TLS' over insecure connection before attempting to bind with user credentials. And this fails at least with 389ds:

conn=4 fd=65 slot=65 connection from local to /run/slapd-example1.socket
conn=4 op=0 EXT oid="1.3.6.1.4.1.1466.20037" name="start_tls_plugin"
conn=4 op=0 RESULT err=0 tag=120 nentries=0 wtime=0.000065976 optime=0.000030469 etime=0.000094887
conn=4 op=-1 fd=65 Disconnect - Network address type not supported.

ERR - configure_pr_socket - PR_SetSocketOption(PR_SockOpt_NoDelay) failed, Netscape Portable Runtime error -5968 (TCP-specific function attempted on a non-TCP file descriptor.)
ERR - configure_pr_socket - setsockopt(TCP_LINGER2) failed, error 95 (Operation not supported)
ERR - configure_pr_socket - setsockopt(TCP_KEEPIDLE) failed, error 95 (Operation not supported)

(we didn't check other LDAP servers but quick search hints this also might be the same with OpenLDAP).

There is undocumented SSSD config options ldap_auth_disable_tls_never_use_in_production, setting it to true works around the issue. But we clearly can't release this feature with recommendation to use ..._never_use_in_production config option :) Probably we should have disable_tls = true by default for 'ldapi://'... But this is something to discuss.

I am testing with 389ds and don't see this - can you give more detail as to how to trigger this?

@minfrin
Copy link
Contributor Author

minfrin commented Apr 3, 2023

I am testing with 389ds and don't see this - can you give more detail as to how to trigger this?

My config looks like this:

[domain/seawitch-horizon]
id_provider = ldap
autofs_provider = ldap
auth_provider = ldap
chpass_provider = ldap
#sudo_provider = ldap
#access_provider = ldap

# base DN set from the file /etc/device/system/auth/sssd/ldap/seawitch-horizon/suffix.d/suffix.txt
ldap_uri = ldapi://%2frun%2fslapd-seawitch.socket
ldap_search_base = dc=aaa,dc=bbb,dc=ccc
ldap_sasl_mech = EXTERNAL
ldap_user_ssh_public_key = nsSshPublicKey



#ldap_sudo_search_base = ou=SUDOers,

ldap_autofs_map_object_class = automountMap
ldap_autofs_map_name = ou
ldap_autofs_entry_object_class = automount
ldap_autofs_entry_key = cn
ldap_autofs_entry_value = automountInformation

cache_credentials = True
debug_level=6

[sssd]
services = nss, pam, ssh, sudo, autofs
domains = seawitch-horizon

[nss]
homedir_substring = /home

[pam]

[sudo]

@alexey-tikhonov
Copy link
Member

I am testing with 389ds and don't see this - can you give more detail as to how to trigger this?

Did you use user auth (like ssh user@localhost or su)?
If 'yes', could you please paste domain logs after 'START TLS'?

@minfrin
Copy link
Contributor Author

minfrin commented Apr 6, 2023

Did you use user auth (like ssh user@localhost or su)? If 'yes', could you please paste domain logs after 'START TLS'?

I do yes, but I'm probably configured differently to you, which is why everything just works for me. Can you share your exact configuration so I can reproduce what you're seeing?

@alexey-tikhonov
Copy link
Member

I do yes, but I'm probably configured differently to you, which is why everything just works for me. Can you share your exact configuration so I can reproduce what you're seeing?

Is there 'START TLS' in sssd logs in your setup? Does it succeed?

I talked to 389ds maintainers and TLS isn't supported for local unix sockets...

@aborah-sudo, could you please share sssd.conf you used?

@aborah-sudo
Copy link
Contributor

[root@fedora37 sssd]# cat sssd.conf
[sssd]
config_file_version = 2
services = nss, pam
domains = example1

[domain/example1]
ldap_search_base = dc=example,dc=test
id_provider = ldap
auth_provider = ldap
ldap_user_home_directory = /home/%u
ldap_uri = ldapi://%2Frun%2Fslapd-example1.socket
ldap_tls_cacert = /etc/openldap/cacerts/cacert.pem
use_fully_qualified_names = False
debug_level = 9
disable_tls = True

[pam]
debug_level = 9

Below commands works well . Only the ssh part does not work.

[root@fedora37 sssd]# systemctl stop sssd ;rm -rf /var/log/sssd/* ; rm -rf /var/lib/sss/db/* ; systemctl start sssd[root@fedora37 sssd]# systemctl stop sssd ;rm -rf /var/log/sssd/* ; rm -rf /var/lib/sss/db/* ; systemctl start sssd
[root@fedora37 sssd]# id foo1
uid=14583101(foo1) gid=14564100(ldapusers) groups=14564100(ldapusers)
[root@fedora37 sssd]# sssctl user-checks foo1
user: foo1
action: acct
service: system-auth

SSSD nss user lookup result:

  • user name: foo1
  • user id: 14583101
  • group id: 14564100
  • gecos: foo1 User
  • home directory:
  • shell: /bin/bash

SSSD InfoPipe user lookup result:

  • name: foo1
  • uidNumber: 14583101
  • gidNumber: 14564100
  • gecos: foo1 User
  • homeDirectory: not set
  • loginShell: /bin/bash

testing pam_acct_mgmt

pam_acct_mgmt: Success

PAM Environment:

  • no env -
    [root@fedora37 sssd]# getent passwd foo1
    foo1:*:14583101:14564100:foo1 User::/bin/bash
    [root@fedora37 sssd]# ssh foo1@localhost
    foo1@localhost's password:
    Permission denied, please try again.
    foo1@localhost's password:
    Permission denied, please try again.
    foo1@localhost's password:
    foo1@localhost: Permission denied (publickey,gssapi-keyex,gssapi-with-mic,password).
    [root@fedora37 sssd]#

@minfrin
Copy link
Contributor Author

minfrin commented Apr 6, 2023

I wonder if this line is bouncing the TLS on:

ldap_tls_cacert = /etc/openldap/cacerts/cacert.pem

TLS makes no sense for unix domain sockets, as if your unix domain socket is compromised your kernel is compromised and your TLS will be too.

Is there a place in the TLS code where this can be forced off for ldapi connections? Let me follow the ldap_auth_disable_tls_never_use_in_production path and see where that is set. ldapi should just be off.

@minfrin
Copy link
Contributor Author

minfrin commented Apr 6, 2023

Digging some more, I see TLS is switched on in two different places, firstly in a function called decide_tls_usage():

decide_tls_usage(enum connect_tls force_tls, struct dp_option *basic,

Then a second time TLS is forced on using the ldap_auth_disable_tls_never_use_in_production option, which artificially forces TLS to be on when not explicitly turned off, overriding decide_tls_usage() above.

use_tls = !dp_opt_get_bool(state->ctx->opts->basic, SDAP_DISABLE_AUTH_TLS);

I am assuming the fix is to move the ldap_auth_disable_tls_never_use_in_production code into the decide_tls_usage() function?

@minfrin
Copy link
Contributor Author

minfrin commented Apr 8, 2023

Below commands works well . Only the ssh part does not work.

I eventually found TLS was being forced on in the auth section, and have added a code path that bypassed that when ldapi is present.

I am struggling to test this, as in my case sssd appears to refuse to check the password. It complains the password is wrong, but there is nothing in the LDAP access log to show any attempt to bind to check the password:

Apr  8 23:18:21 seawitch sudo[2321]: pam_unix(sudo:auth): auth could not identify password for [minfrin]
Apr  8 23:18:23 seawitch sudo[2321]: minfrin : 1 incorrect password attempt ; TTY=pts/1 ; PWD=/ ; USER=root ; COMMAND=/bin/su -

Standard access is for me using ssh keys, not passwords, and those are working great.

I've also noticed the sssctl tool refuses to install on RHEL9, complaining that "/usr/bin/python33 is needed by sssd-tools".

@aborah-sudo
Copy link
Contributor

[root@fedora37 sssd]# cat sssd.conf
[sssd]
config_file_version = 2
services = nss, pam
domains = example1

[pam]
debug_level = 9

[domain/example1]
ldap_search_base = dc=example,dc=test
id_provider = ldap
auth_provider = ldap
ldap_user_home_directory = /home/%u
ldap_uri = ldapi://%2Frun%2Fslapd-example1.socket
ldap_tls_cacert = /etc/openldap/cacerts/cacert.pem
use_fully_qualified_names = False
debug_level = 9

[root@fedora37 sssd]# systemctl restart sssd
[root@fedora37 sssd]# id foo1
uid=14583101(foo1) gid=14564100(ldapusers) groups=14564100(ldapusers)
[root@fedora37 sssd]# ssh foo1@localhost
foo1@localhost's password:
Last login: Tue Feb 7 20:56:54 2023 from ::1
Could not chdir to home directory : No such file or directory
[foo1@fedora37 /]$ rpm -qa | grep sssd
python3-sssdconfig-pr6484-02124.fc37.noarch
sssd-nfs-idmap-pr6484-02124.fc37.x86_64
sssd-client-pr6484-02124.fc37.x86_64
sssd-common-pr6484-02124.fc37.x86_64
sssd-krb5-common-pr6484-02124.fc37.x86_64
sssd-common-pac-pr6484-02124.fc37.x86_64
sssd-dbus-pr6484-02124.fc37.x86_64
sssd-ad-pr6484-02124.fc37.x86_64
sssd-krb5-pr6484-02124.fc37.x86_64
sssd-ldap-pr6484-02124.fc37.x86_64
sssd-proxy-pr6484-02124.fc37.x86_64
sssd-ipa-pr6484-02124.fc37.x86_64
sssd-pr6484-02124.fc37.x86_64
sssd-tools-pr6484-02124.fc37.x86_64
sssd-kcm-pr6484-02124.fc37.x86_64
[foo1@fedora37 /]$ exit
logout
Connection to localhost closed.
[root@fedora37 sssd]# id foo09
id: ‘foo09’: no such user
[root@fedora37 sssd]# id foo9
uid=14583109(foo9) gid=14564100(ldapusers) groups=14564100(ldapusers)
[root@fedora37 sssd]# ssh foo0@localhost
foo0@localhost's password:
Could not chdir to home directory : No such file or directory
[foo0@fedora37 /]$ getent passwd foo9@example1
foo9:*:14583109:14564100:foo9 User::/bin/bash
[foo0@fedora37 /]$

Looks like auth is working this time

@minfrin
Copy link
Contributor Author

minfrin commented Apr 12, 2023

Looks like auth is working this time

Perfect, thank you for confirming.

@sumit-bose
Copy link
Contributor

Below commands works well . Only the ssh part does not work.

I eventually found TLS was being forced on in the auth section, and have added a code path that bypassed that when ldapi is present.

I am struggling to test this, as in my case sssd appears to refuse to check the password. It complains the password is wrong, but there is nothing in the LDAP access log to show any attempt to bind to check the password:

Apr  8 23:18:21 seawitch sudo[2321]: pam_unix(sudo:auth): auth could not identify password for [minfrin]
Apr  8 23:18:23 seawitch sudo[2321]: minfrin : 1 incorrect password attempt ; TTY=pts/1 ; PWD=/ ; USER=root ; COMMAND=/bin/su -

Hi,

it looks like you have to modify your PAM configuration so that pam_sss is tried as well for authentication.

bye,
Sumit

Standard access is for me using ssh keys, not passwords, and those are working great.

I've also noticed the sssctl tool refuses to install on RHEL9, complaining that "/usr/bin/python33 is needed by sssd-tools".

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks, my tests with ldapi were successful and I haven't see a regression with ldap or ldaps, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Hi @minfrin,

besides addressing recent comment from Sumit, could you please also add a release note to the commit message, following template:

# :feature: New feature description.

Those are used to automatically generate upstream release notes.

Done.

It should be in the commit message of one of patches, not in the PR description.

Otherwise ACK.

@alexey-tikhonov
Copy link
Member

Hi @minfrin,

could you please also rebase and resolve a trivial conflict in 'util.h'?

@pbrezina
Copy link
Member

I resolved the conflict and added release note so we can push it.

  • master
    • e004595 - Don't force TLS on if we're a unix domain socket.
    • 2d54cf5 - Rename sdap_get_server_ip_str() to sdap_get_server_peer_str()
    • 4ccd5b9 - Do not set SO_KEEPALIVE on AF_UNIX.
    • 91b7012 - Ensure we touch sockaddr_len in the success case only.
    • f221341 - Align sockaddr_storage to sockaddr for updated API.
    • 9f2d8d6 - Add support for ldapi:// URLs.

@minfrin
Copy link
Contributor Author

minfrin commented Apr 24, 2023

Thanks for this - been up to my eyeballs in apr-util, this was on my todo list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugzilla no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSSD LDAP provider should support ldapi:// for optimized lookups on a local LDAP server
5 participants