Review for Source Package: provd [Summary] This is the backend for Ubuntu "initial setup" provisioning story for Desktop systems, similar to gnome-initial-setup, but enhanced by Ubuntu Pro components and others. It's a relatively new Ubuntu native package, supposed to replace "gnome-initial-setup" and "oem-config" in main. MIR team ACK under the constraint to resolve the below listed required TODOs and as much as possible having a look at the recommended TODOs. This does need a security review, so I'll assign ubuntu-security List of specific binary packages to be promoted to main: provd Specific binary packages built, but NOT to be promoted to main: Notes: #0 Generic questions #0.a Why not ship this as part of the "ubuntu-desktop-init" snap? (i.e. together with its frontend) #0.b Could you please briefly differentiate this tool from cloud-init, which is also used as part of the new Desktop installer? #1 team bug subscriber ~desktop-packages is already subscribed Required TODOs: #2 Dependency on "gnome-initial-setup" needs to be dropped #3 "gnome-initial-setup" & "oem-config" need to be demoted #4 Improve Go packaging, I'm not an expert here, but I think we should at least have an "Built-Using" in debian/control, to indicate the toolchain that was used to build this #5 Add files (debian/README.source) that explains how to refresh the vendored sources #5.a Please give rational why all the vendoring is needed (c.f. recommendation #6) Recommended TODOs: #6 Consider using more "golang-*-dev" packages from the archive where possible, indicated by "Static-Built-Using" in debian/control to avoid some vendoring #7 Consider using more mitigation features (dropping permissions, using temporary environments, restricted users/groups, seccomp, systemd isolation features, apparmor, ...), especially setting suid via systemd #8 Consider running more complex integration tests as autopkgtest (e.g. integration with the ubuntu-desktop-init" snap), in addition to the "go test" unit-tests. #9 Consider fixing some of the lintian warnings (see "Packaging red flags" below) #10 Consider fixing some of the build-time warnings (see "Upstream red flags" below) [Rationale, Duplication and Ownership] - A team is committed to own long term maintenance of this package. (~desktop-packages) - The rationale given in the report seems valid and useful for Ubuntu Problems: - Depends on gnome-initial-setup - There are other package in main providing the same functionality. => gnome-initial-setup and ubiquity/oem-config (to be demoted) => cloud-init (used in desktop-installer) – please differentiate [Dependencies] OK: - no other Dependencies to MIR due to this - SRCPKG checked with `check-mir` - all dependencies can be found in `seeded-in-ubuntu` (already in main) - none of the (potentially auto-generated) dependencies (Depends and Recommends) that are present after build are not in main - no -dev/-debug/-doc packages that need exclusion - No dependencies in main that are only superficially tested requiring more tests now. Problems: None [Embedded sources and static linking] Problems: - embedded source present - static linking - lacking [Static-]Built-Using entries in control file (usage of 'golang-*-dev' is encouraged) dpkg-gencontrol: warning: package provd: substitution variable ${misc:Built-Using} unused, but is defined dpkg-gencontrol: warning: package provd: substitution variable ${misc:Static-Built-Using} unused, but is defined dpkg-gencontrol: warning: package provd: substitution variable ${misc:Built-Using} unused, but is defined dpkg-gencontrol: warning: package provd: substitution variable ${misc:Static-Built-Using} unused, but is defined - Go Package that follows the Debian Go packaging guidelines - not a rust package, no extra constraints to consider in that regard Problems: - golang: static builds are used, the team did not yet confirmed their commitment to the additional responsibilities implied by static builds. - vendoring is used, but the reasoning is not sufficiently explained - Includes vendored code, the package has documented how to refresh this code at debian/README.source <= This file is missing! [Security] OK: - history of CVEs does not look concerning (new package) - does run a daemon (but not as as root??) - does not use webkit1,2 - does not use lib*v8 directly - does not process arbitrary web content - does not integrate arbitrary javascript into the desktop - does not deal with security attestation (secure boot, tpm, signatures) Problems: - new package - running a daemon (as non-root?) - does parse data formats (e.g Ubuntu Pro response) from an untrusted source. Lots of parsers in vendored code, too. - does expose external endpoint (socket:/run/gnome-initial-setup/desktop-provision/init.socket) - does use centralized online accounts (Ubuntu Pro) - does deal with system authentication (gdm setup, active directory) - does deal with cryptography (password hashes) - this high risk application could make more use of mitigation features (dropping permissions, using temporary environments, restricted users/groups, seccomp, systemd isolation features, apparmor, ...) [Common blockers] OK: - does not FTBFS currently - does have a test suite that runs at build time - test suite fails will fail the build upon error. - does have a non-trivial test suite that runs as autopkgtest - This does not need special HW for build or test - no new python2 dependency - Go package, but using dh-golang Problems: - running "go test" unit tests as autopkgtest. Maybe some integration tests using the "ubuntu-desktop-init" snap would be helpful, too. [Packaging red flags] OK: - Ubuntu does not carry a delta (Ubuntu native package) - symbols tracking not applicable for this kind of code. - debian/watch is not present but also not needed (e.g. native) - Upstream update history is sporadic – its a native package after all - promoting this does not seem to cause issues for MOTUs that so far maintained the package - no massive Lintian warnings - debian/rules is rather clean - It is not on the lto-disabled list Problems: - Debian/Ubuntu update history is sporadic (not a lot of history, being a new package) - the current release is not packaged, upstream (GitHub) contains 0.1.4, should be an easy upgrade from 0.1.2 - lintian warnings: I: provd source: missing-built-using-field-for-golang-package (in section for provd) [debian/control:18] I: provd source: unused-license-paragraph-in-dep5-copyright gpl-3 [debian/copyright:353] Nitpicks: I: provd: spelling-error-in-binary acccessed accessed [usr/libexec/provd] I: provd: spelling-error-in-binary compatibile compatible [usr/libexec/provd] I: provd: spelling-error-in-binary divison division [usr/libexec/provd] I: provd: spelling-error-in-binary standar standard [usr/libexec/provd] I: provd: spelling-error-in-binary standart standard [usr/libexec/provd] [Upstream red flags] OK: - no Errors during the build - no incautious use of malloc/sprintf (the language has no direct MM) => some "unsafe" code in internal/services/user/password_hash.go - no use of user nobody - no important open bugs (crashers, etc) in Debian or Ubuntu - no dependency on webkit, qtwebkit or libseed - not part of the UI for extra checks - no translation present, but none needed for this case Problems: - use of sudo, gksu, pkexec, or LD_LIBRARY_PATH => sprovd/sprovd.go wraps "sudo pro attach" - use of setuid / setgid in /usr/libexec/sprovd => prefer systemd to set those flags, or give rationale why it's done another way - Warnings during the build: go: warning: "./..." matched no packages ? github.com/canonical/ubuntu-desktop-provision/provd [no test files] # github.com/linuxdeepin/go-gir/gobject-2.0 fix_gobject.c: In function ‘_g_type_param_value_array’: fix_gobject.c:74:13: warning: Deprecated pre-processor symbol 74 | GType _g_type_param_value_array() { return G_TYPE_PARAM_VALUE_ARRAY; } | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~