From ccac3bde33600d42e47c5c39b5f2b53b5e08d337 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Can=C3=A9vet?= Date: Thu, 17 Sep 2026 16:17:01 +0200 Subject: [PATCH 1/4] feat: add CustomSecureBootKeysAllower for out-of-band custom key acceptance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New interface controlling whether a platform will accept custom UEFI Secure Boot keys, as opposed to only the vendor-shipped key set. Distinct from SecureBootSetter (enabling/disabling Secure Boot itself) and from SecureBootCertificateImporter (enrolling a specific certificate) - it's the out-of-band gate some vendors require before a certificate import will be accepted at all. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Mickaël Canévet --- bmc/secure_boot.go | 83 +++++++++++++++++++++++++++++++++++++++++ bmc/secure_boot_test.go | 57 ++++++++++++++++++++++++++++ client.go | 14 +++++++ providers/providers.go | 3 ++ 4 files changed, 157 insertions(+) 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/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" ) From 17c947c4882448e14e6d15b1ce78bcd6cd82bc1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Can=C3=A9vet?= Date: Thu, 17 Sep 2026 16:17:29 +0200 Subject: [PATCH 2/4] feat(dell): wire CustomSecureBootKeysAllower MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements bmc.CustomSecureBootKeysAllower via the SecureBootPolicy BIOS attribute (Standard/Custom). Co-Authored-By: Claude Sonnet 5 Signed-off-by: Mickaël Canévet --- providers/dell/idrac.go | 12 +- providers/dell/secure_boot.go | 62 +++++++++ providers/dell/secure_boot_test.go | 194 +++++++++++++++++++++++++++++ 3 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 providers/dell/secure_boot.go create mode 100644 providers/dell/secure_boot_test.go 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..aa95e54b --- /dev/null +++ b/providers/dell/secure_boot.go @@ -0,0 +1,62 @@ +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 currently *applied* value is read first, and a request matching it returns early without +// writing. This only helps when nothing has touched SecureBootPolicy since the last reboot: +// applied state doesn't change until then no matter how many times this is called in between, so +// back-to-back calls in the same boot cycle still write every time regardless of this check. +// +// Known limitation: because the check reads applied state, not pending state, it cannot tell a +// genuinely-already-satisfied request apart from one where a *different* value is already staged +// as pending from an earlier, unrelated call in the same boot cycle - in that case this returns +// early and leaves the stale pending value in place instead of correcting it. The write, when +// attempted, is staged into the Bios/Settings resource and only takes effect on the next POST, so +// a successful change 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 + } + + current, ok := biosConfig[secureBootPolicyAttribute] + if !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 current == want { + return false, nil + } + + 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..b123762a --- /dev/null +++ b/providers/dell/secure_boot_test.go @@ -0,0 +1,194 @@ +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) +} + +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) { + settingsPatched = true + w.WriteHeader(http.StatusOK) + }) + + client := newSecureBootTestConn(t, mux) + + rebootRequired, err := client.AllowCustomSecureBootKeys(context.Background(), true) + require.NoError(t, err) + assert.False(t, rebootRequired) + assert.False(t, settingsPatched, "no BIOS settings job should be scheduled when already Custom") +} + +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) +} From fbdb6f9e1d9e8993ba5106749cae989e0870ee21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Can=C3=A9vet?= Date: Thu, 17 Sep 2026 16:17:45 +0200 Subject: [PATCH 3/4] docs(lenovo): document the CustomSecureBootKeysAllower gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XCC-based systems have an analogous SecureBootPolicy attribute, but it's unconfirmed whether it's reachable through the same generic /Bios attribute PATCH this package already uses, or requires Lenovo's proprietary OneCLI/XCC transport - left undocumented otherwise, this is the FQXSFPU4097G error code callers will hit without it. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Mickaël Canévet --- providers/lenovo/secure_boot.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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) From 0964644b4414620c8782a1a9f6f231dc6bc0b692 Mon Sep 17 00:00:00 2001 From: Mickael Canevet Date: Thu, 10 Sep 2026 17:05:46 +0200 Subject: [PATCH 4/4] fix(dell): don't skip the SecureBootPolicy write when applied state matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AllowCustomSecureBootKeys read the attribute's currently applied value and returned early when it already matched what was requested. Confirmed live, that's unsafe: 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). Skipping the write left that stale pending value in place, silently reverting an explicit request on the next reboot - observed as a certificate import rejected because SecureBootPolicy committed as Standard despite requesting Custom moments earlier. It's the same class of bug stmcginnis/gofish#571 fixes one layer down, in SetBiosConfiguration's own diff baseline - but this early return happens before SetBiosConfiguration is ever called, so #571 can't reach it. The attribute is still read first, but only to reject platforms that don't expose it at all. The write itself is now unconditional. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Mickaël Canévet --- providers/dell/secure_boot.go | 29 ++++++-------- providers/dell/secure_boot_test.go | 63 ++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 21 deletions(-) diff --git a/providers/dell/secure_boot.go b/providers/dell/secure_boot.go index aa95e54b..e8b97c12 100644 --- a/providers/dell/secure_boot.go +++ b/providers/dell/secure_boot.go @@ -19,17 +19,17 @@ const ( // AllowCustomSecureBootKeys sets the SecureBootPolicy BIOS attribute to // Custom or Standard. // -// The currently *applied* value is read first, and a request matching it returns early without -// writing. This only helps when nothing has touched SecureBootPolicy since the last reboot: -// applied state doesn't change until then no matter how many times this is called in between, so -// back-to-back calls in the same boot cycle still write every time regardless of this check. -// -// Known limitation: because the check reads applied state, not pending state, it cannot tell a -// genuinely-already-satisfied request apart from one where a *different* value is already staged -// as pending from an earlier, unrelated call in the same boot cycle - in that case this returns -// early and leaves the stale pending value in place instead of correcting it. The write, when -// attempted, is staged into the Bios/Settings resource and only takes effect on the next POST, so -// a successful change always reports rebootRequired true. +// 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) { @@ -38,8 +38,7 @@ func (c *Conn) AllowCustomSecureBootKeys(ctx context.Context, enable bool) (rebo return false, err } - current, ok := biosConfig[secureBootPolicyAttribute] - if !ok { + 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", ) @@ -50,10 +49,6 @@ func (c *Conn) AllowCustomSecureBootKeys(ctx context.Context, enable bool) (rebo want = secureBootPolicyCustom } - if current == want { - return false, nil - } - if err := c.redfishwrapper.SetBiosConfiguration(ctx, map[string]string{secureBootPolicyAttribute: want}); err != nil { return false, errors.Wrapf(err, "failed to set %s", secureBootPolicyAttribute) } diff --git a/providers/dell/secure_boot_test.go b/providers/dell/secure_boot_test.go index b123762a..3d245480 100644 --- a/providers/dell/secure_boot_test.go +++ b/providers/dell/secure_boot_test.go @@ -104,6 +104,11 @@ func TestAllowCustomSecureBootKeys_EnableFromStandard(t *testing.T) { 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 @@ -119,16 +124,66 @@ func TestAllowCustomSecureBootKeys_EnableAlreadyCustom(t *testing.T) { _, _ = fmt.Fprintf(w, biosWithSecureBootPolicy, secureBootPolicyCustom) }) mux.HandleFunc("/redfish/v1/Systems/System.Embedded.1/Bios/Settings", func(w http.ResponseWriter, r *http.Request) { - settingsPatched = true - w.WriteHeader(http.StatusOK) + 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.False(t, rebootRequired) - assert.False(t, settingsPatched, "no BIOS settings job should be scheduled when already Custom") + 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) {