diff --git a/internal/apply/process.go b/internal/apply/process.go index d572664..7eb2a4d 100644 --- a/internal/apply/process.go +++ b/internal/apply/process.go @@ -562,8 +562,7 @@ func DaemonRoot() string { return daemonRoot } // unitDifference names what differs between what a process's record says was written and what is wanted now, both // "sha256: ": its bundle when the digest differs, and in the unit file each directive and each environment -// word that differs, with inside a word the keys of a JSON object (and, one level down, of its objects) or the names of -// the name=value entries of a list that differ. Names only, never a value, so a secret in a word is never said. Empty +// word that differs, with inside a word the keys of a JSON object (and, one level down, of its objects) that differ. Names only, never a value, so a secret in a word is never said. Empty // when nothing differs or there was no record. func unitDifference(was, want string) string { if was == "" { @@ -630,40 +629,11 @@ func valueDifference(was, now string, depth int) string { if string(a[k]) == string(b[k]) { continue } - inner := "" - var x, y string - if json.Unmarshal(a[k], &x) == nil && json.Unmarshal(b[k], &y) == nil { - inner = valueDifference(x, y, 0) - } else { - inner = valueDifference(string(a[k]), string(b[k]), depth-1) - } + inner := valueDifference(string(a[k]), string(b[k]), depth-1) names = append(names, k+inner) } return " (" + strings.Join(names, ", ") + ")" } - entries := func(list string) map[string]string { - m := map[string]string{} - for _, e := range strings.Split(list, ",") { - if name, _, ok := strings.Cut(e, "="); ok { - m[e] = name - } - } - return m - } - before, after := entries(was), entries(now) - named := map[string]bool{} - for e, name := range before { - if _, ok := after[e]; !ok { - named[name] = true - } - } - for e, name := range after { - if _, ok := before[e]; !ok { - named[name] = true - } - } - if len(named) == 0 { - return "" - } - return " (" + strings.Join(slices.Sorted(maps.Keys(named)), ", ") + ")" + // Anything else is a value: said as differing, never split, since a name before an "=" in it can be a secret. + return "" } diff --git a/internal/apply/process_test.go b/internal/apply/process_test.go index 6428667..146ce1b 100644 --- a/internal/apply/process_test.go +++ b/internal/apply/process_test.go @@ -364,30 +364,27 @@ func TestAProcessAppliedAgainIsUnchangedAndNotRestarted(t *testing.T) { if err != nil { t.Fatal(err) } - if o := outcomeOf(fourth, "node-tools.runtime"); o.Action != "updated" || !strings.Contains(o.Detail, "its unit's Environment MESH_TOOL_MODULES (b)") { - t.Errorf("fourth apply: %+v, want updated and the word and entry that changed named", o) + if o := outcomeOf(fourth, "node-tools.runtime"); o.Action != "updated" || !strings.Contains(o.Detail, "its unit's Environment MESH_TOOL_MODULES") || + strings.Contains(o.Detail, "/y/") { + t.Errorf("fourth apply: %+v, want updated and the word that changed named, no value", o) } -} - -// A restart nobody asked for names its cause (issue 1966): the unit's own words that differ, the key inside a word that -// is a JSON object and the entry of a list, never a value. -func TestAProcessRestartedForAnInputSaysWhichInputChanged(t *testing.T) { for _, row := range []struct { name, was, now, says string }{ {"a word of a module inside a JSON word", `{"files":{"OWNED":"/a"},"mail":{"X":"1"}}`, `{"files":{"OWNED":"/b"},"mail":{"X":"1"}}`, "Environment TOOL_ENV (files (OWNED))"}, - {"an entry of a list", "a=/x/1,b=/y/2", "a=/x/1,b=/y/3", "Environment TOOL_MODULES (b)"}, + {"a secret-shaped value", "c2VjcmV0MQ==", "c2VjcmV0Mg==", "Environment TOOL_VALUE"}, + {"a URL with a password", "postgres://u:hunter1@h/db?sslmode=x", "postgres://u:hunter2@h/db?sslmode=x", "Environment TOOL_VALUE"}, {"nothing changed", "a=/x/1", "a=/x/1", ""}, } { t.Run(row.name, func(t *testing.T) { - word := "TOOL_MODULES" + word := "TOOL_VALUE" if strings.HasPrefix(row.was, "{") { word = "TOOL_ENV" } + r := aProcess() mk := func(v string) string { - r := aProcess() - r.Env = map[string]string{word: v, "SECRET": "never-said-" + v} + r.Env = map[string]string{word: v} return "sha256:" + strings.Repeat("a", 64) + " " + unitFor(r) } got := unitDifference(mk(row.was), mk(row.now)) @@ -397,8 +394,13 @@ func TestAProcessRestartedForAnInputSaysWhichInputChanged(t *testing.T) { } return } - if !strings.Contains(got, row.says) || strings.Contains(got, "never-said") || strings.Contains(got, "/b") { - t.Fatalf("said %q, want it to name %q and no value", got, row.says) + for _, leak := range []string{"c2VjcmV0", "hunter", "sslmode", "postgres", "/b"} { + if strings.Contains(got, leak) { + t.Fatalf("said %q, which holds part of a value (%s)", got, leak) + } + } + if !strings.Contains(got, row.says) { + t.Fatalf("said %q, want it to name %q", got, row.says) } }) } diff --git a/internal/compose/resolution/seat_paths_test.go b/internal/compose/resolution/seat_paths_test.go index ece92a3..82b7e22 100644 --- a/internal/compose/resolution/seat_paths_test.go +++ b/internal/compose/resolution/seat_paths_test.go @@ -28,6 +28,26 @@ func TestAGeneratedResourceOnASeatsPathIsRefused(t *testing.T) { t.Errorf("the refusal does not name %s: %v", want, err) } } + // And the paths a module owns do not change with its build (issue 1966): the holder of node-files is told them in a + // word of the tool runner's unit, so a place named for each build restarted the tool runner with each build. + digest := "sha256:" + strings.Repeat("ab12", 16) + for _, row := range []struct { + name string + res map[string]any + owns string + }{ + {"a build's own place", map[string]any{"id": "next", "type": "archive", "digest": digest, "path": "/usr/lib/x/versions/ab12ab12ab12"}, "/usr/lib/x/versions"}, + {"another build's place", map[string]any{"id": "next", "type": "archive", "digest": "sha256:" + strings.Repeat("cd34", 16), "path": "/usr/lib/x/versions/cd34cd34cd34"}, "/usr/lib/x/versions"}, + {"a fixed place", map[string]any{"id": "app", "type": "archive", "digest": digest, "path": "/opt/app"}, "/opt/app"}, + {"a file", map[string]any{"id": "f", "type": "file", "path": "/etc/f"}, "/etc/f"}, + } { + t.Run(row.name, func(t *testing.T) { + got := DeclaredPaths(mcat.Manifest{Module: "m", Resources: []map[string]any{row.res}}) + if len(got) != 1 || got[0] != row.owns { + t.Fatalf("owns %v, want [%s]", got, row.owns) + } + }) + } } // The stepping back comes from the seat, and is of a rendered file only: an uplink holder declaring the @@ -54,27 +74,3 @@ func TestAnUplinkDeclaringTheResolverFileAsAResourceIsRefusedBesideTheResolver(t t.Errorf("the uplink's holder renders %v beside the resolver", got.Facts) } } - -// The paths a module owns do not change with its build (issue 1966): the holder of node-files is told them in an -// environment word of the tool runner's unit, so a place named for each build moved with every build of its module and -// restarted the tool runner each time. -func TestAPathNamedForABuildIsOwnedAsTheDirectoryHoldingEveryBuild(t *testing.T) { - digest := "sha256:" + strings.Repeat("ab12", 16) - for _, row := range []struct { - name string - res map[string]any - owns string - }{ - {"a build's own place", map[string]any{"id": "next", "type": "archive", "digest": digest, "path": "/usr/lib/x/versions/ab12ab12ab12"}, "/usr/lib/x/versions"}, - {"another build's place", map[string]any{"id": "next", "type": "archive", "digest": "sha256:" + strings.Repeat("cd34", 16), "path": "/usr/lib/x/versions/cd34cd34cd34"}, "/usr/lib/x/versions"}, - {"a fixed place", map[string]any{"id": "app", "type": "archive", "digest": digest, "path": "/opt/app"}, "/opt/app"}, - {"a file", map[string]any{"id": "f", "type": "file", "path": "/etc/f"}, "/etc/f"}, - } { - t.Run(row.name, func(t *testing.T) { - got := DeclaredPaths(mcat.Manifest{Module: "m", Resources: []map[string]any{row.res}}) - if len(got) != 1 || got[0] != row.owns { - t.Fatalf("owns %v, want [%s]", got, row.owns) - } - }) - } -}