-
Notifications
You must be signed in to change notification settings - Fork 590
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
o/devicestate,asserts: sign confdb-control assertions with the device key #14723
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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
Thanks. I'll refactor this after #14705 lands. |
be04655
to
4ae7fad
Compare
52f3add
to
e097810
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
// 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you
There was a problem hiding this 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
asserts/database.go
Outdated
|
||
pubKey, err = custom.signKey(roDB) | ||
if err != nil { | ||
return fmt.Errorf("cannot check signature: %w", err) |
There was a problem hiding this comment.
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
asserts/confdb.go
Outdated
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") |
There was a problem hiding this comment.
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/
asserts/confdb.go
Outdated
"serial": cc.Serial(), | ||
}) | ||
if err != nil { | ||
return nil, fmt.Errorf("cannot find matching serial: %w", err) |
There was a problem hiding this comment.
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/
overlord/devicestate/devicemgr.go
Outdated
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
8327094
to
bde530a
Compare
bde530a
to
b92e3ff
Compare
The failing spread tests are unrelated to the changes:
|
The process to update a
confdb-control
assertion (see SD172 & SD186) looks like this:/v2/confdbs
with an action likedelegate
orrevoke
This PR adds a method to
devicestate.DeviceManager
that can be used to signconfdb-control
assertions.Its usage would (roughly) look like:
daemon/api_confdbs
:CC @miguelpires @pedronis