VerifiedBooter - NOT FOR MERGE #28
Conversation
zaolin
commented
Jun 11, 2018
•
zaolin
commented
- 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.
There was a problem hiding this comment.
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.
verifiedboot/main.go
Outdated
| if *doDebug { | ||
| debug = log.Printf | ||
| recoverer = recovery.SecureRecoverer{ | ||
| Reboot: false, |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
partly done, let's fix that finally afterwards
verifiedboot/main.go
Outdated
|
|
||
| // Initialize random seeding | ||
| err := rng.UpdateLinuxRandomness(recoverer) | ||
| if err != nil { |
There was a problem hiding this comment.
just one line is enough:
if err := rng.UpdateLinuxRandomness(recoverer); err != nil {
...
}
verifiedboot/main.go
Outdated
| } | ||
|
|
||
| // Initialize the TPM | ||
| if *bootMode == booter.BootModeMeasured || *bootMode == booter.BootModeBoth { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
okay let me think about it. Follow up
verifiedboot/main.go
Outdated
| flag.Parse() | ||
| log.Print(banner) | ||
|
|
||
| var recoverer recovery.Recoverer |
There was a problem hiding this comment.
maybe it's time to rename Recoverer to RecoveryHandler? :)
verifiedboot/main.go
Outdated
| } | ||
|
|
||
| // Check FIT image existence and read it into memory | ||
| fitImage := mountPath + *fitFilePath |
There was a problem hiding this comment.
this should use path.Join too
verifiedboot/main.go
Outdated
| // Verify signature of FIT image on device | ||
| if *bootMode == booter.BootModeVerified || *bootMode == booter.BootModeBoth { | ||
| // Read fit image signature into memory | ||
| fitImageSignature := mountPath + *fitFilePath + SignatureFileExt |
pkg/booter/verifiedbooter.go
Outdated
| BootMode string `json:"boot_mode"` | ||
| DeviceUUID string `json:"device_uuid"` | ||
| FitFile string `json:"fit_file"` | ||
| Debug string `json:"debug"` |
There was a problem hiding this comment.
this should be bool. The JSON parser will take care of setting this appropriately for you
verifiedboot/main.go
Outdated
| @@ -78,91 +78,85 @@ func main() { | |||
| flag.Parse() | |||
| log.Print(banner) | |||
|
|
|||
| var recoverer recovery.Recoverer | |||
| var RecoveryHandler recovery.Recoverer | |||
There was a problem hiding this comment.
I actually meant to change the name in the recovery package
pkg/booter/verifiedbooter.go
Outdated
| return nil, fmt.Errorf("False boot mode for VerifiedBooter: %s", nb.BootMode) | ||
| } | ||
|
|
||
| if _, err := uuid.FromString(nb.DeviceUUID); err != nil { |
There was a problem hiding this comment.
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 .
zaolin
changed the title
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
