Add CBnT 2.1 and replace code generation in intel/metadata

[micgor32]

CBnT 2.1 (MTL and newer) brings breaking changes:

• PDRS and CNBS are now mandatory element of PCD (see d75ae02).
• SACM v5 moves the header to 0x00050004, given that the R/W methods for all SACM versions are same, the logic is moved out to the generic implementation (see 1800718, lines 357-377).
• BPM header is > 0x21, thus 0x22 to 0x25 is recognized as BootGuardVersion.Version21.

Replace codegen for manifest/structure methods. This is done by moving out the logic to a dedicated struct (Common), that is then embed into the remaining structs. These have to provide their layout descriptor on each call to Common. This reduces the amount of the code in general in the intel/metadata. Additionally, bg and cbnt are now merged by letting the per-type constructor to return the correct type implementation. Both concepts are documented in pkg/intel/metadata/README.md in 5903d58.

Since the amount of (+- lines) changes is huge, I've tried to split commits into one per type + the unit test for it. There are few types that are just moved from either bg or cbnt, and contribute to the modified lines, but do not contain any meaningful logic change. I've noted such cases in the commit description.

Renamed common/unittest to common/integration to avoid confusion with the purpose of it and *_test.go in cbnt. The integration test is now extended a bit, and its full scope is document in pkg/intel/metadata/common/integration/README.md in c96a8bd.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 58.61942% with 1121 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.76%. Comparing base (5620ca1) to head (7ce450a).

Files with missing lines	Patch %	Lines
pkg/intel/metadata/cbnt/bootpolicy/manifest.go	43.47%	316 Missing and 61 partials ⚠️
pkg/intel/metadata/cbnt/bootpolicy/se.go	54.94%	151 Missing and 22 partials ⚠️
pkg/intel/metadata/cbnt/bootpolicy/pcd.go	54.31%	89 Missing and 17 partials ⚠️
pkg/intel/metadata/cbnt/bootpolicy/pm.go	28.44%	80 Missing and 3 partials ⚠️
...g/intel/metadata/cbnt/keymanifest/manifest_cbnt.go	63.58%	45 Missing and 14 partials ⚠️
pkg/intel/metadata/cbnt/common.go	68.36%	29 Missing and 27 partials ⚠️
pkg/intel/metadata/cbnt/bootpolicy/txt.go	54.78%	48 Missing and 4 partials ⚠️
pkg/intel/metadata/cbnt/bootpolicy/bpmh.go	74.14%	29 Missing and 9 partials ⚠️
pkg/intel/metadata/cbnt/keymanifest/manifest_bg.go	53.52%	27 Missing and 6 partials ⚠️
pkg/intel/metadata/cbnt/keymanifest/hash.go	56.16%	28 Missing and 4 partials ⚠️
... and 11 more
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #450       +/-   ##
===========================================
+ Coverage   34.13%   44.76%   +10.62%     
===========================================
  Files         209      170       -39     
  Lines       14120    12243     -1877     
===========================================
+ Hits         4820     5480      +660     
+ Misses       8411     5939     -2472     
+ Partials      889      824       -65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[micgor32]

ping @orangecms, @rminnich

[orangecms]

ping @orangecms, @rminnich

seen it! It's a bit of a change... :D
Not sure when I can finf the time to review this thoroughly.

I would also want to port the general logic and structs to
https://github.com/platform-system-interface/intel_fw

[MDr164]

I'll take a look as well, will take a bit of time though...

[rminnich]

WOW, this is an impressive and overwhelming PR! I guess it can not be done in smaller chunks, it has to be one thing?

[rminnich]

Please put a function comment here, thanks.

[rminnich]

I much prefer this one-line structure to the multiline structures above, it makes it far easier to read.
Can you change the ones above to match this format?

[micgor32]

I could do something like

func() uint64 { if s.PME == nil { return 0 } return s.PME.TotalSize() }

as oneliner.
But the check have to stay there for all substructs of BPM that are optional. Otherwise things might fail quite bad :D

[rminnich]

usijg -> using

[rminnich]

do you really need the named return parameters? In the code below in many places you use different return values.

[micgor32]

right, similar case as with size in pm.go, my bad. Though for the error it make sense to keep it as named parameter, since it is used in the closure few lines below.

[rminnich]

I think on all these exported functions, you need a comment.
If RehashRecursive is part of an interface (I forgot to look) it can be as simple as
// xyz implements xyz
or some such.

[rminnich]

I wonder why it can't just
return int64(binary.Size(uint16(0))) + uint64(binary.Size(s.DataSize))

[micgor32]

Right, my bad. In here I just blindly moved PM.DataTotalSize logic into the closure (https://github.com/linuxboot/fiano/blob/main/pkg/intel/metadata/cbnt/cbntbootpolicy/pm_manifestcodegen.go#L199)

[rminnich]

Is this comment really necessary?

Is this from AI?

[micgor32]

It's leftover from the manifestcodegen https://github.com/linuxboot/fiano/blob/main/pkg/intel/metadata/cbnt/cbntbootpolicy/signature_manifestcodegen.go#L39 and https://github.com/linuxboot/fiano/blob/main/pkg/intel/metadata/common/manifestcodegen/cmd/manifestcodegen/template_methods.tpl.go#L62.

Not sure whether it makes sense to have it there, but I assumed there was a reason for it.

[rminnich]

For all these New functions that set non-zero values:
what if someone does this: s := &TXT{}
and does not call this function? Will the zero value work or cause panics?
Should there be a test for the zero value?

[micgor32]

Didn't thought about that, will check and add a test for that if needed.

[micgor32]

So I have checked two cases of such usage with bg-prov of CSS.
changing this part to:

func (g *generateBPMCmdv2) Run(ctx *context) error {
	var b bootguard.BootGuard
	b.Version = cbnt.Version20
	if g.Config != "" {
		err := b.ReadJSON(g.Config)
		if err != nil {
			return err
		}
	} else {
		cbntbpm := &bootpolicy.ManifestCBnT{}
		bp := &bootpolicy.BPMHCBnT{}

		b.VData.CBNTbpm = cbntbpm
		b.VData.CBNTbpm.BPMHCBnT = *bp

works fine (i.e. no panics), though the values that would have been set to non-zero by New<*> func's won't be spec compliant.

[micgor32]

the linked part should be different line... https://github.com/9elements/converged-security-suite/blob/cbnt21-refactor/cmd/core/bg-prov/cmd.go#L668 sorry :)

[rminnich]

my inclination is that we need to accept this, although I don't understand it all ... and hope nothing will break.

[micgor32]

WOW, this is an impressive and overwhelming PR! I guess it can not be done in smaller chunks, it has to be one thing?

It could be split into smaller ones yes, the thing is, these would not be functional on their own i.e. after applying it would leave the metadata packages broken partially. Though if it makes life easier for reviewing, I'll split it :)