fix(daemon): require admin access for POSTs and file pull API #406
Conversation
This file contains 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
Most of this was introduced in PR #358, when we ported the AccessChecker changes from snapd, but accidentally set all the WriteAccess fields to UserAccess{} instead of AdminAccess{}. Previously there was a r.Method=="GET" check in Command.canAccess that handled this case. Additionally: - We lock down the files "pull" API to require admin. Even though it's a read (GET), this meant any user could potentially read sensitive files. - We lock down the task-websocket endpoint to admin. This is a GET endpoint, but these websockets are used by exec to send stdin/out/err and commands to the exec'd process, so they should require admin too. I've added some tests for these to ensure we don't accidentally change them in future, without noticing. How valuable these tests are I'm not sure, as they only cover a subset of the API endpoints, but it seems better than nothing.
hpidcock
reviewed
Apr 3, 2024
This was actually not working earlier, and fixed (unintentionally) by the CheckAccess PR.
hpidcock
approved these changes
Apr 3, 2024
benhoyt
added a commit
that referenced
this pull request
Apr 3, 2024
Most of this was introduced in PR #358, when we ported the AccessChecker changes from snapd, but accidentally set all the WriteAccess fields to UserAccess{} instead of AdminAccess{}. Previously there was a r.Method=="GET" check in Command.canAccess that handled this case. Additionally: - We lock down the files "pull" API to require admin. Even though it's a read (GET), this meant any user could potentially read sensitive files. - We lock down the task-websocket endpoint to admin. This is a GET endpoint, but these websockets are used by exec to send stdin/out/err and commands to the exec'd process, so they should require admin too. I've added some tests for these to ensure we don't accidentally change them in future, without noticing. How valuable these tests are I'm not sure, as they only cover a subset of the API endpoints, but it seems better than nothing.
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.
Most of this was introduced in PR #358, when we ported the AccessChecker changes from snapd, but accidentally set all the WriteAccess fields to UserAccess{} instead of AdminAccess{}. Previously there was a r.Method=="GET" check in Command.canAccess that handled this case.
Additionally:
I've added some tests for these to ensure we don't accidentally change them in future, without noticing. How valuable these tests are I'm not sure, as they only cover a subset of the API endpoints, but it seems better than nothing.