search

thelounge / thelounge-docker

🐳 ‎ ‎Docker image for The Lounge, a self-hosted web IRC client
https://ghcr.io/thelounge/thelounge
MIT License
306 stars 73 forks source link

Rebuilding the image only when there is a new version #138

Open mhajder opened 2 years ago

mhajder commented 2 years ago

Recently a cron has been added which rebuilds the image once a week: https://github.com/thelounge/thelounge-docker/blob/8d1cfb8bbff6bde7e87fccc6ddc2b6038fd21d24/.github/workflows/docker-image-push.yml#L5

As a result, many people who updated the container automatically with the watchtower had to disable it or their instance will be restarted once a week..

Maybe a better solution would be to rebuild the :latest image and build the image with the new version only when the new version is added to thelounge/thelounge repository?

An example of how this can be done is described HERE or HERE.

brunnre8 commented 2 years ago

That's not enough. At a minimum you need to rebuild when the base image changes for whatever reason and when a TL release happens.

mhajder commented 2 years ago

But is it really necessary to rebuild old version images that aren't really used anymore and stay only as archival versions? This is not some typical government software that has a high risk of attack, if there is an exploit on a base image, you can rebuild everything manually.

brunnre8 commented 2 years ago

Yes it is necessary, no it is not ok to leave things in the repo which are vulnerable to known CVEs.

Why do you care if an older version gets bumped? You are supposed to pull in a specific version / label of a container so that should not impact you at all if some lower version does anything.

brunnre8 commented 2 years ago

Also, what do you consider old archived images? As far as I can tell the rebuild applied to the "latest" label plus the latest major release which is a sensible thing to rebuild

mhajder commented 2 years ago

Why do you care if an older version gets bumped?

Because in that case the watchtower pull the new version of the same container and restarts my thelounge instance. I don't mean the old versions, but mostly the tag :latest.

You are supposed to pull in a specific version / label of a container so that should not impact you at all if some lower version does anything.

But in that case the automatic update will not work.

Also, what do you consider old archived images? As far as I can tell the rebuild applied to the "latest" label plus the latest major release which is a sensible thing to rebuild

You're right, I didn't notice it, I looked at the tag :4 and thought previous versions are built automatically too.

Also, shouldn't the :latest tag be the last production version as it is done in other popular containers?

brunnre8 commented 2 years ago

You absolutely should pull in the new container when the base image updates, else you're running an insecure version.

That's the whole point of automatic updates.

Also, shouldn't the :latest tag be the last production version

Both ways have merits, not my choice and not really related to the issue at hand

williamboman commented 2 years ago

Hello!

As a result, many people who updated the container automatically with the watchtower had to disable it or their instance will be restarted once a week..

Ah yeah I was a bit worried this would end up happening. Just out of curiosity, when you say many people, how many are we talking about and where did you gather this data?

Maybe a better solution would be to rebuild the :latest image and build the image with the new version only when the new version is added to thelounge/thelounge repository?

The reason for rebuilding and republishing the latest image versions is to harden them. There are primarily two vectors to keep in mind:

I'd be fine with not doing this and just building fresh images whenever there's a new release upstream, but it's important to consider the lifecycle of TL's releases. A release may remain "latest" for a very long time (roughly counting 4.2.0 -> 4.3.0 took 15 months, for example).

Also, shouldn't the :latest tag be the last production version as it is done in other popular containers?

The :latest tag points to the latest stable release (not release candidates or pre-releases).

I'd be OK with not rebuilding on a scheduled basis but only when there's a reason that warrants a rebuild (for example flagged CVEs). I won't have the time to look into figuring out the best way to do this, but PRs are always welcomed.

mhajder commented 2 years ago

Ah yeah I was a bit worried this would end up happening. Just out of curiosity, when you say many people, how many are we talking about and where did you gather this data?

From thelounge IRC channel.

These are not hundreds of people, but rather several. But probably many people do not know about this possibility, so one day we could add to the documentation information about the auto pull of the image through the watchtower.

A release may remain "latest" for a very long time (roughly counting 4.2.0 -> 4.3.0 took 15 months, for example).

You are right about that. I didn't think it through.

I'd be OK with not rebuilding on a scheduled basis but only when there's a reason that warrants a rebuild (for example flagged CVEs). I won't have the time to look into figuring out the best way to do this, but PRs are always welcomed.

In that case, I will look for what options we have available so that the container will be built when CVE is detected in the base image.

brunnre8 commented 2 years ago

Keep it simple and stupid, just update whenever the base image updates or TL releases.

No need to look for CVEs

williamboman commented 2 years ago

Keep it simple and stupid, just update whenever the base image updates or TL releases.

Yeah I think this is a good start.

No need to look for CVEs

I think it'd be nice to capture CVEs flagged in any of the installed node dependencies, don't you think?

brunnre8 commented 2 years ago

Considering the version pins that wouldn't do anything for this repo and on the main thelounge one it is already tracked (dependabot) assuming you mean the npm deps of TL

brunnre8 commented 2 years ago

and the whole node installation is supposed to be managed by the base image layer, including deps node itself uses

mhajder commented 2 years ago

You are right. As it is done now, it is correct. It makes no sense to look for any other option.

williamboman commented 2 years ago

Considering the version pins that wouldn't do anything for this repo and on the main thelounge one it is already tracked (dependabot)

I just saw that thelounge pins the dependency versions in package.json, so yeah that'd be pointless. I was thinking it'd at least allow for unbounded patch releases, which could be used to distribute CVE fixes. Base image upgrades only it is!

brunnre8 commented 2 years ago

yeah well, looking at https://snyk.io/blog/peacenotwar-malicious-npm-node-ipc-package-vulnerability/ and similar happy events, allowing patch releases without vetting is bound to end in tears