Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

o/devicestate,asserts: sign confdb-control assertions with the device key #14723

Merged
merged 7 commits into from
Jan 8, 2025

Conversation

st3v3nmw
Copy link
Contributor

@st3v3nmw st3v3nmw commented Nov 14, 2024

The process to update a confdb-control assertion (see SD172 & SD186) looks like this:

  1. Call /v2/confdbs with an action like delegate or revoke
  2. The assertion is updated as required
  3. The assertion is signed with the device key
  4. The new revision of the assertion is acknowledged

This PR adds a method to devicestate.DeviceManager that can be used to sign confdb-control assertions.

Its usage would (roughly) look like:

daemon/api_confdbs:

func doDelegateOrRevoke(c *Command, r *http.Request, _ *auth.UserState) Response {
	// request validation
	[...]

	// the existing assertion is updated
	[...]
        groups = existingAs.Groups()
	revision = existingAs.Revision() + 1

	// lock device state
	state := c.d.overlord.State()
	state.Lock()
	defer state.Unlock()

	// sign the assertion
	deviceMgr := c.d.overlord.DeviceManager()
	newAs, err = deviceMgr.SignConfdbControl(groups, revision)

	// acknowledge the new revision
	client.Ack(asserts.Encode(newAs))
}

CC @miguelpires @pedronis

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.22%. Comparing base (24a0034) to head (b92e3ff).
Report is 91 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14723      +/-   ##
==========================================
+ Coverage   78.20%   78.22%   +0.01%     
==========================================
  Files        1151     1157       +6     
  Lines      151396   152919    +1523     
==========================================
+ Hits       118402   119623    +1221     
- Misses      25662    25909     +247     
- Partials     7332     7387      +55     
Flag Coverage Δ
unittests 78.22% <100.00%> (+0.01%) ⬆️

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.

@st3v3nmw st3v3nmw marked this pull request as ready for review November 15, 2024 09:50
@bboozzoo bboozzoo requested a review from miguelpires November 15, 2024 15:00
Copy link
Contributor

@miguelpires miguelpires left a comment

Choose a reason for hiding this comment

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

this can be locked down to only sign confdb-control assertions the way SignDeviceSessionRequest does... but we'd have to wait for #14705 to go in first.

I'd say that's the better approach. 14705 is close to landing anyway so it should be fine

@pedronis pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Nov 20, 2024
@st3v3nmw
Copy link
Contributor Author

st3v3nmw commented Dec 3, 2024

I'd say that's the better approach. 14705 is close to landing anyway so it should be fine

Thanks. I'll refactor this after #14705 lands.

@st3v3nmw st3v3nmw changed the title o/devicestate: add general method to sign no-authority assertions o/devicestate: add method to sign confdb-control assertions Dec 10, 2024
@st3v3nmw st3v3nmw marked this pull request as draft December 10, 2024 09:53
@st3v3nmw st3v3nmw force-pushed the sign-assertion-with-device-key branch from be04655 to 4ae7fad Compare December 10, 2024 10:28
@st3v3nmw st3v3nmw changed the title o/devicestate: add method to sign confdb-control assertions o/devicestate,asserts: sign confdb-control assertions with the device key Dec 10, 2024
@st3v3nmw st3v3nmw force-pushed the sign-assertion-with-device-key branch from 52f3add to e097810 Compare December 10, 2024 12:49
@st3v3nmw st3v3nmw marked this pull request as ready for review December 10, 2024 12:49
@st3v3nmw st3v3nmw requested a review from miguelpires December 10, 2024 14:06
@pedronis pedronis self-requested a review December 10, 2024 15:04
Copy link
Contributor

@miguelpires miguelpires left a comment

Choose a reason for hiding this comment

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

thanks, some comments

c.Assert(cc.Revision(), Equals, 5)

// Confirm we can ack it
// AddMany panics on error, that's why we aren't c.Assert'ing anything
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mentioned in AddMany's doc and we already call it above, so I don't think we need this comment

Suggested change
// AddMany panics on error, that's why we aren't c.Assert'ing anything

}

// signKey returns the public key of the device that signed this assertion.
func (cc *ConfdbControl) signKey(db RODatabase) (PublicKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this requires a new interface, it's just a different implementation but the customSigner abstraction still works. We can add an RODatabase parameter to customSigner and have ConfdbControl implement that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the changes to customSigner.

// validation failure
groups := []interface{}{map[string]interface{}{"operator-id": "jane"}}
_, err = s.mgr.SignConfdbControl(groups, 4)
c.Assert(
Copy link
Contributor

Choose a reason for hiding this comment

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

The failing validation checks should be in individual tests. Maybe they can be in a single table test if doesn't make it too annoying to deal with the prerequisites for each case

Copy link
Contributor

@miguelpires miguelpires left a comment

Choose a reason for hiding this comment

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

lgtm, thank you

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you, did a pass


pubKey, err = custom.signKey(roDB)
if err != nil {
return fmt.Errorf("cannot check signature: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this error needs to be a bit more specific I think:

cannot check no-authority assertion type %q: %w

perhaps

serial := a.(*Serial)
key := serial.DeviceKey()
if key.ID() != cc.SignKeyID() {
return nil, errors.New("confdb-control's signing key doesn't match the device's key")
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/device's key/device key/

"serial": cc.Serial(),
})
if err != nil {
return nil, fmt.Errorf("cannot find matching serial: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/serial/device serial assertion/

func (m *DeviceManager) SignConfdbControl(groups []interface{}, revision int) (*asserts.ConfdbControl, error) {
serial, err := m.Serial()
if err != nil {
return nil, fmt.Errorf("cannot sign confdb-control without a serial: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the %w, err bits here don't add more information

@pedronis pedronis self-requested a review January 7, 2025 14:34
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

@st3v3nmw st3v3nmw force-pushed the sign-assertion-with-device-key branch from 8327094 to bde530a Compare January 8, 2025 06:49
@st3v3nmw st3v3nmw force-pushed the sign-assertion-with-device-key branch from bde530a to b92e3ff Compare January 8, 2025 07:05
@st3v3nmw
Copy link
Contributor Author

st3v3nmw commented Jan 8, 2025

The failing spread tests are unrelated to the changes:

  • openstack:centos-9-64: cannot create instance: request (https://nova.ps6.canonical.com:8774/v2.1/servers) returned unexpected status: 400; error info: {"badRequest": {"code": 400, "message": "Block Device Mapping is Invalid: failed to get snapshot 967396d5-37eb-4acc-8cf4-50118d93d2ef."}}
  • Cannot allocate openstack:fedora-41-64: cannot allocate new Openstack server jan080720-211353: timeout waiting for openstack:fedora-41-64 (jan080720-211353) to provision
  • openstack:opensuse-tumbleweed-64: sh -c microk8s kubectl get pods | MATCH Running failed with code 1 - ImagePullBackOff for nginx
  • google-arm:ubuntu-20.04-arm-64:tests/main/auto-refresh-gating: expected a new auto-refresh change with id greater than 1, but it didn'''t happen
  • google:ubuntu-16.04-64: (can't install packages) - Temporary failure resolving 'archive.ubuntu.com', unable to contact snap store
  • google:ubuntu-20.04-64:tests/main/interfaces-microstack-support (failing cause of a missing file that's expected by the spread test)
  • Error retrieving serial output: failed to retrieve the serial console for server openstack:debian-12-64 (jan080720-910592): timeout reached

@ernestl ernestl merged commit a24fe7a into canonical:master Jan 8, 2025
52 of 59 checks passed
@st3v3nmw st3v3nmw deleted the sign-assertion-with-device-key branch January 8, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants