Comment 2 for bug 2071717

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Review for Source Package: linuxp2p

[Summary]
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: linuxptp
Specific binary packages built, but NOT to be promoted to main: -

Required TODOs:
- #1 This is exposed externally and runs with high permission as well as
     influence due to the importance of time to certificate checks. I really
     would want to see better use of isolation features before we encourage
     use more widely. In particular please look at:
     - surely: use a lof of the systemd isolation features
     - surely: create and test apparmor profiles for the binaries that run
               as service
     - maybe: running less privileged
- #6 Please merge the latest version (4.2 at the moment)

Recommended TODOs:
- #2 I'm happy to see that you have added clknetsim based autopkgtests already.
     But since this has no build time tests at all and also upstream testing
     seems to be manual according to https://linuxptp.sourceforge.net/ we should
     try to do a bit more. The content in debian/tests/linuxptp-testsuite is the
     same as clknetsim - so I guess that can be removed.
     I'm not sure what can be done without upstreams support on e.g. unit
     tests. But I'd ask to discuss it with them and refer the discussion here.
     If there is something that could be done, do it. Otherwise at least we
     know why.
- #3 You mentioned that checkbox, but that includes setup steps with physical
     access to the device. Could you outline how one could trigger these tests
     (e.g. after a bigger change or merge) themselve. Or if it really needs
     a person in front of the device who would be needed to get contacted to
     run such a test against a new build e.g. in a PPA?
- #4 If we'd onboard that today (not a MIR rule, but a server team rule since
     we shall own it later) it would also need some reasonable documentation
     entries like https://ubuntu.com/server/docs/how-to-serve-the-network-time-protocol-with-chrony
     Along your testing e.g. to get isolation right you will have gained the
     experience how to set this up well which will enable you to write that.
- #5 submit the Delta (your testing and the former config path change) to Debian
     Nothing we have is super-special, mostly added QA which helps us as well
     if spotted earlier or even in Debian already.
- #7 There are some bugs open that can get very painful later but need some
     time to dive deeper to get fixed. In particular into how it can manage
     service dependencies to its configured network devices better.
     Some are easy, other might need complex things, maybe even netlink
     enablement in the code? But it does not have to be that complex, the
     services should wait on %I network device and php2sys waits on a
     synchronized ptp4l. Yet the bug exists so give these an evaluation so
     we only skip them once we have a good reason why:
      - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1004744
      - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1070847

[Rationale, Duplication and Ownership]
There is no other package in main providing the same functionality.
It is easily mistaken for doing the same as systemd-timesyncd and chrony, but
it is different. For a large or partially uncontrollable network NTP based
implementations will be better. Yet on small, controlled networks where
it can be ensured that all HW on the path supported PTP it is better to use
PTP.
There is a fusion of the two using PTP based HW features as PHC clock in chrony
but that isn't the same either.

A team is committed to own long term maintenance of this package.
In this particular case the current approach is (we will let you know
if that changes) that PE/Industrial will do the enablement work to get it
ready for main and the Server team will do the long term maintenance and
monitoring comittment.

The rationale given in the report seems valid and useful for certain sets
of Ubuntu users.

[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 unexpected Built-Using entries
- not a go package, no extra constraints to consider in that regard
- not a rust 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 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, ...)

Problems:
- does run three daemons as root.
- does expose an external endpoint via ports.
- does parse data formats of the PTP messages which could be externally
  influenced.
- this does not make appropriate (for its exposure) use of established risk
  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 non-trivial test suite that runs as autopkgtest
- This needs special HW for build or test so it can't be
  automatic at build or autopkgtest time. But as outlined
  by the requester in [Quality assurance - testing] there:
  - HW and test: via checkbox tests
  - simulation in autopkgtest via clknetsim
- no new python2 dependency

Problems:
- Does not have a test suite that runs at build time, but makes up for
  it in autopkgtest to some extent.

[Packaging red flags]
OK:
- Ubuntu does carry a delta, but it is reasonable and maintenance under
  control. It needs to be submitted though to not divereg too much making
  maintenance harder than it has to be as nothing we have is super-special.
  Mostly added QA which helps us as well if spotted earlier.
- symbols tracking not applicable for this kind of code.
- debian/watch is present and looks ok
- Upstream update history is good
- Debian/Ubuntu update history is ok
- 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:
- the current release not is packaged, 4.2 needs to be merged

[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 / setgid
- no dependency on webkit, qtwebkit or libseed
- not part of the UI for extra checks
- no translation present, but none needed for this case (user visible)?

Problems:
- important open bugs (crashers, etc) in Debian or Ubuntu. In particular
  before we encourage wider adoption we should address
  - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1004744
    (partially fixed in Ubuntu with one of our deltas, the service
     dependency needs investigation)
  - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1070847
    Such cases are a constant source of pain, it should be tried to resolved
    before it is a critical issue for someone