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

Fix swarm smtp auth #563

Closed
wants to merge 3 commits into from
Closed

Conversation

muhlemmer
Copy link
Member

XCLIENT check was done against FRONT_ADDRESS, which is resolved in start.py from the front hostname. In Swarm mode this would resolve to the single VIP of all instances inside the front service.

Consequently, instances of front would try to authenticate using their individual IP != VIP. This would fail.

This PR allows XCLIENT from anywhere inside the docker network, as defined as RELAYNETS in .env.

Using the correct docker-compose.yml settings, authentication is now working in Swarm mode. Issue #530 could be closed.

I would also like to make reference to Kubernets PR #559, which could also make good use of this change. In configmap.yaml, the current setting would allow only for a single font instance. (Replication: 1)

Tested send/receive in both docker-compose up and docker stack deploy.

@muhlemmer
Copy link
Member Author

Note afterwards: if it is not preferred to trust all containers in this network, there is the possibility to set up separate network overlays for different tasks. It should be possible to set up a "trusted" network for authentication related authentication.

Nevertheless, XCLIENT should reflect a network range and not a single IP.

Copy link
Contributor

@ofthesun9 ofthesun9 left a comment

Choose a reason for hiding this comment

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

I think the following is more suitable:

smtpd_authorized_xclient_hosts={{ FRONT_ADDRESS }} {{ POD_ADDRESS_RANGE }}

@muhlemmer
Copy link
Member Author

Personally, I would like to refrain from using POD_ADDRESS_RANGE. The name of the variable name does not make any sense in a regular Docker deployment. It is a Kubernetes variable, which I misused in a past solution to pass the RELAYHOSTS network to Dovecot.

So I think, in any standard code we should use RELAYHOSTS variable, including in the Dovecot config. Subsequently, Kubernetes .yml files can set RELAYHOSTS=POD_ADDRESS_RANGE.

Any comments are welcome!

@ofthesun9
Copy link
Contributor

I think RELAYHOSTS is too similar to RELAYHOST, which was a bit confusing to me ;-).
As POD_ADDRESS_RANGE is already in the code, in order to avoid the introduction of another variable, I am in favor to reuse it if possible....
We also have to assume that POD_ADDRESS_RANGE commit is already used by the kubernetes users, => if we replace in Mailu code POD_ADDRESS_RANGE by RELAYHOSTS, we will break something here

Beyond the name of the variable itself, there are other places in the code where this is needed. I started a branch (swarm-vip) on my fork to prepare this and also to update the swarm documentation accordingly.

I propose to first find all placeswhere we need to update the code (at least dovecot, postfix and rspamd) and then see if we can/need mutualize with kubernetes

@muhlemmer
Copy link
Member Author

Excuse my English. I really meant to say RELAYNETS. It is already used in Mailu as "the Docker network".

I don't think we should support all the different implementation's variables. Keep our own set of variables and the startup files from docker-compose, docker-compose for swarm and kubernetes have to make sure those variables are assigned accordingly.

So this pull request makes sure that RELAYNETS is used instead of only the FRONT_ADDRESS. Subsequently, RELAYNETS is already set in .env and it will work for both the classic docker-compose and Docker swarm. Al that needs to be done is update the Kubernetes.

Then, as a general comment and probably off-topic for this PR, I would like to suggest to use the RELAYNETS in a uniform way, instead of POD_ADDRESS_RANGE which name does not make much sense if one is not involved in Kubernetes. (What the hell does POD mean anyways? 😛 ). At this moment POD_ADDRESS_RANGE is only used in the Dovecot config and is not yet integrated in any release version. I mean, look at this line of code:

 SELECT NULL as password, 'Y' as nopassword, '{% if POD_ADDRESS_RANGE %}{{ POD_ADDRESS_RANGE }}{% else %}{{ FRONT_ADDRESS }}{% if WEBMAIL_ADDRESS %},{{ WEBMAIL_ADDRESS }}{% endif %}{% endif %}' as allow_nets \

3 inline if statements to cover all the possible cases. Ughh. If we replace those 3 variables by the single variable of RELAYNETS it would cover all the cases in a uniform and clean way.

@ofthesun9
Copy link
Contributor

Hi,
RELAYNETS is used to declare nets that can use postfix as a relay server. This is not necessarily the same than what we want to do here... I think it is preferable to use another variable, and I guess that is why POD_ADDRESS_RANGE was introduced.

I suggest that we use the same POD_ADDRESS_RANGE variable in the other parts of Mailu code where it seems necessary for swarm deployment. I wouldn't be surprised that it is needed anyway for kubernetes..

On a side note, It seems that we might not need a range but rather a single IP. At least on my setup, this is the case: the logs of smtp &imap show that the originator IP is always 10.0.1.4, which is not the front service neither the front containers... By inspecting the mailu_default network, I found this IP under the name "mailu_default-endpoint". But I don't know how to resolve this IP from within mailu code, otherwise we could have another approach and set the FRONT_ADDRESS to this mailu_default-endpoint (only whan under swarm mode of course)...

@muhlemmer
Copy link
Member Author

muhlemmer commented Sep 23, 2018

RELAYNETS is used to declare nets that can use postfix as a relay server. This is not necessarily the same than what we want to do here...

docs/compose/.env:

Networks granted relay permissions, make sure that you include your Docker
internal network (default to 172.17.0.0/16)

POD_ADDRESS_RANGE will just be redundant as it uses the same network range.

I think it is preferable to use another variable, and I guess that is why POD_ADDRESS_RANGE was introduced.

I suggest that we use the same POD_ADDRESS_RANGE variable in the other parts of Mailu code where it seems necessary for swarm deployment. I wouldn't be surprised that it is needed anyway for kubernetes..

If you run a repository search, POD_ADDRESS_RANGE is only used once and is introduced only for kubernetes. A "POD" is a kubernetes thingy I guess and has nothing to do with regular Docker.

On a side note, It seems that we might not need a range but rather a single IP. At least on my setup, this is the case: the logs of smtp &imap show that the originator IP is always 10.0.1.4, which is not the front service neither the front containers... By inspecting the mailu_default network, I found this IP under the name "mailu_default-endpoint". But I don't know how to resolve this IP from within mailu code, otherwise we could have another approach and set the FRONT_ADDRESS to this mailu_default-endpoint (only whan under swarm mode of course)...

Are you still using dnsrr as workaround, as mentioned in #530? The purpose of what I'm trying to do is getting swarm mode to work without dnsrr. In such a case you will not find the mentioned endpoint and originating IP's are spread throughout the network range. See see comments in that issue for background info.

@muhlemmer
Copy link
Member Author

I will change it to POD_ADDRESS_RANGE as it seems accepted elsewhere. After the build errors on master are resolved, I'll rebase and re-submit.

@ofthesun9. What is you intention on using the dnsrr workaround?

@ofthesun9
Copy link
Contributor

dnsrr workaround will not be needed anymore, see #604

RELAYNETS is set in .env as the Docker network. This change will allow all instances on that network to authenticate. This will allow more replications of front to authenticate correctly. (Swarm and most probably Kubernets)
@muhlemmer
Copy link
Member Author

@ofthesun9. Please re-evaluate your review.

@muhlemmer muhlemmer requested a review from kaiyou October 6, 2018 16:40
@kwek kwek mentioned this pull request Oct 15, 2018
@muhlemmer
Copy link
Member Author

Duplicated (but more complete) in PR #604

@muhlemmer muhlemmer closed this Oct 15, 2018
@muhlemmer muhlemmer deleted the fix-swarm-smtp-auth branch October 24, 2018 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants