introduce F2B v0.11 by georglauterbach · Pull Request #1965 · docker-mailserver/docker-mailserver

georglauterbach · 2021-05-12T09:45:56Z

Description

This PR replaces F2B 0.10 with the latest F2B 0.11.2. This is done as Debian 11 has still no ETA. This change is easily revertible. When Debian 11 launches with the latest F2B, this PR can be reverted in a manner of minutes.

Fixes #1810

Type of change

Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

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 · 2021-05-12T10:22:04Z

@casperklein Although unit tests fail, it is only one tests, which is version related:

 not ok 28 checking setup.sh: setup.sh debug fail2ban
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 231,
#  in test file test/mail_fail2ban.bats, line 143)
#   `assert_output --partial "Unbanned IP from dovecot: 192.0.66.4"' failed
# 
# -- output does not contain substring --
# substring (1 lines):
#   Unbanned IP from dovecot: 192.0.66.4
# output (3 lines):
#   Unbanned IP from dovecot: 1
#   Unbanned IP from postfix: 0
#   Unbanned IP from postfix-sasl: 0
# --
#

Can you have a look?

georglauterbach · 2021-05-12T10:22:41Z

@docker-mailserver/maintainers If someone is not fine with getting the latest Fail2Ban, please share your concerns here :)

wernerfred · 2021-05-12T10:34:51Z

@docker-mailserver/maintainers If someone is not fine with getting the latest Fail2Ban, please share your concerns here :)

If it doesn't solve the issue discussed in #1810 and still needs the "hacking" i see no real benefit. If there is an advantage then go for it (imho)

georglauterbach · 2021-05-12T10:37:12Z

@wernerfred You right: I think that

I've tested 0.11 on the host and it flawlessly banned all the "auth failures"

is one good arguments. The other one may be that we should be at the forefront of security. I like having the latest versions :D

wernerfred · 2021-05-12T10:41:25Z

  razor rpm2cpio rsyslog sasl2-bin spamassassin supervisor \
  unrar-free unzip whois xz-utils >/dev/null && \
+  # Fail2Ban
+  curl -Lso fail2ban.deb https://github.com/fail2ban/fail2ban/releases/download/0.11.2/fail2ban_0.11.2-1.upstream1_all.deb && \


If we do this to be on forefront of security then we should check the downloaded files integrity/signing shouldn't we?

Yes, we should do that. Can you help? Not sure what's the best solution, I mean, how to check this. The release page of F2B offers a link to check a hash sum.

Give me a moment

One easy method, without using GPG, would be to check the pre-computed SHA152 sum like this:

sha512sum --check --status <<< "09d91ad0f499b0825b9d6da1d175400aa32498aff5705624e88b3d7e02cf727403c49b967c732e42182e25d4f439de0faf681f9f6d3598364e3f2f2e3453f8bf fail2ban_0.11.2-1.upstream1_all.deb"

which could be written directly into the Dockerfile. Moreover, sha512sum is from the coreutiles package - no extra package dependency necessary. The downside is: just an integrity check, no signature verification.

This should be enough, to ensure the file was not altered.

However, my checksum differs:

echo "44fe3d67e7d24bba4fc81ee672059dcb67980258db6101bcd8740736a9d9110f *fail2ban.deb" | sha256sum --check --status

or

sha256sum --check --status <<< "44fe3d67e7d24bba4fc81ee672059dcb67980258db6101bcd8740736a9d9110f *fail2ban.deb"

Is there a reason for the package pinning in that particular case

no just personal habit ;) sorry

IMO we don't need FAIL2BAN_VERSION at all. There are not that much releases of f2b. And as you said, when there is a new version, we have to adjust the Dockerfile and maybe f2b_url either way.

Agree 👍🏻

In context of security, this feels wrong: wget -q --no-check-certificate.

True but doesn't matter if the signing is correct. There is no need to use wget, we can also use curl if that does behave other than wget in CI. If you look at the linked example in my comment above i do not use --no-check-certificate and have no issues with gh action or am i missing smth?

What kind of improved security do we get from this, compared to the very simple file hash comparison

We are not only checking if the file got altered but also if it was created by fail2ban or did i get it wrong?
Nevertheless the hash comparison adds that bit of security compared to having no check at all that i am fine with both ☑️

ARG should be ENV

I disagree as we have no benefit of having these as ENV in the Container later on. We only need them during build an therefore ARG should be enough. But as the behavior does not change i do not insist of using ARG

docker build passes without error or warning, i'm running the test suite right now

docker build passes without error or warning, i'm running the test suite right now

I will commit some of the suggested changes, but you'd need to do final adjustments to this yourself:)

We are not only checking if the file got altered but also if it was created by fail2ban or did i get it wrong?

Correct. I considered this only as a temporary addition (until Debian 11 is released). So manually sign checking once and then hard-coding the checksum would be enough (way less additions to Dockerfile needed).

If we are going this route long-term, then the gpg solution is more complete and convenient 👍

We only need them during build an therefore ARG should be enough.

Good point. I had the wrong assumption, that using ARG in Dockerfile and not providing a build-arg for that during the build would throw a warning. That is not the case.
Maybe we should think about replacing most ENV with ARG in the whole Dockerfile. The ENVs are populated by the start-mailserver.sh script. So we don't need to bake them into the image.

The ENVs are populated by the start-mailserver.sh script. So we don't need to bake them into the image.

Good point

wernerfred

I suggested changes to drop the FAIL2BAN_VERSION and removed the version pinning of packages

Co-authored-by: Frederic Werner <[email protected]>

georglauterbach · 2021-05-13T08:46:15Z

@casperklein @wernerfred I'm low on time today and tomorrow (Bachelor's thesis is waiting for me). Can you take care of the changes you suggested. I'm more than happy to review this in the end.

georglauterbach · 2021-05-14T20:32:07Z

LGTM

Anything left to do?

wernerfred

I think this is fine to merge.
Do we need a documentation Update?

georglauterbach · 2021-05-15T09:11:03Z

I think this is fine to merge.
Do we need a documentation Update?

I don't think docs needs an update. I will go ahead and merge this.

casperklein · 2021-05-15T10:03:48Z

Not doc, but we should mention it in the changelog as a breaking change, when v10 will be released.

casperklein · 2021-05-15T10:09:39Z

I just wondered, why there is no new image @ dockerhub. --> build-multiarch-and-publish failed 😕

wernerfred · 2021-05-15T11:01:29Z

Do we have an arch problem?

casperklein · 2021-05-15T11:10:29Z

Yes. But what exactly goes wrong is hard to tell, because all error output is suppressed.

wernerfred · 2021-05-15T15:58:09Z

What are the next steps? How to debug this? Should we open a PR after #1970 got merged to see the build output on arm or do we revert this PR?

casperklein · 2021-05-15T18:35:25Z

#1970 does not add additional output. It will only help for future PRs, so that we know in advance before merging, the build fails.

To get more output and track down which command exactly fails, we would have to remove all the &>/dev/null, -qq etc parts.

I did this locally, but unfortunately, my build for arm64/armv7 both succeed. I looked for the "exit code 60" in google and the first result was about a certificate problem of curl. @aendeavor mentioned:

The --no-check-certificate is needed in CI as wget behaves strangely in pipelines

If I had to blindly guess, I would say, that curl behaves similar. curl --insecure should then fix it.

To track down this and future problems more easily, it's probably a good idea, to remove the &>/dev/null statements.

georglauterbach · 2021-05-15T18:39:40Z

I did this locally, but unfortunately, my build for arm64/armv7 both succeed. I looked for the "exit code 60" in google and the first result was about a certificate problem of curl. @aendeavor mentioned:

The --no-check-certificate is needed in CI as wget behaves strangely in pipelines

If I had to blindly guess, I would say, that curl behaves similar. curl --insecure should then fix it.

That is probably it.

To track down this and future problems more easily, it's probably a good idea, to remove the &>/dev/null statements.

Yes.

casperklein · 2021-05-15T18:44:59Z

Another problem: With the commit of #1970 test_merge_requests.yml got broken, because it's not using buildx compared to default_on_push.yml

As long this is not fixed, we cannot test #1971.

casperklein · 2021-05-15T18:58:12Z

I can try to adjust test_merge_requests.yml to use buildx, but that will take some time as I am not that familiar with gh actions.

@wernerfred @aendeavor if one of you could do it, I wouldn't be sad 😘

Or do you have any other suggestions how to proceed?

georglauterbach · 2021-05-15T19:42:22Z

I can try to adjust test_merge_requests.yml to use buildx, but that will take some time as I am not that familiar with gh actions.

@wernerfred @aendeavor if one of you could do it, I wouldn't be sad 😘

Or do you have any other suggestions how to proceed?

Still low on time to be honest 🙈 I have not done the build stuff at all. But I will help if I can. Remember, we've got time, no need to hurry. 9.1.0 is out there and solid :D

casperklein · 2021-05-15T20:08:23Z

I had a look and will give it a try. Doesn't seem that hard as I thought.

Edit: See #1972

wernerfred · 2021-05-15T20:37:04Z

If I had to blindly guess, I would say, that curl behaves similar. curl --insecure should then fix it.

But why does it work in the PR test then? This run is also in CI environment. Strange

casperklein · 2021-05-15T20:40:17Z

Good point. That would indicate it's just related to arm builds? Or the curl guess is wrong.

wernerfred · 2021-05-16T09:14:03Z

To get more output and track down which command exactly fails, we would have to remove all the &>/dev/null, -qq etc parts.

We can still revert #1965 and restore the f2b-0.11 branch. Then master will be clean again and we can use the recovered branch to create another PR that doesn't use -qq etc. to nail down the issue. If it passes (bc your PR #1970 will show it then) we can add back those arguments and merge.

georglauterbach · 2021-05-16T09:31:47Z

When #1972 passes on master, do we really need to revert this PR? We could just add another that removes the -qq and [&]>/dev/null, if we want to.

wernerfred · 2021-05-16T09:44:32Z

Absolute, will have the Same result. Just a Point of having a broken state commited to master for a certain amount of time

georglauterbach · 2021-05-16T09:52:49Z

Absolute, will have the Same result. Just a Point of having a broken state commited to master for a certain amount of time

I can live with that, since master represents :edge and does not build stable images :D

casperklein · 2021-05-17T16:45:50Z

Another downside of the &>/dev/null snippets is, that we missed the apt-get info, that gnupg is already available and don't need to be installed for fail2ban 😉

docker-mailserver/Dockerfile

Line 54 in 225e21e

ed fetchmail file gamin gnupg gzip iproute2 iptables \

This new script is a clean way of handling the installation of packages. I think the huge `RUN` command in `Dockerfile` was hard to read and maintain. Using a script is a non-issue, as the image is rebuilt whenever the script is touched. Moreover, this commit implicitly removed the explicit Fail2Ban installation and therefore #1965, BECAUSE Debian 11 now ships with the latest and greatest Fail2Ban version (even more up-to-date than out current version it seems).

introduce F2B v0.11

8e60ed1

georglauterbach added area/dependency meta/needs triage This issue / PR needs checks and verification from maintainers priority/medium service/security/fail2ban kind/improvement Improve an existing feature, configuration file or the documentation labels May 12, 2021

georglauterbach requested review from a team and casperklein May 12, 2021 09:45

georglauterbach self-assigned this May 12, 2021

Merge branch 'master' into f2b-0.11

3d9c96b

Merge branch 'master' into f2b-0.11

7453eea

wernerfred reviewed May 12, 2021

View reviewed changes

adjusted tests to reflect latest version output

2272901

georglauterbach mentioned this pull request May 12, 2021

PGP/GPG Public-Key & Fingerprint for Release fail2ban/fail2ban#3026

Closed

georglauterbach added 3 commits May 12, 2021 14:14

further test adjustment (same test)

e4b21b8

adding PGP test for F2B

a5ab4c6

log adjustment

4e07265

wernerfred reviewed May 12, 2021

View reviewed changes

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

Comment thread Dockerfile Outdated

georglauterbach and others added 4 commits May 12, 2021 19:28

Update Dockerfile

d0ef6e7

Co-authored-by: Frederic Werner <[email protected]>

Update Dockerfile

016fdc8

Co-authored-by: Frederic Werner <[email protected]>

Update Dockerfile

ab34e43

Co-authored-by: Frederic Werner <[email protected]>

Update Dockerfile

df109ac

Co-authored-by: Frederic Werner <[email protected]>

wernerfred added 2 commits May 13, 2021 12:29

chore: replace wget with curl and remove asc in cleanup

3b56e84

style: streamline linewidth

89d2d26

casperklein approved these changes May 14, 2021

View reviewed changes

wernerfred approved these changes May 14, 2021

View reviewed changes

georglauterbach merged commit 75e74e4 into master May 15, 2021

georglauterbach deleted the f2b-0.11 branch May 15, 2021 09:11

casperklein mentioned this pull request May 15, 2021

Add ARM platforms #1970

Merged

11 tasks

casperklein mentioned this pull request May 15, 2021

ARM build fix #1971

Merged

11 tasks

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

polarathene mentioned this pull request May 24, 2021

Update check #1951

Merged

11 tasks

georglauterbach mentioned this pull request Aug 30, 2022

ci: enhance build process #2755

Merged

7 tasks

Uh oh!

Conversation

georglauterbach commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

georglauterbach commented May 12, 2021

Uh oh!

georglauterbach commented May 12, 2021

Uh oh!

wernerfred commented May 12, 2021

Uh oh!

georglauterbach commented May 12, 2021

Uh oh!

wernerfred May 12, 2021

Choose a reason for hiding this comment

Uh oh!

georglauterbach May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georglauterbach May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein May 12, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georglauterbach May 12, 2021

Choose a reason for hiding this comment

Uh oh!

casperklein May 12, 2021

Choose a reason for hiding this comment

Uh oh!

wernerfred May 12, 2021

Choose a reason for hiding this comment

Uh oh!

wernerfred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented May 13, 2021

Uh oh!

georglauterbach commented May 14, 2021

Uh oh!

wernerfred left a comment

Choose a reason for hiding this comment

Uh oh!

georglauterbach commented May 15, 2021

Uh oh!

casperklein commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperklein commented May 15, 2021

Uh oh!

wernerfred commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casperklein commented May 15, 2021

Uh oh!

georglauterbach commented May 12, 2021 •

edited

Loading

georglauterbach May 12, 2021 •

edited

Loading

wernerfred May 12, 2021 •

edited

Loading

georglauterbach May 12, 2021 •

edited

Loading

wernerfred May 12, 2021 •

edited

Loading

wernerfred May 12, 2021 •

edited

Loading

casperklein commented May 15, 2021 •

edited

Loading

wernerfred commented May 15, 2021 •

edited

Loading

casperklein commented May 15, 2021 •

edited

Loading

georglauterbach commented May 16, 2021 •

edited

Loading

casperklein commented May 17, 2021 •

edited

Loading