[MIR] google-guest-agent

[Balint Reczey]

[Availability]
Google-guest-agent is in universe and only depends on packages provided in main or by the source package itself. The package is new in Groovy, but it replaces part of old gce-compute-image-packages. The package builds for all architectures.

[Rationale]

This package is included on the GCE images and the Ubuntu Foundations team has been supporting it as such. We'd like to get it included in main as that's the right thing to do.

[Security]
This is a new package, and as such has no security history to speak of. Since it will be installed on every Ubuntu system in GCE and performs system configuration and network communication as well a security review is warranted thus I'm subscribing the Security Team

[Quality assurance]
There are currently 0 open bug reports (excluding this one) about the package and the Ubuntu Foundations team (foundations-bugs) is subscribed to bugs about the package.

The package build runs the build-time testsuite.

Packaging is minimal. There is an ongoing discussion about configuration file handling started in https://bugs.launchpad.net/ubuntu/+bug/1870314/comments/7 .

[Dependencies]
All binary dependencies are from main or come from the source package itself.
Per the Golang policy exception Go build dependencies must also be in main.

Golang build dependency chain with MIR bugs:
google-guest-agent:
 golang-github-go-ini-ini LP: #1894731
 golang-github-golang-groupcache LP: #1894731
 golang-github-kardianos-service LP: #1894731
  golang-github-kardianos-osext LP: #1894731
 golang-github-tarm-serial LP: #1894731
 golang-github-gcp-guest-logging-go LP: #1894731
  golang-google-genproto
 golang-google-cloud
  golang-github-golang-mock (test only?)
  golang-github-google-btree LP: #1894731
  golang-github-google-martian
  golang-github-google-pprof
   golang-github-chzyer-readline
   golang-github-ianlancetaylor-demangle
  golang-github-googleapis-gax-go
  golang-go.opencensus
   golang-github-hashicorp-golang
  golang-golang-x-net LP: #1894731
   golang-google-cloud-compute-metadata
  golang-golang-x-time LP: #1894731
  golang-google-api
   golang-golang-x-oauth2 LP: #1894731
  golang-google-genproto
  golang-rsc-binaryregexp
 golang-google-grpc
  golang-golang-x-sys LP: #1894731
 golang-goprotobuf (main)
  golang-google-protobuf LP: #1894731

[Standards compliance]
Conforms to Debian Policy 4.5.0

[Maintenance]
The Ubuntu Foundations Team will continue to maintain the package as they have been doing.

[Background information]
The split of the old gce-compute-image-packages is described in https://bugs.launchpad.net/ubuntu/+bug/1870314/comments/2

[Didier Roche-Tolomelli (didrocks)]

[Summary]
- All dependencies should be filed as MIR (and their dependencies).
- Would be nice to have the services more confined via systemd.
- Once done and dependencies are ACKed, this is fine by me, but will need a security review.

[Duplication]
OK:
Nothing to add over the top request as it’s a code split.

[Dependencies]
TO BE FIXED:
Most of build-deps are in universe:
 * golang-github-go-ini-ini-dev binary and source package is in universe
 * golang-github-golang-groupcache-dev binary and source package is in universe
 * golang-github-kardianos-service-dev binary and source package is in universe
 * golang-github-tarm-serial-dev binary and source package is in universe
 * golang-github-gcp-guest-logging-go-dev does not exist (pure virtual?)
 * golang-google-cloud-dev binary and source package is in universe
 * golang-google-grpc-dev binary and source package is in universe
 * golang-goprotobuf-dev binary and source package is in universe
Contrary to other languages, those needs to be in main even if they are build dependencies. Indeed, the static linking nature of Go will make the code embedeed and executed, and so, the main package rules apply for them.
I only check one level deep, you should attach to this MIR any dependencies of those dependencies ofc.

[Embedded sources and static linking]
OK:
- no embedded source present
- staticly link Go packages as the nature of the coede.

[Security]
OK:
- no CVEs, but really fresh new package.
- does not use webkit2,2
- does not use lib*v9 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 open a port directly (but will communicate through opened port via sane and zeroconf subcription)

Problems:
- multiple services running as root: can they be confined via systemd directives? Needs a security review
- comunicate with external services: Needs a security review

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time (good size)
  - test suite fails will fail the build upon error.
- no translation on CLI tool (but this is only a debugging discover command, common to not have them here). Messages returned to Sane are translated though.
- not a python package, no extra constraints to consider in that regard
- use of dh_golang
- Team subscription is OK

TO FIX:
- debian/copyright is wrong: copyright holder should be Copyright: 2017-2020 Google Inc from source headers

[Packaging red flags]
OK:
- Ubuntu only package
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok
- Upstream update history is good
- Ubuntu update history is good, but short
- Upstream is active, but without release. Note that d/watch is pointing correctly to fetch a version. However, the project is seeing fewer changes for the past months (only small fixes), so the lack of release isn’t much of an issue for stable code.
- promoting this does not seem to cause issues for MOTUs that so far maintained the package
- no lintian issue
- d/rules is clean and minimal
- d/control standard f...

[Balint Reczey (rbalint)]

Thank you for the review! I've fixed the copyright in 20200617.00-0ubuntu3. I'm continuing with the MIRs of the dependencies, I indeed missed this golang-specific requirement.

[Balint Reczey (rbalint)]

@didrocks Could the Security Team please start the security review while I'm preparing MIRs for the build dependencies?

[Balint Reczey (rbalint)]

I used debtree to collect all transitive dependencies:

debtree -b --no-alternatives --no-version google-guest-agent

[Alex Murray (alexmurray)]

I reviewed google-guest-agent 20200617.00-0ubuntu3 as checked into groovy.
This shouldn't be considered a full audit but rather a quick gauge of
maintainability.

google-guest-agent provides the google_guest_agent and
google_metadata_script_runner binaries and associated systemd services
etc. google_guest_agent runs on each boot and watches the internal google
cloud metadata API for changes and updates the machines sshd, users,
groups, sudo and PAM configuration to allow the users listed via the
metadata API to login etc. google_metadata_script_runner is design to run
on each boot and run arbitrary scripts as specified by the metadata API.

- No CVE History:
- There are only autogenerated pre/post inst/rm scripts
- No init scripts
- systemd units
  - Uses separate one-shot units to start and stop the metadata script
    runner
  - Single simple service unit for google_guest_agent
- No dbus services
- No setuid binaries
- binaries in PATH
  - rwxr-xr-x root/root 11419336 2020-08-26 03:11 ./usr/bin/google_guest_agent
  - rwxr-xr-x root/root 12321672 2020-08-26 03:11 ./usr/bin/google_metadata_script_runner
- No sudo fragments
- No polkit files
- No udev rules
- No autopkgtests
- Unit tests run during build
- No cron jobs
- Build logs:
  - Lintian warnings:
    - missing man pages for the two binaries
    - neither binary is compiled as PIE

- Processes spawned
  - Processes are spawned frequently for adding / removing users, modifying
    the routing table, adding interface addresses and other functions -
    these take input from the configuration files and so if a user could
    modify the configuration file they could get root command execution
- Memory management
  - This is golang so memory management is careful
- File IO
  - Uses hard-coded paths for configuration files and parsing /etc/passwd
    or sudoers files etc
- Logging is careful
- No environment variable usage
- No use of privileged functions
- No use of cryptography / random number sources etc
- Use of temp files
  - google_metadata_script_runner uses a temporary directory to download
    script files into and run them via the ioutil.TempDir() standard
    library function which is safe.
- Use of networking
  - Parses metadata API as json
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck results
- No significant Coverity results
  - False positive URL_MANIPULATION on use of etag from the metadata API possibly tainting
    future calls to the metadata API (this is the whole point of etag) plus
    we trust the metadata API anyway
  - Irrelevant RISKY_CRYPTO warning about windows account handling code's
    use of SHA1
- No significant shellcheck results
- No significant bandit results

The nature of this package (a root daemon that is used to manage remote
login capabilities and run random scripts etc) means it is a sensitive and
privileged component - however, since it only seems to interact with the
trusted metadata service (and nothing else) via the .internal URL this
seems safe. In general the code looks reasonably defensive, however the use
of many vendored golang dependencies may make tracking of possible relevant
vulnerabilities a bit trickier.

Securi...

[Christian Ehrhardt  (paelzer)]

All reviews are in, this still wait on:
- feedback or best an implementation of the MIR review question "Would be nice to have the services more confined via systemd"
- all dependencies to be resolved in bug 1894731

For now it is incomplete at least missing feedback, so I'm re-assigning to rbalint.

Once resolved we should make this a dup and copy the reviews into 1894731 because only there we have the full overview of the massive dependency chain.

[Balint Reczey (rbalint)]

Thank you for the review.

I've updated the package to ship the Go build dependencies by vendorizing them in 20200617.00-0ubuntu4. As far as I know the Security Team approved that, but please take a new look at the package.

Google-shutdown-scripts.service and google-startup-scripts.service run user provided arbitrary scripts thus they can't be confined.

Google-guest-agent.service adds login entries, rewrites sshd config and restarts ssh server, thus those operations can't be prevented. It interfaces only with the cloud's metadata service, thus the attack surface is fairly narrow and the service is written is Go which is considered safe. I think successful attacks against it are unlikely, but I can ask upstream to consider securing this service.

[Christian Ehrhardt  (paelzer)]

Summary:
- bug 1896246 essentially is a switch to fully vendorize and done
  - there in https://bugs.launchpad.net/ubuntu/+source/google-osconfig-agent/+bug/1896246/comments/2 security signs off on that approach
- thereby bug 1894731 is essentially dead and I updated it to reflect that

That makes the remaining TODOs a MIR and a security re-evaluate of this (now vendorized) package.

@Security:
for your re-review:
1. much more code to look at now compared to your forer check
2. you might also reconsider details like the aforementioned:
"Google-shutdown-scripts.service and google-startup-scripts.service run user provided arbitrary scripts thus they can't be confined."
As IMHO that makes it a prime target to inject attacks if exploitable in any way.
I'd love to see you sign off on that being ok to be sure.

[Christian Ehrhardt  (paelzer)]

Compared to the old review we had these open topics:
1. confine services - you evaluated it (thanks a lot) and outlined why it can't be done
   due to conceptual issues. Ok if the security Team is.
2. Dependencies - well, now all vendored as shown in bug 1896246 and ok as exception
   Thanks for dropping the build-deps, that should ensure it really contains what we assume.
3. copyright - is fixed

New topics:
4. Tests: in several of the - formerly separate - packages we challenged the "lack of" or "disabling" of tests. The thought was that at least overarching as the eventual use case that testing has to happen. Now that all are built form one source we have that spot.

4a. At buld I only see it running "go test -vet=off -v -p 4 github.com/GoogleCloudPlatform/guest-agent/google_guest_agent github.com/GoogleCloudPlatform/guest-agent/google_guest_agent/snapshot_service github.com/GoogleCloudPlatform/guest-agent/google_metadata_script_runner".
So by vendorizing all libs we essentially have further reduced an already bad test coverage.
Is there any way we could have it run the sub-tests of vendored code AND/OR do some (simulated since we lack the real data service) end-to-end test?

4b. Does upstream have any sort of test suite to test the final combined thing?
    If so could we integrate that into an autopkgtest?

So almost as expected the weak testing should be improved. That is but is a strong, but no blocking TODO for you. It would be awesome if you could spend some time for that while waiting for the security re-review!

I didn't find other issues, but Security - as mentioned - needs to re-review due to the added extra code and a re-visit of the unconfined externally-controlled services situation.

P.S. Looking back at recent Go promotions we might need (independent to this bug) a re-review of our process as it seems to me all go an exception route eventually, so it seems a dysfunctional process to me. /me missed an engineering sprint ...

[Dimitri John Ledkov (xnox)]

Alex Murray (alexmurray) can you please rereview given the change of vendored packages?

[Alex Murray (alexmurray)]

Regarding the vendored packages, I have updated our CVE tracker to try and capture as many of these as possible in https://git.launchpad.net/ubuntu-cve-tracker/commit/?id=e31511491f1d3258c609e449ecab26765ecf0f9f - this should allow the security team to automatically have CVEs that are in one of those vendored components be marked against google-guest-agent as well. This was based on the list in debian/extra/vendor/modules.txt so assuming that is up-to-date, consider this an ACK for that addition.

Regarding the unconfined externally-controlled services, this feels like a primary function of this package from what I can see, so whilst this is clearly a prime target to attack for remote-code-execution etc, from a cursory look, I can't see any obvious vulnerabilities in the current implementation so I don't think it makes sense to NAK this MIR based on that. I would definitely prefer to see these confined via an AppArmor profile or similar if possible however I understand that this may not be achievable.

As such, Security Team ACK (again) for promoting google-guest-agent to main.

[Balint Reczey (rbalint)]

Since the ACK from the Security Team arrived I'm moving the state to Fix Committed per https://wiki.ubuntu.com/MainInclusionProcess .
@alexmurray thanks for the review!

@paelzer Enabling vendored tests is a tricky question in multiple ways. The test dependencies may not be present (even in the archive), but is is generally solvable.

The other issue is more interesting. The dependencies are vendored partly because the whole Golang stack (at versions required by the agent) does not meet the quality requirements imposed on packages in the Ubuntu Archive and it is always possible that tests exercising the part of the vendored stack _not_ used by google-guest-agent are failing.
Fixing those test or modules would diverge from the the stack's state required and presumably tested by upstream thus would add more risk breaking something.

Upstream for this packages is the partner for whom the package is maintained and who also leads the Golang ecosystem in a way that is not that pleasant to work with on distributions' side. The root cause of the problems I faced while trying to update the related part of the Golang stack to a newer consistent state is the lack of release coordination among modules which is a Golang ecosystem level problem.

Testing the vendored dependencies does not seem to worth the extra complexity in the packaging but I'd encourage upstream to keep the whole stack in a state that allows proper packaging so the Ubuntu package can switch back to using the packaged modules which are rigorously tested to ensure that they work well with all related packages. (The modules missing in older releases would have to be still vendored for SRUs.)

[Didier Roche-Tolomelli (didrocks)]

$ ./change-override -c main -S google-guest-agent
Override component to main
google-guest-agent 20200617.00-0ubuntu5 in groovy: universe/devel -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy amd64: universe/devel/optional/100% -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy arm64: universe/devel/optional/100% -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy armhf: universe/devel/optional/100% -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy ppc64el: universe/devel/optional/100% -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy riscv64: universe/devel/optional/100% -> main
google-guest-agent 20200617.00-0ubuntu5 in groovy s390x: universe/devel/optional/100% -> main
Override [y|N]? y
7 publications overridden.