search

hardware / mailserver

:warning: UNMAINTAINED - Simple and full-featured mail server using Docker
https://store.docker.com/community/images/hardware/mailserver
MIT License
1.29k stars 322 forks source link

Full LDAP Support #347

Closed PeerD closed 5 years ago

PeerD commented 5 years ago

Description

This pull request adds LDAP support to this mailserver as requested in #218

In its current state it should already provide a working integration of this mailserver with LDAP. It supports most features of the SQL variant of this mailserver plus some extra ones. This setup should be completly production viable but I'd like to do more testing on it before it might get released. Still, I wanted to publish my progess here to get feedback. I expect you might have some variable names you'd like to be changed or things that could be done differently in the run.sh init script as I am not that firm with bash scripting.

LDAP mode can be enabled by setting LDAP_ENABLED=true. If this is not done, the behaviour of the mailserver should not change at all and should be completly backwards compatible. No changes have been done on the SQL part of the mailserver.

I have written extensive tests to your test suite (using a docker LDAP server and two LDAP mailservers and all neccessary configuration). This should cover most new features and test most aspects of LDAP integration while also ensuring that the SQL mode did not change or break.

I have tested it against OpenLDAP. They should work against ActiveDirectory (Microsoft) or other LDAP based systems, but I have not tested that (yet).

Please feel free to ask me any questions you might have as you said you are not really familiar with LDAP. I am also looking forward for your feedback!

What will not work in LDAP mode:

Type of change

Status

Todo List

How has this been tested ?

Extensive tests have been added to the test suit to test most features in LDAP mode and to ensure that it does not interfere with the default SQL mode. Butt additional "real world" testing should still be done before commiting this pull request.

hardware commented 5 years ago

What a massive pull request :smiley:

The changes look well structured and documented, and all checks have passed, it's a good sign :+1:

I think I'll have to merge the @Matchlighter's refactor (#336) first because run.sh need a serious refactor, LDAP support brings a lot of extra variables. Then LDAP will be tested from a development branch with a specific docker tag.

But you have added too many tests, it would be better to put only those related to LDAP integration with mailserver_ldap, mailserver_ldap2. You do not need to duplicate all the check if ldap support only changes part of them.

Great job.

PeerD commented 5 years ago

Great :)

Then I will adjust the run.sh once the refactoring is done. It does not add too many changes anyways. Just a lot of variables :)

And I will check through the tests and see which ones can be removed.

I also first tried to use DBDRIVER for LDAP aswell but used a different variable (LDAP_ENABLED) in the end as I think this will be easier to maintain as LDAP changes most things fundamentally compared to SQL. So now there is a clear separation between LDAP and SQL mode. But it could be integrated into DBDRIVER aswell if you prefere that. Technically LDAP ist just a different type of database aswell.

johan-smits commented 5 years ago

I'm testing the branch and it is almost working for me. I'm having issues with sending email. The error I get is:

Sender address rejected: not owned by user johan.smits@domain.eu’.

While the debug logs says:

2019-02-06T20:56:50.997774+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_lookup: In dict_ldap_lookup
2019-02-06T20:56:50.997779+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_lookup: Using existing connection for LDAP source /etc/postfix/ldap/sender-login-maps.cf
2019-02-06T20:56:50.997789+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_lookup: /etc/postfix/ldap/sender-login-maps.cf: Searching with filter (&(|(mail=johan.smits@domain.eu)(mailalias=johan.smits@domain.eu))(objectClass=mailAccount))
2019-02-06T20:56:50.998700+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_get_values[1]: Search found 1 match(es)
2019-02-06T20:56:50.998721+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_get_values[1]: Leaving dict_ldap_get_values
2019-02-06T20:56:50.998726+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: dict_ldap_lookup: Search returned nothing
2019-02-06T20:56:50.998733+00:00 mail-server-7987d59548-m5gw4 postfix/smtps/smtpd[729]: maps_find: smtpd_sender_login_maps: johan.smits@domain.eu: not found

So it found one match but the result is not found. When I do a normal ldap search there is one result (the correct one). But when I query with postmap like: postmap -q johan.smits@example.eu ldap:/etc/postfix/ldap/sender-login-maps.cf it returns also nothing. But this is the log van de ldap server when performing the postmap:

5c5b4bec conn=43385 op=1 SRCH base="ou=Users,dc=domain,dc=eu" scope=2 deref=0 filter="(&(|(mail=johan.smits@domain.eu)(mailalias=johan.smits@domain.eu))(objectClass=mailAccount))"
5c5b4bec conn=43385 op=1 SRCH attr=name
5c5b4bec <= mdb_equality_candidates: (mail) not indexed
5c5b4bec <= mdb_equality_candidates: (mailalias) not indexed
5c5b4bec conn=43385 op=1 SEARCH RESULT tag=101 err=0 nentries=1 text=
5c5b4bec conn=43385 op=2 UNBIND
5c5b4bec conn=43385 fd=24 closed

which confirms 1 entry.

EDIT: the value needs to be mail instead of name. I have it solved now. Will continue to test the branch and let you know if something is going wrong.

loic-lopez commented 5 years ago

Really nice work! i would like that this pull request be merged!

johan-smits commented 5 years ago

We are running the PR in production now (on Kubernetes), if there are issues I'll report them.

PeerD commented 5 years ago

Once the refactoring PR is merged I will add these changes to the new run.sh script so we can hopefully move on with this. I'm running it on a low traffic server since one and a half months now and it seems to be very stable so far.

Also I might want to change some variable names but I am not sure yet. Especially using DBDRIVER=ldap instead on LDAP_ENABLED for better consistency. Any optinions?

loic-lopez commented 5 years ago

I think that something like swapping between DBDRIVER=ldap and DBDRIVER=postfixadmin give more flexibility.

johan-smits commented 5 years ago

@PeerD DBDRIVER=ldap makes more sense indeed.

Some variables could be reused like:

but from here it makes no sense to reuse other variables.

PeerD commented 5 years ago

The reason why I did not use DBDRIVER=ldap is because it is used in many of the SQL config files directly. They should not be loaded in the ldap config anyways, but I wasn't 100% sure about that. But if this is the prefered way of doing it (which in my eyes it would be aswell) I will test it in the next iteration to see if there are any unexpected sideeffects. There shouldn't be any (besides some invalid config files that should never get loaded anyways) but you never know :)

loic-lopez commented 5 years ago

MAIL_DBDRIVER=ldap can be a replacement?

loic-lopez commented 5 years ago

Any updates?

PeerD commented 5 years ago

I am still waiting for the refactoring to be done. Once that is finished I will reintegrate the LDAP support and push a new version.

Aphris-Karu commented 5 years ago

When can we expect to see this in the main mailserver docker container?

stkac commented 5 years ago

Hi @PeerD, I want to confirm your Full LDAP Support work with 389 Directory Server as well, thanks a lot! Great Job!!

@hardware do not let this project fall, please :)

johan-smits commented 5 years ago

Feedback: running since 6 feb in production and no issues from my side. All working as expected!

zxtwonder commented 5 years ago

Another successful deployment. Really appreciate the work!

johansmitsnl commented 5 years ago

@PeerD I have right to merge the PR, one question before I do so, is it ready?

I personally use this branch on 2 production servers without any issues.

sknight80 commented 5 years ago

Before we do it, do we want to wait for the refactor PR merged to master?

johansmitsnl commented 5 years ago

@sknight80 is this a pr that is open or WIP?

sknight80 commented 5 years ago

@johansmitsnl I am referred to this PR: https://github.com/hardware/mailserver/pull/366

I mean PR = Pull Request (sorry brought this from my workplace. :))

Matchlighter commented 5 years ago

hardware felt that it should be merged after #336. There will probably be a merge conflict, but I think it will be easier to resolve in this PR rather than in #336 / #366. #366 (once the former is merged) should be conflict free.

PeerD commented 5 years ago

After #336 has now been merged, I updated my PR to the new startup scripts.

@johan-smits: I think this PR, although working, is still WIP. The last version (old startup scripts) have been running successfull in some deployments now but I'd like to test the new setup some more.

Is it normal for CirrusCI to fail? Or is that a problem on my end?

IMPORTANT BREAKING CHANGES

LDAP_ENABLED and LDAP_HOST enviroment variables are no longer used! Set DBDRIVER to "ldap" and DBHOST and DBPORT to your ldap server instead!

sknight80 commented 5 years ago

Looks like we need to fix some tests/scripts based on the Docker Build task.

davydov-vyacheslav commented 5 years ago

Is there any luck go get this in the production, please?

johansmitsnl commented 5 years ago

Is there any luck go get this in the production, please?

Will try to find some time this week to test this new branch.

@PeerD all work done?

sknight80 commented 5 years ago

Optional: Can we add a warning message to the startup script if we are using LDAP setup with the old config? In case someone forgets to check/read the changelog.

johansmitsnl commented 5 years ago

Optional: Can we add a warning message to the startup script if we are using LDAP setup with the old config? In case someone forgets to check/read the changelog.

The branch is officially not released. This "warning" support is only needed when the branch is merged and get changes like this.

hardware commented 5 years ago

Is there any luck go get this in the production, please?

We need people who are capable to maintain a full LDAP version of the mailserver project. I look carefully that to not end up with no one behind to maintain this. It's a major change in the testing process, all of that need to be secured to be futur-proof.

I think @johansmitsnl is capable but one more maintainer could be needed. @PeerD are you interested to have full write access on the repository ?

hardware commented 5 years ago

This PR can't be merged without an active maintainer on that part. Without any response before the next 2 months, this PR will be closed.

Thank you for your understanding.

johansmitsnl commented 5 years ago

Currently testing the branch and so far all running well. Please wait for one more week of testing before I want to say all good.

This is a major update for the project and I want to be sure it is ok. I'll need to provide support on it 🙂

hardware commented 5 years ago

This is a major update for the project and I want to be sure it is ok. I'll need to provide support on it.

All right. I will check all the changes this weekend and make a review. If no issue is found, the LDAP support will be merged in the master branch in a couple days, then in the stable branch in september.

Thank you to @PeerD and all the contributors for this PR.

hardware commented 5 years ago

@PeerD can you push an empty commit please (--allow-empty) ? I removed the Cirrus CI check, this step is bugged with your PR I think.

Anyway, Cirrus CI is not useful with Travis CI and ci/dockercloud, mailserver use both of these already, we do not need a third one, linked nowhere.

PeerD commented 5 years ago

Hi all,

sorry, I have been on holidays the last weeks, thus no reply from me recently. I pushed a new (empty) commit as requested.

@hardware : Obviously I am interested to support this project in the future as I have already invested a lot of time into this PR and am planing to use this for all my mailservers in the future. Also it is currently the only project I found that fulfills all my requirements. Well, as soon as this PR in through. ;)

I am willing to support this project and maintain the LDAP part of it, but I don't think that I will be necessarily needing write access for that. I think I prefere if someone takes a look at my changes before they are merged in. I have run my own mailservers for years, but never this complex and with that many subsystems and people using it.

What should already work:

Things that still need to be done:

johansmitsnl commented 5 years ago

Just to inform from my side all good to go :+1:

padrino121 commented 5 years ago

Working on this now, when utilizing DBDRIVER=ldap and DBHOST as either an IP or name or my LDAP server that can be resolved via /etc/hosts the system reports an error on startup.

[INFO] MariaDB/PostgreSQL hostname not found in /etc/hosts [ERROR] Container IP not found with embedded DNS server... Abort ! [ERROR] Check your DBHOST environment variable