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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

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.28%. Comparing base (24a0034) to head (37063a5).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14723      +/-   ##
==========================================
+ Coverage   78.20%   78.28%   +0.07%     
==========================================
  Files        1151     1153       +2     
  Lines      151396   152156     +760     
==========================================
+ Hits       118402   119114     +712     
- Misses      25662    25686      +24     
- Partials     7332     7356      +24     
Flag Coverage Δ
unittests 78.28% <100.00%> (+0.07%) ⬆️

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

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.

3 participants