daemon: add security logging for admin actions (part 5.2)#17169
Open
ernestl wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
the
Needs Documentation -auto-
ernestl
changed the title
|
Mon Jun 15 14:53:45 UTC 2026 Failures:Executing:
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 (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
ernestl
changed the title
ernestl
added
the
Auto rerun spread
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends snapd’s security/audit logging to cover administrative API authorization decisions, emitting structured authz_admin (success) and authz_fail (rejection) events and recording per-stage authorization check outcomes for requests over snapd.socket.
Changes:
- Add new security-log event data structures (
Peer,Endpoint,AuthzChecks) and structuredReasonfields (code, optionalkind,message) with slog LogValuer support. - Rework daemon access checking to return
(apiError, authz_checks, accessLevel)and gate authz audit emission on administrative access levels. - Add unit + spread coverage verifying emitted authz audit events and structured payloads.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/main/security-logging/task.yaml | Extends spread test to assert authz_admin/authz_fail events for admin API calls and adjusts login-failure expectations. |
| seclog/slog.go | Updates Reason logging shape and adds LogValue for Endpoint, Peer, and AuthzChecks. |
| seclog/slog_test.go | Updates tests for new structured Reason format and adds coverage for authz-related structured attrs. |
| seclog/seclog.go | Adds LogAdminActivity and LogUnauthorizedAccess helpers emitting AUTHZ category events. |
| seclog/seclog_test.go | Adds tests for new authz logging helpers and updates login-failure expectations. |
| seclog/eventdata.go | Redefines Reason schema and introduces Peer, Endpoint, AuthzChecks (+ constructors/stringers). |
| seclog/eventdata_test.go | Adds tests for new Peer/Endpoint string formatting and updates Reason.String() expectations. |
| daemon/export_test.go | Exposes APIError.reason() as APIErrorReason for tests. |
| daemon/export_access_test.go | Exposes new access-check helpers/types for tests (AccessLevel, prerequisites/authz funcs, etc.). |
| daemon/errors.go | Adds apiError.reason() to convert API errors into structured seclog.Reason. |
| daemon/errors_test.go | Adds unit coverage for APIErrorReason. |
| daemon/daemon.go | Integrates authz audit logging into request dispatch and adds shared action-body parsing helpers. |
| daemon/daemon_test.go | Adds tests verifying authz audit emission/gating and requestAction parsing behavior. |
| daemon/api_users.go | Switches login-failure logging to use structured apiError.reason(). |
| daemon/api_users_test.go | Updates login-failure assertions to match structured reasons (HTTP code + kind). |
| daemon/access.go | Refactors access checking to return authz stage outcomes + intended access level; adds admin gating helper. |
| daemon/access_test.go | Updates existing access tests for new signature and adds coverage for prerequisites/authz-stage results. |
Comment on lines
+264
to
+278
| func readBodyAndParseAction(r *http.Request, limit int64) (string, error) { | ||
| bodyBytes, err := io.ReadAll(io.LimitReader(r.Body, limit)) | ||
| r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes)) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| var data struct { | ||
| Action string `json:"action"` | ||
| } | ||
| if err := json.Unmarshal(bodyBytes, &data); err != nil { | ||
| return "", nil | ||
| } | ||
| return data.Action, nil | ||
| } |
Comment on lines
163
to
171
| rspe, checks, level := access.CheckAccess(c.d, r, ucred, user) | ||
| admin := isAdministrativeAccess(level) | ||
|
|
||
| if rspe != nil { | ||
| if admin { | ||
| logUnauthorizedAccess(c, r, ucred, user, access, level, rspe, checks) | ||
| } | ||
| rspe.ServeHTTP(w, r) | ||
| return |
Comment on lines
+304
to
+306
| func logAdminActivity(c *Command, r *http.Request, ucred *ucrednet, user *auth.UserState, ac accessChecker, level accessLevel, checks seclog.AuthzChecks) { | ||
| seclog.LogAdminActivity(snapdUser(user), peerFromUcred(ucred), endpointFromRequest(c, r, ac, level, requestAction(r)), checks) | ||
| } |
Comment on lines
+308
to
+310
| func logUnauthorizedAccess(c *Command, r *http.Request, ucred *ucrednet, user *auth.UserState, ac accessChecker, level accessLevel, rspe *apiError, checks seclog.AuthzChecks) { | ||
| seclog.LogUnauthorizedAccess(snapdUser(user), peerFromUcred(ucred), endpointFromRequest(c, r, ac, level, requestAction(r)), checks, rspe.reason()) | ||
| } |
Comment on lines
+197
to
+199
| if admin { | ||
| logAdminActivity(c, r, ucred, user, access, level, checks) | ||
| } |
ernestl
force-pushed
the
ernestl/seclog-part5.2-daemon-access-changes
branch
2 times, most recently
from
c70af8e to
7f6bcdd
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17169 +/- ##
==========================================
+ Coverage 79.12% 79.15% +0.02%
==========================================
Files 1390 1386 -4
Lines 193561 193566 +5
Branches 2466 2466
==========================================
+ Hits 153156 153212 +56
+ Misses 31209 31169 -40
+ Partials 9196 9185 -11
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:
|
ernestl
force-pushed
the
ernestl/seclog-part5.2-daemon-access-changes
branch
5 times, most recently
from
ba37cb3 to
a9cb70e
Compare
Emit authz_admin and authz_fail for admin API calls, populate authz_checks during checkAccess, skip dispatch-only byActionAccess failures via accessLevelNotEvaluated, and add spread coverage.
ernestl
force-pushed
the
ernestl/seclog-part5.2-daemon-access-changes
branch
from
a9cb70e to
db7b62b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add security logging for admin actions.
Spec SD236
Part 5 adds security logging for administrative actions: Full reference implementation.
[Done] Part 4 adds security logging for adding, updating and removing a snapd user.
[Done] Part 1, 2, & 3 provides the logger implementation.