Comment 2 for bug 1948748

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

Review for Package: swtpm

The package is overall in good shape, already has plenty of users and use-cases
despite being rather new to the archive. Nevertheless some polishing seems
to be needed.

This does need a security review - even more so for any potential guest->host
attack surface, so I'll assign ubuntu-security

To make quick progress I'll already pass it on to security despite that we
are waiting for these mentioned improvements/cleanups.

MIR team ACK, under the condition of fulfillling the outlined required
and at least looking at recommended TODOs.
- List of specific binary packages to be promoted to main: swtpm, swtpm-tools
- Specific binary packages built, but NOT to be promoted to main: none

Required TODOs:
- Please cut the dependency of swtpm-tools to gnutls-bin as discussed
  as it would otherwise imply more MIRs.
- libtpms-dev doesn't exist on ppc64el and thereby IMHO blocks too many
  important use cases from being generally working. Please investigate to
  add that and/or explain why this shall be considered not a problem.
- The autopkgtest suite needs to pass (actually run) on arm64 (stalled
  by long queue)

Recommended TODOs:
- While the lib is internal, .symbols tracking usually is cheap and protects
  even internal libs from some mistakes, consider adding it.
- Version 0.7.0 seems rather close please update it later this cycle
  https://github.com/stefanberger/swtpm/issues/587
- The package should get a team bug subscriber before being promoted
  Right now I do not see it subscribed by "foundations-bugs" yet
- evaluate the possibility and impact of having "tcsd" in the build environment
- Have a look at bug 1949155 if you want to change anything for it

Already completed TODOs:
- Please fixup the user/group creation (see bug 1949060)

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this (This = bin:swtpm)
  - checked with check-mir
  - not listed in seeded-in-ubuntu
  - none of the built reverse-depends are in universe
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems:
- Dependencies of swtpm-tools:
  - swtpm-tools would depend on gnutls-bin which is a binary not in main
    from src:gnutls28 in main.
  - This in turn depends pkg:libopts25 on from autogen (universe)
  After chatting with Steve, we want to keep swtpm-tools to be promoted (as
  it is needed by e.g. libvirt), but cut the dependency of swtpm-tools
  to gnutls-bin.

[Embedded sources and static linking]
- 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

[Security]
OK:
- 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)

Problems:
- does not run a daemon as root, but is often run with too high privilegded
  (user tss), which is why we have spawned bug 1948880 and bug 1949060
- does parse data formats (partically VM guest controlled)
- does open a port/socket (in the common use-case)
- does deal with security attestation
- history of CVEs does not look concerning, but the use case make it a target

[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
  (same as the build time test)
  => https://autopkgtest.ubuntu.com/packages/swtpm
- no new python2 dependency
- Python package, but using dh_python
- Go package, but using dh-golang

Problems:
- Does not build on ppc64el, since this is meant to be used for encrypted disk
  and VM tpm usage I think it might need to work on all architectures.
  => https://launchpad.net/ubuntu/+source/swtpm/0.6.1-0ubuntu2/+build/22337599
  libtpms-dev isn't available there, I guess we need to have a look
- The package is present on arm64
    swtpm | 0.6.1-0ubuntu2 | jammy/universe | source, amd64, arm64, armhf, riscv64, s390x
  And the test suite only has:
    Restrictions: allow-stderr
    Depends: @, @builddeps@
  Nevertheless, it is not run on arm64 yet (super long test queue).
  Before promotion we should see this working.

[Packaging red flags]
OK:
- Ubuntu does not carry a delta (not yet in Debian)
- d/watch is present and looks ok
- Upstream update history is good
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- d/rules is rather clean
- It is not on the lto-disabled list

Problems:
- The current release is packaged, but we are just a bit away from 0.7.0
  which would be nice to get before Ubutnu 22.04 completes
- Debian/Ubuntu update history is undefined (rather new)
- symbols tracking is not in place, I know it is only an internal lib
  But the increased effort usually is minimal and it helps to catch e.g.
  unwanted effects of backports.
- Some Lintian warnings around postinst issues, I've brought them up as part
  of bug 1949060 already

[Upstream red flags]
OK:
- 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)?

Problems:
- use of user "tss" which comes with device permissions we do not
  want/need. I know you are aware, but since I found a few related issues I
  spawned bug 1949060 for it.
- Errors/warnings during the build show
  "configure: WARNING: tcsd could not be found; typically need it for tss
  user account and tests"
  I wonder if evaluating that could give us more testing or anything else?