chore(daemon): port AccessChecker interface from snapd #358

thp-canonical · 2024-02-13T12:49:13Z

Port the accessChecker API from snapd for easier customization and more fine-grained access control.

UserState is currently empty, but it might sense to drag it along (to be in line with snapd's function signature) and allow user-specific state data to be extracted from the HTTP request in a central place (userFromRequest() in daemon.go).

This ports canonical/snapd#10126 from snapd, so that ucrednetGet() has the same return type. This change is in preparation for porting the accessChecker API from snapd (canonical/snapd#10127): #358

benhoyt

Looking good! Let's just update from the master branch and resolve conflicts, add those tests, and I've commented on some minor issues.

I'd also like to get @flotter's review on this.

internals/daemon/access.go

internals/daemon/access_test.go

internals/daemon/access.go

internals/daemon/api.go

internals/daemon/daemon.go

internals/daemon/daemon_test.go

internals/daemon/export_test.go

thp-canonical · 2024-02-14T13:56:07Z

internals/daemon/daemon_test.go


-func (s *daemonSuite) TestLoggedInUserAccess(c *C) {


The TestLoggedInUserAccess test was removed, as daemon.userFromRequest(...) doesn't do anything yet, and will always return a nil user state. We can re-add those test cases when an AccessChecker is implemented that accesses the user parameter.

I've left the user parameter in AccessChecker.CheckAccess(...), as we do have a UserState struct, and this might be useful for @hpidcock when implementing token support.

internals/daemon/daemon_test.go

thp-canonical · 2024-02-14T14:11:13Z

@benhoyt This is now ready for review. It ended up spilling into many areas.

Let me know if you want me to split off any or all of those topics into separate PRs for easier review:

Public-making of ucrednet (f315281)
Removal of DELETE (89f05b6)

Split off already:

As part of working on #358, we found out that the untrusted socket is not used, and so can be removed (in preparation for porting the `AccessChecker` changes from snapd in #358). **Indicators that it's not used:** If we look at how `canAccess` works, if we match on `untrustedSocketPath` (`isUntrusted`), the only way for `canAccess` to allow the request is when `c.UntrustedOK` is `true` (otherwise it unconditionally returns `accessUnauthorized` immediately): ```golang if isUntrusted { if c.UntrustedOK { return accessOK } return accessUnauthorized } ``` So in order for any API calls to be allowed with the untrusted socket (assuming all API calls go through `canAccess`), we would need to have a `Command` defined with `UntrustedOK: true`. Checking the Pebble codebase, no such `Command` definition exists, which means that even if any application would use the untrusted socket currently, all API calls would return `accessUnauthorized` unconditionally for this socket. The untrusted socket as well as `UntrustedOK` in `Command` were already part of the initial import commit (50466ba), so seem to be an inheritance from snapd that haven't seen use in Pebble since then. The corresponding [snapd sources from around November 10th, 2020](https://github.com/snapcore/snapd/tree/e2581af241a941856a755035d816047ff9aa15d8) seem to call these [`SnapOK`](https://github.com/snapcore/snapd/blob/e2581af241a941856a755035d816047ff9aa15d8/daemon/daemon.go#L139) (`UntrustedOK`), [`dirs.SnapSocket`](https://github.com/snapcore/snapd/blob/e2581af241a941856a755035d816047ff9aa15d8/daemon/daemon.go#L160C23-L160C38) (`untrustedSocketPath`) and [`snapListener`](https://github.com/snapcore/snapd/blob/489358223f0bd03da01e62a4062174eb7e9e0ffa/daemon/daemon.go#L72) (`untrustedListener`). Due to `gofmt` and removal of struct members with the longest names, this PR is best reviewed with the "hide whitespace" option.

This brings the error response namings in line with snapd: [`daemon/errors.go` in snapd](https://github.com/snapcore/snapd/blob/489358223f0bd03da01e62a4062174eb7e9e0ffa/daemon/errors.go#L112). Split off from #358. While currently the error responses are used exclusively in the `daemon` package, once `AccessChecker` is available, it should be possible for other packages to create error responders to implement their `AccessChecker` variants.

benhoyt · 2024-02-19T04:45:29Z

@thp-canonical Can you please bring this branch up to date and resolve conflicts, now that #360 and #361 are merged?

thp-canonical · 2024-02-19T07:20:52Z

@thp-canonical Can you please bring this branch up to date and resolve conflicts, now that #360 and #361 are merged?

@benhoyt PR is now updated against the latest master branch, with some further cleanups after looking at the diff again. Ready for re-review from my side.

benhoyt

This looks good -- just a couple of minor changes requested in the tests.

internals/daemon/daemon_test.go

benhoyt

Thanks for the tweaks. Looks good now -- let's just fix the bracing style.

internals/daemon/daemon_test.go

benhoyt · 2024-02-20T22:02:54Z

I've also asked for @hpidcock's review, as he's pretty close to Pebble and has been part of these discussions.

hpidcock

~~I'd retain the delete method as a write only method, but it should be allowed as it is a useful concept, even if we don't use it.~~

LGTM thank-you.

hpidcock · 2024-02-22T03:22:21Z

internals/daemon/daemon.go

 	case "POST":
 		rspf = c.POST
-	case "DELETE":


What happened to DELETE here?

Earlier I had suggested removing it as snapd removed it some time ago (and Pebble wasn't using it either). We can always add it back if we need it, but this was to be more in line with snapd.

OK, happy for it to go, since it isn't used and inline with snapd.

Most of this was introduced in PR canonical#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.

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.

thp-canonical added 2 commits February 13, 2024 12:43

daemon: make ucrednetGet() return *ucrednet

c658437

feat(daemon): Port accessChecker API from snapd

aa74f21

thp-canonical force-pushed the access-checker branch from a513795 to aa74f21 Compare February 13, 2024 13:11

thp-canonical mentioned this pull request Feb 13, 2024

daemon: make ucrednetGet() return *ucrednet #357

Merged

benhoyt requested changes Feb 14, 2024

View reviewed changes

benhoyt requested a review from flotter February 14, 2024 03:46

thp-canonical added 13 commits February 14, 2024 11:16

Merge remote-tracking branch 'origin/master' into access-checker

fe74b54

pr fix: Header for newly-added files

778f8eb

pr fix: CheckAccess documentation

2b43c34

pr fix: Drop WriteAccess/ReadAccess without a corresponding verb

06daf05

pr fix: Remove WriteAccess from unit test

42cbc15

pr fix: remove DELETE from Command

89f05b6

pr fix: export AccessChecker

528fae3

pr fix: export Ucrednet

f315281

pr fix: export OpenAccess, UserAccess, RootAccess

7694d55

pr fix: export error responders from daemon.response

bcbc1b0

pr fix: golangci-lint

bceb710

pr fix: align daemon.ServeHTTP with snapd

f030019

pr fix: fix unit test and return value (Unauthorized vs Forbidden)

6da7520

thp-canonical changed the title ~~Port accessChecker interface from snapd~~ Port AccessChecker interface from snapd Feb 14, 2024

thp-canonical added 2 commits February 14, 2024 14:42

pr fix: change RootAccess to AdminAccess + allow current uid

2cb2393

pr fix: port test cases for guest/user/admin access

9c0e195

thp-canonical commented Feb 14, 2024

View reviewed changes

internals/daemon/daemon_test.go Outdated Show resolved Hide resolved

thp-canonical commented Feb 14, 2024

View reviewed changes

internals/daemon/daemon_test.go Show resolved Hide resolved

thp-canonical marked this pull request as ready for review February 14, 2024 14:07

This was referenced Feb 14, 2024

fix(daemon): Export error responders from daemon.response #360

Merged

chore(daemon): remove untrusted socket #361

Merged

thp-canonical added 3 commits February 19, 2024 07:41

Merge remote-tracking branch 'origin/master' into access-checker

727ab38

pr fix: improve comment description

2501679

pr fix: remove socketPath, as it isn't used by tests

86e504e

thp-canonical requested a review from benhoyt February 19, 2024 07:20

benhoyt requested changes Feb 20, 2024

View reviewed changes

internals/daemon/daemon_test.go Outdated Show resolved Hide resolved

internals/daemon/daemon_test.go Outdated Show resolved Hide resolved

internals/daemon/daemon_test.go Outdated Show resolved Hide resolved

benhoyt changed the title ~~Port AccessChecker interface from snapd~~ chore(api): port AccessChecker interface from snapd Feb 20, 2024

benhoyt changed the title ~~chore(api): port AccessChecker interface from snapd~~ chore(api): port AccessChecker interface from snapd Feb 20, 2024

thp-canonical added 5 commits February 20, 2024 08:19

pr fix: remove c parameter from doTestReqFunc

4283186

pr fix: don't set RemoteAddr unnecessarily

82a8582

pr fix: access checker test case struct + helper

5fdeed8

pr fix: golangci-lint

f5b35b0

pr fix: align with previous test cases

4b91e33

thp-canonical requested a review from benhoyt February 20, 2024 08:31

benhoyt approved these changes Feb 20, 2024

View reviewed changes

internals/daemon/daemon_test.go Outdated Show resolved Hide resolved

benhoyt requested review from hpidcock and removed request for flotter February 20, 2024 22:03

pr fix: terse brace style

646485a

hpidcock approved these changes Feb 22, 2024

View reviewed changes

benhoyt changed the title ~~chore(api): port AccessChecker interface from snapd~~ chore(daemon): port AccessChecker interface from snapd Feb 22, 2024

benhoyt merged commit 150cb9f into canonical:master Feb 22, 2024
15 checks passed

thp-canonical deleted the access-checker branch February 22, 2024 07:24

benhoyt mentioned this pull request Apr 3, 2024

fix(daemon): require admin access for POSTs and file pull API #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(daemon): port AccessChecker interface from snapd #358

chore(daemon): port AccessChecker interface from snapd #358

thp-canonical commented Feb 13, 2024 •

edited

Loading

benhoyt left a comment

thp-canonical Feb 14, 2024

thp-canonical commented Feb 14, 2024 •

edited

Loading

benhoyt commented Feb 19, 2024

thp-canonical commented Feb 19, 2024

benhoyt left a comment

benhoyt left a comment

benhoyt commented Feb 20, 2024

hpidcock left a comment •

edited

Loading

hpidcock Feb 22, 2024

benhoyt Feb 22, 2024

hpidcock Feb 22, 2024

chore(daemon): port AccessChecker interface from snapd #358

chore(daemon): port AccessChecker interface from snapd #358

Conversation

thp-canonical commented Feb 13, 2024 • edited Loading

benhoyt left a comment

Choose a reason for hiding this comment

thp-canonical Feb 14, 2024

Choose a reason for hiding this comment

thp-canonical commented Feb 14, 2024 • edited Loading

benhoyt commented Feb 19, 2024

thp-canonical commented Feb 19, 2024

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt commented Feb 20, 2024

hpidcock left a comment • edited Loading

Choose a reason for hiding this comment

hpidcock Feb 22, 2024

Choose a reason for hiding this comment

benhoyt Feb 22, 2024

Choose a reason for hiding this comment

hpidcock Feb 22, 2024

Choose a reason for hiding this comment

thp-canonical commented Feb 13, 2024 •

edited

Loading

thp-canonical commented Feb 14, 2024 •

edited

Loading

hpidcock left a comment •

edited

Loading