[MIR] ledmon

[Woodrow Shen]

== 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

[Christian Ehrhardt  (paelzer)]

Hi Woodrow,
AFAIK the sections above are non optional.
Please use [1] to fill out all of them.

Especially Package ownership is a gating thing, someone responsible has to own it.
Since you wrote TBD I assume you know that and will update it later on.

It seems this originally was [3] but intel has taken over in [2] and was active in 2017.
There was no packaging activity until recently , are there actions to ensure it does not fall unmaintained again?

Please address all of the above (full template and subscriber at least) and then set it back to new for a re-review.

[1]: https://wiki.ubuntu.com/UbuntuMainInclusionRequirements
[2]: https://github.com/intel/ledmon/tags
[3]: https://github.com/dell/ledmon/commits/debian

[Christian Ehrhardt  (paelzer)]

Thanks Woodrow,
If Foundations will subscribe to that they need to do so on https://launchpad.net/ubuntu/+source/ledmon

I'm subscribing xnox (last upload) and Steve as Team Lead to consider doing so.

[Woodrow Shen (woodrow-shen)]

Hi Christian,

As we're discussing with foundation team about ledmon, the reason behind this MIR is OEM base image would include ledmon. However the foundation team suggests we may need to file a MIR request and they will make a decision for figuring out the resource of maintenance.

For me, I'm willing to apply MOTU now and to see if ledmon still needs to be maintained in universe or not.

[Christian Ehrhardt  (paelzer)]

Package quality, dependencies and all else LGTM and review is rather easy as it is a relative small codebase ~9kloc.
I ran a few code checkers and they only found minor things not worth to bother.
Also the packaging seems fine since the major update by xnox.

Expecting the subscription by the foundations Team to happen there isn't much more that would stop the package from the MIR Teams POV.
Under the constraint that this is completed before this is over we can already kick off the next step which is the security review.

Given this has no ports, sockets or anything like it open the attack surface is rather small.
But it is the security Team that has to check that -> assigning to security.

[Christian Ehrhardt  (paelzer)]

@Woodrow: please realize that the Debian Maintainer Daniel Jared Dominguez is @Dell.com and Dell stopped at Version 0.79 - Intel pushed it forward to 0.90 of today, but I'd assume Dell has no business to pushing this further anymore. You had xnox do the update for you, but this might have to repeat to stay updated.

[Woodrow Shen (woodrow-shen)]

Hi Christian,

Thanks for sharing more information for above, and seems like we'll just follow Intel's changes.

[Steve Langasek (vorlon)]

> 1. OEM projects needs to include ledmon for VROC support

It remains unclear to me why this is the case. The ledmon package ships a daemon but provides no integration to cause it to start by default. It also ships a 'ledctl' command to allow manually changing the status LEDs on a disk enclosure, but this doesn't seem to have been missed in main before now.

ledmon itself clearly needs updates in order to work with a new class of hardware interface (VROC). But I don't understand there to be anything novel in the nature of VROC that *requires* ledmon, where previous hardware interfaces did not; and in any case, the package as it currently exists fails to meaningfully integrate with the rest of Ubuntu.

[Steve Langasek (vorlon)]

Subscribing Pat, who would ultimately decide on Foundations maintenance.

[Jamie Strandboge (jdstrand)]

@Christian, actually, the security team doesn't typically decide if it needs a security review; that is part of the ubuntu-mir team's review. We have criteria for when something requires it: https://wiki.ubuntu.com/MIRTeam#Security. If it doesn't meet those (or similar) criteria, then it doesn't need us. Of course, you can always ask us to help decide, but this is a case by case basis rather than as a matter of course.

There are no CVEs, no open ports, you need to be admin to run the software and it doesn't process untrusted input as root. Unassigning security.

[Patricia Gaughen (gaughen)]

Woodrow - Would you please provide clarity regarding Steve's comments (see #7). I would like clarity before signing up the foundations team for maintenance.

[Woodrow Shen (woodrow-shen)]

Hi Patricia,

I agree with Steve mentioned that ledmon should *not* be critical requirement for VROC but more likely accessory to me, especially ledmon package is not covering the systemd unit to trigger the service when it's installed. What OEM Base image project proposed to include ledmon was based on our PRD (Bug #1759225), so I'm thinking if Stephane can explain more details precisely.

Stephane,

Can you help me to give more information to clarify this situation?

[Launchpad Janitor (janitor)]

[Expired for ledmon (Ubuntu) because there has been no activity for 60 days.]

[Yuan-Chen Cheng (ycheng-twn)]

per check ledmon in focal, it does run as a daemon.

I got access to a machine that can use ledctl from ledmon to control the led status of the nvme module enclosure.

AI: try to build vroc raid on it, and check if the raid status will cause the led pattern change in certain statuses of the raid (ex: rebuild or failure) with ledmon running in the background.

ref: https://www.intel.com/content/dam/support/us/en/documents/memory-and-storage/ssd-software/Linux_VROC_6-0_User_Guide.pdf

[Christian Ehrhardt  (paelzer)]

# Rechecking #

Resolved:
- The old issue of being slightly outdated is resolved (we have regular updates recently).
- the package indeed now has services to start properly on install

Left open:
- I'm unsure about the reasoning. The initial one in #1759225 counts as resolved since 2018-11-02 via bug 1789786. That updated, but did not promote ledmon to main. Since there everyone agreed to set it Fix-Released is this really needed to be promoted still? If the Author or Yuan-Chen as recent updater could please explain.
- @Matthieu Clemenceau I subscribed and assigned you as the old "would foundations own it" question nowadays goes to you.

Next steps:
If a case is being made that really needs this fully supported in main AND foundations want to own it I'd then re-review todays code again and (if needed) pass it to security afterwards.

[Yuan-Chen Cheng (ycheng-twn)]

per test today, I set up the vroc nvme raid on a dell machine.

On that, I can use a command like

ledctl locate=/dev/nvme0n1

to make the led on the encluse module blink.

And use command

ledctl locate_off=/dev/nvme0n1

to make the led stop blinking.

As I config two nvme a mirror raid as the boot device, I run below command

mdadm /dev/md126 --fail /dev/nvme0n1

If the ledmon daemon is installed and running, then with the above command, the led on the enclosure will start to blink.

If ledmon is not installed and running, the above mdadm command won't make the led blink.

Given so, I think ledmon is useful for these use cases. I think we should consider including it in main.

[Yuan-Chen Cheng (ycheng-twn)]

I think I've done my part to test it and provide the use case. Give so, mark fix committed in oem-priority.

[Yuan-Chen Cheng (ycheng-twn)]

note #15 is test on focal.

[Łukasz Zemczak (sil2100)]

Ok, so I jumped in to see if there's anything I can do to help getting this moving. Per Christian's question about the rationale of this MIR and if it still holds, I asked Michael if subiquity has any plans of using ledmon by default for VROC / RST support and he said that there are no plans right now. There was no request of including this package into server-ship-live or anything. So at least from subiquity POV this is out-of-scope right now.

Looking at #15, I am wondering: what are the plans from the OEM perspective for ledmon after it gets included in main? Will it be installed by default, will it be part of some media? Since just it being useful seems like not a great rationale as-is, since users can install ledmon from universe if they want to. I think having it in main, in this situation, only makes sense if we want it to be available to users by default.

@Yuan-Chen can you elaborate a bit more on how will OEM use ledmon once it's in main? What is the story? Thanks!

[Rex Tsai (chihchun)]

The ledmon have the different partners of blinking helps the user to understand the status of the disk and raid, it's critical feature for user experience.

I would suggest to include ledmon to offer better support. ledmon is been preloaded on workstations of major OEMs in the factory image, and it will be a key feature for enterprise users who may purchase a support service.

[Yuan-Chen Cheng (ycheng-twn)]

@Łukasz I think for OEM images for HW that have the corresponding component, we should include those by default.

Given that, I think we should include it in main.

[Łukasz Zemczak (sil2100)]

Ok, after discussing this with multiple people, we have decided to go ahead. The Foundations team is willing to take ownership of this package in main.

Dear MIR team: can we continue the MIR process?

[Christian Ehrhardt  (paelzer)]

Thanks Łukasz for clarifying that!

Comments:
- #4 was almost an MIR Ack
- #9 confirmed that a security review should not be needed.

All this happened long time in the past, I'm rechecking the state as of today but most likely we will approve this rather soon.

[Christian Ehrhardt  (paelzer)]

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 no...

[Łukasz Zemczak (sil2100)]

Thank you for the review Christian! Due to lack of possible testing hardware, no critical use cases and lack of resources, we have decided to postpone this MIR until needed.

[Christian Ehrhardt  (paelzer)]

There has been not further update for too long, for now we consider it invalid.
Feel free to re-open if there is effort backing it up and motivation to bring it to main.