yggdrasil-go icon indicating copy to clipboard operation
yggdrasil-go copied to clipboard

Multiple quality issues with the generated deb

Open jgoerzen opened this issue 2 years ago • 5 comments

Hi,

This is both a conversation starter and a volunteer to help. The generated .deb is assembled very manually - down to the level of calling ar - and this is causing a number of quality issues.

  • The installed files are owned by uid 3434, group 3434 (corresponding to circleci on the build system) rather than root. If a uid 3434 exists on a system, this is a pretty significant security issue.
  • This also applies to the postinst and prerm files
  • As mentioned in #820, the systemd files shouldn't be installed to /etc.
    • Furthermore, files installed to /etc should be marked as conffiles so that user edits aren't blown away... but installing these to the correct location would address that.
  • There is no dependency information in the generated .deb, not even for libc.
  • The postinst script doesn't do proper error checking (set -e) and also should only run when called with configure as $1, instead of always.
  • The postinst script shouldn't modify a user's config file, which may blow away their comments and such.
  • The postinst script uses echo to display messages to the user, which the user often will not see (that's why we have debconf and other such things, because if it's in the middle of installing 100 packages, it will just scroll on by.)
  • There is a whole long list of output from lintian of other issues.

Using the Debian native toolchain, including with debhelper, would itself address almost all of these issues, and the rest could be resolved fairly trivially as well.

I am a Debian developer and am considering packaging this up for inclusion in Debian. If so, I will prepare Debian packaging for it. I could also prepare Debian packaging using the native toolchain for the git repo, though that would need to run in a Debian or Ubuntu CircleCI container. I don't know what OS is running in the CircleCI container right now.

jgoerzen avatar Jan 07 '22 15:01 jgoerzen

Lintian report attached. lintian.txt

jgoerzen avatar Jan 07 '22 15:01 jgoerzen

Absolutely agree that there are all sorts of things we could do better with the Debian package. The current package is a script I hacked together early on in the project just to get it to a usable and installable state. It’s certainly not state of the art or following any best practice whatsoever.

Increasingly we lean on the community now to provide up-to-date packages, since the burden of maintaining packages for countless distributions and platforms could quite easily take up more time than maintaining the project itself.

Would absolutely welcome any help in producing a better package. We actually did something similar a while ago for RPMs (separately, in the yggdrasil-network/yggdrasil-package-rpm repository) and it may be that the best option is for us to do the same thing here — break out into a new repository for Debian packaging.

Right now our CircleCI pipeline runs the Go 1.17 Linux image — see here.

Let me know if there’s anything I can do to help.

neilalexander avatar Jan 09 '22 00:01 neilalexander

Sure, I will be happy to do that. It will probably be next week due to some busy things on my end this week.

jgoerzen avatar Jan 11 '22 14:01 jgoerzen

Hello,

I have prepared a packge for Debian and uploaded it to Debian. The review process will likely take a few weeks. After it is approved to unstable and migrates to testing, I will also upload it to bullseye-backports (which will be subject to a new approval). After that, it will be faster to make new releases.

The Debian packaging lives here https://salsa.debian.org/go-team/packages/yggdrasil . I will highlight the changes I made between from the tree here:

  • It builds using the Debian native toolchain. The packaging content is in the debian/ directory.
  • The systemd unit file is installed to /lib, not /etc (see #821 )
  • Yggdrasil is not run as root. The systemd unit file uses the suggestions I and others made previously in #816 and also does not use CAP_NET_RAW per the discussion on Matrix. The Debian service file can be found here
  • Since there is no usable default configuration for Yggdrasil, the package ships without a default configuration and leaves the systemd service file disabled. Instructions to the admin for generating a config file and enabling the service are in README.Debian
  • The system does not attempt to restart the Yggdrasil service, or adjust the config file, on upgrade, since the Yggdrasil connection may be actively in use supporting ssh or whatever is being used to upgrade the system. This is also documented in README.Debian. It particularly does not automatically munge a user's configuration.
  • The source is patched so that the default run path is /var/run/yggdrasil/yggdrasil.sock and the default config path is /etc/yggdrasil/yggdrasil.conf.
  • The run path is changed so that the parent directory can be created with permissions that allow a rootless yggdrasil process to create a socket there.
  • /etc/yggdrasil is created, mode 0750, owned by root:yggdrasil. This helps prevent key leakage by way of editor backup files, etc.
  • genkey is installed as yggdrasil-genkey
  • All binaries are installed to /usr/sbin per the filesystem hierarchy standard (there is perhaps a small case to be made for genkey being in /usr/bin but that's probably not a big concern)
  • There are only three warnings from lintian: that each of the binaries lacks a manpage.

I am attaching the amd64 deb here. github requires that I zip it first, sigh.

yggdrasil-0.4.2.zip

jgoerzen avatar Jan 20 '22 15:01 jgoerzen

The next step will be to see if this can be easily modified to be applicable within this source tree. It will require a CircleCI runner that is Debian or Ubuntu, but I think that the answer is "yes, this should be fairly easy".

It would be ideal if there were a better way than patching the source to give the default run and config file locations.

jgoerzen avatar Jan 20 '22 16:01 jgoerzen