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

chore(image): Update Debian base image to Debian 11 "Bullseye" #2116

Merged
merged 23 commits into from
Dec 10, 2021

Conversation

georglauterbach
Copy link
Member

Description

This PR updates the base image to Debian 11 "Bullseye". Debian 11 is going to be released on the 14th of August, 2021. We can already go ahead and see what our CI pipeline says about this change to prepare for the update.

Type of change

  • Possibly 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 georglauterbach added area/ci area/dependency priority/low kind/improvement Improve an existing feature, configuration file or the documentation labels Aug 11, 2021
@georglauterbach georglauterbach added this to the v10.2.0 milestone Aug 11, 2021
@georglauterbach georglauterbach requested review from a team August 11, 2021 09:53
@georglauterbach georglauterbach self-assigned this Aug 11, 2021
williamdes
williamdes previously approved these changes Aug 11, 2021
@georglauterbach

This comment has been minimized.

@casperklein
Copy link
Member

btw, I would prefer version numbers instead of code names in the future --> debian:11-slim

@georglauterbach

This comment has been minimized.

@georglauterbach

This comment has been minimized.

@casperklein

This comment has been minimized.

@casperklein

This comment has been minimized.

@casperklein casperklein marked this pull request as draft August 11, 2021 14:26
@georglauterbach

This comment has been minimized.

@NorseGaud

This comment has been minimized.

@casperklein

This comment has been minimized.

@georglauterbach

This comment has been minimized.

@casperklein

This comment has been minimized.

@georglauterbach

This comment has been minimized.

@georglauterbach georglauterbach added stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI meta/feature freeze On hold due to upcoming release process labels Nov 15, 2021
@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Nov 29, 2021
@georglauterbach
Copy link
Member Author

Now that v10.3.0 was released, I think we can go ahead an merge this PR as well. Are there currently any objections?

@casperklein
Copy link
Member

casperklein commented Nov 29, 2021

I propose, changing SSL_TYPE=letsencrypt to e.g. SSL_TYPE=certbot. I think, that is more clear. You don't have to use SSL_TYPE=letsencrypt, when using a LE certificate. That have confused some people in the past. Also in the meantime, there are new cert providers supporting free certificates via ACME, like ZeroSSL. So certbot can also use other provides instead of LE.

@williamdes
Copy link
Contributor

I propose, changing SSL_TYPE=letsencrypt to e.g. SSL_TYPE=certbot. I think, that is more clear. You don't have to use SSL_TYPE=letsencrypt, when using a LE certificate. That have confused some people in the past. Also in the meantime, there are new cert providers supporting free certificates via ACME, like ZeroSSL. So certbot can also use other provides instead of LE.

We should add "certbot" and keep the other options IMO. having "certbot" would mean, pick any provider you want.
Another value than "certbot" would also be nice, maybe "automatic-ca" or "automatic".
Ref: https://github.com/acmesh-official/acme.sh#supported-ca

@georglauterbach
Copy link
Member Author

The discussion about LE vs certbot is very valid, but I'd rather like to keep this PR as simple as possible. The discussion could very well be solved with a future PR so we do not mix up concerns here.

@casperklein
Copy link
Member

Why would we want to keep a outdated/not clear option naming? This is a major release, expected to introduce breaking changes.
SSL_TYPE=certbot nails it, because that's exactly what happens internally (handling a certbot generated json file).

PS: I don't know if that json file is certbot specific or a standardized acme json file.

@williamdes
Copy link
Contributor

The discussion about LE vs certbot is very valid, but I'd rather like to keep this PR as simple as possible. The discussion could very well be solved with a future PR so we do not mix up concerns here.

One last message about it, for me certbot is a tool and LE a CA. Mixing the two is just incorrect (IMO). And changing a LE value to certbot makes it worse creating more confusion. People need to understand that certbot is a tool and LE one of many CAs. This is only my thoughts, let's discuss about all this in a dedicated GH discussion ;)
And also, the day we want to throw out certbot for acme.sh for example we will be using a tool name for the wrong tool used. You get it

@georglauterbach
Copy link
Member Author

Why would we want to keep a outdated/not clear option naming? This is a major release, expected to introduce breaking changes. SSL_TYPE=certbot nails it, because that's exactly what happens internally (handling a certbot generated json file).

I agree, but we do not immediately release v11 after this PR is merged, right? This is on :edge, and we can add other PRs afterwards as far as I understood it.

@casperklein
Copy link
Member

casperklein commented Nov 29, 2021

I haven't read the whole thread again, but I was the opinion, there were some other breaking changes mentioned, when introducing debian 11 🤷

PS: My answer above was primary directed to @williamdes, because he wanted to keep the "letsencrypt" option.

@polarathene
Copy link
Member

polarathene commented Nov 29, 2021

This is only my thoughts, let's discuss about all this in a dedicated GH discussion ;)

As we don't enable "Discussions" here and our internal GH teams area splits moderators and maintainers it seems I can't pick it up over there. That'd leave creating a new issue, but I think I'll just weigh in here since my response probably resolves it.


I propose, changing SSL_TYPE=letsencrypt to e.g. SSL_TYPE=certbot. I think, that is more clear. You don't have to use SSL_TYPE=letsencrypt, when using a LE certificate.

While I agree with the concerns about SSL_TYPE=letsencrypt, I don't agree with certbot for reasons similar to @williamdes .

In the past I have considered adding certbot or acme.sh internally and making that available as a mode to potentially simplify cert management/setup for some users that would prefer it, and that would be relevant as an SSL_TYPE then.

certbot would be incorrect for replacing letsencrypt however as nothing about this type internally is related to certbot really. We also document the letsencrypt mode for other providers such as nginx-proxy which uses acme.sh internally to provision it's LE certs.

Another value than certbot would also be nice, maybe automatic-ca or automatic.

Yeah, at one point I considered acme but since working on the refactor recently of letsencrypt support and getting familiar with that part of the support we provide via that mode, it's really just "automatic" discovery. At the same time automatic kinda conveys we do more than we actually do, we don't do any acutal provisioning.

We just check a hard-coded mount path for a few variations of expected cert folders and then for a few hard-coded pem filenames (cert: fullchain.pem, key: key.pem / privkey.pem). You'll see from the comments there, that we already maintain for compatibility beyond certbot as a provisioner, which while they seem to standardize on the cert filename, there is a little variance for the private key filename.

automatic would be reasonably more accurate, but it still requires meeting certain expectations, hence our documentation groups a few providers under the "Let's Encrypt" mode section for what we know is supported by us officially.

because that's exactly what happens internally (handling a certbot generated json file).
PS: I don't know if that json file is certbot specific or a standardized acme json file.

It's neither. You can view the linked lines of code to the letsencrypt handling or the letsencrypt tests and both have been documented by me to clarify that acme.json is specific to Traefik support (which effectively falls under the letsencrypt mode, but only because we extract the files to the expected location and filenames to be compatible).

People need to understand that certbot is a tool and LE one of many CAs.

Absolutely agree.


TL;DR

👎 to SSL_TYPE=certbot, that would not be any better IMO. The mode will become irrelevant in future after a refactoring (late Dec or sometime in Jan), see below for details.


FWIW, refactoring this particular feature has been on my TODO for quite a while, but I've been busy outside of the project as well as spending time for this project on assisting with other PRs / issues.

With the refactoring that was started in v10.3.0 the linked TLS logic from setup-stack.sh will be extracted out into a helper as check-for-changes.sh needs it for an edge case bug, and then afterwards a larger refactoring of the handling will take place, likely to unify all the modes as there is a lack of feature parity and generally no need for them to be distinct modes, rather separate complimentary ENVs will address those needs better and the current letsencrypt support will just become a fallback for discovery if explicit paths are not provided.

As for internally using certbot or acme.sh, for the time being, it may be better supported via documentation examples of using user-patches.sh as I can't see us officially supporting in issues or documentation beyond that (they've both got massive docs and all we care about is the end result files).

@polarathene
Copy link
Member

I haven't read the whole thread again, but I was the opinion, there were some other breaking changes mentioned, when introducing debian 11 🤷

Those IIRC were related to actual changes from the Debian upgrade, such as the default hashing for user accounts changing and if we should consider adopting that for Dovecot (which spurred some discussion about alternatives like argon2id).

I've tried to manage the noise in the discussion by hiding no longer relevant messages, while keeping some (mostly mine) that were still deemed of value for someone going over the PR, some which are buried now until you expand the collapsed entries to get the full PR discussion listed such as:

  • This one relevant to release notes
  • And this one regarding notable changes to discuss about password hashing and CVE/bugs like bsd-mailx with fail2ban.

Regarding those changes, password hashing won't be touched anytime soon due to effort required to investigate implementation and compatibility concerns (it was discussed in the WIP encryption feature thread too). And for the bsd-mailx concern where @williamdes expresses interest in that continuing to work, so someone (perhaps @williamdes ) will need to verify that works fine and the CVE/bug concerns (that I linked to) aren't applicable to keep the feature around.


Now that v10.3.0 was released, I think we can go ahead an merge this PR as well. Are there currently any objections?

We may want to wait a week, one of the open issues about letsencrypt renewal bug is going to be confirmed if fixed or not. It'd be a bit easier to address from feedback via a hotfix (useful in general for holding off on merging this PR after a new fresh release) or new minor release if such a release doesn't have to dance around changes like this one would add, since I don't anticipate v11 is being released until late Dec or in Jan? Or are you wanting to release v11 with minimal changes beyond this PR?

No objections from me, other than that concern.

@casperklein
Copy link
Member

certbot would be incorrect for replacing letsencrypt however as nothing about this type internally is related to certbot really.

I agree. I admit, I should have take a deeper look before. My point was, that SSL_TYPE=letsencrypt is not suitable and we should choose a better wording.

The mode will become irrelevant in future after a refactoring (late Dec or sometime in Jan), see below for details.

Ack. I am fine with not changing the wording for now.

@georglauterbach
Copy link
Member Author

We can with merging this PR for a week, sure. There is no rush with releasing v11.

@georglauterbach georglauterbach linked an issue Dec 5, 2021 that may be closed by this pull request
@georglauterbach
Copy link
Member Author

georglauterbach commented Dec 8, 2021

@polarathene please reach back to me whe you're fine with merging this (when the issues seem resolved, I guess), or go ahead and merge this yourself by all accounts :)

@polarathene polarathene merged commit 8a47e7d into master Dec 10, 2021
@polarathene polarathene deleted the debian-11 branch December 10, 2021 22:24
@polarathene polarathene modified the milestones: v11.0.0, v10.4.0 Dec 18, 2021
@polarathene polarathene removed the stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI label Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci 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.

Upgrade base image to Debian 11
5 participants