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

snapctl: support running by the snap app itself, not only just from hooks #2644

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
13e3e77
Context Map skeleton.
stolowski Jan 9, 2017
224d154
Pass hookmgr.
stolowski Jan 9, 2017
586f024
Compiles.
stolowski Jan 9, 2017
9a3875a
SnapContext getter.
stolowski Jan 10, 2017
4543fd8
Merge branch 'master' into snapctl-outside-hooks
stolowski Jan 10, 2017
6da737b
Actually create context files.
stolowski Jan 11, 2017
b5704fc
Make existing tests happy.
stolowski Jan 11, 2017
1364229
Cleanup.
stolowski Jan 11, 2017
94e9305
Set SNAP_CONTEXT via snap-confine.
stolowski Jan 12, 2017
cd37165
Updated apparmor profile of snap-confine.
stolowski Jan 12, 2017
b9e3467
Added context-support, 1 part of changes suggested by zyga.
stolowski Jan 13, 2017
b773c05
Compiles.
stolowski Jan 13, 2017
43e6b3a
Use open/read instead of stdio.
stolowski Jan 13, 2017
f38433d
Added ensureState method.
stolowski Jan 13, 2017
8ca1d0c
Don't overwrite SNAP_CONTEXT in snap-confine as this breaks hooks.
stolowski Jan 13, 2017
aba6e2e
Get SnapContext in the api if hook context doesn't exist.
stolowski Jan 13, 2017
17efd17
Restore contexts from the state.
stolowski Jan 13, 2017
daa5093
Don't access task in Context if null, keep state reference in Context.
stolowski Jan 16, 2017
c078891
Spread test.
stolowski Jan 16, 2017
3957a36
Added missing configure hook to the test. Remove snap context when sn…
stolowski Jan 17, 2017
05c5d4c
Test that context file exists / is removed.
stolowski Jan 17, 2017
e82353d
Handle remove-snap-context.
stolowski Jan 17, 2017
3dfbbf3
Merge branch 'master' into snapctl-outside-hooks
stolowski Jan 17, 2017
e5cdff7
Fix test failure.
stolowski Jan 17, 2017
19cba55
Merge branch 'master' into snapctl-outside-hooks
stolowski Jan 18, 2017
d68694f
Addressed most of the comments re snap-confine.
stolowski Jan 18, 2017
bda5326
Use sc_error.
stolowski Jan 18, 2017
ba86d40
Verify snap name.
stolowski Jan 18, 2017
9b8a95f
Overwrite SNAP_CONTEXT unless running a hook.
stolowski Jan 19, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/snap-confine/Makefile.am
Expand Up @@ -67,7 +67,9 @@ snap_confine_SOURCES = \
ns-support.c \
ns-support.h \
apparmor-support.c \
apparmor-support.h
apparmor-support.h \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please merge with master and alter the (now single) Makefile.am

context-support.c \
context-support.h

snap_confine_CFLAGS = -Wall -Werror $(AM_CFLAGS)
snap_confine_LDFLAGS = $(AM_LDFLAGS)
Expand Down
80 changes: 80 additions & 0 deletions cmd/snap-confine/context-support.c
@@ -0,0 +1,80 @@
/*
* Copyright (C) 2017 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

#include "cleanup-funcs.h"
#include "config.h"
#include "context-support.h"
#include "utils.h"

#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#define CONTEXTS_DIR "/var/lib/snapd/contexts"
Copy link
Contributor

@niemeyer niemeyer Feb 13, 2017

Choose a reason for hiding this comment

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

Let's please use singular for this one: /var/lib/snapd/context. It reads much better (try to pronounce both) and it's also the more common rule under /var/lib/snapd/ (apparmor, environment, mount, seccomp, desktop, ...).


char *sc_nonfatal_context_get_from_snapd(const char *snap_name,
struct sc_error **errorp)
{
char context_path[PATH_MAX];
char *context_val = NULL;
struct sc_error *err = NULL;

if (snap_name == NULL) {
die("SNAP_NAME is not set");
}

int fd __attribute__ ((cleanup(sc_cleanup_close))) = -1;
must_snprintf(context_path, sizeof(context_path), "%s/snap.%s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, move must_sprintf above the int fd declaration please.

CONTEXTS_DIR, snap_name);
fd = open(context_path, O_RDONLY | O_NOFOLLOW | O_CLOEXEC);
if (fd < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should die() here instead or rename this to _nonfatal and produce an error object.

err =
sc_error_init(SC_CONTEXT_DOMAIN, 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just use the errno domain. Check the documentation of the error system. Unless you need to switch/case on the error number the domain is useless. Using the errno domain also gives you free strerror handling.

"cannot open context file %s, SNAP_CONTEXT will not be set: %s",
context_path, strerror(errno));
goto out;
}
// context is a 32 bytes, base64-encoding makes it 44.
context_val = calloc(1, 45);
if (context_val == NULL) {
die("failed to allocate memory for snap context");
}
if (read(fd, context_val, 44) < 0) {
free(context_val);
context_val = NULL;
err =
sc_error_init(SC_CONTEXT_DOMAIN, 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above. Just use the errno domain for this.

"failed to read context file %s: %s",
context_path, strerror(errno));
goto out;
}

out:
sc_error_forward(errorp, err);
return context_val;
}

void sc_context_set_environment(const char *context)
{
if (context != NULL) {
// Overwrite context env value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The discussion about context management for hooks vs snaps made me realize that you were right initially and we should preserve the hook-specific context.

NB: I feel that context overloading is somewhat confusing and we should use two variables for this. One will never be set by snap-confine and the other one will always be set by snap-confine.

setenv("SNAP_CONTEXT", context, 1);
}
}
47 changes: 47 additions & 0 deletions cmd/snap-confine/context-support.h
@@ -0,0 +1,47 @@
/*
* Copyright (C) 2017 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

#ifndef SNAP_CONFINE_CONTEXT_SUPPORT_H
#define SNAP_CONFINE_CONTEXT_SUPPORT_H

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document both functions carefully. Look at other files for inspiration.

#include "error.h"

/**
* Error domain for errors related to snap context handling.
**/
#define SC_CONTEXT_DOMAIN "context"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you agree with my suggestions and there's no code that needs to examine why context operation failed you can remove this.


/**
* Return snap context string for given snap.
*
* The context value is read from /var/lib/snapd/contexts/snap.<snapname>
* file. The caller of the function takes the ownership of the returned context
* string.
* If the file cannot be read then an error is returned in errorp and
* the function returns NULL.
**/
char *sc_nonfatal_context_get_from_snapd(const char *snap_name,
struct sc_error **errorp);

/**
* Set the snap context environment variable.
*
* Set the SNAP_CONTEXT environment variable with the value of context.
**/
void sc_context_set_environment(const char *context);

#endif
3 changes: 3 additions & 0 deletions cmd/snap-confine/snap-confine.apparmor.in
Expand Up @@ -257,6 +257,9 @@
# Allow snap-confine to move to the void
/var/lib/snapd/void/ r,

# Allow snap-confine to read snap contexts
/var/lib/snapd/contexts/snap.* r,

# Support for the quirk system
/var/ r,
/var/lib/ r,
Expand Down
30 changes: 26 additions & 4 deletions cmd/snap-confine/snap-confine.c
Expand Up @@ -37,6 +37,7 @@
#include "quirks.h"
#include "secure-getenv.h"
#include "apparmor-support.h"
#include "context-support.h"

int main(int argc, char **argv)
{
Expand Down Expand Up @@ -88,6 +89,26 @@ int main(int argc, char **argv)
die("need to run as root or suid");
}
#endif

char *snap_context __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
const char *snap_name = getenv("SNAP_NAME");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to validate that snap name is not malicious here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Can you find the other place that cares about SNAP_NAME and either also verify it there or obtain the value once and pass it around please.

if (snap_name != NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

SNAP_NAME cannot be NULL so I guess you can de-indent the whole part a little. If SNAP_NAME is NULL we should die as well.

if (!verify_snap_name(snap_name)) {
die("invalid SNAP_NAME");
}
// Do no get snap context value if running a hook (we don't want to overwrite hook's SNAP_CONTEXT)
if (!verify_is_hook_security_tag(security_tag)) {
struct sc_error *err
__attribute__ ((cleanup(sc_cleanup_error))) = NULL;
snap_context =
sc_nonfatal_context_get_from_snapd(snap_name, &err);
if (err != NULL) {
error("cannot get context: %s",
sc_error_msg(err));
}
}
}

struct sc_apparmor apparmor;
sc_init_apparmor_support(&apparmor);
#ifdef HAVE_SECCOMP
Expand All @@ -108,13 +129,12 @@ int main(int argc, char **argv)
debug
("skipping sandbox setup, classic confinement in use");
} else {
const char *group_name = getenv("SNAP_NAME");
if (group_name == NULL) {
if (snap_name == NULL) {
die("SNAP_NAME is not set");
}
sc_initialize_ns_groups();
struct sc_ns_group *group = NULL;
group = sc_open_ns_group(group_name, 0);
group = sc_open_ns_group(snap_name, 0);
sc_lock_ns_mutex(group);
sc_create_or_join_ns_group(group, &apparmor);
if (sc_should_populate_ns_group(group)) {
Expand Down Expand Up @@ -154,7 +174,9 @@ int main(int argc, char **argv)
#if 0
setup_user_xdg_runtime_dir();
#endif

if (snap_context != NULL) {
sc_context_set_environment(snap_context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you check for NULL in both places maybe just call this sc_maybe_set_context_environment() and allow it to be called with NULL.

}
// https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement
sc_maybe_aa_change_onexec(&apparmor, security_tag);
#ifdef HAVE_SECCOMP
Expand Down
28 changes: 28 additions & 0 deletions cmd/snap-confine/snap.c
Expand Up @@ -43,3 +43,31 @@ bool verify_security_tag(const char *security_tag)

return (status == 0);
}

bool verify_is_hook_security_tag(const char *security_tag)
{
const char *whitelist_re =
"^snap\\.[a-z](-?[a-z0-9])*\\.(hook\\.[a-z](-?[a-z])*)$";

regex_t re;
if (regcomp(&re, whitelist_re, REG_EXTENDED | REG_NOSUB) != 0)
die("can not compile regex %s", whitelist_re);

int status = regexec(&re, security_tag, 0, NULL, 0);
regfree(&re);

return (status == 0);
}

bool verify_snap_name(const char *name)
{
const char *whitelist_re = "[a-z](-?[a-z0-9])*$";
regex_t re;
if (regcomp(&re, whitelist_re, REG_EXTENDED | REG_NOSUB) != 0)
die("can not compile regex %s", whitelist_re);

int status = regexec(&re, name, 0, NULL, 0);
regfree(&re);

return (status == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can drop the ( ), I think

}
2 changes: 2 additions & 0 deletions cmd/snap-confine/snap.h
Expand Up @@ -21,5 +21,7 @@
#include <stdbool.h>

bool verify_security_tag(const char *security_tag);
bool verify_is_hook_security_tag(const char *security_tag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

verify_is_hook_security_tag feels mis-named. Is it checking if a security tag is a hook security tag or checking if a hook security tag is valid? Please name it more appropriately.

bool verify_snap_name(const char *name);

#endif
3 changes: 3 additions & 0 deletions daemon/api.go
Expand Up @@ -2147,6 +2147,9 @@ func runSnapctl(c *Command, r *http.Request, user *auth.UserState) Response {
// Right now snapctl is only used for hooks. If at some point it grows
// beyond that, this probably shouldn't go straight to the HookManager.
context, _ := c.d.overlord.HookManager().Context(snapctlOptions.ContextID)
if context == nil {
context, _ = c.d.overlord.HookManager().SnapContext(snapctlOptions.ContextID)
}
stdout, stderr, err := ctlcmd.Run(context, snapctlOptions.Args)
if err != nil {
if e, ok := err.(*flags.Error); ok && e.Type == flags.ErrHelp {
Expand Down
2 changes: 2 additions & 0 deletions dirs/dirs.go
Expand Up @@ -51,6 +51,7 @@ var (
SnapDeviceDir string

SnapAssertsDBDir string
SnapContextsDir string
SnapTrustedAccountKey string
SnapAssertsSpoolDir string

Expand Down Expand Up @@ -123,6 +124,7 @@ func SetRootDir(rootdir string) {
SnapSocket = filepath.Join(rootdir, "/run/snapd-snap.socket")

SnapAssertsDBDir = filepath.Join(rootdir, snappyDir, "assertions")
SnapContextsDir = filepath.Join(rootdir, snappyDir, "contexts")
SnapAssertsSpoolDir = filepath.Join(rootdir, "run/snapd/auto-import")

SnapStateFile = filepath.Join(rootdir, snappyDir, "state.json")
Expand Down
52 changes: 49 additions & 3 deletions overlord/hookstate/context.go
Expand Up @@ -34,6 +34,7 @@ import (
// Context represents the context under which a given hook is running.
type Context struct {
task *state.Task
state *state.State
setup *HookSetup
id string
handler Handler
Expand All @@ -56,13 +57,45 @@ func NewContext(task *state.Task, setup *HookSetup, handler Handler) (*Context,

return &Context{
task: task,
state: task.State(),
setup: setup,
id: base64.URLEncoding.EncodeToString(idBytes),
handler: handler,
cache: make(map[interface{}]interface{}),
}, nil
}

// NewSnapContext returns a new snap Context.
func NewSnapContext(state *state.State, setup *HookSetup, handler Handler) (*Context, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two functions are almost exactly the same. Let's please just teach the one above to take task == nil as well instead of adding a new one.

// Generate a secure, random ID for this context
idBytes := make([]byte, 32)
_, err := rand.Read(idBytes)
if err != nil {
return nil, fmt.Errorf("cannot generate context ID: %s", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is copy & paste from the above, but we have a function to generate random yet readable IDs at strutil.MakeRandomString. Let's please use it here.


return &Context{
task: nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this means things like Context.Get/Set will explode, I wonder whether we should have a Context interface with the shared bits and two implementations SnapContext and TaskContext, hmm

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but so far they still feel very close to each other for the splitting to pay off.

For Get and Set, we actually want them working. The ctlcmd code should not have to know whether the context is ephemeral or not before stashing data for recovering in Done, etc.

state: state,
setup: setup,
id: base64.URLEncoding.EncodeToString(idBytes),
handler: handler,
cache: make(map[interface{}]interface{}),
}, nil
}

// NewSnapContextWithID returns a new snap Context with a predefined contextID (must be base64-encoded).
func NewSnapContextWithID(state *state.State, setup *HookSetup, handler Handler, contextID string) *Context {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, we can make contextID an optional parameter on that one function. If empty, we set it.

return &Context{
task: nil,
state: state,
setup: setup,
id: contextID,
handler: handler,
cache: make(map[interface{}]interface{}),
}
}

// SnapName returns the name of the snap containing the hook.
func (c *Context) SnapName() string {
return c.setup.Snap
Expand Down Expand Up @@ -92,14 +125,22 @@ func (c *Context) Handler() Handler {
// and OnDone/Done).
func (c *Context) Lock() {
c.mutex.Lock()
c.task.State().Lock()
if c.task != nil {
c.task.State().Lock()
} else {
c.state.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always set, so let's always do this instead of branching.

}
atomic.AddInt32(&c.mutexChecker, 1)
}

// Unlock releases the lock for this context.
func (c *Context) Unlock() {
atomic.AddInt32(&c.mutexChecker, -1)
c.task.State().Unlock()
if c.task != nil {
c.task.State().Unlock()
} else {
c.state.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

}
c.mutex.Unlock()
}

Expand Down Expand Up @@ -165,7 +206,7 @@ func (c *Context) Get(key string, value interface{}) error {

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make Get and Set work, as described above. If task == nil, we can work with a memory-only "hook-context" out of c.cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, not "hook-context".. that's awkward given the idea here. Maybe "ephemeral-context".

// State returns the state contained within the context
func (c *Context) State() *state.State {
return c.task.State()
return c.state
}

// Cached returns the cached value associated with the provided key. It returns
Expand Down Expand Up @@ -211,3 +252,8 @@ func (c *Context) Done() error {

return firstErr
}

// IsSnapContext returns true if the context is a snap context.
func (c *Context) IsSnapContext() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's please rename this to IsEphemeral. More details on the naming below.

return c.task == nil
}
5 changes: 5 additions & 0 deletions overlord/hookstate/ctlcmd/set.go
Expand Up @@ -79,6 +79,11 @@ func (s *setCommand) Execute(args []string) error {

transaction.Set(s.context().SnapName(), key, value)
}
context.Lock()
if context.IsSnapContext() {
transaction.Commit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks suspicious, we should instead call context.Done() I think from some place higher even in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when the snapctl call comes in, we should test if IsEphemeral right before returning and call its Done method right there if that's the case. The *Context won't be saved anywhere, so it'll be garbage collected shortly after.

}
context.Unlock()

return nil
}