Review for Package: src: v4l2-relayd [Summary] We have a very new application (and package) here that can be used to capture video of MIPI cameras and transform/relay it into a standard v4l2 format, so it can be used by the usual desktop applications. Thank you for preparing the packaging and getting it into universe already! I think there is a bit of work to do in order to get this package in shape to fulfill the quality criteria required in main with a focus on 3 primary topics: + Dependencies to be MIRed + Security hardening + Testing
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 not need a security review List of specific binary packages to be promoted to main: 4vl2-relayd Specific binary packages built, but NOT to be promoted to main: <None> Notes: - Please improve the rationale a bit, is there any other way you tried enabling those MIPI cameras with tools that are already available? - This is looking good security wise (except the daemon being run as root without any systemd service hardening). I expect we do not need security review, if we improve the systemd service security a bit. Required TODOs: - The v4l2loopback-dkms dependency needs to be MIRed, too - The daemon is run as root. Its systemd service should use hardening features. See below and http://0pointer.de/blog/projects/security.html to avoid security concerns. - In addition to basic automatic testing, the manual test-plan/script should be improved to show individual steps (for setup & running of the test) and expected outcome (see below) - Consider dropping the Ubuntu delta, or explain why it is needed (see below) - Fix certain lintian hints (see below): + v4l2-relayd: systemd-service-file-missing-hardening-features + v4l2-relayd source: quilt-patch-missing-description + v4l2-relayd: no-manual-page Recommended TODOs: - The package should get a team bug subscriber before being promoted - the testing situation should be improved either during build or at autopkgtest stage (loading of the v4l2loopback-dkms kernel module shouldn't be a blocker inside autopkgtest's qemu environments). - Provide basic upstream documentation (README.md) - The current maintainer/uploader could consider applying for PPU to avoid sponsored maintenance [Duplication] There is no other package in main providing the same functionality. I found uv4l/mjpegstream, which seems to provide similar functionality, but that is not currently packaged in Ubuntu. https://www.linux-projects.org/uv4l/tutorials/turn-mjpeg-stream-into-camera/ [Dependencies] OK: - no -dev/-debug/-doc packages that need exclusion - No dependencies in main that are only superficially tested requiring more tests now. Problems: - depends on v4l2loopback-dkms (in universe) [Embedded sources and static linking] OK: - no embedded source present - no static linking - does not have odd Built-Using entries - not a go package, no extra constraints to consider in that regard Problems: None [Security] OK: - history of CVEs does not look concerning - does not use webkit1,2 - does not use lib*v8 directly - does not parse data formats - does not open a port/socket - 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) Problems: - does run a daemon as root, should be locked down (maybe using User=/Group= stanzas and similar to how it was done here: https://github.com/canonical/ubuntu-advantage-desktop-daemon/pull/8) [Common blockers] OK: - does not FTBFS currently - no new python2 dependency Problems: - does NOT have a test suite that runs at build time - does NOT have a non-trivial test suite that runs as autopkgtest - if special HW does prevent build/autopkgtest is there a test plan, code, log provided? => I'm wondering why there can't be some kind of autopkgtest, loading a kernel module in qemu shouldn't be a problem. We cannot test the real HW camera, but maybe we could simulate an input, capture an output frame and compare that to the input to see if relaying works? - if a non-trivial test on this level does not make sense (the lib alone is only doing rather simple things), is the overall solution (app+libs) extensively covered i.e. via end to end autopkgtest? => => The manual test plan on LP: #1958108 and "check if camera is working via webrtc" seems a bit light to me. Could we please formalize this somewhere in the wiki or so, describing individual steps (also for the preparation of the test environment) and the expected results? [Packaging red flags] OK: - symbols tracking not applicable for this kind of code. - d/watch is present and looks ok (if needed, e.g. non-native) - Upstream update history is good => OK, but very new package - Debian/Ubuntu update history is good => OK, but very new package - the current release is packaged - d/rules is rather clean - It is not on the lto-disabled list Problems: - why carry an Ubuntu delta and distro patches in debian/patches/ if we are the upstream? - The current maintainer/uploader (while not a MOTU) does not have upload rights to main and needs sponsoring, maybe they should apply for PPU? - Some lintian hints to improve upon: X: v4l2-relayd: systemd-service-file-missing-hardening-features lib/systemd/system/v4l2-relayd.service N: N: The specified systemd .service file does not appear to enable any N: hardening options. N: N: systemd has support for many security-oriented features such as isolating N: services from the network, private /tmp directories, as well as control N: over making directories appear read-only or even inaccessible, etc. N: N: Please consider supporting some options, collaborating upstream where N: necessary about any potential changes. N: N: Please refer to the systemd.service(5) manual page and N: http://0pointer.de/blog/projects/security.html for details. I: v4l2-relayd source: quilt-patch-missing-description debian/patches/adjust-defaults.patch N: N: quilt patch files should start with a description of patch. All lines N: before the start of the patch itself are considered part of the N: description. You can edit the description with quilt header -e when the N: patch is at the top of the stack. N: N: As well as a description of the purpose and function of the patch, the N: description should ideally contain author information, a URL for the bug N: report (if any), Debian or upstream bugs fixed by it, upstream status, the N: Debian version and date the patch was first included, and any other N: information that would be useful if someone were investigating the patch N: and underlying problem. Please consider using the DEP 3 format for this N: information. N: N: Please refer to https://dep-team.pages.debian.net/deps/dep3/ for details. W: v4l2-relayd: no-manual-page usr/bin/v4l2-relayd N: N: Each binary in /usr/bin, /usr/sbin, /bin, /sbin or /usr/games should have N: a manual page N: N: Note that though the man program has the capability to check for several N: program names in the NAMES section, each of these programs should have its N: own manual page (a symbolic link to the appropriate manual page is N: sufficient) because other manual page viewers such as xman or tkman don't N: support this. N: N: If the name of the manual page differs from the binary by case, man may be N: able to find it anyway; however, it is still best practice to match the N: exact capitalization of the executable in the manual page. N: N: If the manual pages are provided by another package on which this package N: depends, Lintian may not be able to determine that manual pages are N: available. In this case, after confirming that all binaries do have manual N: pages after this package and its dependencies are installed, please add a N: Lintian override. N: N: Please refer to Debian Policy Manual section 12.1 for details. [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 - no use of setuid - 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 (user visible)? Problems: - Documentation is very light, there should be some manpage and an improved README.md file ** Changed in: v4l2-relayd (Ubuntu Jammy) Status: New => Incomplete -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1958109 Title: [MIR] v4l2-relayd To manage notifications about this bug go to: https://bugs.launchpad.net/oem-priority/+bug/1958109/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
