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

Rootless Podman security update #2393

Conversation

p-fruck
Copy link
Contributor

@p-fruck p-fruck commented Feb 5, 2022

Description

This PR includes security related documentation to run docker-mailserver with podman in rootless mode. See the linked issue for more details on the security concerns.

Personally I would like to set PERMIT_DOCKER=none to the new default in mailserver.env because it enforces authentication even from localhost and therefore is more secure, but this would result in a breaking change for users that have authentication-less setups for their localhost. The decision is up to you :)

Fixes #2377

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • 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 the documentation under docs/)
  • 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

@georglauterbach
Copy link
Member

I recently merged a PR that changed the README. You'll need to resolve the (small) conflict there. I'd also like you to relocate the information in the documentation, at the end of the ### Create a docker-compose Environment section, above the ### Get up and running heading. I think this place is more fitting.

@georglauterbach georglauterbach added area/documentation kind/improvement Improve an existing feature, configuration file or the documentation priority/low labels Feb 5, 2022
@georglauterbach georglauterbach added this to the v10.5.0 milestone Feb 5, 2022
@p-fruck p-fruck force-pushed the issues/2377/podman-rootless-security branch from 8cf2b61 to 9b9fa95 Compare February 5, 2022 12:44
@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 5, 2022

I've rebased to the latest master branch, missed the last commit 😅

@casperklein
Copy link
Member

casperklein commented Feb 5, 2022

Not much time right now, so just two quick thoughts:

  1. This could break the update-check mail does not connect to postfix, but put a file directly in the spool directory.
    echo "${MAIL}" | mail -s "Mailserver update available! [ ${VERSION} --> ${LATEST} ]" "${POSTMASTER_ADDRESS}" && \
  2. You might want to add a test for this in https://github.com/docker-mailserver/docker-mailserver/blob/master/test/permit_docker.bats

Copy link
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

Other than the one comment, @casperklein mentioned the update check. If these two concerns are resolved, I think this PR is fine :D

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
p-fruck and others added 6 commits February 8, 2022 18:21
In rootless podman all IPs are resolved as localhost. This setting allows to
enforce authentication even from localhost to circumvent this issue.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Since `none` was added as a new value to the `PERMIT_DOCKER` variable, it requires
an additional testcase to check if authentication is enforced on localhost.

The methods `setup` and `teardown` have been changed to `setup_file` and `teardown_file`
respectively to speed up the testing process.
@p-fruck p-fruck force-pushed the issues/2377/podman-rootless-security branch from 9b9fa95 to c9d4c89 Compare February 8, 2022 17:25
@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 8, 2022

I've applied the suggestion and added a simply testcase. @casperklein you mentioned the issue with the updatescript, how should we overcome that? Authenticate against the server using credentials supplied as environment variables, or simply output to the terminal and leave it up to the user to handle the output if PERMIT_DOCKER is explicitly set to none?

mailserver.env Outdated Show resolved Hide resolved
Co-authored-by: Casper <casperklein@users.noreply.github.com>
@casperklein
Copy link
Member

I revoked my comment about the update-check (see my edited comment above). But to be sure, you can test it with something like this:

docker exec -it mailserver bash -c 'echo "10.0.0" > /VERSION && supervisorctl restart update-check'

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 8, 2022

I see, just did a quick test and it works as you described, thanks 👍

Copy link
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

Excellent first contribution 👌

Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2022

Documentation preview for this PR is ready! 🎉

Built with commit: cbcb09e

@georglauterbach georglauterbach merged commit 4c3af32 into docker-mailserver:master Feb 9, 2022
@casperklein
Copy link
Member

Two more things that came to my mind are LOGWATCH and PFLOGSUMM. Do you have a config like that and can check if you still get those mails daily?

- LOGWATCH_INTERVAL=daily
- LOGWATCH_RECIPIENT=foo@example.com
- PFLOGSUMM_TRIGGER=daily_cron
- PFLOGSUMM_RECIPIENT=foo@example.com

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 16, 2022

I don't have at the moment, but I could set it up within the next few days if you like

@casperklein
Copy link
Member

That would be great.

@casperklein
Copy link
Member

@docker-mailserver/maintainers Is there a way to detect within the container, if it's run by podman? If that is possible, we should print a open-relay warning, if PERMIT_DOCKER is empty.

We could also make PERMIT_DOCKER=none as default in the future to provide a more secure out-of-the-box solution, despite podman isn't officially supported. But then LOGWATCH and PFLOGSUMM needs to be adjusted to use authentication (if neccesary, let's wait for p-frucks feedback).

@wernerfred
Copy link
Member

Just did a short Research and found the following:

The best check If the runtime being Docker or Podman would be to look for /run/.containerenv (Podman) vs /run/.dockerenv (Docker)

Haven't tested it myself though

@polarathene
Copy link
Member

.dockerenv

That is old advice IIRC, I remember looking into detecting if operating in a Docker container a while back and that approach no longer works (it was never officially documented by Docker either) as the file is no longer present AFAIK.

The easiest approach of course would be to just add a podman prefix/suffix tag variant that sets an ENV to detect, or very early on in README/docs explictly direct Podman users to a relevant docs page that details anything they should be aware of (might be missed if following third-party guides elsewhere though).

Changing defaults is probably fine too, but should wait until a major release right?

@casperklein
Copy link
Member

casperklein commented Feb 16, 2022

.dockerenv [..] as the file is no longer present AFAIK.

confirm 😞

@polarathene
Copy link
Member

There was some other approaches I recall being advised, one regarding cgroups but it was flakey IIRC (there's v1 and v2, along with differences between container runtimes and possibly kernel versions I think..) and was specifically for detecting running in a containerized environment.

If Podman does still make that file available, that's all you'd really need I guess to pull off the detection, but if it's not officially documented anywhere, it might disappear one day like the Docker one did. We could also just add a PODMAN=1 ENV or similar for users running podman to use, but that seems kinda redundant since awareness of our docs for podman (with PERMIT_DOCKER warning added) would be just as effective.

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 16, 2022

In case you want to consider a rather hacky solution, you might give this command a try

if grep 'host.containers.internal' /etc/hosts > /dev/null; then echo podman; else echo docker; fi

Also I just confirmend @polarathene's guess that podman (tested with v3.4.4) still creates the /run/.containerenv file

@polarathene
Copy link
Member

When running with host mode networking, at least with Docker it will use the host system /etc/hosts instead IIRC, so that approach may not be reliable for some users (maybe podman handles that differently though?).

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 17, 2022

You are right, running podman in host mode does not add the host.containers.internal entry to the hosts file. On the other hand I'm not sure if the IP rebinding issues exist when running the container in host mode, because from what I understood the IP rebinding happens in podmans rootless overlay network, which is unused in host mode. Either way the /run/.containerenv might be the more reliable option

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 17, 2022

Another thing that came to my mind during the setup of my new mailserver: Do you think it's reliable to check for the present network interfaces? That way also the default network interface could be dynamically changed and podman users would not have to specify NETWORK_INTERFACE=tap0 in their environment file.

Regarding the LOGWATCH and PFLOGSUMM checks: I need to wait for my current email provider to update their DNS cache so I can send mails to my other address 😒 How would you test such a setup? Do you have two local mail servers that communicate with each other?

@casperklein
Copy link
Member

casperklein commented Feb 17, 2022

I am not sure if I understand you correctly. Why would you need a second DMS instance?

Another podman question: Can you confirm, that this solution does also prevent an open-relay?
That might be a solution, if LOGWATCH doesn't work anymore with PERMIT_DOCKER=none set.

@casperklein
Copy link
Member

podman users would not have to specify NETWORK_INTERFACE=tap0 in their environment file.

Podman is not officially supported, because non of the maintainers use it afaik. DMS seems to work with podman, but there are some things to care for. So forcing podman users to read the manual and the security warnings is a good behaviour IMO 😉
There were lots of issue lately from people not reading the documentation properly 😞

@p-fruck
Copy link
Contributor Author

p-fruck commented Feb 17, 2022

people not reading the documentation properly

One of them being me, I guess 😅

Why would you need a second DMS instance?

In case I wanted to test inter server communication. But you are right, it's not required for the current usecase

Can you confirm, that #2411 (comment) solution does also prevent an open-relay

I can, but I didn't expect the result. Using the slirp4netns approach (compatible with v10.4.0 and older) I get a client host rejected message:

HELO example.com
250 mail.example.com
mail from: admin@example.com          
250 2.1.0 Ok
rcpt to: a.b@c.de
554 5.7.1 <mail.example.com[10.88.2.2]>: Client host rejected: Access denied

The solution shown in die issue produces a relay access denied error:

HELO example.com
220 mail.example.com ESMTP
250 mail.example.com
mail from: admin@example.com
250 2.1.0 Ok
rcpt to: a.b@c.de
554 5.7.1 <a.b@c.de>: Relay access denied

Not what I've expected, but both solutions seem to work

@casperklein
Copy link
Member

We could also make PERMIT_DOCKER=none as default in the future to provide a more secure out-of-the-box solution, despite podman isn't officially supported. But then LOGWATCH and PFLOGSUMM needs to be adjusted to use authentication (if neccesary, let's wait for p-frucks feedback).

I did my own tests with PERMIT_DOCKER=none and the mails from LOGWATCH & PFLOGSUMM were send successful.

p-fruck added a commit to p-fruck/docker-mailserver that referenced this pull request Feb 21, 2022
The attribute `none` has been introduced as value for `PERMIT_DOCKER` in docker-mailserver#2393.
The current release v10.4.0 does not support this option yet. To prevent podman
users from misconfiguring their server, supported version hints have been added
to the corresponding documentation.
p-fruck added a commit to p-fruck/docker-mailserver that referenced this pull request Feb 21, 2022
The attribute `none` has been introduced as value for `PERMIT_DOCKER` in docker-mailserver#2393.
The current release v10.4.0 does not support this option yet. To prevent podman
users from misconfiguring their server, supported version hints have been added
to the corresponding documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation kind/improvement Improve an existing feature, configuration file or the documentation priority/low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security concerns using podman in rootless mode
5 participants