Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update Debian packaging #13582

Closed
wants to merge 6 commits into from
Closed

Conversation

behrmann
Copy link
Contributor

@behrmann behrmann commented Aug 22, 2022

Pull Request Checklist

This PR updates the Debian packaging to a more recent version of debhelper (within the limits of current Debian targets, judging from scripts-dev/build_debian_packages.py). It drops stuff that is related to init scripts, since the package doesn't ship any anymore.

I also added an explicit system group for the system user, as currently the files owned by the matrix-synapse user will belong to the nobody group, which is not ideal, since that group will regularly be used for unrelated things, so having data owned by matrix-synapse possibly readable by other system users by accident. If there is some deeper meaning behind this, I'll happily drop this commit, but since the repo that initally did the Debian packaging doesn't seem to exist anymore, I couldn't find a reasoning right away.

The major reason for me wanting to touch the Debian packaging (something I generally try to avoid), was that I wanted to fold the worker config into the regular packaging. The idea is to ship the worker service file, but not enable it. This has the benefit, that the debhelper generated postinst script will restart the target, which leads to the workers being restarted, too. This is something that can bite people using workers, since only the main service unit will be restarted.

I also moved the hardening config right into the service files. While I understand the idea of wanting to have a simple, easy to read service file, I think the upside of having a really locked down service, is a great benefit. I've been running that config for over a year by now (and a similar one before that). If you'd like to hide this, there would also be the option of moving all the hardening config into matrix-.service or matrix-.service.d (units have multiple namespaces, separated by dashes, so matrix-synapse.serviceactually looks for config inmatrix-synapse.service, matrix-synapse.service.d/.config, matrix-.service, matrix-.service.d/.config). This way the file itself would be simple, but systemd would still pick it up by default (and e.g. show it in the output of systemctl cat matrix-synapse.service`)

What I haven't touched yet is the systemd readme and service file in contrib/, but the (already now) the quality of the service file shipped in the Debian packaging is higher than the example there, so maybe they should be dropped in favour of links to the ones in the Debian packaging.

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@behrmann behrmann requested a review from a team as a code owner August 22, 2022 16:46
Otherwise the files of the synapse user are readable by the nobody user, which
is unsafe.

Signed-off-by: Jörg Behrmann <[email protected]>
Don't call dh_installinit anymore, because it has been deprecated, and use
dh_installsystemd instead of dh_systemd_enable for the same reason.

Signed-off-by: Jörg Behrmann <[email protected]>
It was used for reasons of interactions of dh_systemd_start and dh_installinit,
which have both be deprecated

Signed-off-by: Jörg Behrmann <[email protected]>
Signed-off-by: Jörg Behrmann <[email protected]>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thanks! Before we take a proper look at this, please could you split it up into several smaller PRs that aren't changing lots of things at once (eg, make the debhelper change one PR, and the removal of the init stuff another)

@behrmann
Copy link
Contributor Author

@richvdh I spun out the first three commits into #13593 and #13594. I'll leave the others for later, since they rest somewhat on these first ones.

@DMRobertson
Copy link
Contributor

@richvdh I spun out the first three commits into #13593 and #13594. I'll leave the others for later, since they rest somewhat on these first ones.

It looks like those two changes landed. Please advise: is there more to extract from this PR, or should we close this?

@behrmann
Copy link
Contributor Author

This one's not done yet and I picked up some more along the way, but I've been swamped with work the past few weeks and hadn't had time to update my PRs, sorry. I'll try to make room for them soon.

@DMRobertson
Copy link
Contributor

(Not to worry---just checking if there's any tidying up to do on our part.)

@clokep
Copy link
Member

clokep commented Jan 25, 2023

@behrmann Hello! Any chance of finishing up this pull request?

@behrmann
Copy link
Contributor Author

I'll close this one, as it's got to many conflicts anyhow and is better done piecemeal, but I'll get back to #13696.

@behrmann behrmann closed this Jan 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants