diff --git a/bmc/secure_boot.go b/bmc/secure_boot.go index d53a4660..cd0d90e6 100644 --- a/bmc/secure_boot.go +++ b/bmc/secure_boot.go @@ -94,6 +94,31 @@ type secureBootCertificateImporterProvider struct { SecureBootCertificateImporter } +// CustomSecureBootKeysAllower controls whether the platform will accept +// custom UEFI Secure Boot keys, as opposed to using only the key set the +// firmware shipped with. Vendors expose this differently (a BIOS attribute on +// some platforms, a setup-menu-only setting on others, and not at all on +// platforms that never gate enrollment). +// +// Implementations MUST NOT alter the contents of any Secure Boot key database +// as a side effect. Callers rely on this to sequence a key-store reset and +// this call independently. A platform whose equivalent setting is coupled to +// key-store initialization cannot satisfy this contract and MUST NOT +// implement this interface. +// +// Implementations MUST be idempotent. +// +// rebootRequired reports that the change is staged and will not be in effect +// until the host has been power cycled. +type CustomSecureBootKeysAllower interface { + AllowCustomSecureBootKeys(ctx context.Context, enable bool) (rebootRequired bool, err error) +} + +type customSecureBootKeysAllowerProvider struct { + name string + CustomSecureBootKeysAllower +} + func secureBootState(ctx context.Context, generic []secureBootStateGetterProvider) (enabled bool, metadata Metadata, err error) { metadata = newMetadata() Loop: @@ -224,6 +249,32 @@ Loop: return metadata, multierror.Append(err, errors.New("failure to import secure boot certificate")) } +func allowCustomSecureBootKeys(ctx context.Context, generic []customSecureBootKeysAllowerProvider, enable bool) (rebootRequired bool, metadata Metadata, err error) { + metadata = newMetadata() +Loop: + for _, elem := range generic { + if elem.CustomSecureBootKeysAllower == nil { + continue + } + select { + case <-ctx.Done(): + err = multierror.Append(err, ctx.Err()) + break Loop + default: + metadata.ProvidersAttempted = append(metadata.ProvidersAttempted, elem.name) + rebootRequired, vErr := elem.AllowCustomSecureBootKeys(ctx, enable) + if vErr != nil { + err = multierror.Append(err, errors.WithMessagef(vErr, "provider: %v", elem.name)) + continue + } + metadata.SuccessfulProvider = elem.name + return rebootRequired, metadata, nil + } + } + + return rebootRequired, metadata, multierror.Append(err, errors.New("failure to set secure boot key management")) +} + // GetSecureBootStateFromInterfaces returns whether UEFI Secure Boot is enabled using // the first successful SecureBootStateGetter implementation found in generic. func GetSecureBootStateFromInterfaces(ctx context.Context, generic []interface{}) (enabled bool, metadata Metadata, err error) { @@ -380,3 +431,35 @@ func ImportSecureBootCertificateFromInterfaces(ctx context.Context, generic []in return importSecureBootCertificate(ctx, implementations, database, certificatePEM) } + +// AllowCustomSecureBootKeysFromInterfaces enables/disables acceptance of custom UEFI +// Secure Boot keys using the first successful CustomSecureBootKeysAllower +// implementation found in generic. +func AllowCustomSecureBootKeysFromInterfaces(ctx context.Context, generic []interface{}, enable bool) (rebootRequired bool, metadata Metadata, err error) { + implementations := make([]customSecureBootKeysAllowerProvider, 0) + for _, elem := range generic { + if elem == nil { + continue + } + temp := customSecureBootKeysAllowerProvider{name: getProviderName(elem)} + switch p := elem.(type) { + case CustomSecureBootKeysAllower: + temp.CustomSecureBootKeysAllower = p + implementations = append(implementations, temp) + default: + e := fmt.Sprintf("not a CustomSecureBootKeysAllower implementation: %T", p) + err = multierror.Append(err, errors.New(e)) + } + } + if len(implementations) == 0 { + return rebootRequired, metadata, multierror.Append( + err, + errors.Wrap( + bmclibErrs.ErrProviderImplementation, + ("no CustomSecureBootKeysAllower implementations found"), + ), + ) + } + + return allowCustomSecureBootKeys(ctx, implementations, enable) +} diff --git a/bmc/secure_boot_test.go b/bmc/secure_boot_test.go index 039a33fe..e8fa17d8 100644 --- a/bmc/secure_boot_test.go +++ b/bmc/secure_boot_test.go @@ -69,6 +69,19 @@ func (m *mockSecureBootCertificateImporter) Name() string { return "mock" } +type mockCustomSecureBootKeysAllower struct { + rebootRequired bool + err error +} + +func (m *mockCustomSecureBootKeysAllower) AllowCustomSecureBootKeys(ctx context.Context, _ bool) (bool, error) { + return m.rebootRequired, m.err +} + +func (m *mockCustomSecureBootKeysAllower) Name() string { + return "mock" +} + func TestGetSecureBootStateFromInterfaces(t *testing.T) { testCases := []struct { name string @@ -272,3 +285,47 @@ func TestImportSecureBootCertificateFromInterfaces(t *testing.T) { }) } } + +func TestAllowCustomSecureBootKeysFromInterfaces(t *testing.T) { + testCases := []struct { + name string + generic []interface{} + errMsg string + expectedRebootRequired bool + }{ + { + name: "success, reboot required", + generic: []interface{}{&mockCustomSecureBootKeysAllower{rebootRequired: true}}, + expectedRebootRequired: true, + }, + { + name: "not an implementation", + generic: []interface{}{&mockSecureBootStateGetter{}}, + errMsg: "no CustomSecureBootKeysAllower implementations found", + }, + { + name: "no implementations", + generic: []interface{}{}, + errMsg: "no CustomSecureBootKeysAllower implementations found", + }, + { + name: "error from enabler", + generic: []interface{}{&mockCustomSecureBootKeysAllower{err: errors.New("foobar")}}, + errMsg: "foobar", + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + rebootRequired, _, err := AllowCustomSecureBootKeysFromInterfaces(context.Background(), tt.generic, true) + + if tt.errMsg == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, tt.errMsg) + } + + assert.Equal(t, tt.expectedRebootRequired, rebootRequired) + }) + } +} diff --git a/client.go b/client.go index 02b414b3..3bc96762 100644 --- a/client.go +++ b/client.go @@ -757,6 +757,20 @@ func (c *Client) ImportSecureBootCertificate(ctx context.Context, database bmc.S return err } +// AllowCustomSecureBootKeys enables or disables the platform's out-of-band acceptance +// of custom UEFI Secure Boot keys. rebootRequired reports that the change is staged and +// takes effect only after a power cycle. +func (c *Client) AllowCustomSecureBootKeys(ctx context.Context, enable bool) (rebootRequired bool, err error) { + ctx, span := c.traceprovider.Tracer(pkgName).Start(ctx, "AllowCustomSecureBootKeys") + defer span.End() + + rebootRequired, metadata, err := bmc.AllowCustomSecureBootKeysFromInterfaces(ctx, c.registry().GetDriverInterfaces(), enable) + c.setMetadata(metadata) + metadata.RegisterSpanAttributes(c.Auth.Host, span) + + return rebootRequired, err +} + // FirmwareInstall pass through library function to upload firmware and install firmware func (c *Client) FirmwareInstall(ctx context.Context, component, operationApplyTime string, forceInstall bool, reader io.Reader) (taskID string, err error) { ctx, span := c.traceprovider.Tracer(pkgName).Start(ctx, "FirmwareInstall") diff --git a/providers/dell/idrac.go b/providers/dell/idrac.go index af4d9793..670959eb 100644 --- a/providers/dell/idrac.go +++ b/providers/dell/idrac.go @@ -56,6 +56,7 @@ var ( providers.FeatureResetSecureBootKeys, providers.FeatureResetSecureBootDatabaseKeys, providers.FeatureImportSecureBootCertificate, + providers.FeatureAllowCustomSecureBootKeys, } errManufacturerUnknown = errors.New("error identifying device manufacturer") @@ -108,12 +109,13 @@ func WithUseBasicAuth(useBasicAuth bool) Option { } } -// compile-time assertions that the provider implements the BIOS configuration interfaces. +// compile-time assertions that the provider implements these interfaces. var ( - _ bmc.BiosConfigurationGetter = (*Conn)(nil) - _ bmc.BiosConfigurationSetter = (*Conn)(nil) - _ bmc.HTTPBootURISetter = (*Conn)(nil) - _ bmc.NetworkBootEnabledSetter = (*Conn)(nil) + _ bmc.BiosConfigurationGetter = (*Conn)(nil) + _ bmc.BiosConfigurationSetter = (*Conn)(nil) + _ bmc.HTTPBootURISetter = (*Conn)(nil) + _ bmc.NetworkBootEnabledSetter = (*Conn)(nil) + _ bmc.CustomSecureBootKeysAllower = (*Conn)(nil) ) // Conn details for redfish client diff --git a/providers/dell/secure_boot.go b/providers/dell/secure_boot.go new file mode 100644 index 00000000..e8b97c12 --- /dev/null +++ b/providers/dell/secure_boot.go @@ -0,0 +1,57 @@ +package dell + +import ( + "context" + + "github.com/pkg/errors" + + bmclibErrs "github.com/bmc-toolbox/bmclib/v2/errors" +) + +// secureBootPolicyAttribute is Dell's vendor-specific BIOS attribute name and +// MUST NOT leak into any exported bmclib identifier. +const ( + secureBootPolicyAttribute = "SecureBootPolicy" + secureBootPolicyCustom = "Custom" + secureBootPolicyStandard = "Standard" +) + +// AllowCustomSecureBootKeys sets the SecureBootPolicy BIOS attribute to +// Custom or Standard. +// +// The attribute is read first only to reject platforms that don't expose it at all - not to +// skip the write when the currently *applied* value already matches what's requested. +// Currently-applied state can match while a different value is genuinely *pending* from an +// earlier call in the same boot cycle (e.g. a stale pending PATCH left staged by an interrupted, +// unrelated caller); confirmed live, skipping the write in that case silently left +// SecureBootPolicy staged as Standard despite a request to set it to Custom - the same class of +// bug stmcginnis/gofish#571 fixes one layer down, in SetBiosConfiguration's own diff baseline. +// This early return happens before SetBiosConfiguration is ever called, so #571 can't reach it. +// The write is staged into the +// Bios/Settings resource and only takes effect on the next POST, so a successful call always +// reports rebootRequired true. +// +// Implements bmc.CustomSecureBootKeysAllower. +func (c *Conn) AllowCustomSecureBootKeys(ctx context.Context, enable bool) (rebootRequired bool, err error) { + biosConfig, err := c.redfishwrapper.GetBiosConfiguration(ctx) + if err != nil { + return false, err + } + + if _, ok := biosConfig[secureBootPolicyAttribute]; !ok { + return false, bmclibErrs.NewErrUnsupportedHardware( + secureBootPolicyAttribute + " BIOS attribute not present: platform has no out-of-band Secure Boot key management control", + ) + } + + want := secureBootPolicyStandard + if enable { + want = secureBootPolicyCustom + } + + if err := c.redfishwrapper.SetBiosConfiguration(ctx, map[string]string{secureBootPolicyAttribute: want}); err != nil { + return false, errors.Wrapf(err, "failed to set %s", secureBootPolicyAttribute) + } + + return true, nil +} diff --git a/providers/dell/secure_boot_test.go b/providers/dell/secure_boot_test.go new file mode 100644 index 00000000..3d245480 --- /dev/null +++ b/providers/dell/secure_boot_test.go @@ -0,0 +1,249 @@ +package dell + +import ( + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + bmclibErrs "github.com/bmc-toolbox/bmclib/v2/errors" +) + +// biosWithSecureBootPolicy is a minimal Bios resource with an OnReset-capable +// @Redfish.Settings block, matching a real iDRAC. +const biosWithSecureBootPolicy = `{ + "@odata.type": "#Bios.v1_1_0.Bios", + "@odata.id": "/redfish/v1/Systems/System.Embedded.1/Bios", + "Id": "Bios", + "Name": "BIOS Configuration Current Settings", + "AttributeRegistry": "BiosAttributeRegistry.v1_0_3", + "Attributes": { + "SecureBootPolicy": "%s" + }, + "@Redfish.Settings": { + "@odata.type": "#Settings.v1_3_0.Settings", + "SettingsObject": { + "@odata.id": "/redfish/v1/Systems/System.Embedded.1/Bios/Settings" + }, + "SupportedApplyTimes": ["OnReset"] + } +}` + +// biosWithoutSecureBootPolicy models an older platform generation that +// doesn't expose the attribute. +const biosWithoutSecureBootPolicy = `{ + "@odata.type": "#Bios.v1_1_0.Bios", + "@odata.id": "/redfish/v1/Systems/System.Embedded.1/Bios", + "Id": "Bios", + "Name": "BIOS Configuration Current Settings", + "AttributeRegistry": "BiosAttributeRegistry.v1_0_3", + "Attributes": { + "BootMode": "Uefi" + } +}` + +func newSecureBootTestConn(t *testing.T, mux *http.ServeMux) *Conn { + t.Helper() + + server := httptest.NewTLSServer(mux) + t.Cleanup(server.Close) + + parsedURL, err := url.Parse(server.URL) + require.NoError(t, err) + + client := New(parsedURL.Hostname(), "", "", logr.Discard(), WithPort(parsedURL.Port()), WithUseBasicAuth(true)) + require.NoError(t, client.Open(context.Background())) + t.Cleanup(func() { _ = client.Close(context.Background()) }) + + return client +} + +func TestAllowCustomSecureBootKeys_EnableFromStandard(t *testing.T) { + var settingsPatched bool + var patchedBody string + + mux := http.NewServeMux() + mux.HandleFunc("/redfish/v1/", endpointFunc("/serviceroot.json")) + mux.HandleFunc("/redfish/v1/Systems", endpointFunc("/systems.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1", endpointFunc("/systems_embedded.1.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + _, _ = fmt.Fprintf(w, biosWithSecureBootPolicy, secureBootPolicyStandard) + }) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios/Settings", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + case http.MethodPatch: + settingsPatched = true + b, _ := io.ReadAll(r.Body) + patchedBody = string(b) + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), true) + require.NoError(t, err) + assert.True(t, rebootRequired) + assert.True(t, settingsPatched, "expected a BIOS settings job to be scheduled") + assert.Contains(t, patchedBody, secureBootPolicyCustom) +} + +// TestAllowCustomSecureBootKeys_EnableAlreadyCustom verifies the write happens when a different +// value is genuinely pending from an earlier, unrelated call in the same boot cycle, even though +// currently-applied state already matches what's requested (see the doc comment on +// AllowCustomSecureBootKeys) - skipping based on applied state alone would silently leave that +// stale pending value in place. +func TestAllowCustomSecureBootKeys_EnableAlreadyCustom(t *testing.T) { + var settingsPatched bool + + mux := http.NewServeMux() + mux.HandleFunc("/redfish/v1/", endpointFunc("/serviceroot.json")) + mux.HandleFunc("/redfish/v1/Systems", endpointFunc("/systems.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1", endpointFunc("/systems_embedded.1.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + _, _ = fmt.Fprintf(w, biosWithSecureBootPolicy, secureBootPolicyCustom) + }) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios/Settings", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + // A stale pending value from an earlier, unrelated call - genuinely differs from + // what's being requested here, even though applied state already matches. + _, _ = w.Write([]byte(`{"Attributes": {"SecureBootPolicy": "Standard"}}`)) + case http.MethodPatch: + settingsPatched = true + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), true) + require.NoError(t, err) + assert.True(t, rebootRequired) + assert.True(t, settingsPatched, "expected the write to happen to correct the stale pending value") +} + +// TestAllowCustomSecureBootKeys_DisableAlreadyStandard mirrors +// TestAllowCustomSecureBootKeys_EnableAlreadyCustom in the other direction: the bug is +// symmetric, so both currently-applied values need the same always-reach-the-write behavior. +func TestAllowCustomSecureBootKeys_DisableAlreadyStandard(t *testing.T) { + var settingsPatched bool + + mux := http.NewServeMux() + mux.HandleFunc("/redfish/v1/", endpointFunc("/serviceroot.json")) + mux.HandleFunc("/redfish/v1/Systems", endpointFunc("/systems.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1", endpointFunc("/systems_embedded.1.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + _, _ = fmt.Fprintf(w, biosWithSecureBootPolicy, secureBootPolicyStandard) + }) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios/Settings", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + // A stale pending value from an earlier, unrelated call - genuinely differs from + // what's being requested here, even though applied state already matches. + _, _ = w.Write([]byte(`{"Attributes": {"SecureBootPolicy": "Custom"}}`)) + case http.MethodPatch: + settingsPatched = true + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), false) + require.NoError(t, err) + assert.True(t, rebootRequired) + assert.True(t, settingsPatched, "expected the write to happen to correct the stale pending value") +} + +func TestAllowCustomSecureBootKeys_Disable(t *testing.T) { + var settingsPatched bool + var patchedBody string + + mux := http.NewServeMux() + mux.HandleFunc("/redfish/v1/", endpointFunc("/serviceroot.json")) + mux.HandleFunc("/redfish/v1/Systems", endpointFunc("/systems.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1", endpointFunc("/systems_embedded.1.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + _, _ = fmt.Fprintf(w, biosWithSecureBootPolicy, secureBootPolicyCustom) + }) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios/Settings", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + case http.MethodPatch: + settingsPatched = true + b, _ := io.ReadAll(r.Body) + patchedBody = string(b) + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusMethodNotAllowed) + } + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), false) + require.NoError(t, err) + assert.True(t, rebootRequired) + assert.True(t, settingsPatched, "expected a BIOS settings job to be scheduled") + assert.Contains(t, patchedBody, secureBootPolicyStandard) +} + +func TestAllowCustomSecureBootKeys_AttributeAbsent(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/redfish/v1/", endpointFunc("/serviceroot.json")) + mux.HandleFunc("/redfish/v1/Systems", endpointFunc("/systems.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1", endpointFunc("/systems_embedded.1.json")) + mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + _, _ = w.Write([]byte(biosWithoutSecureBootPolicy)) + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), true) + require.Error(t, err) + assert.False(t, rebootRequired) + + var unsupported *bmclibErrs.ErrUnsupportedHardware + assert.ErrorAs(t, err, &unsupported) +} diff --git a/providers/lenovo/secure_boot.go b/providers/lenovo/secure_boot.go index f27b0e00..1f48d2c0 100644 --- a/providers/lenovo/secure_boot.go +++ b/providers/lenovo/secure_boot.go @@ -36,6 +36,18 @@ func (c *Conn) ResetSecureBootDatabaseKeys(ctx context.Context, database bmc.Sec // ImportSecureBootCertificate enrolls a certificate into a single UEFI Secure Boot key database. // +// Known gap: on Lenovo XCC-based systems this fails with a Lenovo-native +// Redfish error (documented by Lenovo as FQXSFPU4097G) unless the +// SecureBootConfiguration.SecureBootPolicy BIOS attribute is already +// "Custom Policy" - Lenovo's direct analog of Dell's SecureBootPolicy +// (Standard/Custom). Unlike Dell, this provider does not implement +// bmc.CustomSecureBootKeysAllower to flip that attribute out-of-band, +// because it is unconfirmed whether SecureBootPolicy is reachable through +// the generic /Bios attribute PATCH this package's Get/SetBiosConfiguration +// use, or requires Lenovo's proprietary OneCLI/XCC transport instead. See +// https://pubs.lenovo.com/uefi_edge_v2/secure_boot_configuration and +// https://pubs.lenovo.com/se360-v2/FQXSFPU4097G. +// // Implements bmc.SecureBootCertificateImporter. func (c *Conn) ImportSecureBootCertificate(ctx context.Context, database bmc.SecureBootDatabase, certificatePEM string) (err error) { return c.redfishwrapper.ImportSecureBootCertificate(ctx, database, certificatePEM) diff --git a/providers/providers.go b/providers/providers.go index d79d8dad..87e79995 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -98,4 +98,7 @@ const ( // FeatureImportSecureBootCertificate means an implementation that can enroll a certificate into a single UEFI Secure Boot key database FeatureImportSecureBootCertificate registrar.Feature = "importsecurebootcertificate" + + // FeatureAllowCustomSecureBootKeys means an implementation that can enable/disable acceptance of custom UEFI Secure Boot keys + FeatureAllowCustomSecureBootKeys registrar.Feature = "allowcustomsecurebootkeys" )