Skip to content
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/state,daemon: add snap-run-inhibit notice #13769

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions daemon/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@
package daemon

import (
"fmt"
"net/http"
"os"
"path/filepath"
"strconv"

"github.com/snapcore/snapd/client"
Expand All @@ -38,6 +41,8 @@ var polkitCheckAuthorization = polkit.CheckAuthorization

var checkPolkitAction = checkPolkitActionImpl

var osReadlink = os.Readlink

func checkPolkitActionImpl(r *http.Request, ucred *ucrednet, action string) *apiError {
var flags polkit.CheckFlags
allowHeader := r.Header.Get(client.AllowInteractionHeader)
Expand Down Expand Up @@ -255,3 +260,41 @@ func (ac interfaceAuthenticatedAccess) CheckAccess(d *Daemon, r *http.Request, u

return Unauthorized("access denied")
}

// isRequestFromSnapCmd checks that the request is coming from snap command.
//
// It checks that the request process "/proc/PID/exe" points to one of the
// known locations of the snap command. This not a security-oriented check.
func isRequestFromSnapCmd(r *http.Request) (bool, error) {
ucred, err := ucrednetGet(r.RemoteAddr)
if err != nil {
return false, err
}
exe, err := osReadlink(fmt.Sprintf("/proc/%d/exe", ucred.Pid))
if err != nil {
return false, err
}

// SNAP_REEXEC=0
if exe == filepath.Join(dirs.GlobalRootDir, "/usr/bin/snap") {
return true, nil
}

// Check if re-exec in snapd
path := filepath.Join(dirs.SnapMountDir, "snapd/*/usr/bin/snap")
if matched, err := filepath.Match(path, exe); err != nil {
return false, err
} else if matched {
return true, nil
}

// Check if re-exec in core
path = filepath.Join(dirs.SnapMountDir, "core/*/usr/bin/snap")
if matched, err := filepath.Match(path, exe); err != nil {
return false, err
} else if matched {
return true, nil
}

return false, nil
}
82 changes: 79 additions & 3 deletions daemon/api_notices.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package daemon

import (
"context"
"encoding/json"
"errors"
"fmt"
"math"
Expand All @@ -25,19 +26,23 @@ import (
"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/overlord/auth"
"github.com/snapcore/snapd/overlord/state"
"github.com/snapcore/snapd/snap/naming"
"github.com/snapcore/snapd/strutil"
)

var noticeReadInterfaces = map[state.NoticeType][]string{
state.ChangeUpdateNotice: {"snap-refresh-observe"},
state.RefreshInhibitNotice: {"snap-refresh-observe"},
state.SnapRunInhibitNotice: {"snap-refresh-observe"},
}

var (
noticesCmd = &Command{
Path: "/v2/notices",
GET: getNotices,
ReadAccess: interfaceOpenAccess{Interfaces: []string{"snap-refresh-observe"}},
Path: "/v2/notices",
GET: getNotices,
POST: postNotices,
ReadAccess: interfaceOpenAccess{Interfaces: []string{"snap-refresh-observe"}},
WriteAccess: openAccess{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think perhaps a different type of access, something like snapClientAccess{} might be appropriate here? Again, for the time being, we don't want to allow anyone outside of snapd/the snap client to be able to create notices. Your solution of checking /proc/<pid>/exe works, but the endpoint being openAccess suggests to me that anyone should be able to create notices of any type, neither of which are true.

Something like snapClientAccess could implement the check you added below while maintaining that this behaves differently from the way openAccess works in other places, (afaik) without restrictions about the caller (beyond it not being a snap).

Copy link
Contributor Author

@ZeyadYasser ZeyadYasser Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly disagree with forcing something like snapClientAccess{} on POST /v2/notices. We cannot rule out the possibility of having other notices that are generated outside the snap command.

I think of it more as validation per notice type, it just happens that we have only one notice type for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Furthermore, in the future, we may wish to allow snaps to create notices, in which case it would become interfaceOpenAccess, which is definitely not compatible with a snapClientAccess. Using openAccess as you suggest is analogous to the way interfaceOpenAccess grants access only to particular notice types depending on the connected interface, so there is precedent as well.

So +1 from me for your current approach. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this endpoint currently protected in pebble?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Meulengracht It is not protected, it uses an access checker that similar to openAccess.

}

noticeCmd = &Command{
Expand All @@ -47,6 +52,12 @@ var (
}
)

// addedNotice is the result of adding a new notice.
type addedNotice struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks! updated

// ID is the id of the newly added notice.
ID string `json:"id"`
}

func getNotices(c *Command, r *http.Request, user *auth.UserState) Response {
query := r.URL.Query()

Expand Down Expand Up @@ -232,6 +243,71 @@ func allowedNoticeTypesForInterface(iface string) []state.NoticeType {
return types
}

func postNotices(c *Command, r *http.Request, user *auth.UserState) Response {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If at all possible, I'd like to avoid being able to post to notices, at least for "snap-run-inhibit" notices. As far as I understand, this notice type is something produced from within snapd that a user or non-snapd client should never need to issue, so I don't think it makes sense to be created via the API. Then, a user could create a "snap-run-inhibit" notice if they post to the snapd API, triggering a polkit prompt, and it would be nonsensical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, this notice type is something produced from within snapd

Not really, snap-run-inhibit indicate that snap run couldn't start because of an ongoing refresh. snapd have no way of telling when snap run run is called.

Thank you for pointing this out, it means that I should document this better.

requestUID, err := uidFromRequest(r)
if err != nil {
return Forbidden("cannot determine UID of request, so cannot create notice")
}

decoder := json.NewDecoder(r.Body)
var inst noticeInstruction
if err := decoder.Decode(&inst); err != nil {
return BadRequest("cannot decode request body into notice instruction: %v", err)
}

st := c.d.overlord.State()
st.Lock()
defer st.Unlock()

if err := inst.validate(r); err != nil {
return err
}

noticeId, err := st.AddNotice(&requestUID, state.SnapRunInhibitNotice, inst.Key, nil)
if err != nil {
return InternalError("%v", err)
}

return SyncResponse(addedNotice{ID: noticeId})
}

type noticeInstruction struct {
Action string `json:"action"`
Type state.NoticeType `json:"type"`
Key string `json:"key"`
// NOTE: Data and RepeatAfter fields are not needed for snap-run-inhibit notices.
}

func (inst *noticeInstruction) validate(r *http.Request) *apiError {
if inst.Action != "add" {
return BadRequest("invalid action %q", inst.Action)
}
if err := state.ValidateNotice(inst.Type, inst.Key, nil); err != nil {
return BadRequest("%s", err)
}

switch inst.Type {
case state.SnapRunInhibitNotice:
return inst.validateSnapRunInhibitNotice(r)
default:
return BadRequest(`cannot add notice with invalid type %q (can only add "snap-run-inhibit" notices)`, inst.Type)
}
}

func (inst *noticeInstruction) validateSnapRunInhibitNotice(r *http.Request) *apiError {
if fromSnapCmd, err := isRequestFromSnapCmd(r); err != nil {
return InternalError("cannot check request source: %v", err)
} else if !fromSnapCmd {
return Forbidden("only snap command can record notices")
}

if err := naming.ValidateInstance(inst.Key); err != nil {
return BadRequest("invalid key: %v", err)
}

return nil
}

func getNotice(c *Command, r *http.Request, user *auth.UserState) Response {
requestUID, err := uidFromRequest(r)
if err != nil {
Expand Down
Loading
Loading