Use dovecot's LDAP uris option instead of hosts (fixes #1510) by moqmar · Pull Request #1901 · docker-mailserver/docker-mailserver

moqmar · 2021-04-12T19:40:20Z

Description

According to https://doc.dovecot.org/configuration_manual/authentication/ldap/, the "uris" option has to be used when using LDAPS, while the "hosts" option seems to only work with STARTTLS. So, this PR fixes the problem that docker-mailserver can't be used with LDAPS.

The change is made in a way that allows backwards-compatibility with configurations that assume the "hosts" option to be set by LDAP_SERVER_HOST, in which case it's using "ldap://" as a default protocol if none is specified. The only change for the user is that LDAP_SERVER_HOST now supports specifying a protocol like "ldaps://...".

Fixes #1510

Type of change

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the documentation)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

…ver#1510)

…1901

georglauterbach

I can approve this PR style-wise. I cannot say anything about the functionality. I re-requested a review from @wernerfred

moqmar · 2021-04-16T11:27:23Z

Whoops, seems like I have broken something with the style changes. Will evaluate. 🙃

Edit: seems like I just used a wrong variable somewhere: https://github.com/docker-mailserver/docker-mailserver/compare/f9f9fc4570a15c0268e30bcca0be4e229d8d432f..18b4f1a26e126a2dff281897d7b89ff3b59f039d

georglauterbach · 2021-04-16T11:37:17Z

Please also note that your commit signatures are invalid - you may want to fix this as well :D

wernerfred · 2021-04-16T11:41:16Z

@reneploetz you commented on #1902. Can you please review here as well?

…1901

reneploetz · 2021-04-17T20:57:41Z

@reneploetz you commented on #1902. Can you please review here as well?

As I'm not using dovecot + LDAP I'm unable to test this on an actual test system (my system is using Kopano + LDAP). But the change appears to fine to be given that there is a test that is checking the correct replacement inside the config after the container start as well as a test that dovecot starts with that configuration. Afterwards there is a test on whether one can connect with a username that is present in ldap.
If you feel uncomfortable to merge this then I can create local test environment for this tough.

georglauterbach · 2021-04-18T10:04:13Z

If you feel uncomfortable to merge this then I can create local test environment for this tough.

That'd be awesome!

moqmar · 2021-04-18T10:21:37Z

I think the most interesting test would be LDAP with StartTLS, as I think that it hasn't been tested yet with the change.

It seems like it should work with the ldap:// protocol and tls = yes, ~~but I'm not sure if there is currently a way to set tls = yes at all with Dovecot right now?~~ which seems to be doable by setting DOVECOT_TLS=yes.

wernerfred · 2021-04-18T10:52:13Z

If you feel uncomfortable to merge this then I can create local test environment for this tough.

If you could to this it would be great! If you do not find time then I would consider it safe to merge as we have the same opinion by just looking at the changes. But only a real test can show if we are right.

Thanks in advance!

georglauterbach · 2021-04-18T11:19:30Z

If you could to this it would be great! If you do not find time then I would consider it safe to merge as we have the same opinion by just looking at the changes. But only a real test can show if we are right.

I agree.

georglauterbach · 2021-04-18T13:31:56Z

@wernerfred If you're comfortable merging this, I am too. I will update the branch first :)

wernerfred · 2021-04-18T13:57:26Z

I would wait for a reply of @reneploetz :)
Depending on this we will merge or wait until he finished testing.

reneploetz · 2021-04-19T01:51:50Z

I can confirm that the following configurations are working:

LDAP_SERVER_HOST=testing_ldap
DOVECOT_TLS=yes

DOVECOT_URIS=ldap://testing_ldap
DOVECOT_TLS=yes

All of them lead to this content in dovecot-ldap.conf.ext which allows dovecot to successfully connect to LDAP via TLS:

uris = ldap://testing_ldap
tls = yes

So as far as I'm concerned the configuration works with TLS.

As a side note in case somebody asks later: we currently do not support custom certificates for LDAP due to the lack of "tls_require_cert" or "tls_ca_cert_*" settings in the configuration.

georglauterbach

Then I will approve these changes. @wernerfred if you're fine with this too, go ahead and merge this.

We can afterwards merge the PR about Gh actions

wernerfred · 2021-04-19T06:56:47Z

- => Specify a space separated list of LDAP hosts.
+- => Specify a space separated list of LDAP uris.
+- Note: If the protocol is missing, `ldap://` will be used.
+- Note: This deprecates `DOVECOT_HOSTS` (as it didn't allow to use LDAPS), which is currently still supported for backwards compatibility.


This is actually a breaking change. @aendeavor another reason for v10.0.0 😄
The note should be removed at some time in the future.

Yes - I said this earlier that this note will go with 10.0.0

wernerfred · 2021-04-19T06:57:45Z

Does this change require documentation updates?

wernerfred · 2021-04-19T07:00:33Z

Ah @moqmar proposed to do this with a separate PR in #1902 (comment).
Therefore I will merge

moqmar force-pushed the bugfix/dovecot-ldaps branch from 7228e98 to 6ac6c24 Compare April 12, 2021 19:57

georglauterbach reviewed Apr 13, 2021

View reviewed changes

Comment thread target/scripts/startup/setup-stack.sh Outdated

georglauterbach requested a review from a team April 13, 2021 07:24

georglauterbach assigned moqmar Apr 13, 2021

georglauterbach added area/enhancement service/ldap labels Apr 13, 2021

wernerfred reviewed Apr 13, 2021

View reviewed changes

Comment thread target/dovecot/dovecot-ldap.conf.ext Outdated

georglauterbach added the pr/needs review label Apr 15, 2021

Use dovecot's LDAP uris option instead of hosts (fixes docker-mailser…

0a3ab87

…ver#1510)

moqmar force-pushed the bugfix/dovecot-ldaps branch from 6ac6c24 to 0a3ab87 Compare April 16, 2021 08:38

Merge branch 'master' into bugfix/dovecot-ldaps

5b99e33

georglauterbach requested changes Apr 16, 2021

View reviewed changes

Comment thread target/scripts/startup/setup-stack.sh Outdated

Comment thread target/scripts/startup/setup-stack.sh Outdated

georglauterbach added the priority/medium label Apr 16, 2021

moqmar commented Apr 16, 2021

View reviewed changes

Comment thread target/scripts/startup/setup-stack.sh Outdated

moqmar added a commit to moqmar/docker-mailserver that referenced this pull request Apr 16, 2021

Clean up variables & environment documentation for docker-mailserver#…

f9f9fc4

…1901

georglauterbach previously approved these changes Apr 16, 2021

View reviewed changes

wernerfred mentioned this pull request Apr 16, 2021

Add LDAP_QUERY_FILTER_SENDERS setting for spoof protection with LDAP #1902

Merged

8 tasks

wernerfred requested a review from fbartels April 16, 2021 11:39

moqmar added a commit to moqmar/docker-mailserver that referenced this pull request Apr 16, 2021

Clean up variables & environment documentation for docker-mailserver#…

aa3f829

…1901

moqmar dismissed georglauterbach’s stale review via aa3f829 April 16, 2021 12:16

moqmar force-pushed the bugfix/dovecot-ldaps branch from f9f9fc4 to aa3f829 Compare April 16, 2021 12:16

Clean up variables & environment documentation for docker-mailserver#…

18b4f1a

…1901

moqmar force-pushed the bugfix/dovecot-ldaps branch from aa3f829 to 18b4f1a Compare April 16, 2021 12:19

Merge branch 'master' into bugfix/dovecot-ldaps

8a36e69

Merge branch 'master' into bugfix/dovecot-ldaps

17b315d

georglauterbach mentioned this pull request Apr 18, 2021

Adjusted documentation for service name and Traefik certificate issuance #1918

Merged

11 tasks

Merge branch 'master' into bugfix/dovecot-ldaps

f5a44c8

georglauterbach approved these changes Apr 19, 2021

View reviewed changes

wernerfred approved these changes Apr 19, 2021

View reviewed changes

wernerfred merged commit 94b5ac4 into docker-mailserver:master Apr 19, 2021

wernerfred mentioned this pull request May 18, 2021

[BUG] SASLAUTHD_LDAP_SSL=1 does not set ldaps:// #1983

Closed

wernerfred added this to the v10.0.0 milestone May 18, 2021

Uh oh!

Conversation

moqmar commented Apr 12, 2021 • edited by wernerfred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach left a comment

Choose a reason for hiding this comment

Uh oh!

moqmar commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach commented Apr 16, 2021

Uh oh!

wernerfred commented Apr 16, 2021

Uh oh!

reneploetz commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach commented Apr 18, 2021

Uh oh!

moqmar commented Apr 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wernerfred commented Apr 18, 2021

Uh oh!

georglauterbach commented Apr 18, 2021

Uh oh!

georglauterbach commented Apr 18, 2021

Uh oh!

wernerfred commented Apr 18, 2021

Uh oh!

reneploetz commented Apr 19, 2021

Uh oh!

georglauterbach left a comment

Choose a reason for hiding this comment

Uh oh!

wernerfred Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

georglauterbach Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

wernerfred commented Apr 19, 2021

Uh oh!

wernerfred commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

moqmar commented Apr 12, 2021 •

edited by wernerfred

Loading

moqmar commented Apr 16, 2021 •

edited

Loading

reneploetz commented Apr 17, 2021 •

edited

Loading

moqmar commented Apr 18, 2021 •

edited

Loading