efi: allow other architectures in check_host_security and friends - #563
efi: allow other architectures in check_host_security and friends#563alexclewontin wants to merge 3 commits into
Conversation
|
Please rebase on master once PR#562 is merged. |
|
Does |
It uses Working on another commit that puts the thinnest possible layer around cpuid so that we can unpin the default env pieces. |
| intelDevices := func(status []byte, withIOMMU bool) []internal_efi.SysfsDevice { | ||
| attrs := map[string][]byte{ |
There was a problem hiding this comment.
Instead of embedding a func here, wouldn't it be more legible to have it as a plain regular named function? That would also be easier to document and let runChecksPlatformHostFixtures() focus on the list that it returns?
| expectedWarningsMatch: `3 errors detected: | ||
| required := runChecksHostCapabilityValid | runChecksHostCapabilityFirmwareTPM | ||
| for _, fixture := range runChecksHostFixturesFor(c, required) { | ||
| c.Logf("running with host fixture %q", fixture.name) |
There was a problem hiding this comment.
These are worth being displayed when tests are run (for identifying which host fixture raised an error, or for comparison of which host fixtures get tested from a version to another...).
They appear with -check.vv, but this option also activates other cumbersome log messages.
So, is there a way to give better visibility when running tests? And possibly ensure that Makefile and .github/workflows/test.yaml (or run-tests) take advantage of it.
3daef88 to
628c6df
Compare
There was a problem hiding this comment.
This is not a rename, it's an addition of a file & the removal of a file but because both files are small and the license header dominates, it's something like 52% similar so detected as a rename
github.com/canonical/cpuid does not compile on non-x86 architectures, which forced the AMD64 host environment to be split across a filename gated default_env_amd64.go and a default_env_amd64_null.go stub. That made the implementation invisible to the compiler, and untestable, on every other architecture. Confine that dependency to a new internal/cpuid package. It is a deliberately thin wrapper, and the only place in the tree that is gated by architecture at build time. It exposes VendorIdentificator, Family and HasFeature, delegating to the upstream package on amd64 and returning zero values elsewhere. internal/efi is now architecture neutral: it carries no build constraints, the AMD64 implementation collapses into default_env.go, and AMD64() selects on a mockable runtimeGOARCH rather than a build tag. This mirrors the runtime dispatch used elsewhere for host security checks. The CPUIDFeature* constants stay in internal/efi, so callers are unchanged. Their drift guards against the upstream bit positions move to internal/cpuid, which is now the only package able to see both. The AMD64 tests lose their build constraints and drive the CPU identity through new mock hooks instead of mutating upstream package variables, so they run everywhere. TestNotAMD64Host no longer depends on the host architecture, and therefore now runs on amd64 too.
Signed-off-by: Alex Lewontin <alex.lewontin@canonical.com>
628c6df to
44b4c70
Compare
|
Closing in favor of #565, which I can make a stacked PR :) |
2 participants
Checking host security today makes assumes in many ways secboot will only be used on amd64. This PR attempts to lay the groundwork to allow other architectures to leverage much of the same logic.
Big themes:
This is based on top of #562 and is part of an attempt to split up #560.