It's really nice to have upstream and the debian maintainer looking at this issue too, thanks for joining the conversation and your recent release! :)
Review for Source Package: nbd (already in main) with focused on promoting nbd-client [Summary] MIR team ACK under the constraint to resolve the list below. This does not need a security review as -server package was already promoted previously and the client doesn’t have a large attack surface. List of specific binary packages to be promoted to main in addition to exising one: nbd-client Notes: I mainly focused my review on the -client binary package. However, we always use this opportunity to rereview the other parts of the code and packaging. Packages are modified, upstream source code too, so the blanket "this is already in main" doesn't prevent us to do another round of review (ideally, that would be done also outside of the MIR cycle), and I used that opportunity for this. Also, the MIR requirements evolves (hopefully in a good direction) with time, and this is the perfect opportunity to get the required TODO being fixed! For instance, I see the recent upload migrated to source 3.0 (quilt) format and this is excellent! I really appreciated the detailed bug description. However, some elements were missing on the MIR template rules and checks, like the output of lintian pedantic running as per https://github.com/canonical/ubuntu-mir. Also, some information are not exact, you state: "* The package installs no services, timers, or recurring jobs". However, nbd-client installs /lib/systemd/system/nbd@.service (not nbd-client which is symlinked to /dev/null). I have then had to recheck and find the informations myself. Please try to follow the template thoughtfully next time so that the burden on the MIR team is lighter, and we can process MIR bugs quicker. :) Required TODOs: - I see that we have a really trivial autopkgtests as of now (I read the discussion above about it too). I think this one covers the real basics of the packages but it’s not comprehensive enough. Since the package was introduced in main, the MIR rules have changed and now required better testing story than when it was promoted. This include autopkgtests too. If non trivial autopkgtests is not feasable, we have then to fallback to a manual test plan, that is documented, updated, and run with any upload of the package. This will though not prevent from reverse dependency regression, meaning that the test plan has to be run manually at regular intervals too. As you can see, autopkgtest is a way more future-proof looking in term of maintainance :) - We do run nbd-client as a root process for each nbd device block found via a systemd service. However, I see this one has no systemd confinement set. It would be great to have as much restriction on the systemd unit itself to limit the attack surface. [Rationale, Duplication and Ownership] The rationale given in the report seems valid and useful for Ubuntu [Dependencies] OK: - no other Dependencies to MIR due to this - nbd 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. [Embedded sources and static linking] OK: - no embedded source present - no static linking - does not have unexpected Built-Using entries OK: - not a go package, no extra constraints to consider in that regard - not a rust package, no extra constraints to consider in that regard - Does not include vendored code [Security] OK: - history of CVEs does not look concerning - does not run a daemon as root - does not use webkit1,2 - does not use lib*v8 directly - does not parse data formats (files [images, video, audio, xml, json, asn.1], network packets, structures, ...) from an untrusted source. - does not expose any external endpoint (port/socket/... or similar) - does not process arbitrary web content - does not use centralized online accounts - does not integrate arbitrary javascript into the desktop - does not deal with system authentication (eg, pam), etc) - does not deal with security attestation (secure boot, tpm, signatures) - does not deal with cryptography (en-/decryption, certificates, signing, ...) - this makes appropriate (for its exposure) use of established risk mitigation features (dropping permissions, using temporary environments, restricted users/groups, seccomp, systemd isolation features, apparmor, ...) Problems: - does run nbd-client as a root process for each nbd device block found via a systemd service. However, I see this one has no systemd confinement set. It would be great to have as much restriction on the systemd unit itself to limit the attack surface. [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 - no new python2 dependency Problems: - there is a really trivial autopkgtests (which is good for bootstrapping), but does not cover most of the functionality of the package. We should either get a more exhaustive autopkgtest testsuite or (as a fallback) a manual test plan [Packaging red flags] OK: - Ubuntu does not carry a delta - symbols tracking not applicable for this kind of code. - Upstream update history is good - Debian/Ubuntu update history is good - the current release is packaged - 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/watch is not present, but the debian maintainer is upstream too :) [Upstream red flags] OK: - no Errors/warnings during the build - no incautious use of malloc/sprintf (as far as we can check it) - no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests) - no use of user nobody (consider at least `grep -Hrn nobody` for it and run `find . -user nobody` in source and built binaries) - use of setuid, but ok because this is not in the -client package but -server which already had a security review - no important open bugs (crashers, etc) in Debian or Ubuntu - no dependency on webkit, qtwebkit, seed or libgoa-* - not part of the UI for extra checks - no translation present, but none needed for this case Problems: - Some usage of setuid, but ok as this is not in the -client package but -server. ** Changed in: nbd (Ubuntu) Status: New => Incomplete ** Changed in: nbd (Ubuntu) Assignee: Didier Roche-Tolomelli (didrocks) => (unassigned) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/2054480 Title: [MIR] nbd-client To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/nbd/+bug/2054480/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
