Repository: zulip/zulip
Pull request: puppet: Require libldap-common be installed.
Zulip instances without a database included, like the Docker image, would not fail to use TLS properly, since `TLS_REQCERT` was not set in `/etc/ldap/ldap.conf`. While there's a few other ways we could fix this, just installing libldap-common on app frontend instances seems like a good solution, and has no impact on other Zulip systems, and it was already being installed through a "Recommends" tier apt dependency indirectly from the PostgreSQL server package.

Fixes zulip/docker-zulip#454.


Problem statement:
Version 9.0 and 9.1 "ZulipLDAPAuthBackend" Issue with TLS on ldaps 636
Hello,

we are running zulip docker for about a year now, starting from Version 7 and really enjoying the product! Good work!

Now that we upgraded our "dev" machine to version 9.0 to test the upgrade we are facing issues with authenticating through ldap. In the logs we just see:

`ldap.SERVER_DOWN: {'result': -1, 'desc': "Can't contact LDAP server", 'ctrls': [], 'info': '(unknown error code)'}`

also we found out that if you add 

`echo 'TLS_REQCERT allow' >> /etc/ldap/ldap.conf`

inside the zulip docker container, it works again. But we don't want to ignore certificate checks for ldap tls. 
We use a official publicly trusted certificate on ldaps which is bought from GoDaddy, which properly verifys when using openssl command even from inside the zulip docker cli. We also tried version 9.1 but it is the same result.

Please help us :) 
Thank you!

BR,
Felix

Hints from issue discussion:
We are facing the same issue. How do we can add a SSL certificate to avoid such behavior?
Also would be nice, if LOGS will be more informative over this issue
Facing the same issue, would be nice to have a fix for this
Hi,

can we get an update on this? Please :)

BR,
Felix
Hello,

we were facing the same issue.
As a workaround, it helped to add the following two lines to `Dockerfile` in order to point ldap to `/etc/ssl/certs`:

```
RUN mkdir /etc/ldap && \
    echo 'TLS_CACERTDIR /etc/ssl/certs' >> /etc/ldap/ldap.conf
```

This is maybe a bit more sensible than to just accept all certificates.
(And then of course you have to rebuild & restart the container)
Also encountered this issue, which broke a previously-working setup on upgrade since both LDAP+STARTTLS and LDAPS stopped working.

[This issue from another Docker repo](https://github.com/netbox-community/netbox-docker/issues/799) suggests that installing `libldap-common` in the Dockerfile is a sufficient fix. I'm going to test this myself (though it's very painful to rebuild the image.)
Folks, [chat.zulip.org](https://zulip.com/development-community/) is the best place to get help with issues setting up a server. 

@ivanbakel did your test determine that installing `libldap-common` fixed this issue for you? If so, that seems like an easy tweak to the `Dockerfile` that we could include in 9.3.
I can confirm that installing `libldap-common` leads to the `/etc/ldap/ldap.conf` file being present with the desired cert settings on the image, and this fixes the LDAP issue.
It looks like the reason this hasn't come up outside Docker is that a postgres database ends up pulling this in:
```
$ aptitude why libldap-common
i   postgresql-16 Depends    libldap-2.5-0 (>= 2.5.4)
i A libldap-2.5-0 Recommends libldap-common          
```

Commit message: puppet: Require libldap-common be installed.

Zulip instances without a database included, like the Docker image,
would not fail to use TLS properly, since `TLS_REQCERT` was not set in
`/etc/ldap/ldap.conf`. While there's a few other ways we could fix
this, just installing libldap-common on app frontend instances seems
like a good solution, and has no impact on other Zulip systems, and it
was already being installed through a "Recommends" tier apt dependency
indirectly from the PostgreSQL server package.

Fixes zulip/docker-zulip#454.

Patch to review:
diff --git a/puppet/zulip/manifests/app_frontend_base.pp b/puppet/zulip/manifests/app_frontend_base.pp
index 4bf5a8f696fd8..9d723533b225b 100644
--- a/puppet/zulip/manifests/app_frontend_base.pp
+++ b/puppet/zulip/manifests/app_frontend_base.pp
@@ -14,8 +14,20 @@
     # package already includes the client.
     include zulip::postgresql_client
   }
-  # For Slack import
-  zulip::safepackage { 'unzip': ensure => installed }
+  zulip::safepackage {
+    [
+      # For Slack import.
+      'unzip',
+      # Ensures `/etc/ldap/ldap.conf` exists; the default
+      # `TLS_CACERTDIR` specified there is necessary for LDAP
+      # authentication to work. This package is "Recommended" by
+      # `libldap` where is required by postgres, so has been on most
+      # Zulip servers by default; adding it here explicitly ensures it
+      # is available on those that don't include the database server.
+      'libldap-common'
+      ]:
+        ensure => installed,
+  }
 
   file { '/etc/nginx/zulip-include/app':
     require => Package[$zulip::common::nginx],


Code review comments:
diff_hunk: @@ -14,8 +14,20 @@
     # package already includes the client.
     include zulip::postgresql_client
   }
-  # For Slack import
-  zulip::safepackage { 'unzip': ensure => installed }
+  zulip::safepackage {
+    [
+      # For Slack import.
+      'unzip',
+      # Ensures `/etc/ldap/ldap.conf` exists; the default
+      # `TLS_CACERTDIR` specified there is necessary for LDAP
+      # authentication to work. This package is "Recommended" by
+      # `libldap` where is required by postgres, so has been on most
+      # Zulip servers by default; adding it here explicitly ensures it
+      # is available on those that don't include the database server.
+      'libldap-common'
+      ]:
+        ensure => installed,
path: puppet/zulip/manifests/app_frontend_base.pp
text: @user1:
```suggestion
      'libldap-common',
    ]:
      ensure => installed,
```