carlos / platform Public

objstore: grant walk review fixes — kind-aware delivery, value + name rails

Four review findings, each mutation-checked against its own test:

- EnvDelivered was presence-only, so a delivered name occupied by the
  WRONG kind (a key half clobbered into a plaintext env var) read as
  healthy and the walk refused to repair the one state it exists to
  repair. Kind is now part of "present": coordinates "env", both key
  halves "secret".
- The minted credential reached appconfig as an opaque envelope, so an
  EnvironmentFile-unsafe byte would seal fine and then stall every
  host's config render. Both halves are ValidVarValue'd before sealing;
  the refusal names the variable and never the value.
- adoptBucket flowed unvalidated into the IAM policy ARN. ValidBucketName
  (exported for Task 9's door) gates the resolved name — adopted,
  carried-forward or minted — before any backend call or record write.
- A deployment that dropped its endpoint override left a stale _ENDPOINT
  pointing hosts at an address nobody uses. writeEnv unsets it, and the
  short-circuit now compares the record's endpoint against the
  deployment's so the drop is unhealthy enough to reach that unset.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 68a688d10385e07744baa130add355dba8e1f53a parent 675e3ea

3 files changed, +279 −21

internal/objstore/memprovisioner.go +8 −0
internal/objstore/walk.go +82 −21
internal/objstore/walk_test.go +189 −0

diff --git a/internal/objstore/memprovisioner.go b/internal/objstore/memprovisioner.go

index 50ec61215..cb8be9090 100644

--- a/internal/objstore/memprovisioner.go

+++ b/internal/objstore/memprovisioner.go

@@ -30,6 +30,11 @@ type MemProvisioner struct {

// to that method fails with the given error, then the map entry is

// gone and subsequent calls succeed normally.

FailNext map[string]error

+ // MintSecret, when non-empty, is the secret every MintKey hands back

+ // instead of a generated one — the seam for testing what a walk does

+ // with a secret it cannot deliver (a real backend picks the bytes, so

+ // a walk must not assume they are renderable).

+ MintSecret string

}

// NewMemProvisioner returns an empty fake that answers region for every

@@ -93,6 +98,9 @@ func (p *MemProvisioner) MintKey(ctx context.Context, user string) (string, stri

p.nextKey++

id := fmt.Sprintf("MEMKEY%d", p.nextKey)

secret := fmt.Sprintf("MEMSEC%d", p.nextKey)

+ if p.MintSecret != "" {

+ secret = p.MintSecret

+ }

u.Keys[id] = secret

return id, secret, nil

}

diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go

index b8d85859b..a1011b798 100644

--- a/internal/objstore/walk.go

+++ b/internal/objstore/walk.go

@@ -9,6 +9,7 @@ import (

"crypto/ecdh"

"errors"

"fmt"

+ "regexp"

"time"

"github.com/carlosframework/platform/internal/appconfig"

@@ -52,34 +53,62 @@ func MintedBucket(d Deployment, acct, app string) string {

// app, and the user's inline policy names exactly one bucket.

func UserName(acct, app string) string { return "carlos-store-" + acct + "-" + app }

-// EnvDelivered reports which of the delivered variable names are NOT present

-// in the app's config manifest — an empty result means fully delivered.

-// _ENDPOINT counts only when wantEndpoint (a deployment with no endpoint

-// never delivers one). A manifest that does not exist reads as every name

-// missing, which is what an undelivered app is.

+// EnvDelivered reports which of the delivered variables are NOT present in

+// the app's config manifest AS THIS PACKAGE WRITES THEM — an empty result

+// means fully delivered. _ENDPOINT counts only when wantEndpoint (a

+// deployment with no endpoint never delivers one). A manifest that does not

+// exist reads as every name missing, which is what an undelivered app is.

+//

+// Kind is part of "present": the coordinates must be plain "env" vars and

+// BOTH key halves must be "secret". A name occupied by the wrong kind counts

+// as missing, because that is what it is to the app — a key half stored as a

+// plaintext "env" var is a credential the app cannot use as one (and a

+// coordinate stored as a sealed "secret" renders from an envelope nobody

+// sealed for it). Overwriting drift like that is exactly what §7 promises a

+// grant or rotate does; reading it as healthy would leave the walk refusing

+// to repair the one state it exists to repair. Values are deliberately NOT

+// compared: this seam answers "is the set delivered", and the walk rewrites

+// every value it owns whenever it runs anyway.

// The grant walk's idempotence short-circuit and the console's missing-vars

// report are the same question asked twice, so they ask it here: a member who

-// unsets a delivered var must see the same "not delivered" the walk acts on.

+// unsets or clobbers a delivered var must see the same "not delivered" the

+// walk acts on.

func EnvDelivered(ctx context.Context, s release.Store, acct, app, prefix string, wantEndpoint bool) (missing []string, err error) {

m, err := appconfig.Get(ctx, s, acct, app)

if err != nil {

return nil, err

}

bucket, region, endpoint, accessKey, secretKey := EnvNames(prefix)

- want := []string{bucket, region}

+ type wantVar struct{ name, kind string }

+ want := []wantVar{{bucket, "env"}, {region, "env"}}

if wantEndpoint {

- want = append(want, endpoint)

+ want = append(want, wantVar{endpoint, "env"})

}

- want = append(want, accessKey, secretKey)

- for _, name := range want {

- if _, ok := m.Vars[name]; !ok {

- missing = append(missing, name)

+ want = append(want, wantVar{accessKey, "secret"}, wantVar{secretKey, "secret"})

+ for _, w := range want {

+ if v, ok := m.Vars[w.name]; !ok || v.Kind != w.kind {

+ missing = append(missing, w.name)

}

return missing, nil

}

+// bucketNameRe is the conservative intersection of S3 bucket naming rules:

+// 3-63 characters of lowercase letters, digits, dots and hyphens, starting

+// and ending alphanumeric. It deliberately refuses some names AWS would take

+// (uppercase and underscores are legal in some legacy us-east-1 buckets)

+// rather than admit any name that is not safely embeddable.

+var bucketNameRe = regexp.MustCompile(`^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$`)

+// ValidBucketName gates every bucket name this package acts on — the minted

+// one and, more importantly, an operator-supplied adopt name, which is

+// interpolated straight into the IAM policy ARN the credential is scoped by.

+// An unchecked name there is a scope the operator did not mean to grant, so

+// the rail sits in front of the walk and in front of the console door (Task

+// 9 reuses this function, so the API's refusal and the walk's cannot drift).

+func ValidBucketName(name string) bool { return bucketNameRe.MatchString(name) }

// Grant provisions the declared store and delivers its credentials, in spec

// §5 order. It is idempotent: a healthy provisioned store makes a re-run a

// no-op (no fresh key, no rewrite), and an unhealthy one — a crash anywhere

@@ -103,6 +132,12 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,

if bucket == "" {

bucket = MintedBucket(d, acct, app)

}

+ // Before anything is created or recorded: the name is about to become the

+ // IAM policy's resource ARN, so it is checked whether it was minted from a

+ // deployment prefix or handed over by an operator adopting a bucket.

+ if !ValidBucketName(bucket) {

+ return nil, fmt.Errorf("objstore: invalid bucket name %q", bucket)

+ }

if rec.Grant == nil {

rec.Grant = &GrantBlock{Backend: d.Backend, Bucket: bucket, At: now, By: actor}

if etag, err = Put(ctx, s, acct, app, rec, etag); err != nil {

@@ -110,10 +145,14 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,

}

// Idempotence short-circuit: "healthy" means the record says provisioned

- // AND every delivered var is present in config.json. Anything less and the

- // walk runs, because a missing var means the app cannot reach its store.

- if rec.Provisioned != nil {

- missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, rec.Provisioned.Endpoint != "")

+ // with THIS deployment's endpoint AND every delivered var is present, of

+ // the right kind, in config.json. Anything less and the walk runs, because

+ // a missing var means the app cannot reach its store — and an endpoint the

+ // deployment has since changed or dropped means it is reaching for the

+ // wrong one, which only a re-walk (which rewrites the set, and unsets

+ // _ENDPOINT when there is no longer an endpoint) can put right.

+ if rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint {

+ missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, d.Endpoint != "")

if err != nil {

return nil, err

}

@@ -171,6 +210,13 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,

// write, so an app never reads a bucket name paired with the previous key.

// Both key halves are sealed for the deployment's secrets key; the plaintext

// secret exists nowhere else, in no other object and no log line.

+//

+// A deployment with no endpoint UNSETS the endpoint variable rather than

+// leaving whatever is there: dropping an endpoint override is a real change

+// (the app must go back to the backend's own address), and a stale

+// _ENDPOINT would keep pointing every host at an address the deployment no

+// longer uses. The set half is always non-empty, so this never trips

+// applyAt's refusal to create a bundle on an unset-only call.

func writeEnv(ctx context.Context, s release.Store, sealPub *ecdh.PublicKey,

acct, app, prefix, bucket, region, endpoint, keyID, secret string) error {

bucketName, regionName, endpointName, accessKeyName, secretKeyName := EnvNames(prefix)

@@ -178,17 +224,32 @@ func writeEnv(ctx context.Context, s release.Store, sealPub *ecdh.PublicKey,

bucketName: {Kind: "env", Value: bucket},

regionName: {Kind: "env", Value: region},

}

+ var unsets []string

if endpoint != "" {

sets[endpointName] = appconfig.Var{Kind: "env", Value: endpoint}

+ } else {

+ unsets = append(unsets, endpointName)

}

- for name, plain := range map[string]string{accessKeyName: keyID, secretKeyName: secret} {

- env, err := appconfig.Seal(sealPub, []byte(plain))

+ // Validate the credential halves BEFORE sealing. appconfig checks "env"

+ // values at set time for exactly this reason, but a sealed secret reaches

+ // it as an opaque envelope, so an unrenderable byte would sail through

+ // here and stall the app's WHOLE config render on every host, hours later

+ // and out of sight. Refusing now costs one wasted key, which the next

+ // walk's reap collects. The errors name the variable and NEVER the value:

+ // one half is a live secret, and these strings reach logs and HTTP bodies.

+ for _, half := range []struct{ name, plain string }{

+ {accessKeyName, keyID}, {secretKeyName, secret},

+ } {

+ if err := appconfig.ValidVarValue(half.plain); err != nil {

+ return fmt.Errorf("objstore: minted credential for %s is not EnvironmentFile-safe: %w", half.name, err)

+ }

+ env, err := appconfig.Seal(sealPub, []byte(half.plain))

if err != nil {

- return fmt.Errorf("objstore: seal %s: %w", name, err)

+ return fmt.Errorf("objstore: seal %s: %w", half.name, err)

}

- sets[name] = appconfig.Var{Kind: "secret", Envelope: env}

+ sets[half.name] = appconfig.Var{Kind: "secret", Envelope: env}

}

- if _, err := appconfig.Apply(ctx, s, acct, app, sets, nil); err != nil {

+ if _, err := appconfig.Apply(ctx, s, acct, app, sets, unsets); err != nil {

return fmt.Errorf("objstore: deliver store config for %s/%s: %w", acct, app, err)

}

return nil

diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go

index 3d329253a..669a4dc7c 100644

--- a/internal/objstore/walk_test.go

+++ b/internal/objstore/walk_test.go

@@ -4,6 +4,7 @@ import (

"context"

"crypto/ecdh"

"errors"

+ "strings"

"testing"

"time"

@@ -341,6 +342,194 @@ func TestGrantIdempotentWhenHealthy(t *testing.T) {

}

+// A member (or an older tool) can occupy a delivered NAME with the wrong

+// KIND — a key half stored as a plaintext env var is the motivating case. The

+// app cannot use it, so the store is not delivered and the walk must repair.

+func TestGrantRepairsWrongKindVar(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ priv, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ declare(t, s, "bab", "titogo", "TITO_S3")

+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC()); err != nil {

+ t.Fatal(err)

+ }

+ // Clobber the secret half with a plaintext env var of the same name.

+ if _, err := appconfig.Apply(ctx, s, "bab", "titogo",

+ map[string]appconfig.Var{"TITO_S3_SECRET_KEY": {Kind: "env", Value: "oops"}}, nil); err != nil {

+ t.Fatal(err)

+ }

+ missing, err := EnvDelivered(ctx, s, "bab", "titogo", "TITO_S3", false)

+ if err != nil || len(missing) != 1 || missing[0] != "TITO_S3_SECRET_KEY" {

+ t.Fatalf("wrong-kind secret: missing = %v, %v", missing, err)

+ }

+ // Same for a coordinate occupied by a sealed secret.

+ sealed, err := appconfig.Seal(pub, []byte("carlos-store-tito-bab-titogo"))

+ if err != nil {

+ t.Fatal(err)

+ }

+ if _, err := appconfig.Apply(ctx, s, "bab", "titogo",

+ map[string]appconfig.Var{"TITO_S3_BUCKET": {Kind: "secret", Envelope: sealed}}, nil); err != nil {

+ t.Fatal(err)

+ }

+ if missing, err = EnvDelivered(ctx, s, "bab", "titogo", "TITO_S3", false); err != nil || len(missing) != 2 {

+ t.Fatalf("wrong-kind coordinate: missing = %v, %v", missing, err)

+ }

+ rec, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC())

+ if err != nil {

+ t.Fatal(err)

+ }

+ vars := mustVars(t, s, "bab", "titogo")

+ if v := vars["TITO_S3_BUCKET"]; v.Kind != "env" || v.Value != "carlos-store-tito-bab-titogo" {

+ t.Fatalf("bucket not repaired: %+v", v)

+ }

+ sk := vars["TITO_S3_SECRET_KEY"]

+ if sk.Kind != "secret" || sk.Value != "" {

+ t.Fatalf("secret not repaired: %+v", sk)

+ }

+ live := p.Users["carlos-store-bab-titogo"].Keys[rec.Provisioned.KeyID]

+ got, err := appconfig.Open(priv, sk.Envelope)

+ if err != nil || string(got) != live {

+ t.Fatalf("repaired secret = %q, %v; want %q", got, err, live)

+ }

+ if missing, err = EnvDelivered(ctx, s, "bab", "titogo", "TITO_S3", false); err != nil || len(missing) != 0 {

+ t.Fatalf("after repair: missing = %v, %v", missing, err)

+ }

+func TestValidBucketName(t *testing.T) {

+ for name, want := range map[string]bool{

+ "carlos-store-tito-bab-titogo": true,

+ "titogo-uploads": true,

+ "a.b-c9": true,

+ "abc": true,

+ "ab": false, // too short

+ "": false,

+ "UPPER-case": false,

+ "has_underscore": false,

+ "-leading": false,

+ "trailing-": false,

+ "has space": false,

+ "arn:aws:s3:::other/*": false,

+ "x" + string(make([]byte, 70)): false,

+ } {

+ if ValidBucketName(name) != want {

+ t.Errorf("ValidBucketName(%q) != %v", name, want)

+ }

+ // 63 legal, 64 not

+ if !ValidBucketName("a" + strings.Repeat("b", 61) + "c") {

+ t.Error("63-char name refused")

+ }

+ if ValidBucketName("a" + strings.Repeat("b", 62) + "c") {

+ t.Error("64-char name accepted")

+ }

+// An adopt name is interpolated into the IAM policy ARN, so a bad one must

+// never reach the backend — nor be recorded as this app's bucket.

+func TestGrantRefusesInvalidBucketName(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ declare(t, s, "bab", "titogo", "TITO_S3")

+ _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "Other-Bucket/*", time.Now().UTC())

+ if err == nil || !strings.Contains(err.Error(), "invalid bucket name") {

+ t.Fatalf("adopt of an invalid name: %v", err)

+ }

+ if len(p.Buckets) != 0 || len(p.Users) != 0 {

+ t.Fatalf("refused grant touched the backend: %v %v", p.Buckets, p.Users)

+ }

+ rec, _, err := Get(ctx, s, "bab", "titogo")

+ if err != nil || rec == nil || rec.Grant != nil {

+ t.Fatalf("refused grant was recorded: %+v, %v", rec, err)

+ }

+ // A deployment whose prefix mints an illegal name is refused the same way.

+ d := testDeployment()

+ d.BucketPrefix = "Carlos_Store-"

+ if _, err := Grant(ctx, s, p, d, pub, "bab", "titogo", "op", "", time.Now().UTC()); err == nil ||

+ !strings.Contains(err.Error(), "invalid bucket name") {

+ t.Fatalf("minted invalid name: %v", err)

+ }

+// The backend picks the secret's bytes. One that cannot be rendered into an

+// EnvironmentFile must be refused HERE, where the operator sees it, not hours

+// later on every host — and the refusal must not echo the secret.

+func TestGrantRefusesUnrenderableSecret(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ p.MintSecret = "bad\"secret\nwith\\newline"

+ declare(t, s, "bab", "titogo", "TITO_S3")

+ _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC())

+ if err == nil {

+ t.Fatal("unrenderable secret was delivered")

+ }

+ if !strings.Contains(err.Error(), "TITO_S3_SECRET_KEY") {

+ t.Fatalf("error does not name the variable: %v", err)

+ }

+ if strings.Contains(err.Error(), p.MintSecret) || strings.Contains(err.Error(), "bad") {

+ t.Fatalf("error echoed the secret: %v", err)

+ }

+ if rec, _, gerr := Get(ctx, s, "bab", "titogo"); gerr != nil || rec.Provisioned != nil {

+ t.Fatalf("record claims provisioned after a refused delivery: %+v, %v", rec, gerr)

+ }

+ if v := mustVars(t, s, "bab", "titogo"); len(v) != 0 {

+ t.Fatalf("config written despite refusal: %v", v)

+ }

+ // The wasted key is collected by the next walk's reap.

+ p.MintSecret = ""

+ rec, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC())

+ if err != nil {

+ t.Fatal(err)

+ }

+ if u := p.Users["carlos-store-bab-titogo"]; len(u.Keys) != 1 || u.Keys[rec.Provisioned.KeyID] == "" {

+ t.Fatalf("keys after recovery = %v (recorded %q)", u.Keys, rec.Provisioned.KeyID)

+ }

+// Dropping a deployment's endpoint override must converge: the stale

+// _ENDPOINT is unset, not left pointing hosts at an address nobody uses.

+func TestGrantUnsetsStaleEndpoint(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ declare(t, s, "bab", "titogo", "TITO_S3")

+ withEndpoint := testDeployment()

+ withEndpoint.Endpoint = "https://s3.example.net"

+ if _, err := Grant(ctx, s, p, withEndpoint, pub, "bab", "titogo", "op", "", time.Now().UTC()); err != nil {

+ t.Fatal(err)

+ }

+ if _, ok := mustVars(t, s, "bab", "titogo")["TITO_S3_ENDPOINT"]; !ok {

+ t.Fatal("endpoint never delivered")

+ }

+ // Same app, deployment no longer has an endpoint: the record's endpoint no

+ // longer matches, so the walk must run rather than short-circuit.

+ rec, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC())

+ if err != nil {

+ t.Fatal(err)

+ }

+ if rec.Provisioned.Endpoint != "" {

+ t.Fatalf("record still carries the endpoint: %+v", rec.Provisioned)

+ }

+ if v, ok := mustVars(t, s, "bab", "titogo")["TITO_S3_ENDPOINT"]; ok {

+ t.Fatalf("stale endpoint left behind: %+v", v)

+ }

+ missing, err := EnvDelivered(ctx, s, "bab", "titogo", "TITO_S3", false)

+ if err != nil || len(missing) != 0 {

+ t.Fatalf("after endpoint drop: missing = %v, %v", missing, err)

+ }

func TestEnvDeliveredReportsMissing(t *testing.T) {

ctx := context.Background()

s := release.NewDirStore(t.TempDir())