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

stolowski · 2017-01-17T11:53:02Z

Support running snapctl outside of hooks by the apps from the snaps. This is realized by generating a unique SNAP_CONTEXT cookie-file on per-snap basis when the snap is installed, and passing it via the env in snap-confine.

TODO: support upgrades (generate context files for already existing snaps). Needs security review (design notes, in progress - CANNOT BE LANDED without this).

…ap is removed.

pedronis

some first pass comments

pedronis · 2017-01-17T12:25:22Z

overlord/hookstate/ctlcmd/set.go

@@ -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()


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

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.

pedronis · 2017-01-17T12:26:50Z

overlord/hookstate/context.go

+	}
+
+	return &Context{
+		task:    nil,


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

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.

pedronis · 2017-01-17T13:28:53Z

overlord/snapstate/snapmgr_test.go

@@ -154,6 +154,7 @@ func verifyInstallUpdateTasks(c *C, opts, discards int, ts *state.TaskSet, st *s
 		"copy-snap-data",
 		"setup-profiles",
 		"link-snap",
+		"setup-snap-context",


I think this would better happen before link-snap

Right, we definitely need the file in place before the snap is considered linked. We can probably have it inside link itself, though.

pedronis · 2017-01-17T13:29:44Z

overlord/snapstate/snapmgr_test.go

@@ -790,6 +793,7 @@ func (s *snapmgrTestSuite) TestRemoveTasks(c *C) {
 		"discard-snap",
 		"clear-aliases",
 		"discard-conns",
+		"remove-snap-context",


this seems ok though might come a bit earlier but after unlink

Same (inside unlink). Please remember to clear the cache.

zyga

Thanks for changing snap-confine. I've marked a few details I'd like to see changed before this lands. I'll review the rest later.

zyga · 2017-01-17T18:56:37Z

cmd/snap-confine/context-support.c

+
+char *read_snap_context(const char *snap_name)
+{
+	char context_path[1000];


Use PATH_MAX for this

zyga · 2017-01-17T18:56:57Z

cmd/snap-confine/context-support.c

+		die("SNAP_NAME is not set");
+	}
+
+	must_snprintf(context_path, 1000, "%s/snap.%s", CONTEXTS_DIR,


Use sizeof context_path for this

zyga · 2017-01-17T18:57:52Z

cmd/snap-confine/context-support.c

+		      snap_name);
+
+	int fd = open(context_path, O_RDONLY);
+	if (fd < 0) {


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

zyga · 2017-01-17T18:58:10Z

cmd/snap-confine/context-support.c

+	int fd = open(context_path, O_RDONLY);
+	if (fd < 0) {
+		error
+		    ("Cannot open context file %s, SNAP_CONTEXT will not be set: %s",


All the error messages are lower case.

zyga · 2017-01-17T18:59:14Z

cmd/snap-confine/context-support.c

+	if (context_val == NULL) {
+		die("Failed to allocate memory for snap context");
+	}
+	if (read(fd, context_val, 45) < 0) {


The context must be a valid C string so the maximum size is actually one byte less. This also doesn't zero-terminate the value you've obtained.

zyga · 2017-01-17T19:02:51Z

cmd/snap-confine/snap-confine.c

-
+	if (snap_context != NULL) {
+		set_snap_context_env(snap_context);
+		free(snap_context);


Let's remove this free and use a cleanup function instead.

zyga · 2017-01-17T19:03:13Z

cmd/snap-confine/context-support.c

+	must_snprintf(context_path, 1000, "%s/snap.%s", CONTEXTS_DIR,
+		      snap_name);
+
+	int fd = open(context_path, O_RDONLY);


Please add O_NOFOLLOW | O_CLOEXEC here.

zyga · 2017-01-17T19:03:36Z

cmd/snap-confine/context-support.c

+	must_snprintf(context_path, 1000, "%s/snap.%s", CONTEXTS_DIR,
+		      snap_name);
+
+	int fd = open(context_path, O_RDONLY);


Please initialize this to -1 and use a cleanup function to close the file descriptor.

zyga · 2017-01-17T19:04:23Z

cmd/snap-confine/context-support.c

+	if (fd < 0) {
+		error
+		    ("Cannot open context file %s, SNAP_CONTEXT will not be set: %s",
+		     context_path, strerror(errno));


Use die or switch to errors, they handle strerror part internally.

If the design is to make the absence of context non-fatal then switch to errors and ensure the function has nonfatal in the name.

zyga · 2017-01-17T19:05:39Z

cmd/snap-confine/context-support.c

+	}
+	if (read(fd, context_val, 45) < 0) {
+		free(context_val);
+		error("Failed to read context file %s: %s", context_path,


Same comments about errors as above.

zyga · 2017-01-17T19:14:30Z

cmd/snap-confine/snap-confine.c

@@ -88,6 +89,13 @@ int main(int argc, char **argv)
 		die("need to run as root or suid");
 	}
 #endif
+
+	char *snap_context = NULL;
+	const char *snap_name = getenv("SNAP_NAME");


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

zyga · 2017-01-19T10:25:53Z

cmd/snap-confine/Makefile.am

@@ -67,7 +67,9 @@ snap_confine_SOURCES = \
 	ns-support.c \
 	ns-support.h \
 	apparmor-support.c \
-	apparmor-support.h


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

zyga

Thank you for all the changes to the C code, it looks much better. I left a few more suggestions, some simplifications and some correction for you to consider.

zyga · 2017-01-19T10:26:37Z

cmd/snap-confine/context-support.c

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


Nitpick, move must_sprintf above the int fd declaration please.

zyga · 2017-01-19T10:27:49Z

cmd/snap-confine/context-support.c

+	fd = open(context_path, O_RDONLY | O_NOFOLLOW | O_CLOEXEC);
+	if (fd < 0) {
+		err =
+		    sc_error_init(SC_CONTEXT_DOMAIN, 0,


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.

zyga · 2017-01-19T10:28:26Z

cmd/snap-confine/context-support.c

+		free(context_val);
+		context_val = NULL;
+		err =
+		    sc_error_init(SC_CONTEXT_DOMAIN, 0,


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

zyga · 2017-01-19T10:30:36Z

cmd/snap-confine/context-support.c

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


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.

zyga · 2017-01-19T10:30:58Z

cmd/snap-confine/context-support.h

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


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

zyga · 2017-01-19T10:31:58Z

cmd/snap-confine/snap-confine.c

@@ -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");


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.

zyga · 2017-01-19T10:33:44Z

cmd/snap-confine/snap-confine.c

+
+	char *snap_context __attribute__ ((cleanup(sc_cleanup_string))) = NULL;
+	const char *snap_name = getenv("SNAP_NAME");
+	if (snap_name != NULL) {


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.

zyga · 2017-01-19T10:35:48Z

cmd/snap-confine/snap-confine.c

@@ -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);


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.

zyga · 2017-01-19T10:37:10Z

cmd/snap-confine/snap.c

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


We can drop the ( ), I think

zyga · 2017-01-19T10:38:17Z

cmd/snap-confine/snap.h

@@ -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);


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.

niemeyer · 2017-02-13T09:32:31Z

overlord/hookstate/context.go

+	_, err := rand.Read(idBytes)
+	if err != nil {
+		return nil, fmt.Errorf("cannot generate context ID: %s", err)
+	}


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.

niemeyer · 2017-02-13T09:33:40Z

overlord/hookstate/context.go

@@ -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 {


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

niemeyer · 2017-02-13T09:34:46Z

overlord/hookstate/context.go

+	if c.task != nil {
+		c.task.State().Lock()
+	} else {
+		c.state.Lock()


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

niemeyer · 2017-02-13T09:36:27Z

overlord/hookstate/context.go

+	if c.task != nil {
+		c.task.State().Unlock()
+	} else {
+		c.state.Unlock()


niemeyer · 2017-02-13T09:43:16Z

overlord/hookstate/context.go

 		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) {


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.

niemeyer · 2017-02-13T10:16:07Z

overlord/hookstate/context.go

+}
+
+// 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 {


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

niemeyer · 2017-02-13T10:27:07Z

overlord/hookstate/context.go

+	}
+
+	return &Context{
+		task:    nil,


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.

niemeyer · 2017-02-13T10:28:39Z

overlord/hookstate/context.go

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



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.

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

niemeyer · 2017-02-13T10:34:00Z

overlord/hookstate/hookmgr.go

+		runner:       runner,
+		repository:   newRepository(),
+		contexts:     make(map[string]*Context),
+		snapContexts: newSnapContexts(s),


We actually don't want these saved in memory persistently. Every time a snapctl call comes over, we need a new context which should live during that call only (thus ephemeral), otherwise two concurrent snapctl calls will corrupt each other's state. This is similar to how hooks work: two hooks, two contexts, even though the snap and the hook may be exactly the same.

niemeyer · 2017-02-13T10:56:11Z

overlord/hookstate/ctlcmd/set.go

@@ -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()


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.

niemeyer · 2017-03-10T07:40:50Z

@stolowski This needs some love, when the current tasks are out of our pipeline.

niemeyer · 2017-03-13T14:44:37Z

This is open for a very long time, has open reviews for a month, and is not being worked on. Closing until @stolowski can work on it.

stolowski added 22 commits January 9, 2017 09:19

Context Map skeleton.

13e3e77

Pass hookmgr.

224d154

Compiles.

586f024

SnapContext getter.

9a3875a

Merge branch 'master' into snapctl-outside-hooks

4543fd8

Actually create context files.

6da737b

Make existing tests happy.

b5704fc

Cleanup.

1364229

Set SNAP_CONTEXT via snap-confine.

94e9305

Updated apparmor profile of snap-confine.

cd37165

Added context-support, 1 part of changes suggested by zyga.

b9e3467

Compiles.

b773c05

Use open/read instead of stdio.

43e6b3a

Added ensureState method.

f38433d

Don't overwrite SNAP_CONTEXT in snap-confine as this breaks hooks.

8ca1d0c

Get SnapContext in the api if hook context doesn't exist.

aba6e2e

Restore contexts from the state.

17efd17

Don't access task in Context if null, keep state reference in Context.

daa5093

Spread test.

c078891

Added missing configure hook to the test. Remove snap context when sn…

3957a36

…ap is removed.

Test that context file exists / is removed.

05c5d4c

Handle remove-snap-context.

e82353d

pedronis reviewed Jan 17, 2017

View reviewed changes

stolowski added 2 commits January 17, 2017 14:51

Merge branch 'master' into snapctl-outside-hooks

3dfbbf3

Fix test failure.

e5cdff7

zyga requested changes Jan 17, 2017

View reviewed changes

zyga reviewed Jan 17, 2017

View reviewed changes

stolowski added 2 commits January 18, 2017 09:27

Merge branch 'master' into snapctl-outside-hooks

19cba55

Addressed most of the comments re snap-confine.

d68694f

stolowski added 3 commits January 18, 2017 14:00

Use sc_error.

bda5326

Verify snap name.

ba86d40

Overwrite SNAP_CONTEXT unless running a hook.

9b8a95f

zyga reviewed Jan 19, 2017

View reviewed changes

niemeyer requested changes Feb 13, 2017

View reviewed changes

niemeyer closed this Mar 13, 2017

This was referenced May 16, 2017

many: snapctl outside hooks v2 #3328

Closed

many: snapctl outside hooks #3340

Merged

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

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

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

Conversation

stolowski commented Jan 17, 2017

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Mar 10, 2017

niemeyer commented Mar 13, 2017