diff --git a/cmd/mesh-control/plan.go b/cmd/mesh-control/plan.go index 738585a..e9f1579 100644 --- a/cmd/mesh-control/plan.go +++ b/cmd/mesh-control/plan.go @@ -91,14 +91,14 @@ func planFor(ctx context.Context, open *stores, nodeName string) (catalogue.Reso resolved.Needs[i].Sealed = sealed continue } - secret, err := inv.SecretFor(ctx, n.Name, nodeName, n.From) + secret, err := inv.SecretFor(ctx, n.Name, nodeName, n.For, n.From) if err != nil { // Said rather than skipped. A machine that resolves cleanly and receives no // credential is one that will fail to authenticate at some later, less obvious // moment. return catalogue.Resolution{}, nil, fmt.Errorf( - "%s needs %s from %s and no credential could be made for it: %w", - nodeName, n.Name, n.From, err) + "%s on %s needs %s from %s and no credential could be made for it: %w", + n.For, nodeName, n.Name, n.From, err) } resolved.Needs[i].Sealed = secret.ForConsumer } @@ -403,10 +403,17 @@ func grantsFor(ctx context.Context, open *stores, node string) ([]catalogue.Gran // run would have the provider create a user nothing uses. continue } - from, values, err := plan.ContributionsTo(s.Name, settings) + values, asks, err := plan.ContributionsFrom(s.Name, s.ConsumerModule, settings) if err != nil { return nil, err } + from := s.ConsumerModule + if !asks { + // That module no longer wants this. Left empty, which is what the declaration reads + // as "nobody asks for it any more" — and is how a login is withdrawn rather than kept + // working for ever after its consumer went away. + from = "" + } out = append(out, catalogue.Grant{ Provision: s.Name, Consumer: s.Consumer, At: onNetwork[s.Consumer], From: from, Values: values, Sealed: s.ForProvider}) diff --git a/cmd/mesh-control/rotate.go b/cmd/mesh-control/rotate.go index a69aab3..e7e7c96 100644 --- a/cmd/mesh-control/rotate.go +++ b/cmd/mesh-control/rotate.go @@ -81,19 +81,21 @@ func rotateCommand(ctx context.Context, args []string) error { fmt.Printf("rotating %s for %d holder(s):\n", provision, len(holders)) for _, h := range holders { - fmt.Printf(" %s from %s\n", h.Consumer, h.Provider) + // The module, because a machine may hold several credentials for one provision and + // rotating "anchor's database password" now means rotating three of them. + fmt.Printf(" %s on %s, from %s\n", h.ConsumerModule, h.Consumer, h.Provider) } for _, h := range holders { - if err := inv.RotateSecret(ctx, h.Provision, h.Consumer, h.Provider); err != nil { + if err := inv.RotateSecret(ctx, h.Provision, h.Consumer, h.ConsumerModule, h.Provider); err != nil { // Partly rotated, and said so plainly. What is gone is remade on the next push, so // the remedy is to run this again rather than to repair anything — but a machine // whose secret was discarded and not resent is holding a credential the provider is // about to stop honouring, and that is worth knowing now. return fmt.Errorf( - "rotating %s for %s from %s: %w\n\nSome credentials were discarded and not yet "+ - "sent. Run this again once the cause is fixed", - h.Provision, h.Consumer, h.Provider, err) + "rotating %s for %s on %s from %s: %w\n\nSome credentials were discarded and "+ + "not yet sent. Run this again once the cause is fixed", + h.Provision, h.ConsumerModule, h.Consumer, h.Provider, err) } } diff --git a/examples/objectstore-provisioner/main.go b/examples/objectstore-provisioner/main.go index 5eb0f7b..bad4567 100644 --- a/examples/objectstore-provisioner/main.go +++ b/examples/objectstore-provisioner/main.go @@ -192,7 +192,10 @@ func run(ctx context.Context) error { return fmt.Errorf("%s's credential should be at %s and is not there", c.Node, c.Secret) } - key := mark + c.Node + // One access key per consumer, and a consumer is a module on a machine (novox/hq + // 04-ISSUES/022) — otherwise every service on a node shares one key, and the policy that + // confines each to its own bucket confines none of them. + key := mark + c.Node + "_" + c.From wanted[key] = true if err := ensureBucket(ctx, bucket); err != nil { return err @@ -215,7 +218,12 @@ func run(ctx context.Context) error { // in the same sequence and a log can be compared against another. func sorted(given []contribution) []contribution { out := append([]contribution(nil), given...) - sort.Slice(out, func(i, j int) bool { return out[i].Node < out[j].Node }) + sort.Slice(out, func(i, j int) bool { + if out[i].Node != out[j].Node { + return out[i].Node < out[j].Node + } + return out[i].From < out[j].From + }) return out } diff --git a/examples/postgres-provisioner/main.go b/examples/postgres-provisioner/main.go index 7aadcc9..52c48d8 100644 --- a/examples/postgres-provisioner/main.go +++ b/examples/postgres-provisioner/main.go @@ -201,7 +201,12 @@ func run(ctx context.Context) error { return fmt.Errorf("%s's credential should be at %s and is not there", c.Node, c.Secret) } - role := mark + c.Node + // **Named after the module and the machine, not the machine** (novox/hq 04-ISSUES/022). + // A node routinely runs several services against one database server, and one role for + // all of them means gitea's login opens keycloak's data — created exactly as asked, with + // nothing anywhere to say so. It also makes withdrawal impossible: one role cannot be + // removed for one consumer while another still holds it. + role := mark + c.Node + "_" + c.From wanted[role] = true if err := ensureRole(ctx, db, role, strings.TrimSpace(string(password))); err != nil { return err @@ -217,7 +222,32 @@ func run(ctx context.Context) error { return revokeOrphans(ctx, db, wanted) } +// identifierLimit is where PostgreSQL stops reading a name: NAMEDATALEN - 1. +const identifierLimit = 63 + +// usableRole refuses a role name PostgreSQL would silently shorten. +// +// **Truncation is a NOTICE, not an error.** A name past the limit is cut to fit and the statement +// succeeds, so two consumers whose names agree for the first 63 bytes become one role — which is +// the exact fault 022 was about, reappearing at a length nobody would think to test. Refusing is +// the only honest answer: the provisioner cannot shorten the name itself without inventing a +// second naming scheme that the mesh does not know about, and would then be creating a login the +// mesh cannot name. +func usableRole(role string) error { + if len(role) <= identifierLimit { + return nil + } + return fmt.Errorf( + "the role for this consumer would be %q, which is %d bytes and PostgreSQL keeps %d — "+ + "it would be shortened silently, and another consumer shortened to the same name "+ + "would share the login. Shorten the node or module name", + role, len(role), identifierLimit) +} + func ensureRole(ctx context.Context, db *pgx.Conn, role, password string) error { + if err := usableRole(role); err != nil { + return err + } var exists bool if err := db.QueryRow(ctx, `select true from pg_roles where rolname = $1`, role).Scan(&exists); err != nil && err != pgx.ErrNoRows { @@ -300,7 +330,12 @@ func revokeOrphans(ctx context.Context, db *pgx.Conn, wanted map[string]bool) er // the output of one can be compared with another. func sorted(given []contribution) []contribution { out := append([]contribution{}, given...) - sort.Slice(out, func(i, j int) bool { return out[i].Node < out[j].Node }) + sort.Slice(out, func(i, j int) bool { + if out[i].Node != out[j].Node { + return out[i].Node < out[j].Node + } + return out[i].From < out[j].From + }) return out } diff --git a/examples/postgres-provisioner/where_test.go b/examples/postgres-provisioner/where_test.go index 49288f6..9c0cf6e 100644 --- a/examples/postgres-provisioner/where_test.go +++ b/examples/postgres-provisioner/where_test.go @@ -67,3 +67,19 @@ func TestAProvisionerWithNoDatabaseSaysSo(t *testing.T) { t.Fatal("a provisioner that does not know which database it owns reported one") } } + +// PostgreSQL cuts an identifier at 63 bytes and says so only as a notice, so two consumers whose +// role names agree that far would quietly become one login — 022 again, at a length nobody tests. +func TestARoleNameTooLongToBeDistinctIsRefused(t *testing.T) { + if err := usableRole("mesh_anchor_gitea"); err != nil { + t.Fatalf("an ordinary name was refused: %v", err) + } + long := "mesh_" + strings.Repeat("n", 40) + "_" + strings.Repeat("m", 40) + err := usableRole(long) + if err == nil { + t.Fatal("a role name PostgreSQL would shorten was accepted") + } + if !strings.Contains(err.Error(), "share the login") { + t.Errorf("the refusal does not say what goes wrong: %v", err) + } +} diff --git a/internal/catalogue/brokered_test.go b/internal/catalogue/brokered_test.go index 3a927ae..09d4cf2 100644 --- a/internal/catalogue/brokered_test.go +++ b/internal/catalogue/brokered_test.go @@ -242,15 +242,19 @@ func TestAnAppIsToldWhereItsDatabaseIs(t *testing.T) { } } -func TestItSaysItCarriesNoCredential(t *testing.T) { +func TestItSaysWhereTheCredentialIsInstead(t *testing.T) { // A missing field looks like a bug; a stated absence looks like a boundary. Somebody wiring // this up must not spend an afternoon looking for the password field. + // + // It used to say only that the mesh had no way to issue one, which stopped being true when + // 021 was fixed. A stated absence is only useful while it is accurate — once it is not, it + // sends the reader somewhere there is nothing to find. got, _ := Resolve(boundShelf(), []string{"meshboard"}, reachable(), World{Offered: map[string][]Provider{"postgres-database": {{Node: "anchor", At: "anchor.internal"}}}}) told := binding(t, mustDeclare(t, got)) note, _ := told["generated"].(string) - if !strings.Contains(note, "no credential") { - t.Fatalf("the file does not say what it does not carry: %v", note) + if !strings.Contains(note, "not here") || !strings.Contains(note, "secrets") { + t.Fatalf("the file does not say where the credential is instead: %v", note) } for key := range told { if strings.Contains(key, "password") || strings.Contains(key, "secret") { diff --git a/internal/catalogue/contributes_test.go b/internal/catalogue/contributes_test.go index fc0aac8..e572736 100644 --- a/internal/catalogue/contributes_test.go +++ b/internal/catalogue/contributes_test.go @@ -287,7 +287,9 @@ func TestTheManifestNamesTheFileRatherThanCarryingTheCredential(t *testing.T) { // where to find it — and the readable half therefore stays readable. out := oneGrant(t) given := grantedTo(t, out) - if given[0].Secret != "/var/lib/postgres/grants/workstation.secret" { + // Named after the machine and the module, because a consumer is both (novox/hq + // 04-ISSUES/022). The machine alone, two modules on one node wrote to one path. + if given[0].Secret != "/var/lib/postgres/grants/workstation.meshboard.secret" { t.Fatalf("the manifest does not name the credential's file: %q", given[0].Secret) } for _, r := range out { @@ -302,7 +304,7 @@ func TestTheManifestNamesTheFileRatherThanCarryingTheCredential(t *testing.T) { func TestTheCredentialItselfLandsSealedBesideIt(t *testing.T) { for _, r := range oneGrant(t) { - if r["path"] != "/var/lib/postgres/grants/workstation.secret" { + if r["path"] != "/var/lib/postgres/grants/workstation.meshboard.secret" { continue } if r["sealed"] != "c2VhbGVk" { diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index e76cdd3..6e32638 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -46,10 +46,14 @@ type OpensPorts interface { type Grant struct { // Provision is what was required. Provision string - // Consumer is the node that will use it, which is also what names the file. + // Consumer is the node that will use it. Consumer string - // From is the module on that machine which asked, so the provider can name what it creates - // after the thing using it rather than after the machine. + // From is the module on that machine which asked. + // + // **Part of who this credential is for, not a label** (novox/hq 04-ISSUES/022). Together with + // Consumer it names one consumer; the node alone does not, because a machine routinely runs + // several modules wanting the same thing. It is also what the provider names the role or the + // bucket or the client after, so withdrawing one consumer does not take another's away. From string // Values are what that module contributed — the name it wants, and anything else the // provision's own vocabulary defines. @@ -180,7 +184,12 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { for _, to := range sortedKeys(m.Secrets) { var found *Needed for i, n := range r.Needs { - if n.Name == to { + // **This module's need, not the provision's** (novox/hq 04-ISSUES/022). Matching + // on the name alone, every consumer of a provision took whichever credential + // happened to be last in the list — so on a node with two of them, one module + // would be given the other's password and fail to authenticate with a valid + // credential belonging to somebody else. + if n.Name == to && n.For == m.Module { found = &r.Needs[i] } } @@ -222,9 +231,9 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { continue } first = append(first, map[string]any{ - "id": GrantID(to, g.Consumer), + "id": GrantID(to, g.Consumer+"."+g.From), "type": "file", - "path": grantPath(m.Grants[to], g.Consumer), + "path": grantPath(m.Grants[to], g.Consumer, g.From), "sealed": g.Sealed, }) } @@ -232,7 +241,7 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { for _, to := range sortedKeys(m.Binds) { var found *Needed for i, n := range r.Needs { - if n.Name == to { + if n.Name == to && n.For == m.Module { found = &r.Needs[i] } } @@ -389,8 +398,13 @@ type Contribution struct { // // Suffixed, so the directory can also hold whatever the module writing it keeps there and so a // node named like something else in that directory cannot collide with it. -func grantPath(directory, consumer string) string { - return strings.TrimRight(directory, "/") + "/" + consumer + ".secret" +// One file per consumer, and a consumer is a module on a machine (novox/hq 04-ISSUES/022). +// +// Named after both. Named after the machine alone, two modules on one node wrote to one path: the +// second overwrote the first, and the provisioner — reading a directory — saw one consumer where +// there were two. +func grantPath(directory, consumer, module string) string { + return strings.TrimRight(directory, "/") + "/" + consumer + "." + module + ".secret" } // contributions collects what every module in this set contributes, by requirement. @@ -411,7 +425,10 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, if sorted[i].Provision != sorted[j].Provision { return sorted[i].Provision < sorted[j].Provision } - return sorted[i].Consumer < sorted[j].Consumer + if sorted[i].Consumer != sorted[j].Consumer { + return sorted[i].Consumer < sorted[j].Consumer + } + return sorted[i].From < sorted[j].From }) for _, g := range sorted { if g.From == "" { @@ -421,7 +438,7 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, } out[g.Provision] = append(out[g.Provision], Contribution{ From: g.From, Node: g.Consumer, At: g.At, Values: g.Values, - Secret: grantPath(directories[g.Provision], g.Consumer), + Secret: grantPath(directories[g.Provision], g.Consumer, g.From), }) } for _, m := range modules { @@ -495,8 +512,12 @@ func boundFile(n Needed, path string) (map[string]any, error) { "from": n.From, "at": where, "serves": n.Serves, + // **Where the credential is, not what it is.** It stopped being true that the mesh + // cannot issue one when 021 was fixed, and a comment asserting a fact about the mesh that + // has become false is worse than none — somebody reads it and stops looking. "generated": "by the mesh — do not edit; replaced whenever this changes. " + - "It carries no credential: the mesh has no way to issue one yet", + "The credential is not here: it is sealed, in the file this module's manifest " + + "names under `secrets`", }, "", " ") if err != nil { return nil, err @@ -507,34 +528,31 @@ func boundFile(n Needed, path string) (map[string]any, error) { }, nil } -// ContributionsTo is what this node's set asked of one requirement, settled. +// ContributionsFrom is what one module on this node asked of one requirement, settled. // // Exported because a provider's grants are assembled from its consumers' resolutions, one machine // at a time, and the alternative was for the control plane to reimplement settling. -func (r Resolution) ContributionsTo(requirement string, settings SettingsBy) ( - string, map[string]any, error) { +// +// **One module, not one machine** (novox/hq 04-ISSUES/022). This used to take a requirement alone +// and refuse whenever two modules wanted it — correctly, given what it had: the credential was +// keyed by node, so the two would have shared one, and sharing is worse than refusing. But the +// arrangement refused is the ordinary one. A node running eight services against one database is +// not an edge case; it is what a machine looks like. Now each consumer has its own credential and +// there is nothing left to refuse. +func (r Resolution) ContributionsFrom(requirement, module string, settings SettingsBy) ( + map[string]any, bool, error) { all, err := r.contributions(settings, nil, nil) if err != nil { - return "", nil, err + return nil, false, err } - given := all[requirement] - if len(given) == 0 { - return "", nil, nil - } - if len(given) > 1 { - // Two modules on one machine wanting the same provision would share one credential, and - // the provider would be told to create one thing under two names. Refused rather than - // resolved by picking, which is the rule everywhere else here. - var who []string - for _, g := range given { - who = append(who, g.From) + for _, g := range all[requirement] { + if g.From == module { + return g.Values, true, nil } - sort.Strings(who) - return "", nil, fmt.Errorf( - "%s has %d modules asking for %q and they would share one credential: %s", - r.Node, len(given), requirement, strings.Join(who, ", ")) } - return given[0].From, given[0].Values, nil + // Nothing on that machine asks for this any more. Said as "not found" rather than as an + // error: it is how a credential is withdrawn, and the provider removes what nobody asks for. + return nil, false, nil } // here is the module on this same machine that answers a requirement, as a binding. diff --git a/internal/catalogue/resolve.go b/internal/catalogue/resolve.go index 6d0763c..a28eda2 100644 --- a/internal/catalogue/resolve.go +++ b/internal/catalogue/resolve.go @@ -388,6 +388,18 @@ func Resolve(catalogue map[string]Manifest, assigned []string, node Node, world } } + // One need per module that wants it, rather than one per name (novox/hq 04-ISSUES/022). + // + // **A consumer is a module on a machine, not a machine.** The walk above is a work-list over + // names, so a requirement three modules share is visited once and produced one need, carrying + // whichever module happened to mention it first. Everything downstream inherited that: one + // credential, named after a node, and the other two consumers given nothing at all — a + // service that resolves cleanly and then cannot authenticate, which is the exact shape of + // 021. + // + // The record pass below already gets this right and says so. It is the same rule. + needs = perConsumer(needs, order, catalogue) + // What is answered by a record rather than by a machine. // // A post-pass, deliberately: nothing about it depends on the order requirements were walked @@ -664,3 +676,38 @@ func providersFirst(order []string, shelf map[string]Manifest) []string { } return out } + +// perConsumer turns one need per provision into one need per module that wants it. +// +// Order follows the resolved modules rather than a map, so the same set always produces the same +// needs — a declaration whose contents move for no reason makes every push look like a change. +// +// A need nothing in the set wants is kept as it is rather than dropped. That should not happen; +// if it does, the honest outcome is an extra credential nobody reads, not a consumer silently +// losing the one it depends on. +func perConsumer(needs []Needed, order []string, catalogue map[string]Manifest) []Needed { + out := make([]Needed, 0, len(needs)) + for _, n := range needs { + var wanted bool + for _, name := range order { + m, known := catalogue[name] + if !known { + continue + } + for _, want := range m.Wants() { + if want != n.Name { + continue + } + copied := n + copied.For = m.Module + out = append(out, copied) + wanted = true + break + } + } + if !wanted { + out = append(out, n) + } + } + return out +} diff --git a/internal/catalogue/two_consumers_test.go b/internal/catalogue/two_consumers_test.go new file mode 100644 index 0000000..f989781 --- /dev/null +++ b/internal/catalogue/two_consumers_test.go @@ -0,0 +1,143 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// A node runs several modules that all want one database (novox/hq 04-ISSUES/022). +// +// **The ordinary arrangement, and it could not be planned at all.** The walk that resolves a node +// is a work-list over names, so a requirement three modules shared was visited once and produced +// one need — carrying whichever module mentioned it first. Everything downstream inherited that: +// one credential, keyed by machine, named after a machine by the provisioner. +// +// The symptom had two halves and only one was loud. The provider refused, naming the modules and +// saying they would share one credential, which reads as a decision. The consumer did not: it +// resolved cleanly, wrote one module's credential file, and left the other two absent — a service +// that starts and cannot authenticate, with nothing anywhere saying why. That is the shape of 021 +// again, on a different axis. + +func threeConsumers() map[string]Manifest { + return shelf( + Manifest{Module: "postgres", Version: "1", Provides: FromAnywhere("postgres-database")}, + Manifest{Module: "gitea", Version: "1", Requires: []string{"postgres-database"}, + Contributes: map[string]map[string]any{"postgres-database": {"name": "gitea"}}, + Secrets: map[string]string{"postgres-database": "/var/lib/gitea/db.secret"}}, + Manifest{Module: "keycloak", Version: "1", Requires: []string{"postgres-database"}, + Contributes: map[string]map[string]any{"postgres-database": {"name": "keycloak"}}, + Secrets: map[string]string{"postgres-database": "/var/lib/keycloak/db.secret"}}, + Manifest{Module: "umami", Version: "1", Requires: []string{"postgres-database"}, + Contributes: map[string]map[string]any{"postgres-database": {"name": "umami"}}, + Secrets: map[string]string{"postgres-database": "/var/lib/umami/db.secret"}}, + ) +} + +func TestEveryConsumerOnANodeGetsItsOwnCredential(t *testing.T) { + got, err := Resolve(threeConsumers(), []string{"gitea", "keycloak", "umami"}, + reachable(), World{Offered: onNetwork("anchor")}) + if err != nil { + t.Fatal(err) + } + if len(got.Needs) != 3 { + t.Fatalf("three modules want a database and the node has %d need(s): %v", + len(got.Needs), got.Needs) + } + for _, want := range []string{"gitea", "keycloak", "umami"} { + var found bool + for _, n := range got.Needs { + if n.For == want { + found = true + } + } + if !found { + t.Errorf("%s wants a database and no credential is made for it", want) + } + } +} + +// Each consumer's own credential reaches its own file. Matching on the provision alone, every +// consumer took whichever need was last — a module handed somebody else's password, which is a +// valid credential and therefore fails in a way that looks like a configuration error. +func TestEachConsumerGetsItsOwnCredentialAndNotAnothersFile(t *testing.T) { + got, err := Resolve(threeConsumers(), []string{"gitea", "keycloak", "umami"}, + reachable(), World{Offered: onNetwork("anchor")}) + if err != nil { + t.Fatal(err) + } + for i := range got.Needs { + got.Needs[i].Sealed = "sealed-for-" + got.Needs[i].For + } + out, err := got.Declaration(Rendering{}) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"gitea", "keycloak", "umami"} { + var seen bool + for _, r := range out { + if r["path"] != "/var/lib/"+want+"/db.secret" { + continue + } + seen = true + if r["sealed"] != "sealed-for-"+want { + t.Errorf("%s was given %v, which belongs to something else", want, r["sealed"]) + } + } + if !seen { + t.Errorf("%s resolved and its credential file was never written", want) + } + } +} + +// The provider is told about all three, separately, and names each grant after the module. +func TestAProviderIsToldAboutEveryConsumerOnOneMachine(t *testing.T) { + provider, err := Resolve(shelf(Manifest{ + Module: "postgres", Version: "1", Provides: FromAnywhere("postgres-database"), + Grants: map[string]string{"postgres-database": "/var/lib/postgres/grants"}, + Receives: map[string]string{"postgres-database": "/var/lib/postgres/grants/mesh.json"}, + }), []string{"postgres"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + + var grants []Grant + for _, who := range []string{"gitea", "keycloak", "umami"} { + grants = append(grants, Grant{ + Provision: "postgres-database", Consumer: "anchor", From: who, + Values: map[string]any{"name": who}, Sealed: "sealed-for-" + who}) + } + out, err := provider.Declaration(Rendering{Grants: grants}) + if err != nil { + t.Fatal(err) + } + + for _, who := range []string{"gitea", "keycloak", "umami"} { + path := "/var/lib/postgres/grants/anchor." + who + ".secret" + var found bool + for _, r := range out { + if r["path"] == path { + found = true + if r["sealed"] != "sealed-for-"+who { + t.Errorf("%s's grant holds %v", who, r["sealed"]) + } + } + } + if !found { + t.Errorf("the provider was never told to create a login for %s", who) + } + } + + // And all three appear in the readable manifest, so the provisioner sees three consumers + // where there are three. Named after one machine, they were one path and the last won. + for _, r := range out { + if r["path"] != "/var/lib/postgres/grants/mesh.json" { + continue + } + body := r["content"].(string) + for _, who := range []string{"gitea", "keycloak", "umami"} { + if !strings.Contains(body, `"`+who+`"`) { + t.Errorf("the provider's manifest never mentions %s: %s", who, body) + } + } + } +} diff --git a/internal/inventory/migrations/0015-a-consumer-is-a-module-on-a-machine.sql b/internal/inventory/migrations/0015-a-consumer-is-a-module-on-a-machine.sql new file mode 100644 index 0000000..3fb6259 --- /dev/null +++ b/internal/inventory/migrations/0015-a-consumer-is-a-module-on-a-machine.sql @@ -0,0 +1,31 @@ +-- A credential belongs to a consumer, and a consumer is a module on a machine. +-- +-- novox/hq 04-ISSUES/022. The key was (provision, consumer node, provider node), so "who is +-- asking" was answered by naming a host. A node running three modules against one database server +-- had one credential between them: the provisioner created one role, `mesh_`, owning every +-- database it was asked for, and gitea's login opened keycloak's data. Nothing anywhere would +-- have said so -- from the provisioner's side it created exactly what it was asked to create. +-- +-- **Two modules on one node are as separate as two on different nodes.** They are different +-- containers, on different networks, with different data. This is the same correction as 021, +-- which found the machine wrongly treated as a trust boundary; here it was wrongly treated as an +-- identity. +-- +-- It also restores withdrawal. One role per node cannot express "this module no longer has a +-- login and the others still do", so a consumer that went away kept a working credential for as +-- long as any other consumer on that machine remained. + +alter table secret add column consumer_module text references module(name) on delete cascade; + +-- Existing rows cannot say which module they were for, because at the time nothing recorded it. +-- +-- **Discarded rather than guessed.** A secret is remade on the next declaration and reaches both +-- ends in the same push, which is exactly what rotation does -- so this costs one rotation and +-- nothing else. Backfilling with "whichever module resolves first" would be inventing an answer +-- to the question this migration exists because nobody could answer. +delete from secret; + +alter table secret alter column consumer_module set not null; + +alter table secret drop constraint secret_pkey; +alter table secret add primary key (name, consumer, consumer_module, provider); diff --git a/internal/inventory/secrets.go b/internal/inventory/secrets.go index dccc1b4..4d4d50b 100644 --- a/internal/inventory/secrets.go +++ b/internal/inventory/secrets.go @@ -14,16 +14,21 @@ import ( // Secret is one provision's credential, sealed to each end. type Secret struct { - Name string - Consumer string - Provider string - ForConsumer string - ForProvider string - ConsumerKey string - ProviderKey string + Name string + Consumer string + // ConsumerModule is which module on that machine it is for. + // + // **Part of the key, not a label** (novox/hq 04-ISSUES/022). Two modules on one node wanting + // the same provision are two consumers, and were one credential until this. + ConsumerModule string + Provider string + ForConsumer string + ForProvider string + ConsumerKey string + ProviderKey string } -// SecretFor is the credential for one provision between two nodes, making one the first time. +// SecretFor is the credential one module uses for one provision, making it the first time. // // **Made once and kept**, rather than regenerated whenever it is asked for. A secret that changed // on every declaration would restart both ends on every push and would mean the password a @@ -34,7 +39,8 @@ type Secret struct { // can no longer open what was sealed to the old one, so keeping the blob would deliver something // unreadable for ever. The new secret reaches both ends in the same push, which is the only // moment they can be changed together. -func (i *Inventory) SecretFor(ctx context.Context, name, consumer, provider string) (Secret, error) { +func (i *Inventory) SecretFor(ctx context.Context, name, consumer, consumerModule, provider string) ( + Secret, error) { consumerKey, err := i.SealingKeyOf(ctx, consumer) if err != nil { return Secret{}, err @@ -56,11 +62,12 @@ func (i *Inventory) SecretFor(ctx context.Context, name, consumer, provider stri var held Secret err = i.store.Pool().QueryRow(ctx, `select for_consumer, for_provider, consumer_key, provider_key from secret - where name = $1 and consumer = $2 and provider = $3`, - name, consumerNode.ID, providerNode.ID). + where name = $1 and consumer = $2 and consumer_module = $3 and provider = $4`, + name, consumerNode.ID, consumerModule, providerNode.ID). Scan(&held.ForConsumer, &held.ForProvider, &held.ConsumerKey, &held.ProviderKey) if err == nil && held.ConsumerKey == consumerKey && held.ProviderKey == providerKey { held.Name, held.Consumer, held.Provider = name, consumer, provider + held.ConsumerModule = consumerModule return held, nil } @@ -69,19 +76,20 @@ func (i *Inventory) SecretFor(ctx context.Context, name, consumer, provider stri return Secret{}, err } _, err = i.store.Pool().Exec(ctx, - `insert into secret (name, consumer, provider, for_consumer, for_provider, + `insert into secret (name, consumer, consumer_module, provider, for_consumer, for_provider, consumer_key, provider_key) - values ($1, $2, $3, $4, $5, $6, $7) - on conflict (name, consumer, provider) do update set + values ($1, $2, $3, $4, $5, $6, $7, $8) + on conflict (name, consumer, consumer_module, provider) do update set for_consumer = excluded.for_consumer, for_provider = excluded.for_provider, consumer_key = excluded.consumer_key, provider_key = excluded.provider_key, created_at = now()`, - name, consumerNode.ID, providerNode.ID, + name, consumerNode.ID, consumerModule, providerNode.ID, made.ForConsumer, made.ForProvider, made.ConsumerKey, made.ProviderKey) if err != nil { return Secret{}, err } - return Secret{Name: name, Consumer: consumer, Provider: provider, + return Secret{Name: name, Consumer: consumer, ConsumerModule: consumerModule, + Provider: provider, ForConsumer: made.ForConsumer, ForProvider: made.ForProvider, ConsumerKey: made.ConsumerKey, ProviderKey: made.ProviderKey}, nil } @@ -94,7 +102,7 @@ func (i *Inventory) SecretFor(ctx context.Context, name, consumer, provider stri // // The new secret then reaches both ends on the same push, together, which is what makes rotation // a single event rather than a fanout with a window where half the mesh holds a dead credential. -func (i *Inventory) RotateSecret(ctx context.Context, name, consumer, provider string) error { +func (i *Inventory) RotateSecret(ctx context.Context, name, consumer, consumerModule, provider string) error { consumerNode, err := i.NodeByName(ctx, consumer) if err != nil { return err @@ -104,8 +112,9 @@ func (i *Inventory) RotateSecret(ctx context.Context, name, consumer, provider s return err } _, err = i.store.Pool().Exec(ctx, - `delete from secret where name = $1 and consumer = $2 and provider = $3`, - name, consumerNode.ID, providerNode.ID) + `delete from secret where name = $1 and consumer = $2 and consumer_module = $3 + and provider = $4`, + name, consumerNode.ID, consumerModule, providerNode.ID) return err } @@ -116,9 +125,9 @@ func (i *Inventory) SecretsFrom(ctx context.Context, provider string) ([]Secret, return nil, err } rows, err := i.store.Pool().Query(ctx, - `select s.name, c.name, s.for_provider from secret s + `select s.name, c.name, s.consumer_module, s.for_provider from secret s join node c on c.id = s.consumer - where s.provider = $1 order by s.name, c.name`, providerNode.ID) + where s.provider = $1 order by s.name, c.name, s.consumer_module`, providerNode.ID) if err != nil { return nil, err } @@ -127,7 +136,7 @@ func (i *Inventory) SecretsFrom(ctx context.Context, provider string) ([]Secret, var out []Secret for rows.Next() { s := Secret{Provider: provider} - if err := rows.Scan(&s.Name, &s.Consumer, &s.ForProvider); err != nil { + if err := rows.Scan(&s.Name, &s.Consumer, &s.ConsumerModule, &s.ForProvider); err != nil { return nil, err } out = append(out, s) @@ -235,7 +244,10 @@ func (i *Inventory) AcceptSecretForModule(ctx context.Context, node, module, nam type Holder struct { Provision string Consumer string - Provider string + // ConsumerModule is which module on that machine holds it. Part of what identifies a + // credential (novox/hq 04-ISSUES/022), so rotating one consumer's does not touch another's. + ConsumerModule string + Provider string } // HoldersOf is every pair sharing a credential for one provision. @@ -249,11 +261,11 @@ type Holder struct { // Empty consumer means all of them. func (i *Inventory) HoldersOf(ctx context.Context, provision, consumer string) ([]Holder, error) { rows, err := i.store.Pool().Query(ctx, - `select s.name, c.name, p.name from secret s + `select s.name, c.name, s.consumer_module, p.name from secret s join node c on c.id = s.consumer join node p on p.id = s.provider where s.name = $1 and ($2 = '' or c.name = $2) - order by c.name, p.name`, provision, consumer) + order by c.name, s.consumer_module, p.name`, provision, consumer) if err != nil { return nil, err } @@ -262,7 +274,7 @@ func (i *Inventory) HoldersOf(ctx context.Context, provision, consumer string) ( var out []Holder for rows.Next() { var h Holder - if err := rows.Scan(&h.Provision, &h.Consumer, &h.Provider); err != nil { + if err := rows.Scan(&h.Provision, &h.Consumer, &h.ConsumerModule, &h.Provider); err != nil { return nil, err } out = append(out, h) diff --git a/internal/inventory/secrets_test.go b/internal/inventory/secrets_test.go index 915cb47..309c47f 100644 --- a/internal/inventory/secrets_test.go +++ b/internal/inventory/secrets_test.go @@ -47,6 +47,15 @@ func twoNodesWithKeys(t *testing.T) (*Inventory, context.Context) { t.Fatal(err) } } + // A credential belongs to a module on a machine, so the modules holding one must exist + // before it can (novox/hq 04-ISSUES/022). Two of them, because "two consumers on one node" + // is the case that key exists for. + for _, m := range []string{"gitea", "keycloak"} { + if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: m, Version: "1"}, + Source{}); err != nil { + t.Fatal(err) + } + } return inv, ctx } @@ -54,11 +63,11 @@ func TestASecretIsMadeOnceAndKept(t *testing.T) { // Regenerating on every declaration would restart both ends on every push, and — worse — the // password a provider was told to create would never be the one its consumer was given. inv, ctx := twoNodesWithKeys(t) - first, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + first, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } - second, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + second, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -72,7 +81,7 @@ func TestTheStoredSecretIsNotTheSecret(t *testing.T) { // what an encrypted column does not achieve, because whoever runs the control plane can read // through it. inv, ctx := twoNodesWithKeys(t) - got, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + got, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -105,7 +114,7 @@ func TestANewSealingKeyMeansANewSecret(t *testing.T) { // A node that rejoined generated a new key and can no longer open what was sealed to the old // one. Keeping the blob would deliver something unreadable for ever, reported as configured. inv, ctx := twoNodesWithKeys(t) - before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -117,7 +126,7 @@ func TestANewSealingKeyMeansANewSecret(t *testing.T) { if err := inv.RecordSealingKey(ctx, node.ID, fresh); err != nil { t.Fatal(err) } - after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -133,14 +142,14 @@ func TestANewSealingKeyMeansANewSecret(t *testing.T) { func TestRotatingReachesBothEnds(t *testing.T) { inv, ctx := twoNodesWithKeys(t) - before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } - if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "provider"); err != nil { + if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } - after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -175,7 +184,7 @@ func TestAProviderIsToldEveryCredentialItMustCreate(t *testing.T) { t.Fatal(err) } for _, who := range []string{"consumer", "second-consumer"} { - if _, err := inv.SecretFor(ctx, "postgres-database", who, "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "postgres-database", who, "gitea", "provider"); err != nil { t.Fatal(err) } } @@ -206,7 +215,7 @@ func TestANodeWithNoSealingKeyCannotBeGivenASecret(t *testing.T) { t.Fatal(err) } } - _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err == nil { t.Fatal("a credential was made for nodes that cannot open one") } @@ -217,7 +226,7 @@ func TestANodeWithNoSealingKeyCannotBeGivenASecret(t *testing.T) { func TestSecretsGoWhenANodeLeaves(t *testing.T) { inv, ctx := twoNodesWithKeys(t) - if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } if _, err := inv.store.Pool().Exec(ctx, `delete from node where name = 'consumer'`); err != nil { @@ -340,7 +349,7 @@ func TestACredentialGoesWhenTheConsumerStopsAskingForIt(t *testing.T) { if err := inv.Assign(ctx, "consumer", "meshboard"); err != nil { t.Fatal(err) } - if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } @@ -370,7 +379,7 @@ func TestACredentialGoesWhenEitherMachineDoes(t *testing.T) { // The case that must not leave a live login behind: a machine removed from the mesh. Its // credentials go with it, and the provider stops being told to keep them. inv, ctx := twoNodesWithKeys(t) - if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } if _, err := inv.store.Pool().Exec(ctx, `delete from node where name = 'consumer'`); err != nil { @@ -464,12 +473,12 @@ func TestEveryHolderOfACredentialCanBeNamed(t *testing.T) { t.Fatal(err) } for _, consumer := range []string{"consumer", "third"} { - if _, err := inv.SecretFor(ctx, "postgres-database", consumer, "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "postgres-database", consumer, "gitea", "provider"); err != nil { t.Fatal(err) } } // And one for a different provision, which must not be swept up. - if _, err := inv.SecretFor(ctx, "cache", "consumer", "provider"); err != nil { + if _, err := inv.SecretFor(ctx, "cache", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } @@ -500,14 +509,14 @@ func TestEveryHolderOfACredentialCanBeNamed(t *testing.T) { // And rotating gives both ends a new credential, together — the same one. func TestRotatingGivesBothEndsTheSameNewCredential(t *testing.T) { inv, ctx := twoNodesWithKeys(t) - before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + before, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } - if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "provider"); err != nil { + if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } - after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "provider") + after, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider") if err != nil { t.Fatal(err) } @@ -534,14 +543,14 @@ func TestRotatingGivesBothEndsTheSameNewCredential(t *testing.T) { if err := inv.RecordSealingKey(ctx, third.ID, key); err != nil { t.Fatal(err) } - untouched, err := inv.SecretFor(ctx, "postgres-database", "third", "provider") + untouched, err := inv.SecretFor(ctx, "postgres-database", "third", "gitea", "provider") if err != nil { t.Fatal(err) } - if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "provider"); err != nil { + if err := inv.RotateSecret(ctx, "postgres-database", "consumer", "gitea", "provider"); err != nil { t.Fatal(err) } - again, err := inv.SecretFor(ctx, "postgres-database", "third", "provider") + again, err := inv.SecretFor(ctx, "postgres-database", "third", "gitea", "provider") if err != nil { t.Fatal(err) } diff --git a/internal/link/enrol_shape_test.go b/internal/link/enrol_shape_test.go index 58aceac..6257950 100644 --- a/internal/link/enrol_shape_test.go +++ b/internal/link/enrol_shape_test.go @@ -11,6 +11,7 @@ import ( "golang.org/x/crypto/nacl/box" + "github.com/novox/mesh-control/internal/catalogue" "github.com/novox/mesh-control/internal/inventory" "github.com/novox/mesh-control/internal/link" ) @@ -77,7 +78,14 @@ func TestWhatANodeSaysWhenItJoinsIsWhatThisMeshReads(t *testing.T) { t.Fatal(err) } - secret, err := inv.SecretFor(ctx, "postgres-database", request.Node, "the-other-end") + // A credential belongs to a module on a machine (novox/hq 04-ISSUES/022), so the module + // holding it has to exist before it can. + if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "gitea", Version: "1"}, + inventory.Source{}); err != nil { + t.Fatal(err) + } + + secret, err := inv.SecretFor(ctx, "postgres-database", request.Node, "gitea", "the-other-end") if err != nil { t.Fatalf("nothing could be sealed to a key that arrived from a real node: %v", err) }