many: improve environment handling, fixing duplicate entries #8242

zyga · 2020-03-10T14:34:53Z

This branch contains two patches: one adding three new types for working with
environment to strutil and another that uses them across the tree, making
correct environment usage easy and incorrect very hard.

The main observation is that environment is handled in one of two forms, as a
map of key=value entries and as a slice of strings that internally (hopefully)
contain key=value entries.

Technically environment is passed to a process in the form of slices. Our code
goes back and forth between the two representations and at one time appends two
slices together. This may, and indeed did, cause duplicate variable definitions
where one key has two successive values.

This is now discouraged by the use of the type system. Not to repeat the
details, I encourage you to read the commit message of both patches. There's
some small churn in snap/snapenv where maps turned to lists of arguments but
otherwise everything should be rather readable. I tried to remove all kinds of
duplicate implementations that turn maps to slices and back.

The branch can be naturally split into two patches. Let me know if this is
preferred.

This patch adds three new types and supporting methods. Environment is an unordered map representing environment entries that is convenient to modify. EnvironmentDelta is an ordered map representing changes to an environment that also supports variable substitution and expansion. Finally RawEnvironment is a slice representing key=value pairs that is useful for low-level system interfaces. Environment and EnvironmentDelta can be modified with simple map-like methods Get, Set, Del. RawEnvironment can only be parsed to an Environment. The patch also contains Environment.ApplyDelta function, which correctly applies a single delta to a given environment. The types are designed to avoid the mistake of appending two slices representing environment strings together. This was the original cause of bug https://bugs.launchpad.net/snapd/+bug/1860369 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This patch replaces all ad-hoc environment handling with the three aforementioned types. Environment is used whenever we are working with the real complete environment (starting from OSEnvironment). EnvironmentDelta is used to modify it according to snap-level and application-level or hook-level overrides. Finally RawEnvironment is only used by snap run and snap-exec, immediately before calling execve. This fixes incorrect handling of environment in snap-exec and also avoids this class of bugs by making it hard, due to type system, to express mistakes again. Fixes: https://bugs.launchpad.net/snapd/+bug/1860369 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga · 2020-03-10T14:41:55Z

cmd/snap-exec/main_test.go

-
+		var env strutil.Environment
+		for k, v := range t.env {
+			env.Set(k, v)


Nitpick, this could become:

env := strutil.NewEnvironment(t.env)

pedronis

skimmed this, some high-level comments/questions

snap/info.go

pedronis · 2020-03-10T15:57:15Z

strutil/env.go

+	return Quoted(raw)
+}
+
+// Environment is an unordered map of key=value strings.


os represents Environment as []string so probably Enviroment itself should be an OrderedMap as well

I started with an ordered map but discarded that later on as it simplifies a few things. Order is irrelevant when there are no duplicates. Environment is specifically not RawEnvironment, with all of the associated complexity of duplicates, malformed entries and the like. When we produce a RawEnvironment we always sort entries so the results are deterministic where it matters.

what does it simplifies? just syntax? a map is a map in the end

I wanted RawEnvironment to be deterministic irrespective of the order of construction of the environment. Currently I produce order sorted by keys, which is natural for an otherwise unordered map.

When Environment had an intrinsic order discarding it felt wrong. At the same time the order of environment entries should really not be anything but an implementation detail so having order there was weird.

I looked back to when app and snap environment overrides were introduced. The only reason we had order was to support expanding expressions. Since Environment specifically has just key=values, not key=expressions, having order was not required.

I only kept order for EnvironmentDelta because we already represented environment overrides (in the yaml) as ordered.

I wanted RawEnvironment to be deterministic irrespective of the order of construction of the environment.

I understand this but it's again quite a change from current behavior of snapd, somebody can have a test expecting overridden things at the tail of what they get, and now they will be sorted.

I'm not sure if I understand your concern correctly but environment order is not something applications perceive through most APIs. Only low level code can observe the order. As long as there are no maliciously crafted entries (not key=value, not duplicated keys) the order is totally irrelevant.

Specifically after we chain through snap-exec the resulting order is not even the order we set in snap-run. It is the order as it was transformed by libc running in snap-confine.

I looked a bit more and snapenv itself scrambles everything because of the through map roundtrip there, so my point here is moot. OTOH we should try to add as little as possible new code for the new types.

I think everything in this tread is addressed.

strutil/env.go

pedronis

a few more comments and suggestions

strutil/env.go

pedronis · 2020-03-11T10:38:12Z

strutil/env.go

+// If multiple keys are transformed into the same key the value of the last
+// (lexicographically) key is used. If the transformed key is empty then the
+// corresponding entry is removed.
+func (env *Environment) Transform(tr func(key, value string) (string, string)) {


I would try to avoid this part of the change for now, also because it's unclear to me that the splitting of responsibilities make sense. If this lives in osutil, the list of setgid discarded stuff could live there and the logic be moved to helpers that load os env and respectively prepare it for exec.

snap/info.go

strutil/env.go

This style is shorter and we use it nearly everywhere else. I'm about to combine more tests into this file and I don't want to invert their style over to the verbose one. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

There's a lot of churn but that's unavoidable in such a move. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

The raw environment is necessary only for executing programs. We can remove the type and replace it with []string in tests (where it has most of the usage) and a few call sites. The corresponding method from Environment was renamed to ForExec(). Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some extra methods such as Get, Set and Delete are retained but they are all arguably unnecessarily and will be removed in another pass. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This includes Get, Delete and Contains but not Set. Set has extra semantics that will be removed in another pass. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This patch removes the leftover map-like method from the Environment type. Some extra care was applied to ensure we don't try to write through a nil map, something that Set protected against. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

The main feature of what was called EnvironmentDelta is the fact it could expand expressions in variable definitions. This makes it more obvious. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

osutil/env.go

There's a lot of extra changes to make the journey more complete. First of all both snap.AppInfo and snap.HookInfo provide a method EnvStack() []osutil.ExpandableEnv which replaces the earlier EnvironmentOverrides. Instead of merging environment entries the entire chain is modeled. This will be relevant when we want to expand values. This also means that support code to make EnvStack mutable is gone. Second of all, snap/snapenv.ExecEnv has reverted to using plain Environment (aka map[string]string) for basicEnv and userEnv. The values never had expressions so using the more sophisticated code there is not really necessary. Delta from master could be reduced further but I went for clarity of the progression over the clarity of the end-to-end diff. Lastly, a small but important change, snap-exec now expands each ExpandableEnv in sequence. First for the snap-level and then for the app or hook level. This should fix the issue that each level wanted to expand but not completely erase the value set previously. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

With the current type it's not any better than just defining a map locally. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

It is only required for OSEnvironment and is not used anywhere outside of the package. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

When initially implemented as a part of this branch, SetExpandableEnv (nee ApplyDelta) behaved in somewhat unusual and "strong" way, where forward references _were_ expanded. This was not previously so and arguably is not needed. As the code stands now, we have proper chaining starting from system environment, to snap-level environment declarations (that can expand variables) and all the way down to application or hook-level environment declarations (also with variable expansions). Since such declarations are ordered this provides the equivalence of shell execution performing the same expansions. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Instead of offering specialized environment transformation functions offer just the things we need: escaping and un-escaping of unsafe variables removed by the dynamic linker when loading binaries with AT_SECURE flag. Various bits move from snap/snapenv to osutil. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga · 2020-03-11T19:02:32Z

@mvo5 squash merging this will make my weekly commit quota look terrible ;-)

codecov-io · 2020-03-11T21:09:15Z

Codecov Report

Merging #8242 into master will decrease coverage by 0.09%.
The diff coverage is 75.64%.

@@            Coverage Diff            @@
##           master    #8242     +/-   ##
=========================================
- Coverage   80.74%   80.65%   -0.1%     
=========================================
  Files         716      717      +1     
  Lines       57123    57254    +131     
=========================================
+ Hits        46124    46177     +53     
- Misses       7410     7481     +71     
- Partials     3589     3596      +7

Impacted Files	Coverage Δ
interfaces/builtin/docker_support.go	`77.5% <ø> (ø)`	⬆️
interfaces/builtin/kubernetes_support.go	`96.29% <ø> (ø)`	⬆️
snap/snaptest/snaptest.go	`70.28% <0%> (-2.1%)`	⬇️
dirs/dirs.go	`97.14% <100%> (+0.02%)`	⬆️
cmd/snap/cmd_info.go	`90.79% <100%> (-0.03%)`	⬇️
snap/info.go	`87.06% <100%> (-0.2%)`	⬇️
overlord/overlord.go	`80.35% <100%> (+0.17%)`	⬆️
cmd/snap-repair/runner.go	`84.89% <100%> (+0.08%)`	⬆️
overlord/snapstate/snapstate.go	`81.79% <100%> (+0.01%)`	⬆️
cmd/snap/cmd_run.go	`61.04% <58.33%> (-0.37%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ec5b9d...5012c15. Read the comment docs.

mvo5 · 2020-03-12T07:15:21Z

@zyga fair enough - without squash there is no way for this to go into 2.44 though

mvo5 · 2020-03-12T07:15:51Z

@zyga well, no way is too strong :) I will not do a backport PR but if you want you can

zyga · 2020-03-12T07:32:00Z

@mvo5 I think it's not going into 2.44 - mainly because we want to not rush it.

pedronis · 2020-03-12T08:08:01Z

@mvo5 I'm going to review this now given that the discussions are fresh on my mind

pedronis

did a pass, looking good, I can address most comments myself later today

pedronis · 2020-03-12T08:40:52Z

cmd/snap-exec/main.go

+	if err != nil {
+		return err
+	}
+	// NOTE: we do not do env.UnescapeSaved()


this is probably not intended, otoh given where hooks are run from for now, it's not material, symmmetry is probably preferable though

pedronis · 2020-03-12T08:42:49Z

osutil/env.go

+// Environment is modified in place. Each variable defined by expandable
+// environment is expanded according to os.Expand, using the environment itself
+// as data source. Undefined variables expand to an empty string.
+func (env *Environment) SetExpandableEnv(eenv ExpandableEnv) {


"Set" is slightly odd because usually refers to setting "single" values. I will rename to "ExtendWithExpanded", bit of a Smalltalk-like naming.

pedronis · 2020-03-12T08:43:00Z

osutil/env.go

+// as data source. Undefined variables expand to an empty string.
+func (env *Environment) SetExpandableEnv(eenv ExpandableEnv) {
+	if *env == nil {
+		*env = make(Environment)


this is not tested.

pedronis · 2020-03-12T08:43:27Z

osutil/env.go


-	return out
+func (env Environment) EscapeUnsafeVariables() {


I would prefer this to be folded into a mode of ForExec via flags.

We also lost some logic to drop already escaped variables that is on master, to be fair it was untested there, but probably best to preserve it.

pedronis · 2020-03-12T08:44:36Z

osutil/env.go

 }
+
+func (env Environment) UnescapeSaved() {


this can probably be a 2nd variant of OSEnviroment

pedronis · 2020-03-12T08:45:26Z

snap/info.go

 // IsService returns whether app represents a daemon/service.
 func (app *AppInfo) IsService() bool {
 	return app.Daemon != ""
 }

+func (app *AppInfo) EnvStack() []osutil.ExpandableEnv {


this lost the doc comment, also to keep the terminology smaller maybe we should call this EnvChain

pedronis · 2020-03-12T08:45:36Z

snap/info.go

-	env := []string{}
-	for _, k := range envMap.Keys() {
-		env = append(env, fmt.Sprintf("%s=%s", k, envMap.Get(k)))
+func (hook *HookInfo) EnvStack() []osutil.ExpandableEnv {


pedronis · 2020-03-12T08:47:50Z

snap/snapenv/snapenv.go

-		preserve = preserveUnsafeFlag
+func ExecEnv(info *snap.Info) (osutil.Environment, error) {
+	// Start with OS environment.
+	env, err := osutil.OSEnvironment()


this probably should change and not find the os enviroment on its own, tests would also be simpler

that was not possible before because the preserving logic was also here

also adjust doc comments and cover ExtendWithExpanded for the nil initial env case

support the escaping/unescaping that we use for preserving vars stripped out by ld.so for snap-confine, via variants of OSEnvironment/ForExec so that is happens at the process boundaries, loading os env and setting env for exec add/change some tests related to this adjust comments/doc comments or add

pedronis

+1

stolowski

Looks good, but one question.

stolowski · 2020-03-18T09:24:04Z

osutil/env.go

+// Environment can be manipulated with available methods and eventually
+// converted to low-level representation necessary when executing programs.
+// This approach discourages operations that could result in duplicate
+// environment variable definitions from being constructed.


stolowski · 2020-03-18T09:34:41Z

osutil/env.go

+func parseEnvEntry(entry string) (string, string, error) {
+	parts := strings.SplitN(entry, "=", 2)
+	if len(parts) != 2 {
+		return "", "", fmt.Errorf("cannot parse environment entry: %q", entry)


Hmm, a

FOO="A=B"

is a valid env value, but we will fail here; I'm not sure how likely it is to encounter it in practice; should we try to be smarter here?

Are you sure? Why would we fail? We expect exactly 2 items and use SplitN

Yeah, you're right, I missed this. Sorry about the noise ;)

stolowski

Thank you!

zyga

LGTM

Thank you for working on this @pedronis :)

zyga added 2 commits March 10, 2020 15:15

zyga added the Bug label Mar 10, 2020

zyga commented Mar 10, 2020

View reviewed changes

pedronis self-requested a review March 10, 2020 15:34

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Mar 10, 2020

pedronis reviewed Mar 10, 2020

View reviewed changes

strutil/env.go Outdated Show resolved Hide resolved

pedronis reviewed Mar 11, 2020

View reviewed changes

strutil/env.go Outdated Show resolved Hide resolved

pedronis reviewed Mar 11, 2020

View reviewed changes

zyga added 8 commits March 11, 2020 13:04

osutil/env: import all symbols from check.v1

10bbebf

This style is shorter and we use it nearly everywhere else. I'm about to combine more tests into this file and I don't want to invert their style over to the verbose one. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: move new environment code to osutil

fb14364

There's a lot of churn but that's unavoidable in such a move. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: make Environment a map[string]string

9473c68

Some extra methods such as Get, Set and Delete are retained but they are all arguably unnecessarily and will be removed in another pass. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: remove most map-like methods from Environment

e7c3280

This includes Get, Delete and Contains but not Set. Set has extra semantics that will be removed in another pass. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: remove Environment.Set

b6723a0

This patch removes the leftover map-like method from the Environment type. Some extra care was applied to ensure we don't try to write through a nil map, something that Set protected against. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

cmd/snap-exec: use NewEnvironment in test code

d64725a

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: rename EnvironmentDelta to ExpandableEnv

5cf9ab3

The main feature of what was called EnvironmentDelta is the fact it could expand expressions in variable definitions. This makes it more obvious. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

pedronis reviewed Mar 11, 2020

View reviewed changes

osutil/env.go Outdated Show resolved Hide resolved

zyga added 7 commits March 11, 2020 18:58

osutil: remove NewEnvironment

5a6e759

With the current type it's not any better than just defining a map locally. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

osutil: make ParseRawEnvironment private

4fdbc2b

It is only required for OSEnvironment and is not used anywhere outside of the package. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

many: rename ApplyDelta to SetExpandableEnv

02404db

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

osutil: update stale documentation referring to delta

681e8e9

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga requested a review from pedronis March 11, 2020 18:51

mvo5 added the Squash-merge Please squash this PR when merging. label Mar 11, 2020

mvo5 removed the Squash-merge Please squash this PR when merging. label Mar 12, 2020

pedronis added the Squash-merge Please squash this PR when merging. label Mar 12, 2020

zyga added this to the 2.45 milestone Mar 12, 2020

pedronis reviewed Mar 12, 2020

View reviewed changes

pedronis added 3 commits March 12, 2020 11:55

many: EnvStack to EnvChain, SetExpandableEnv to ExtendWithExpanded

10cc19f

also adjust doc comments and cover ExtendWithExpanded for the nil initial env case

cmd/snap,snap/snapenv: change snapenv.ExecEnv into ExtendEnvForRun

e8612e5

pedronis approved these changes Mar 12, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/lp-1860369

a670baa

stolowski reviewed Mar 18, 2020

View reviewed changes

stolowski approved these changes Mar 18, 2020

View reviewed changes

zyga commented Mar 20, 2020

View reviewed changes

zyga merged commit 913d765 into canonical:master Mar 20, 2020

zyga deleted the fix/lp-1860369 branch March 20, 2020 10:58

many: improve environment handling, fixing duplicate entries #8242

many: improve environment handling, fixing duplicate entries #8242

Conversation

zyga commented Mar 10, 2020

Choose a reason for hiding this comment

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 Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

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

zyga commented Mar 11, 2020

codecov-io commented Mar 11, 2020

Codecov Report

mvo5 commented Mar 12, 2020

mvo5 commented Mar 12, 2020

zyga commented Mar 12, 2020

pedronis commented Mar 12, 2020

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

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

stolowski 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

stolowski left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

zyga Mar 11, 2020 •

edited

Loading