preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs #338

chrisccoulson · 2024-10-04T13:37:40Z

CheckResult is returned from RunChecks on successful completion. It is
JSON serializable and intended to be supplied later on to
WithAutoTCGPCRProfile along with some user customization flags (defined
by PCRProfileOptionsFlags) in order to generate an option that can be
supplied to secboot_efi.AddPCRProfile.

Note that some user options don't work yet because of missing PCR
support, although these limitations will be addressed later. For now,
these will appear as failures in the CheckResult flags.

CheckResult is returned from RunChecks on successful completion. It is JSON serializable and intended to be supplied later on to WithAutoTCGPCRProfile along with some user customization flags (defined by PCRProfileOptionsFlags) in order to generate an option that can be supplied to secboot_efi.AddPCRProfile. Note that some user options don't work yet because of missing PCR support, although these limitations will be addressed later. Even so, some options may still fail depending on the CheckResult flags.

pedronis

did a first pass, probably a good idea for @valentindavid and maybe someone more from your side to look it, Mate?

pedronis · 2024-11-22T17:16:29Z

efi/preinstall/errors.go

+	br := bufio.NewReader(r)
+	for {
+		line, err := br.ReadString('\n')
+		fmt.Fprintf(w, "%*s%s", n, "", line)


should we really do this print if line is "" and err is io.EOF ?

I've refactored this function a bit (it uses bufio.Scanner now and it's been renamed to make it clearer what its intended use is.

pedronis · 2024-11-22T17:29:34Z

efi/preinstall/result.go

+	"startup-locality-not-protected":             StartupLocalityNotProtected,
+}
+
+type x509CertificateIdJSON struct {


nitpick: I would split out the x509Id types to maybe their own x509id.go file

I've moved these types into their own file.

pedronis · 2024-11-22T17:30:48Z

efi/preinstall/result.go

+	if r.Warnings != nil && len(r.Warnings.Errs) > 0 {
+		fmt.Fprintf(w, "- Warnings:\n")
+		for i := 0; i < len(r.Warnings.Errs); i++ {
+			fmt.Fprintf(w, "%s\n", indentLines(2, "- "+r.Warnings.Errs[i].Error()))
+		}
+	}


this bit of the code is not exercised

I've added a test for this now.

valentindavid

Just minor comments.

valentindavid · 2024-11-25T10:18:16Z

efi/preinstall/errors.go

+	w := new(bytes.Buffer)
+	fmt.Fprintf(w, "one or more errors detected:\n")
+	for _, err := range e.Errs {
+		fmt.Fprintf(w, "%s\n", indentLines(2, "- "+err.Error()))


So if we have errors err1-line1\nerr1-line2 and err2-line1\nerr2-line2, the output will look like:

- err1-line1 err1-line2 - err2-line1 err2-line2

I feel like the - should not be indented so that the lines match.

The expected formatting in the example would be:

- err1-line1 err1-line2 - err2-line1 err2-line2

In any case, I've refactored indentLines now and called it something else (makeIndentedListItem) to make its purpose clearer. I've also added some formatting tests for code that makes use of it too.

valentindavid · 2024-11-25T11:17:18Z

efi/preinstall/profile.go

+				// don't extend anything to the TPM, breaking the root-of-trust. This is true of the Microsoft UEFI CA 2011,
+				// and for now, we assume to be true of the 2023 UEFI CA unless Microsoft are more transparent about what is
+				// signed under this CA). It's also assumed to be true for any unrecognized CAs.
+				// This can be overridden with PCRProfileOptionsTrustCAsForBootCode.


s/PCRProfileOptionsTrustCAsForBootCode/PCRProfileOptionTrustCAsForBootCode/

(Options -> Option)

I've fixed this now.

valentindavid · 2024-11-25T11:17:49Z

efi/preinstall/profile.go

+				// (ie, they may have signed code in the past that can defeat our security model. This is true of the Microsoft
+				// UEFI CA 2011, and for now, we assume to be true of the 2023 UEFI CA unless Microsoft are more transparent about
+				// what they sign under this CA). It's also assumed to be true for any unrecognized CAs.
+				// This can be overridden with PCRProfileOptionsTrustCAsForVARSuppliedDrivers.


And this one.

valentindavid · 2024-11-25T11:26:48Z

efi/preinstall/profile.go

+		{internal_efi.MSUefiCA2011, 0},
+		{internal_efi.MSUefiCA2023, 0}, // be conservative here for now, but will we be able to set the authorityTrustDrivers flag for the MS2023 CA?


I want to make sure I understand here.

We do not really need to put anything with authorityTrust value 0 in this list, because all of those are not trusted. And any certificated not found in the authorityTrustDataSet will make trust &= certTrust be trust &= 0 which will be then distrust. The only use here for knownCAs is that maybe one day some CA certicates are trusted and we will add them in this list with a non 0 value for authorityTrust.

So here MSUefiCA2011 and MSUefiCA2023 are just explicitly distrusted, but if we removed them from the list, it would be the same.

Did I understand it correctly?

Maybe it would be nice to have a test that uses a cert that is not in knownCAs.

You understood correctly - these are both explicitly distrusted, as is any certificate that doesn't appear in this list. Eventually we'll get to the point where we want security features that will require our own CA, which will be added here with both trust flags set (because we know we'll never use it to sign drivers). It also remains to be seen whether Microsoft will be any more open wrt to their signing requirements with the new UEFI CA, particularly wrt the signing of option ROMs. If this improves, we might be able to change one flag for this CA, but right now, I don't think it's been used to sign anything.

Also fix the implementation of RunChecksErrors.Error().

This renames indentLines to makeIndentedListItem to make it clear that it's used to help printing out lists of things (in this context, mostly lists of errors). It also refactors it to use bufio.Scanner. Some additional test cases are added to test the formatting of RunChecksErrors, both on its own and embedded as a warning inside of CheckResult.

pedronis

thanks

valentindavid

Thank you.

chrisccoulson force-pushed the preinstall-add-profile-and-results branch 2 times, most recently from 33a77f7 to 27fef9d Compare October 4, 2024 19:44

chrisccoulson mentioned this pull request Oct 5, 2024

preinstall: Add public RunChecks API #339

Merged

chrisccoulson force-pushed the preinstall-add-profile-and-results branch 5 times, most recently from 0bec84e to 878ee98 Compare October 17, 2024 10:11

chrisccoulson force-pushed the preinstall-add-profile-and-results branch 2 times, most recently from 5b74975 to 42dfed0 Compare November 13, 2024 18:45

chrisccoulson changed the title ~~preinstall: Add CheckResult and WithAutoPCRProfile APIs~~ preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs Nov 15, 2024

chrisccoulson force-pushed the preinstall-add-profile-and-results branch from 42dfed0 to 0c76820 Compare November 21, 2024 11:30

chrisccoulson marked this pull request as ready for review November 21, 2024 11:38

chrisccoulson requested a review from pedronis November 21, 2024 11:38

pedronis reviewed Nov 22, 2024

View reviewed changes

Merge branch 'master' into preinstall-add-profile-and-results

6214a75

valentindavid reviewed Nov 25, 2024

View reviewed changes

chrisccoulson added 5 commits December 3, 2024 12:56

preinstall: update indentLines to break earlier on io.EOF

6964595

preinstall: Move X509CertificateID types to their own file

f6fae02

preinstall: Add test for CheckResult.String() with Warnings

7c0c6e0

Also fix the implementation of RunChecksErrors.Error().

preinstall: fix a couple of doc typos

ac6f140

chrisccoulson requested review from pedronis and valentindavid December 4, 2024 08:07

pedronis approved these changes Dec 4, 2024

View reviewed changes

valentindavid approved these changes Dec 4, 2024

View reviewed changes

chrisccoulson merged commit 1224447 into canonical:master Dec 4, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs #338

preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs #338

chrisccoulson commented Oct 4, 2024 •

edited

Loading

pedronis left a comment

pedronis Nov 22, 2024

chrisccoulson Dec 4, 2024

pedronis Nov 22, 2024

chrisccoulson Dec 4, 2024

pedronis Nov 22, 2024

chrisccoulson Dec 4, 2024

valentindavid left a comment

valentindavid Nov 25, 2024

chrisccoulson Dec 4, 2024

valentindavid Nov 25, 2024

chrisccoulson Dec 4, 2024

valentindavid Nov 25, 2024

chrisccoulson Dec 4, 2024

valentindavid Nov 25, 2024

valentindavid Nov 25, 2024

chrisccoulson Dec 4, 2024

pedronis left a comment

valentindavid left a comment

		{internal_efi.MSUefiCA2011, 0},
		{internal_efi.MSUefiCA2023, 0}, // be conservative here for now, but will we be able to set the authorityTrustDrivers flag for the MS2023 CA?

preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs #338

preinstall: Add CheckResult and WithAutoTCGPCRProfile APIs #338

Conversation

chrisccoulson commented Oct 4, 2024 • edited Loading

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentindavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

valentindavid left a comment

Choose a reason for hiding this comment

chrisccoulson commented Oct 4, 2024 •

edited

Loading