[Bug 1794219] Re: [MIR] ledmon
Christian Ehrhardt
1794219 at bugs.launchpad.net
Tue Dec 14 14:39:23 UTC 2021
Review for Package: ledmon
[Summary]
Since the last time we looked at that plenty of rules got improved
and a re-review was likely to discover things we would have missed in the
past.
Indeed I found some things that are required and some that are recommended
to be resolved before a promotion to 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 not need a security review (see comment #9)
List of specific binary packages to be promoted to main: ledmon
Specific binary packages built, but NOT to be promoted to main: <none>
Since there are required TODOs I'll assign it back to Foundations who want
to own it and mark it incomplete. Please ping once the open TODOs are resolved
for a final Ack to get it promoted.
Required TODOs:
- The service should use some security features like private spaces,
not running as root and so on - please evaluate if we can use this
to lower the risk of any potential exploitation.
- Please add automated tests (likely impossible) or as fallback outline
the test steps that you will execute and the HW this will run on (details
below)
- fix bad dependencies and ledctl failing to start due to bad lib references
(details below)
Recommended TODOs:
- Please try to have the service not start (or exit quickly, but sucessfully)
on the 99.9% of systems it won't be needed.
- Please have a look if this makes no sense to run in a container ever
and if so add a systemd condition to not start there
[Duplication]
There is no other package in main providing the same functionality.
[Dependencies]
OK:
- no other Dependencies to MIR due to this
- 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]
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 not run a daemon as root
=> If this really is only monitoring and controlling leds, could it
run in a much more restricted way please - private tmp/home, less user
permissions. If it needs just one special device probably get that owned
by a group and run the service as that. It seems having it to be root
isn't appropriate for the use case.
[Common blockers]
RULE: - There are plenty of testing requirements, hopefully already resolved
RULE: by the reporter upfront, see "Quality assurance - testing" section of
RULE: the Main Inclusion requirements
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
- special HW does prevent build/autopkgtest.
=> In this case we have too often had packages too rarely tested.
Nowadays teams owngin the package (in this case foundations and or the
requesting OEM team) are asked to describe their regular test plan.
If automated please link to the test code.
Even if it can only be run manually please provide a test log.
I understand that comment #15 can be seen as kind of a test log.
But in that case please ensure you have the HW and people to re-run that test
at least on uploads and feature freeze of a cycle.
[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- 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 rather slow, but ok in this case
- Debian/Ubuntu update history is slow (matching upstream)
- 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
- d/rules is rather clean
- It is not on the lto-disabled list
Problems:
- Service issues I: this comes with a service that is enabled by default
(ledmon.service) We have taken so many efforts to keep services disabled that
are not needed (memory & cpu footprint) that in this case which only applies
to a very few special cases the service should get a way to not start up
(or early exit) if no supported HW is present.
- Service issues II:
In a container it comes up crashes quickly a few times and then
gives up failing with bad state. If it makes no sense to run in a container
then a condition to not start there would be very helpful. Less resources
wasted trying to start it and no bad status service being around after boot.
If there also is no sense to run it in non-container virtualization then also
block this case.
- Currently the delivered control program ledctl does not work in jammy.
Right after install if started it complains about missing libraries.
This indicates a lack of proper dependencies.
root at j-proposed:~# ledctl
ledctl: error while loading shared libraries: libsgutils2-1.45.so.2:
cannot open shared object file: No such file or directory
Seems there was an soname bump and this wasn't updated correctly.
ii libsgutils2-2:amd64 1.46-1 amd64 utilities for devices using the SCSI command set (shared libraries)
root at j-proposed:~# dpkg -L libsgutils2-2
/usr/lib/x86_64-linux-gnu/libsgutils2-1.46.so.2.0.0
[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
- use of setuid, but ok because <TBD> (prefer systemd to set those
for services)
- 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)?
** Changed in: ledmon (Ubuntu)
Assignee: Christian Ehrhardt (paelzer) => Ubuntu Foundations Bugs (foundations-bugs)
** Changed in: ledmon (Ubuntu)
Status: Confirmed => Incomplete
--
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is a bug assignee.
https://bugs.launchpad.net/bugs/1794219
Title:
[MIR] ledmon
Status in OEM Priority Project:
Fix Committed
Status in ledmon package in Ubuntu:
Incomplete
Bug description:
== Requirements ==
[Availability]
Currently in universe.
Package in LP: https://launchpad.net/ubuntu/+source/ledmon
Upstream: https://github.com/intel/ledmon
[Rationale]
1.OEM projects needs to include ledmon for VROC suport (LP: #1759225)
2.Intel still maintains upstream for that (LP: #1668126)
3.Dependencies already in main.
[Security]
No security issues exposed so far. We may need to rely on Intel to be aware of upstream commits for security fixes.
[Quality Assurance]
1.No debconf questions
2.No outstanding bugs
3.I can help to make sure the consistency for status of important bugs in Debian's/Ubuntu's, and upstream's bug (on github).
4.Ledmon only supports Intel related storage controller (e.g. AHCI/iSCSI/VMD controller)
5.No test suite shipped with ledmon
6.No dependencies with obsolete or demoted packages
[UI standards]
1.This is a CLI tool/daemon service. It has normal CLI style short help and man pages. (man ledmon/ledctl)
2.No desktop file required as it is a backend tool.
[Dependencies]
build-depends: perl (main), libsgutils2-dev (main), libudev-dev (main)
binary-depends: openipmi (main)
[Standards Compliance]
The package should meet the FHS and Debian Policy standards.
[Maintenance]
Package owning team: The Foundations team
Debian package maintained by Daniel Jared Dominguez (but seems he didn't maintain the latest one: currently the version 0.90 on upstream and it's 0.79-2 on debian)
https://tracker.debian.org/pkg/ledmon
[Background Information]
ledmon and ledctl are userspace tools designed to control storage enclosure LEDs. The user must have root privileges to use these tools.
These tools use the SGPIO and SES-2 protocols to monitor and control
LEDs. They been verified to work with Intel(R) storage controllers
(i.e. the Intel(R) AHCI controller) and have not been tested with
storage controllers of other vendors (especially SAS/SCSI
controllers).
For backplane enclosures attached to ISCI controllers, support is
limited to Intel(R) Intelligent Backplanes.
== Security checks ==
1.http://cve.mitre.org/cve/search_cve_list.html: Search in the National Vulnerability Database using the package as a keyword
* There are 0 CVE entries that match your search.
2.Check OSS security mailing list (feed 'site:www.openwall.com/lists/oss-security <pkgname>' into search engine)
* No security issue found
3.Ubuntu CVE Tracker
http://people.ubuntu.com/~ubuntu-security/cve/main.htm
* No
http://people.ubuntu.com/~ubuntu-security/cve/universe.html
* No
http://people.ubuntu.com/~ubuntu-security/cve/partner.html
* No
4.Check for security relevant binaries. If any are present, this requires a more in-depth security review.
* Executables which have the suid or sgid bit set.
No
* Executables in /sbin, /usr/sbin.
Yes
* Packages which install services / daemons (/etc/init.d/*, /etc/init/*, /lib/systemd/system/*)
No
* Packages which open privileged ports (ports < 1024).
No
* Add-ons and plugins to security-sensitive software (filters, scanners, UI skins, etc)
No
To manage notifications about this bug go to:
https://bugs.launchpad.net/oem-priority/+bug/1794219/+subscriptions
More information about the foundations-bugs
mailing list