daemon: secureboot updates API endpoint #14541

bboozzoo · 2024-09-26T09:04:01Z

Add a dedicated integration API endpoint for controlling/notifying of secure boot keys databases updates.

This is a draft PR with no unit tests, with the purpose of validating the API endpoint and the policy around it.

Related: SNAPDENG-32364 SNAPDENG-31856

pedronis

thanks, looking reasonable, couple of comments

pedronis · 2024-09-30T09:23:37Z

daemon/api_system_secureboot.go

+	}
+
+	switch req.Action {
+	case "efi-secureboot-db-update":


having action and subaction is a bit unusual, also at least the startup action might be relevant to more types of fw updates than db-updates? can't we fold things into actions:

efi-secureboot-update-startup
efi-secureboot-update-db-prepare
efi-secureboot-update-db-cleanup?

is cleanup perhaps also relevant to other kind of updates?

pedronis · 2024-09-30T09:24:33Z

daemon/api_system_secureboot.go

+		// TODO should this be only allowed for services with fwupd on slot
+		// side?


yes, as disussed we care about the slot side here, not the plug

pedronis · 2024-09-30T09:26:34Z

daemon/api_system_secureboot.go

+var systemSecurebootCmd = &Command{
+	// TODO GET returning whether secure boot is relevant for the system?
+
+	Path: "/v2/system/secureboot",


looking at other end points it seems this might need to be /v2/system-secureboot ?

I was wondering if there's a use case for exposing other properties of the system, eg disks (or storage), gadget, in which case those could be hosted at /v2/system/disks, and so on. wdyt?

pedronis

nitpick about the JSON of the requests

pedronis · 2024-10-02T16:53:54Z

daemon/api_system_secureboot.go

+func postSystemSecurebootActionJSON(c *Command, r *http.Request) Response {
+	var req struct {
+		Action string           `json:"action,omitempty"`
+		Data   *json.RawMessage `json:"data"`


I would avoid having this nested "data" level, just define a top level struct with the field we need with a validate method that checks if field that should be set for a given op are or than't shouldn't aren't

pedronis

thanks, small question

pedronis · 2024-10-03T09:46:23Z

daemon/access.go

+	// Slot when true, the snap must appear on the slot side
+	Slot bool
+	// Plug when true, the snap must appear on the plug side
+	Plug bool


do we need to support setting both to true because the logic below and the descriptions here don't match in that case

If we have the use case for it then I think the we need a more complex structure that holds expectation for optional slot-side and plug-side interfaces for each combo we want to allow.

type interfaceAccessReq struct { PlugInterface string // If set then snap must have a connected plug for specified interface SlotInterface string // If set then snap must have a connected slot for specified interface }

Then func requireInterfaceApiAccessImpl(d *Daemon, r *http.Request, ucred *ucrednet, req interfaceAccessReqs) *apiError would be func requireInterfaceApiAccessImpl(d *Daemon, r *http.Request, ucred *ucrednet, reqs []interfaceAccessReq) *apiError

This adds a lot of complexity, If we don't have the use case now then maybe better as a todo.

ZeyadYasser

Looks really good, Thank you! I did a first pass

overlord/fdestate/fdestate.go

ZeyadYasser · 2024-10-04T08:02:03Z

overlord/fdestate/fdestate.go

+
+// EFISecureBootDBUpdatePrepare notifies notifies that the local EFI key
+// database manager is about to update the database.
+func EFISecureBootDBUpdatePrepare(st *state.State, db EFISecurebooKeystDB, payload []byte) error {


To avoid redoing work on the daemon side, Is there an agreement that those stubbed functions are expected to be there with similar signatures in the actual fdestate package?

no, we don't have the feature yet, so this is best effort guess of what the API may look like

then maybe @valentindavid can take a look if the function signatures make sense

ZeyadYasser · 2024-10-04T08:17:23Z

daemon/access.go

+	// Slot when true, the snap must appear on the slot side
+	Slot bool
+	// Plug when true, the snap must appear on the plug side
+	Plug bool


If we have the use case for it then I think the we need a more complex structure that holds expectation for optional slot-side and plug-side interfaces for each combo we want to allow.

type interfaceAccessReq struct { PlugInterface string // If set then snap must have a connected plug for specified interface SlotInterface string // If set then snap must have a connected slot for specified interface }

Then func requireInterfaceApiAccessImpl(d *Daemon, r *http.Request, ucred *ucrednet, req interfaceAccessReqs) *apiError would be func requireInterfaceApiAccessImpl(d *Daemon, r *http.Request, ucred *ucrednet, reqs []interfaceAccessReq) *apiError

This adds a lot of complexity, If we don't have the use case now then maybe better as a todo.

daemon/access.go

daemon/export_api_system_secureboot_test.go

ZeyadYasser · 2024-10-04T09:12:12Z

daemon/export_api_system_secureboot_test.go

+) (restore func()) {
+	restore = testutil.Backup(&fdestateEFISecureBootDBUpdateCleanup)
+	fdestateEFISecureBootDBUpdateCleanup = f
+	return restore


same as above

daemon/export_api_system_secureboot_test.go

ZeyadYasser

Thank you!

ZeyadYasser · 2024-10-04T11:02:22Z

daemon/api_system_secureboot.go

+		switch r.DB {
+		case "PK", "KEK", "DB", "DBX":
+		default:
+			return fmt.Errorf("invalid key DB %q", r.DB)


maybe error can include supported DBs?

bboozzoo · 2024-10-04T14:20:44Z

overlord/fdestate/fdestate.go

+	return errNotImplemented
+}
+
+type EFISecurebootKeysDB int


this could be named EFISecurebootKeysDatabase to avoid confusion with DB which is the name of the allowed signature key hashes database.

bboozzoo · 2024-10-04T14:21:48Z

daemon/api_system_secureboot.go

+	// DB is used with efi-secureboot-db-prepare action, and indicates the
+	// secureboot keys DB which is a target of the action, possible values are
+	// PK, KEK, DB, DBX
+	DB string `json:"db,omitempty"`


similarly to the other comment in fdestate this could be named:

Suggested change

DB string `json:"db,omitempty"`

Database string `json:"database,omitempty"`

@valentindavid pushed the changes we discussed, please have a look

Stubs Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

… process being root Add a new access checker which verifies that the request is coming from a root user and if the process is a snap, a required interface is connected, with that snap present on the slot side. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

…ureboot key DBs Add a new endpoint for integration with a local manager of EFI secureboot key databases. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo added the FDE Manager Pull requests that target FDE manager branch label Sep 26, 2024

bboozzoo requested review from valentindavid and pedronis September 26, 2024 09:04

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Sep 26, 2024

bboozzoo mentioned this pull request Sep 26, 2024

o/hookstate/ctlcmd, o/fdestate: add efi-secureboot-db-update command, fdestate stubs #14535

Closed

pedronis reviewed Sep 30, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/efi-secureboot-api branch from 68afd04 to 70eed3e Compare October 1, 2024 14:05

bboozzoo marked this pull request as ready for review October 1, 2024 14:06

bboozzoo requested review from pedronis and chrisccoulson October 1, 2024 14:06

pedronis reviewed Oct 2, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/efi-secureboot-api branch from 70eed3e to 1bb4610 Compare October 3, 2024 08:33

bboozzoo requested a review from pedronis October 3, 2024 08:35

pedronis approved these changes Oct 3, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/efi-secureboot-api branch from 1bb4610 to 5ac8233 Compare October 3, 2024 10:32

bboozzoo requested a review from ZeyadYasser October 3, 2024 11:02

ZeyadYasser reviewed Oct 4, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/efi-secureboot-api branch from 5ac8233 to 9d631d1 Compare October 4, 2024 10:30

bboozzoo requested a review from ZeyadYasser October 4, 2024 10:30

ZeyadYasser approved these changes Oct 4, 2024

View reviewed changes

bboozzoo commented Oct 4, 2024

View reviewed changes

bboozzoo added 3 commits October 4, 2024 16:51

overlord/fdestate: stubs for EFI secure boot keys database updates

f088c21

Stubs Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

daemon: add /v2/system-secureboot endpoint for pushing updates to sec…

c6b94bc

…ureboot key DBs Add a new endpoint for integration with a local manager of EFI secureboot key databases. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo force-pushed the bboozzoo/efi-secureboot-api branch from 9d631d1 to c6b94bc Compare October 4, 2024 15:00

pedronis merged commit 0a211fd into canonical:fde-manager-features Oct 7, 2024
41 of 52 checks passed

bboozzoo deleted the bboozzoo/efi-secureboot-api branch October 7, 2024 09:17

bboozzoo mentioned this pull request Oct 7, 2024

plugins: snapd integration for UEFI DBX updates fwupd/fwupd#7861

Draft

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: secureboot updates API endpoint #14541

daemon: secureboot updates API endpoint #14541

bboozzoo commented Sep 26, 2024

pedronis left a comment

pedronis Sep 30, 2024 •

edited

Loading

pedronis Sep 30, 2024

pedronis Sep 30, 2024

bboozzoo Sep 30, 2024

pedronis left a comment

pedronis Oct 2, 2024

bboozzoo Oct 3, 2024

pedronis left a comment

pedronis Oct 3, 2024

ZeyadYasser Oct 4, 2024

ZeyadYasser left a comment •

edited

Loading

ZeyadYasser Oct 4, 2024 •

edited

Loading

bboozzoo Oct 4, 2024

ZeyadYasser Oct 4, 2024

ZeyadYasser Oct 4, 2024

ZeyadYasser Oct 4, 2024

ZeyadYasser left a comment

ZeyadYasser Oct 4, 2024

bboozzoo Oct 4, 2024

bboozzoo Oct 4, 2024

bboozzoo Oct 4, 2024

		// TODO should this be only allowed for services with fwupd on slot
		// side?

	DB string `json:"db,omitempty"`
	Database string `json:"database,omitempty"`

daemon: secureboot updates API endpoint #14541

daemon: secureboot updates API endpoint #14541

Conversation

bboozzoo commented Sep 26, 2024

pedronis left a comment

Choose a reason for hiding this comment

pedronis Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser left a comment • edited Loading

Choose a reason for hiding this comment

ZeyadYasser Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Sep 30, 2024 •

edited

Loading

ZeyadYasser left a comment •

edited

Loading

ZeyadYasser Oct 4, 2024 •

edited

Loading