New issue
Advanced search Search tips

Issue 884520 link

Starred by 0 users

Issue metadata

Status: Unconfirmed
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

clobber-state: convert to C++

Project Member Reported by vapier@google.com, Sep 16

Issue description

this shell script is ~600 lines and is pretty critical to the security/recovery of the system.  it should be in a real language w/unittests.
 
Cc: teravest@chromium.org
Cc: fletch...@chromium.org
Mike, I saw you posted a couple of diffs on this last month (f.ex. https://crrev.com/c/1227325), are you actively working on this? I am considering taking it on.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/dca66e5c4288370f14ed1e1b3ee8d2350bb8490e

commit dca66e5c4288370f14ed1e1b3ee8d2350bb8490e
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Thu Nov 08 04:27:05 2018

init: Add integration test for clobber-state

Adds a test for clobber state which creates and mounts a fake
filesystem, populates it with a couple of files, runs clobber-state
on it, and then checks that only the correct files are saved.

BUG=chromium:884520
TEST=quid probat ipso probatorem (who tests the testers?)
CQ-DEPEND=CL:1319388

Change-Id: Ia91d11d9eaab98c775f32585ed01d9c2e2844cbf
Reviewed-on: https://chromium-review.googlesource.com/1297181
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/dca66e5c4288370f14ed1e1b3ee8d2350bb8490e/init/BUILD.gn
[add] https://crrev.com/dca66e5c4288370f14ed1e1b3ee8d2350bb8490e/init/tests/clobber_state_test.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/9c86970858f8a4fc5a5802067c61684b764ee897

commit 9c86970858f8a4fc5a5802067c61684b764ee897
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Thu Nov 08 04:27:05 2018

Add clobber-state tests to ebuild

Adds clobber state integration test to ebuild.

BUG=chromium:884520
TEST=cros_run_unit_tests --package "chromeos-init"
CQ-DEPEND=CL:1297181

Change-Id: I35213da1e85947eb9106f989cbcad5bce96a07ff
Reviewed-on: https://chromium-review.googlesource.com/1319388
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/9c86970858f8a4fc5a5802067c61684b764ee897/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2ed071e696594d252240f2d02b96c114eec5751f

commit 2ed071e696594d252240f2d02b96c114eec5751f
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Wed Dec 12 21:55:49 2018

Revert "Add clobber-state tests to ebuild"

This reverts commit 9c86970858f8a4fc5a5802067c61684b764ee897.

Reason for revert: Test failures, see  http://crbug.com/905683 

Original change's description:
> Add clobber-state tests to ebuild
>
> Adds clobber state integration test to ebuild.
>
> BUG=chromium:884520
> TEST=cros_run_unit_tests --package "chromeos-init"
> CQ-DEPEND=CL:1297181
>
> Change-Id: I35213da1e85947eb9106f989cbcad5bce96a07ff
> Reviewed-on: https://chromium-review.googlesource.com/1319388
> Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
> Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
> Reviewed-by: Justin TerAvest <teravest@chromium.org>

Bug: chromium:884520
Change-Id: I86f166cba83a5689a3c1ef96a3282d3f8d6a5325
Reviewed-on: https://chromium-review.googlesource.com/1372285
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/2ed071e696594d252240f2d02b96c114eec5751f/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2725382de24086df23cb28f789d33223752f2d81

commit 2725382de24086df23cb28f789d33223752f2d81
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Tue Dec 18 08:42:56 2018

Revert "init: Add integration test for clobber-state"

This reverts commit dca66e5c4288370f14ed1e1b3ee8d2350bb8490e.

Reason for revert: Test is flaky and causing failed builds. I'm going to try and move it to an on-device test rather than a unit test so that it doesn't run on builders. See  https://crbug.com/905683 

Original change's description:
> init: Add integration test for clobber-state
>
> Adds a test for clobber state which creates and mounts a fake
> filesystem, populates it with a couple of files, runs clobber-state
> on it, and then checks that only the correct files are saved.
>
> BUG=chromium:884520
> TEST=quid probat ipso probatorem (who tests the testers?)
> CQ-DEPEND=CL:1319388
>
> Change-Id: Ia91d11d9eaab98c775f32585ed01d9c2e2844cbf
> Reviewed-on: https://chromium-review.googlesource.com/1297181
> Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
> Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
> Reviewed-by: Justin TerAvest <teravest@chromium.org>

CQ-DEPEND=CL:1369805
Bug: chromium:884520
Change-Id: Ic6649aebb2d7922d5a356a28ab49b0ddad052e75
Reviewed-on: https://chromium-review.googlesource.com/1369805
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Fletcher Woodruff <fletcherw@chromium.org>

[modify] https://crrev.com/2725382de24086df23cb28f789d33223752f2d81/init/BUILD.gn
[delete] https://crrev.com/1e3e5b70bc2cd7e7ac98e0ea44c7a4eb3b8d6255/init/tests/clobber_state_test.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/e8038e9aa0044aadff5661c0ac67a265137593a1

commit e8038e9aa0044aadff5661c0ac67a265137593a1
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Sat Dec 29 00:14:01 2018

init: Move some clobber-state logic into C++

Creates a C++ wrapper for clobber-state.sh and starts to pull some of the logic
out of shell and into that. Adds argument parsing, root check, and rebooting.

BUG=chromium:884520
TEST=cros_run_unit_tests --board=$BOARD --packages chromeos-init
CQ-DEPEND=CL:1377387

Change-Id: Iac5a72ccf21ddda531e3afd09b74ea798a645f8f
Reviewed-on: https://chromium-review.googlesource.com/1377563
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/BUILD.gn
[add] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/clobber_state_unittest.cc
[rename] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/clobber-state.sh
[add] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/clobber_state.cc
[add] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/clobber_state.h
[add] https://crrev.com/e8038e9aa0044aadff5661c0ac67a265137593a1/init/clobber_state_main.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5751461da4201b22026c7e096bec506e1e34c7b5

commit 5751461da4201b22026c7e096bec506e1e34c7b5
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Sat Dec 29 00:14:02 2018

Add new clobber_state C++ unit tests to ebuild

clobber-state is slowly being re-written in C++, and some pieces have
had unit tests added. Add those unit tests to the ebuild so they will
run.

BUG=chromium:884520
TEST=cros_run_unit_tests works
CQ-DEPEND=CL:1377563

Change-Id: I5de4b0395d0e241809802695111a9d59a4449883
Reviewed-on: https://chromium-review.googlesource.com/1377387
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/5751461da4201b22026c7e096bec506e1e34c7b5/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/3b27a8d1335c4173d58db6ea12c24a5701f80f8b

commit 3b27a8d1335c4173d58db6ea12c24a5701f80f8b
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Tue Jan 01 03:58:48 2019

init: Add wrapper library for crossystem

Adds C++ style wrapper library for crossystem that allows for testing
by swapping to a fake implementation. The library is based on a version
under login_manager but is not identical.

With this, we can have clobber_state.cc use dependency injection,
allowing for easier and more readable testing.

BUG=chromium:884520
TEST=code builds

Change-Id: I61b10fe367eb030ce525f2ac421e1b935a2b6f4f
Reviewed-on: https://chromium-review.googlesource.com/1387205
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Fletcher Woodruff <fletcherw@chromium.org>

[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem.h
[modify] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/BUILD.gn
[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem_fake.cc
[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem_impl.h
[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem_impl.cc
[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem_fake.h
[add] https://crrev.com/3b27a8d1335c4173d58db6ea12c24a5701f80f8b/init/crossystem.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c80e976db425fc236722da18075508bd58f11080

commit c80e976db425fc236722da18075508bd58f11080
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Thu Jan 03 23:04:39 2019

init: move more pieces of clobber-state.sh to C++

Extracts output redirection, biometric sensor entropy reset, developer
mode tagging, TPM clear request, and filesystem sync from
clobber-state.sh into C++.

Adds unit tests for developer mode tagging.

BUG=chromium:884520
TEST=run unit tests, run powerwash on device
CQ-DEPEND=CL:1377387
CQ-DEPEND=CL:1387205

Change-Id: I7deb3c656adae4ee396f408051fac4044318fed4
Reviewed-on: https://chromium-review.googlesource.com/1387208
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Fletcher Woodruff <fletcherw@chromium.org>

[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/BUILD.gn
[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/clobber_state_unittest.cc
[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/clobber-state.sh
[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/clobber_state.cc
[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/clobber_state.h
[modify] https://crrev.com/c80e976db425fc236722da18075508bd58f11080/init/clobber_state_main.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384

commit 18f23c2d05dce58cf5520bc9ca3f8b50a65a2384
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Fri Jan 04 23:30:55 2019

init: Move clobber-state.sh preserve_files to C++

Chop off another piece of clobber-state.sh, namely the code that picks
what files to preserve during a powerwash.

Reworks unit tests to be slightly less verbose.

Removes two parameters from clobber-state.sh that are no longer used as
all relevant logic has been moved to C++.

BUG=chromium:884520
TEST=unit tests and manual on-device test
CQ-DEPEND=CL:1387208

Change-Id: I33447290b4512533b9674fc9722f29d231f89785
Reviewed-on: https://chromium-review.googlesource.com/1392446
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>

[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/crossystem.h
[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/clobber_state_unittest.cc
[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/clobber-state.sh
[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/clobber_state.cc
[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/clobber_state.h
[modify] https://crrev.com/18f23c2d05dce58cf5520bc9ca3f8b50a65a2384/init/crossystem.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/be7071e71ec11a1d991f834b9b94c0e15c3669b1

commit be7071e71ec11a1d991f834b9b94c0e15c3669b1
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Fri Jan 11 21:32:23 2019

secure-erase-file: install header to /usr/include

Since other ebuilds depend on secure_erase_file/secure_erase_file.h,
install it to /usr/include/chromeos directory so that they can read it from
there rather than improperly grabbing from the platform2 source dir.

Update chromeos-init to properly depend on secure-erase-file and other
packages (DEPEND rather than RDEPEND due to the need to compile clobber-state).

BUG=chromium:884520
TEST=build succeeds

Change-Id: Ibf161fd5c63eab52b224a946e8deff4043440d49
Reviewed-on: https://chromium-review.googlesource.com/1404139
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Fletcher Woodruff <fletcherw@chromium.org>

[modify] https://crrev.com/be7071e71ec11a1d991f834b9b94c0e15c3669b1/chromeos-base/chromeos-init/chromeos-init-9999.ebuild
[modify] https://crrev.com/be7071e71ec11a1d991f834b9b94c0e15c3669b1/chromeos-base/secure-erase-file/secure-erase-file-9999.ebuild

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/8c0f3e10a7c9a0ac8041b203a9147d1d2938038f

commit 8c0f3e10a7c9a0ac8041b203a9147d1d2938038f
Author: Fletcher Woodruff <fletcherw@chromium.org>
Date: Fri Jan 11 21:32:22 2019

Add cgpt_find to libvboothost

Includes cgpt_find.c and some dependencies in the make rule for
libvboothost so that they can be used by the new C++ clobber-state.

BUG=chromium:884520
TEST=vboothost successfully builds
BRANCH=none

Change-Id: I4cedd7625c8dd905b4391de39477f42ad0dc4902
Reviewed-on: https://chromium-review.googlesource.com/1395811
Commit-Ready: Fletcher Woodruff <fletcherw@chromium.org>
Tested-by: Fletcher Woodruff <fletcherw@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/8c0f3e10a7c9a0ac8041b203a9147d1d2938038f/Makefile

Sign in to add a comment