o/devicemgmtstate,o/assertstate: add task to validate request messages - #17273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds snapd-side validation for incoming remote device-management request-message assertions, ensuring messages are targeted to the current device, fall within their validity window, and pass subsystem-specific validation before any subsystem Apply() work begins. This fits into the new device management manager pipeline by tightening the pre-dispatch gatekeeping step.
Changes:
- Implement
validate-mgmt-messagetask logic (device targeting, time window checks, handler lookup, and handlerValidate()including unauthorized vs rejected outcomes). - Extend the persisted/request message model to include
assumes, and introduce a device-identity provider dependency for obtaining the current device ID. - Add unit tests covering successful validation and the main rejection/unauthorized paths, plus idempotence for repeated validation failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
overlord/overlord.go |
Updates manager construction to pass identity + signer dependencies into device management manager. |
overlord/devicemgmtstate/export_test.go |
Exposes identity provider type and adds a test helper to mock the identity provider. |
overlord/devicemgmtstate/devicemgmtmgr.go |
Implements doValidateMessage, adds Assumes to RequestMessage, and adds helpers for targeting/time validation plus an UnauthorizedError type. |
overlord/devicemgmtstate/devicemgmtmgr_test.go |
Adds tests for the new validation task behavior and adjusts handler mocks accordingly. |
|
|
||
| s.mgr.MockIdentity(s.makeSerial(c, "other-serial")) | ||
|
|
||
| s.runner.AddHandler("queue-mgmt-response", noopTask, nil) |
There was a problem hiding this comment.
Copying in similar comment from #17254 (comment):
The no-op
s.runner.AddHandler("queue-mgmt-response", ...)overrides in the pre-existing tests are there because those tests assert on intermediate state (that a message was dispatched, validated, or applied correctly) beforedoQueueResponseruns and removes it from the sequence. Now thatdoQueueResponseis implemented and registered by the manager,.settleruns it to completion, which removes theRequestMessagefrom state and breaks the assertions onms.Sequences[...].Messages.The no-op handler suppresses the real task keeping tests focused & their assertions meaningful.
49fc933 to
e88d9e9
Compare
|
Wed Jul 29 07:23:11 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list of the below tests you wish to run (unskip plus test list must be valid yaml)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17273 +/- ##
==========================================
+ Coverage 78.81% 78.88% +0.07%
==========================================
Files 1410 1396 -14
Lines 196929 196842 -87
Branches 2498 2498
==========================================
+ Hits 155208 155281 +73
+ Misses 32367 32210 -157
+ Partials 9354 9351 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ac24e24 to
a247a29
Compare
st3v3nmw
changed the title
54c350f to
afc7389
Compare
miguelpires
left a comment
miguelpires
left a comment
There was a problem hiding this comment.
Thanks. This looks ok overall, but the FetchAccountKeys call will panic. I also left a related question about testing
fe03449 to
8d23ccc
Compare
d571f67 to
a45a58d
Compare
miguelpires
left a comment
miguelpires
left a comment
There was a problem hiding this comment.
Question about possible race when dispatching multiple sequences
| return nil | ||
| } | ||
|
|
||
| defer m.setState(ms) |
There was a problem hiding this comment.
Since FetchAccountKeys might release the lock, can't another sequence finish processing during that time and save state which this would then overwrite with the stale read?
There was a problem hiding this comment.
Yep, I've reproduced the issue & added state re-reads at the points where we might release the lock (8621f56). Right now there are 4 spots where that might happen:
doValidateMessage > FetchAccountKeysdoValidateMessage > handler.Validate(we passst *state.Stateto the handler & the handler might unlock)doApplyMessage > handler.Apply(same as above)doExchangeMessages(since we can only have one device management change at a time, we can't have any other management tasks running when the store exchange task is running)
I've added re-reads after we re-acquire the lock to the first 3. The last one is guarded by the "only one device management change can be in flight at any given time" invariant we have. Not sure if there's a better solution short of breaking up the mgmt state by base message id 🤔.
Added 2 tests that reliably reproduce the race pre-fix.
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
thanks, some initial comments
| err := f.Fetch(ref) | ||
| if err != nil { | ||
| if errors.Is(err, &asserts.NotFoundError{}) { | ||
| logger.Noticef("cannot fetch account-key %q: %v", keyID, err) |
There was a problem hiding this comment.
can you explain why are we ignoring this error?
There was a problem hiding this comment.
do other Fetch helper ignore errors like this?
There was a problem hiding this comment.
For device mgmt purposes, if we don't find the account-key, we fail at the signature check instead. But this doesn't make sense for a general helper, I've refactored this.
| var unauthorizedErr *UnauthorizedError | ||
| status := asserts.MessageStatusRejected | ||
| if errors.As(err, &unauthorizedErr) { | ||
| status = asserts.MessageStatusUnauthorized |
There was a problem hiding this comment.
in this case we control the error and combined with the reason below will produce an awkward error, maybe we should set a reason here directly, and use the generic reason below in a else?
|
|
||
| msg := ms.Sequences["msg1"].Messages[0] | ||
| c.Check(msg.ResponseStatus, Equals, asserts.MessageStatusUnauthorized) | ||
| c.Check(msg.ResponseBody["message"], Equals, `cannot process message: cannot perform action: operator "alice" is not authorized`) |
There was a problem hiding this comment.
the error is a bit awkward as mentioned
| msg.ResponseBody = map[string]any{"message": err.Error()} | ||
| msg.ResponseBody = map[string]any{"message": fmt.Sprintf("cannot process message: %v", applyErr)} |
There was a problem hiding this comment.
maybe it's better to leave this as it was, otherwise we'll get those cannot x: cannot y: ... errors
| // Delay whichever lane reaches this call first: it releases the | ||
| // lock and sleeps, giving the other lane time to acquire the | ||
| // lock, run to completion, and persist its own write. When this | ||
| // lane resumes and re-acquires the lock, it must not overwrite | ||
| // that write with its own stale snapshot. | ||
| if applyCalls.Add(1) == 1 { | ||
| st.Unlock() | ||
| time.Sleep(100 * time.Millisecond) | ||
| st.Lock() | ||
| } |
There was a problem hiding this comment.
This seems a bit brittle. There's no guarantee that the other task runs at this point. You can achieve the same with more control by manually mutating the state, or maybe using AddTaskStatusChangedHandler but that seems complicated.
There was a problem hiding this comment.
I've refactored this so we're sure that the other lane is done before we continue.
| // run to completion, and persist its own write. When this lane | ||
| // resumes and re-acquires the lock, it must not overwrite that | ||
| // write with its own stale snapshot. | ||
| if fetchCalls.Add(1) == 1 { |
| s.mockStore(func(_ context.Context, _ *store.MessageExchangeRequest) (*store.MessageExchangeResponse, error) { | ||
| return &store.MessageExchangeResponse{ | ||
| Messages: []store.MessageWithToken{ | ||
| s.makeStoreRequestMessage(c, "msg1", "unknown-kind", "token-1"), | ||
| }, | ||
| }, nil | ||
| return &store.MessageExchangeResponse{}, nil | ||
| }) | ||
|
|
||
| s.runner.AddHandler("queue-mgmt-response", noopTask, nil) | ||
| ms := &devicemgmtstate.DeviceMgmtState{ | ||
| Sequences: map[string]*devicemgmtstate.SequenceState{ | ||
| "msg1": { | ||
| Messages: []*devicemgmtstate.RequestMessage{ | ||
| { | ||
| AccountID: "my-brand", | ||
| AuthorityID: "my-brand", | ||
| BaseID: "msg1", | ||
| Kind: "unknown-kind", | ||
| Devices: []string{"serial-1.my-model.my-brand"}, | ||
| ValidSince: fixedTestTime, | ||
| ValidUntil: fixedTestTime.Add(24 * time.Hour), | ||
| Body: `{"action": "get"}`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| s.mgr.SetState(ms) | ||
|
|
||
| chg := s.st.NewChange("test", "test change") | ||
| t := s.st.NewTask("apply-mgmt-message", "apply message with unknown kind") | ||
| t.Set("message-id", "msg1") | ||
| chg.AddTask(t) |
There was a problem hiding this comment.
Refactored this to run the task directly. The validate task now rejects messages with unknown kinds, so we wouldn't reach apply's check with .settle.
2ad0445 to
ba12532
Compare
miguelpires
left a comment
miguelpires
left a comment
There was a problem hiding this comment.
Couple of quick comments. There's also a unit test failure in:
FAIL: devicemgmtmgr_test.go:1440: deviceMgmtMgrSuite.TestDoValidateMessageIdempotent
devicemgmtmgr_test.go:1479:
c.Check(validateCalls, Equals, 1)
... obtained int = 3
... expected int = 1
| setMsgResponse := func(status asserts.MessageStatus, message string) error { | ||
| msg.ResponseStatus = status | ||
| msg.ResponseBody = map[string]any{"message": message} | ||
| m.setState(ms) | ||
| return nil | ||
| } | ||
| rejectMsg := func(reason string) error { | ||
| return setMsgResponse(asserts.MessageStatusRejected, reason) | ||
| } |
There was a problem hiding this comment.
I'm a bit lukewarm on passing through the error (always nil) from these calls. Reading the call sites, it looks like the error could be non-nil. It would be more explicit not to return an error here and to return nil at each site. If we keep this, we should leave a comment here flagging this and why it's always nil
There was a problem hiding this comment.
Should be explicit now.
|
|
||
| var fetchCalls atomic.Int32 | ||
| s.AddCleanup(devicemgmtstate.MockFetchAccountKey(func(st *state.State, _ int, _ string) error { | ||
| if fetchCalls.Add(1) == 1 { |
There was a problem hiding this comment.
We don't need the atomic int anymore I think?
There was a problem hiding this comment.
Fixed, switched to a normal bool.
| }, nil | ||
| }) | ||
|
|
||
| var validateCalls atomic.Int32 |
The test was flaky because Since this idempotency test checks that re-runs of the same task aren't doing duplicate work, I've opted to mock |
| // TODO: need to distinguish between: | ||
| // - transient errors (like store unreachable) - retry | ||
| // - permanent errors - determine whether to fail the task or reject the message. |
There was a problem hiding this comment.
If you haven't yet, don't forget to file a ticket to follow up on this
There was a problem hiding this comment.
Yes, I created one 😅.. Thanks for the reviews!!
| return nil | ||
| } | ||
|
|
||
| err = assertstateFetchAccountKey(m.state, 0, a.SignKeyID()) |
There was a problem hiding this comment.
this seems will always consult the store, should we check somehow first if we have the key already locally?
needs a re-review because of the latest change
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
+1 but testing question, also I asked @miguelpires to review again
| }) | ||
|
|
||
| // Remove the account key from the local DB so ensureAccountKey reaches the fetch. | ||
| db, err := asserts.OpenDatabase(&asserts.DatabaseConfig{ |
There was a problem hiding this comment.
do we have a test where it is not removed?
There was a problem hiding this comment.
Yes, TestDoValidateMessageOK and the idempotency & concurrency tests work with an account key that exists in the db set up by SetUpTest (success cases).
| // handler.ResultFromChange may drop the state lock internally. Concurrent tasks | ||
| // in other lanes may have mutated the state in that window, so re-read before mutating. | ||
| ms, _, err = m.getMessageAndState(msgID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
…ead in doQueueResponse
github-actions
Bot
added
the
Auto rerun spread
|
Confirmed that spread test failures are unrelated to these changes: Copilot summary:
|
This PR implements the
validate-mgmt-messagetask. Before a request message is handed off to a subsystem handler for processing, this task runs four checks: the message must target this specific device, the current time must fall within the message's validity window, anyassumesconstraints must be satisfied (TODO), and the subsystem handler'sValidatemethod must pass.Split from #16248.