Skip to content

Conversation

@BAMF0
Copy link
Contributor

@BAMF0 BAMF0 commented Dec 11, 2025

Note: this PR depends on proposed changes made in another PR. Once the first one has landed, this branch will be rebased onto master to reflect the relevant additions and changes. As such, the current scope in terms of additions, removals, and files changed is to be considered misleading.

Overview

This PR is one of three with the intention of extending the secureboot API to accept DB, KEK, and PK updates in addition to DBX:

  1. Refactor to generalize DBX-related occurrences, mentions, and messages (overlord/fdestate: refactor DBX naming to be more generic #16362)
  2. Extend backend unit-tests to include DB, KEK, and PK (overlord/fdestate: extend secureboot update unit tests for DB, KEK, and PK #16364)
  3. Extend API to allow for DB, KEK, and PK calls with API unit and spread tests <- (this PR)

Proposed changes

This PR will change the validation check in the secureboot API to accept DB, KEK, and PK key databases in addition to DBX. It will also include API unit tests for these key databases and add additional spread tests similar to
tests/nested/manual/hybrid-fde-dbx.

Up until now, only DBX has been supported in the snapd API. As such
all logging and function calls have assumed that the update relates
to DBX. This commit will generalize the usage of DBX to include DB,
KEK, and PK as well.

Additionally, this commit will refactor function names to indicate
that they do not just affect DBX.
* overlord/fdestate/secure_boot_update.go: Remove trailing whitespace
  on lines 87-88.
* overlord/fdestate/secure_boot_update_test.go: Fix inconsistent
  argument alignment on line 1235.
The `FromString` and `IsSupportedString` methods will also
be adapted to return an error and moved to the daemon API.
Copilot AI review requested due to automatic review settings December 11, 2025 16:52
@github-actions github-actions bot added Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run only one system Only runs spread tests on one system labels Dec 11, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the secureboot API endpoint to accept DB, KEK, and PK key database updates in addition to DBX. It's the third part of a three-part series that generalizes the DBX-specific secureboot update functionality to support all four UEFI Secure Boot key databases (PK, KEK, DB, and DBX). The changes include API validation updates, comprehensive unit test coverage for all key database types, and necessary refactoring of function names and error messages.

Key Changes

  • Added validation and conversion functions for all four key database types (PK, KEK, DB, DBX) in the API layer
  • Extended unit test coverage to test all key database types with parameterized test helpers
  • Updated function names from EFISecureBootDB* to EFISecurebootDB* for consistency

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
daemon/api_system_secureboot.go Added keyDatabaseFromString() and isValidKeyDatabase() functions to support validation of all four key database types; updated function names for consistency
daemon/api_system_secureboot_test.go Extended test coverage with parameterized helper functions to test PK, KEK, DB, and DBX operations separately
daemon/export_api_system_secureboot_test.go Updated mock function names to match the corrected EFISecureboot* naming convention
overlord/fdestate/secure_boot_update.go Added String() method for EFISecurebootKeyDatabase enum; updated function names, error messages, and logging to be generic rather than DBX-specific
overlord/fdestate/secure_boot_update_test.go Extended comprehensive test coverage with parameterized test helpers for all four key database types; updated error message assertions to be dynamic based on key database type
overlord/fdestate/conflict.go Updated conflict checking to accept key database parameter; generalized error messages and comments from DBX-specific to generic Secureboot Key Database
overlord/fdestate/conflict_test.go Updated test expectations for generalized error messages
overlord/fdestate/fdestate_test.go Updated test expectations for generalized error messages
overlord/fdestate/export_test.go Updated exported test helper function names to match new naming convention
Comments suppressed due to low confidence (4)

overlord/fdestate/secure_boot_update_test.go:1141

  • The comment still refers to "external DBX manager process" but should be updated to refer to "external Secureboot Key Database manager process" or similar, since this test now applies to PK, KEK, DB, and DBX updates.
    overlord/fdestate/secure_boot_update_test.go:1521
  • The comment still refers to "external DBX manager process" but should be updated to refer to "external Secureboot Key Database manager process" or similar, since this test now applies to PK, KEK, DB, and DBX updates.
    overlord/fdestate/secure_boot_update_test.go:1422
  • Typo: "ant" should be "and".
    overlord/fdestate/secure_boot_update_test.go:1492
  • Typo: "ant" should be "and".

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

❌ Patch coverage is 85.56701% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.50%. Comparing base (4fc4cfa) to head (95316b2).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
daemon/api_system_secureboot.go 75.00% 3 Missing and 4 partials ⚠️
overlord/fdestate/secure_boot_update.go 91.37% 5 Missing ⚠️
overlord/fdestate/conflict.go 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16368      +/-   ##
==========================================
+ Coverage   77.47%   77.50%   +0.03%     
==========================================
  Files        1339     1338       -1     
  Lines      182910   183041     +131     
  Branches     2438     2438              
==========================================
+ Hits       141705   141864     +159     
+ Misses      32613    32588      -25     
+ Partials     8592     8589       -3     
Flag Coverage Δ
unittests 77.50% <85.56%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@github-actions
Copy link

github-actions bot commented Dec 11, 2025

Fri Dec 12 18:59:46 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/20173651031

Failures:

Executing:

  • openstack-ps7:ubuntu-24.04-64:tests/main/try
  • openstack-ps7:ubuntu-24.04-64:tests/main/classic-confinement-perms
  • openstack-ps7:ubuntu-24.04-64:tests/main/apparmor-prompting-integration-tests:download_file_defaults

Restoring:

  • openstack-ps7:ubuntu-24.04-64:tests/main/apparmor-prompting-integration-tests:download_file_defaults

A matched error message was not updated correctly in the
nested core20-fde-dbx spread test.
Attempt to generalize API testing for easy extension of multiple
different types of updates instead of only testing DBX.

The proposed test design defines a general private method
that is called by a specific public method for each update type.
Initially, an attempt was made to utilize table driven tests,
however separate test functions turned out to be the most straight-
forward way of restoring state between tests.
@BAMF0 BAMF0 force-pushed the fr-12065-pt3-extend-secureboot-api branch from f170bf9 to 95316b2 Compare December 12, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FDE Marathon Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run only one system Only runs spread tests on one system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants