VerifiedBooter - NOT FOR MERGE by zaolin · Pull Request #28 · systemboot/systemboot

zaolin · 2018-06-11T14:26:41Z

Adds verifiedbooter
Allows booting in secure, measured or both modes.
Makes use of boot config image format.
Kexec support
Seeds /dev/random for more boot entropy.

insomniacslk

I haven't actually reviewed pkg/verifiedbooter.go, just verifiedboot/main.go . That's because I'd split this PR in two parts, one where we implement the verifiedbooter command, and one where we implement the verified booting logic on top. I need to think more on how to plug verified booting on top of local/net booting (wrapper? embedding?) so let's speed up this PR and let's just do the verifiedboot command.

insomniacslk · 2018-06-14T14:48:56Z

+	if *doDebug {
+		debug = log.Printf
+		recoverer = recovery.SecureRecoverer{
+			Reboot: false,


I find counterintuitive that rebooting is a function of verbosity. I would leave Reboot: true in all cases, and add a -reboot-on-error flag that will flip this value.

Also please move the recoverer initialization outside of the if *doDebug check. Something like this:

recoverer = recovery.SecureRecoverer{ Reboot: *doRebootOnError, Sync: false, Debug: *doDebug, } if *doDebug { debug = log.Printf }

partly done, let's fix that finally afterwards

insomniacslk · 2018-06-14T14:53:59Z

+
+	// Initialize random seeding
+	err := rng.UpdateLinuxRandomness(recoverer)
+	if err != nil {


just one line is enough:

if err := rng.UpdateLinuxRandomness(recoverer); err != nil { ... }

insomniacslk · 2018-06-14T15:02:43Z

+	}
+
+	// Initialize the TPM
+	if *bootMode == booter.BootModeMeasured || *bootMode == booter.BootModeBoth {


In cases like this strings can be dangerous and inefficient to carry around, and they should just be a convention for a main package, not for a library (they are defined in pkg/booter). I would rather use a iota (similar, but not equal to an enum), or a bitmask, or even a lightweight struct with two methods, IsVerified and IsMeasured that will check the enum/bitmask (but this is probably overkill)

okay let me think about it. Follow up

insomniacslk · 2018-06-14T15:03:47Z

+	flag.Parse()
+	log.Print(banner)
+
+	var recoverer recovery.Recoverer


maybe it's time to rename Recoverer to RecoveryHandler? :)

insomniacslk · 2018-06-14T15:06:54Z

+	}
+
+	// Check FIT image existence and read it into memory
+	fitImage := mountPath + *fitFilePath


this should use path.Join too

insomniacslk · 2018-06-14T15:34:53Z

+	// Verify signature of FIT image on device
+	if *bootMode == booter.BootModeVerified || *bootMode == booter.BootModeBoth {
+		// Read fit image signature into memory
+		fitImageSignature := mountPath + *fitFilePath + SignatureFileExt


path.Join here too

insomniacslk · 2018-06-14T15:37:23Z

+	BootMode   string `json:"boot_mode"`
+	DeviceUUID string `json:"device_uuid"`
+	FitFile    string `json:"fit_file"`
+	Debug      string `json:"debug"`


this should be bool. The JSON parser will take care of setting this appropriately for you

insomniacslk · 2018-06-14T21:31:44Z

 	log.Print(banner)

-	var recoverer recovery.Recoverer
+	var RecoveryHandler recovery.Recoverer


I actually meant to change the name in the recovery package

insomniacslk

Will review the rest on Monday

insomniacslk · 2018-06-16T15:12:33Z

+		return nil, fmt.Errorf("False boot mode for VerifiedBooter: %s", nb.BootMode)
+	}
+
+	if _, err := uuid.FromString(nb.DeviceUUID); err != nil {


what about using Fiano's uuid package? I wrote that code originally, and it's equivalent in terms of features that we need, plus we have control over. Find it at https://github.com/linuxboot/fiano/tree/master/uuid .

codecov-io · 2018-07-09T19:40:29Z

Codecov Report

Merging #28 into master will increase coverage by 21%.
The diff coverage is 54.94%.

@@           Coverage Diff            @@
##           master      #28    +/-   ##
========================================
+ Coverage   18.64%   39.64%   +21%     
========================================
  Files          17       11     -6     
  Lines         751      570   -181     
========================================
+ Hits          140      226    +86     
+ Misses        591      291   -300     
- Partials       20       53    +33

Impacted Files	Coverage Δ
pkg/booter/bootentry.go	`94.87% <100%> (+0.93%)`	⬆️
pkg/bootconfig/parser.go	`43.56% <43.56%> (ø)`
pkg/booter/verifiedbooter.go	`47.82% <47.82%> (ø)`
pkg/crypto/ed25519.go	`74% <74%> (ø)`
localboot/bootconfig.go
pkg/recovery/securerecoverer.go
localboot/main.go
pkg/rng/entropy.go
pkg/recovery/permissiverecoverer.go
localboot/grub.go
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b3658e...2e5ee26. Read the comment docs.

zaolin requested a review from insomniacslk June 11, 2018 18:22

zaolin self-assigned this Jun 11, 2018

zaolin added the enhancement New feature or request label Jun 11, 2018

insomniacslk requested a review from Natolumin June 12, 2018 09:45

zaolin force-pushed the verifiedbooter branch from 7bd4078 to 4ccc32d Compare June 12, 2018 21:33

insomniacslk changed the title ~~WIP: VerifiedBooter~~ Implemented VerifiedBooter Jun 14, 2018

insomniacslk suggested changes Jun 14, 2018

View reviewed changes

insomniacslk reviewed Jun 14, 2018

View reviewed changes

insomniacslk reviewed Jun 16, 2018

View reviewed changes

Philipp Deppenwiese added 2 commits June 26, 2018 21:43

Add crypto extensions

bd21cc0

Change crypto for ed25519 support

fd16a0b

zaolin changed the title ~~Implemented VerifiedBooter~~ Implemented VerifiedBooter [4/4] Jun 30, 2018

Philipp Deppenwiese added 3 commits July 1, 2018 01:23

Initial checkin

11cdf82

Fix commands.go of configtool

6fe4560

Remove bc.file

fdeee42

zaolin changed the title ~~Implemented VerifiedBooter [4/4]~~ Implemented VerifiedBooter [3/3] Jul 4, 2018

Philipp Deppenwiese added 7 commits July 5, 2018 01:22

Fix remaining issues

0b22e34

Fixing nits

1b49826

Initial commit

3401666

Add tests, clean & fix verified booter

0f5318a

Implement more functionality

6664742

Add recoverer and correct packages

5677de2

Fixing PR issues

ac38439

zaolin changed the title ~~Implemented VerifiedBooter [3/3]~~ VerifiedBooter [3/3] Jul 5, 2018

Fix remaining issues

b061328

zaolin force-pushed the verifiedbooter branch from bf4fb85 to b061328 Compare July 5, 2018 01:26

Update features

6367b0d

zaolin changed the title ~~VerifiedBooter [3/3]~~ VerifiedBooter - NOT FOR MERGE Jul 6, 2018

customization + docs

f248ee0

Philipp Deppenwiese added 3 commits July 11, 2018 16:29

More updates

8463478

Update kexec binaries

8bcd3a6

Update kexec binaries

2e5ee26

Conversation

zaolin commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

insomniacslk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

insomniacslk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zaolin commented Jun 11, 2018 •

edited

Loading

codecov-io commented Jul 9, 2018 •

edited

Loading