diff --git a/internal/cli/aws.go b/internal/cli/aws.go index dd37439..bb4b541 100644 --- a/internal/cli/aws.go +++ b/internal/cli/aws.go @@ -24,12 +24,18 @@ func newAwsCmd(g *GlobalOpts) *cobra.Command { "LocalStack/MinIO host port and prepends --endpoint-url/--region plus dev\n" + "credentials (via the child environment). It does not reimplement any AWS call.\n\n" + "Example: devstack aws -- s3 ls", - Args: cobra.MinimumNArgs(1), + Args: cobra.ArbitraryArgs, DisableFlagParsing: true, RunE: func(cmd *cobra.Command, args []string) error { args = stripLeadingDashDash(args) - if len(args) == 0 { - return fmt.Errorf("usage: devstack aws -- (e.g. devstack aws -- s3 ls)") + // Bare `aws` or a leading -h/--help is a request for THIS shim's help — + // short-circuit before constructing any docker/S3 client or touching the + // daemon. (DisableFlagParsing means cobra does not intercept --help itself, + // so a help token would otherwise be forwarded to the real aws endpoint.) + // A help flag AFTER a subcommand (e.g. `aws -- s3 --help`) passes through + // to the user's aws unchanged. + if len(args) == 0 || isHelpFlag(args[0]) { + return cmd.Help() } awsPath, err := lookupAws() if err != nil { @@ -71,6 +77,16 @@ func lookupAws() (string, error) { return p, nil } +// isHelpFlag reports whether tok is a help request (-h/--help/help) meant for the +// shim itself rather than the forwarded aws command. +func isHelpFlag(tok string) bool { + switch tok { + case "-h", "--help", "help": + return true + } + return false +} + // stripLeadingDashDash drops a leading "--" separator (cobra with // DisableFlagParsing keeps it in args). func stripLeadingDashDash(args []string) []string { diff --git a/internal/cli/db_s3_aws_test.go b/internal/cli/db_s3_aws_test.go index a86d750..21a2765 100644 --- a/internal/cli/db_s3_aws_test.go +++ b/internal/cli/db_s3_aws_test.go @@ -124,6 +124,28 @@ func TestAwsEnvInjectsCredsNotArgv(t *testing.T) { } } +func TestAwsHelpShortCircuits(t *testing.T) { + // In an empty dir there is no workspace, so buildUpDeps + any daemon access + // would fail. `aws --help` (and bare `aws`) must still exit 0 by printing the + // shim's own help BEFORE constructing any docker/S3 client. + for _, args := range [][]string{{"aws", "--help"}, {"aws", "-h"}, {"aws"}} { + t.Run(strings.Join(args, " "), func(t *testing.T) { + t.Chdir(t.TempDir()) + var out strings.Builder + root := NewRootCmd(Options{}) + root.SetArgs(args) + root.SetOut(&out) + root.SetErr(&out) + if err := root.Execute(); err != nil { + t.Fatalf("`devstack %s` should exit 0 via help, got: %v", strings.Join(args, " "), err) + } + if !strings.Contains(out.String(), "aws") || !strings.Contains(out.String(), "Usage") { + t.Errorf("help output missing usage text: %q", out.String()) + } + }) + } +} + func TestAwsAbsentBinaryError(t *testing.T) { t.Setenv("PATH", t.TempDir()) // an empty dir → no `aws` on PATH if _, err := lookupAws(); err == nil { diff --git a/internal/cli/destroy.go b/internal/cli/destroy.go index cd90817..a8f1383 100644 --- a/internal/cli/destroy.go +++ b/internal/cli/destroy.go @@ -66,7 +66,7 @@ func newWorkspaceDestroyCmd(g *GlobalOpts) *cobra.Command { if !yes { dataLine := "Volumes and databases are PRESERVED." if purgeData { - dataLine = "WARNING: --purge-data DROPS every provisioned database/bucket/etc (DATA DESTROYED)." + dataLine = "WARNING: --purge-data DROPS every provisioned database/bucket/etc AND removes the shared volumes (DATA DESTROYED)." } prompt := fmt.Sprintf( "This tears down workspace %q (%d project stack(s)) and releases its refs/ports.\n"+ @@ -94,6 +94,9 @@ func newWorkspaceDestroyCmd(g *GlobalOpts) *cobra.Command { for _, p := range res.PurgedResources { fmt.Fprintf(w, "[ok] dropped %s %s\n", p["kind"], p["name"]) } + for _, v := range res.PurgedVolumes { + fmt.Fprintf(w, "[ok] removed volumes for %s\n", v) + } for _, e := range res.Errors { fmt.Fprintf(w, "[warn] %s\n", e) } @@ -121,6 +124,7 @@ type DestroyResult struct { Projects []string `json:"projects"` // project stacks brought down SharedStopped []string `json:"shared_stopped"` // orphaned shared services warm-stopped PurgedResources []map[string]string `json:"purged_resources,omitempty"` // --purge-data: resources dropped + PurgedVolumes []string `json:"purged_volumes,omitempty"` // --purge-data: shared volumes removed (compose down -v) Errors []string `json:"errors,omitempty"` } @@ -208,6 +212,36 @@ func destroyWorkspace(ctx context.Context, d orchestrate.UpDeps, projects []stri res.SharedStopped = gc.Stopped } + // 3b. --purge-data ALSO removes the shared stack's named volumes (the postgres + // PGDATA, MinIO object store, …) via `compose down -v`. This is gated on the + // shared stack being fully orphaned (no ref rows remain from ANY workspace), so + // destroy can never drop data another workspace still depends on. The + // data-preserving default never reaches here. + if purgeData { + remaining, err := d.DB.AllRefs() + if err != nil { + res.Errors = append(res.Errors, fmt.Sprintf("check remaining refs: %v", err)) + } else if len(remaining) == 0 { + outDir := filepath.Join(d.Model.Root, generate.GenDir, "shared") + composeFile := filepath.Join(outDir, generate.ComposeFile) + if _, err := os.Stat(composeFile); err != nil { + composeFile = "" // label-driven `compose -p devstack-shared down -v` + } + cp := docker.Compose{Project: generate.SharedStackName, File: composeFile, Dir: outDir, Runner: runner} + if err := cp.Down(ctx, true); err != nil { + res.Errors = append(res.Errors, fmt.Sprintf("purge shared volumes: %v", err)) + } else { + // Report the services whose volumes were removed (fall back to the + // stack name if nothing was warm-stopped this pass). + if len(res.SharedStopped) > 0 { + res.PurgedVolumes = append(res.PurgedVolumes, res.SharedStopped...) + } else { + res.PurgedVolumes = append(res.PurgedVolumes, generate.SharedStackName) + } + } + } + } + // 4. remove generated artifacts (.devstack): the workspace-root shared dir and // each project's dir. Best-effort — a missing dir is fine. _ = os.RemoveAll(filepath.Join(d.Model.Root, generate.GenDir)) diff --git a/internal/cli/destroy_test.go b/internal/cli/destroy_test.go index 9c23186..4d33e98 100644 --- a/internal/cli/destroy_test.go +++ b/internal/cli/destroy_test.go @@ -167,3 +167,53 @@ func TestDestroyWorkspaceTeardown(t *testing.T) { t.Error("project .devstack/ should be removed") } } + +func TestDestroyWorkspacePurgeDataRemovesSharedVolumes(t *testing.T) { + d, fr := destroyFixture(t) + ctx := context.Background() + + if err := d.Manager.RegisterUp(ctx, "app"); err != nil { + t.Fatalf("register up: %v", err) + } + + // --purge-data: once this workspace's refs are released the shared stack is + // fully orphaned, so its volumes are removed via `compose down -v`. + res := destroyWorkspace(ctx, d, []string{"app"}, true) + if len(res.Errors) != 0 { + t.Fatalf("destroy errors: %v", res.Errors) + } + if !fr.saw("-p "+generate.SharedStackName, "down", "--volumes") { + t.Errorf("purge-data must `compose down -v` the shared stack: %v", fr.cmds) + } + if len(res.PurgedVolumes) == 0 { + t.Errorf("PurgedVolumes should be populated, got %v", res.PurgedVolumes) + } +} + +func TestDestroyWorkspacePurgeDataKeepsVolumesWhenReferenced(t *testing.T) { + d, fr := destroyFixture(t) + ctx := context.Background() + + if err := d.Manager.RegisterUp(ctx, "app"); err != nil { + t.Fatalf("register up: %v", err) + } + // A SECOND project is genuinely LIVE (a running container) and still references + // the shared postgres, so reconcile keeps its ref and even --purge-data must NOT + // remove the shared volumes (data another consumer depends on survives). + mc := d.Docker.(*docker.MockClient) + mc.Containers = append(mc.Containers, docker.Container{ + ID: "other1", Name: "devstack-other-web-1", State: "running", + Labels: map[string]string{generate.LabelManaged: "true", generate.LabelProject: "other"}, + }) + if err := d.DB.AddRef("other", "web", "shared-postgres"); err != nil { + t.Fatalf("seed foreign ref: %v", err) + } + + res := destroyWorkspace(ctx, d, []string{"app"}, true) + if fr.saw(generate.SharedStackName, "down", "--volumes") { + t.Errorf("must NOT down -v the shared stack while another project refs it: %v", fr.cmds) + } + if len(res.PurgedVolumes) != 0 { + t.Errorf("PurgedVolumes must be empty when refs remain, got %v", res.PurgedVolumes) + } +} diff --git a/internal/cli/s3.go b/internal/cli/s3.go index 60daf2a..233c784 100644 --- a/internal/cli/s3.go +++ b/internal/cli/s3.go @@ -120,6 +120,11 @@ func newS3RbCmd(g *GlobalOpts) *cobra.Command { } } r := resource.Resource{Engine: "minio", Kind: "bucket", Name: bucket, Owner: proj} + if force { + // Recursively purge every object before removing the bucket, so a + // non-empty bucket can be deleted (the provisioner empties it first). + r.Params = map[string]any{"force": true} + } if err := orchestrate.DropResource(cmd.Context(), d, r, true); err != nil { return err } diff --git a/internal/orchestrate/resource_ops.go b/internal/orchestrate/resource_ops.go index 5e0762a..18680e8 100644 --- a/internal/orchestrate/resource_ops.go +++ b/internal/orchestrate/resource_ops.go @@ -20,6 +20,33 @@ import ( // CLI (internal/cli) is a thin wrapper over these; they are tested here with the // same mock docker client + injected Postgres connector the saga tests use. +// generatedCredPath is the deterministic backend identifier a generated +// credential is pushed to: devstack////. Stable across +// re-provisions of the same resource so a rotation overwrites in place. +func generatedCredPath(r resource.Resource) string { + name := r.Name + if name == "" { + name = r.Owner + } + return fmt.Sprintf("devstack/%s/%s/%s/%s", r.Owner, r.Engine, r.Kind, name) +} + +// pushGeneratedCred writes a resource's generated credential to the configured +// secrets Pusher (aws-sm/ssm/infisical). It is a no-op when no Pusher is wired — +// the value is still used to provision but is not persisted, and it is NEVER +// written to a generated file. The value travels via the Pusher's stdin/env path, +// never argv (spec 04 §7.5). +func pushGeneratedCred(ctx context.Context, d UpDeps, r resource.Resource, value string) error { + if d.CredPusher == nil { + return nil + } + entry := secrets.SecretEntry{Path: generatedCredPath(r), Value: value} + if err := d.CredPusher.Push(ctx, []secrets.SecretEntry{entry}); err != nil { + return fmt.Errorf("push generated credential for %s/%s: %w", r.Kind, r.Name, err) + } + return nil +} + // ResourceRegistry exposes the engine→Provisioner registry (Postgres live; other // engines land in Full scope), wired with the injected connector. func ResourceRegistry(connect PgConnector) *resource.Registry { @@ -118,6 +145,11 @@ func CreateResource(ctx context.Context, d UpDeps, r resource.Resource) (resourc r.Params = map[string]any{} } r.Params["password"] = pw + // Route the generated value through the secrets Pusher (when configured) + // so it is persisted to a backend, never a generated file (spec 04). + if err := pushGeneratedCred(ctx, d, r, pw); err != nil { + return nil, err + } } } target, err := engineTarget(ctx, d, r.Engine, instance) diff --git a/internal/orchestrate/resource_ops_test.go b/internal/orchestrate/resource_ops_test.go index b229389..51af7c6 100644 --- a/internal/orchestrate/resource_ops_test.go +++ b/internal/orchestrate/resource_ops_test.go @@ -2,13 +2,75 @@ package orchestrate import ( "context" + "io/fs" + "os" + "path/filepath" "slices" "strings" "testing" "github.com/open-source-cloud/devstack/internal/resource" + "github.com/open-source-cloud/devstack/internal/secrets" ) +// fakePusher records the entries a generated credential is routed to, so a test +// can assert the value went to the Pusher (a backend) and never to disk. +type fakePusher struct{ entries []secrets.SecretEntry } + +func (f *fakePusher) Push(_ context.Context, entries []secrets.SecretEntry) error { + f.entries = append(f.entries, entries...) + return nil +} + +func TestCreateResourceGeneratedCredPushedNeverOnDisk(t *testing.T) { + d, _, _ := upFixture(t) + rp := &recordingPg{} + d.PgConnect = rp.connect + fp := &fakePusher{} + d.CredPusher = fp + + _, err := CreateResource(context.Background(), d, resource.Resource{ + Engine: "postgres", Kind: "database", Name: "reports", Owner: "app", + CredKind: resource.CredGenerated, + }) + if err != nil { + t.Fatalf("CreateResource: %v", err) + } + + // The generated value was routed through the Pusher exactly once. + if len(fp.entries) != 1 { + t.Fatalf("pusher received %d entries, want 1: %+v", len(fp.entries), fp.entries) + } + val := fp.entries[0].Value + if len(val) != 24 { + t.Errorf("generated value len = %d, want 24 (%q)", len(val), val) + } + for _, r := range val { + if !((r >= '0' && r <= '9') || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z')) { + t.Fatalf("generated value not alphanumeric: %q", val) + } + } + if fp.entries[0].Path == "" { + t.Error("pushed entry must carry a backend path") + } + + // Leak assertion: the generated value must appear in NO file under the workspace + // (generated compose, overlays, the ledger — nothing). + _ = filepath.WalkDir(d.Model.Root, func(path string, de fs.DirEntry, werr error) error { + if werr != nil || de.IsDir() { + return nil + } + b, rerr := os.ReadFile(path) + if rerr != nil { + return nil + } + if strings.Contains(string(b), val) { + t.Errorf("generated credential leaked into %s", path) + } + return nil + }) +} + func TestCreateResourceImperative(t *testing.T) { d, fr, db := upFixture(t) rp := &recordingPg{} diff --git a/internal/orchestrate/resources.go b/internal/orchestrate/resources.go index 44d0217..35bb8af 100644 --- a/internal/orchestrate/resources.go +++ b/internal/orchestrate/resources.go @@ -204,19 +204,22 @@ func resourcesPhase(d UpDeps, decls []resDecl) Phase { Params: r.params, CredKind: r.cred, } // A `generated` credential gets a random value here so a consumer - // never sees a predictable password; Pusher delivery to a provider - // lands in Full scope, so the value is currently held only for the - // provisioner call (never written to a generated file). + // never sees a predictable password. When a secrets Pusher is wired + // (d.CredPusher) the value is delivered to the backend; either way it + // is never written to a generated file (spec 04 §7.5). if r.cred == resource.CredGenerated { - pw, err := secrets.RandomPassword(24) - if err != nil { - return err - } - if res.Params == nil { - res.Params = map[string]any{} - } if _, set := res.Params["password"]; !set { + pw, err := secrets.RandomPassword(24) + if err != nil { + return err + } + if res.Params == nil { + res.Params = map[string]any{} + } res.Params["password"] = pw + if err := pushGeneratedCred(ctx, d, res, pw); err != nil { + return err + } } } params := d.Model.Workspace.Shared[r.instance].Params diff --git a/internal/orchestrate/up.go b/internal/orchestrate/up.go index 23d953b..6ef483d 100644 --- a/internal/orchestrate/up.go +++ b/internal/orchestrate/up.go @@ -54,6 +54,12 @@ type UpDeps struct { // Secrets resolves secret:// refs; nil → built from workspace.secrets.providers // with the built-in factories (SOPS+age). Injected for tests. Secrets *secrets.Registry + // CredPusher, when non-nil, receives a resource's `generated` credential so the + // random value is written to a secrets backend (aws-sm/ssm/infisical) instead of + // only living in-process for the provisioner call. The plaintext never lands in a + // generated file (spec 04 valueless-env coupling); nil → no push (the value is + // still generated and used to provision, but not persisted anywhere). + CredPusher secrets.Pusher // Trust installs the local CA when network.proxy.httpsLocal; nil → trust.New(). // Injected for tests (the trust phase is fenced — failure never aborts up). Trust *trust.Trust diff --git a/internal/resource/minio.go b/internal/resource/minio.go index 409566d..b71e5d1 100644 --- a/internal/resource/minio.go +++ b/internal/resource/minio.go @@ -44,6 +44,8 @@ type S3API interface { GetBucketPolicy(context.Context, *s3.GetBucketPolicyInput, ...func(*s3.Options)) (*s3.GetBucketPolicyOutput, error) PutBucketCors(context.Context, *s3.PutBucketCorsInput, ...func(*s3.Options)) (*s3.PutBucketCorsOutput, error) GetBucketCors(context.Context, *s3.GetBucketCorsInput, ...func(*s3.Options)) (*s3.GetBucketCorsOutput, error) + ListObjectsV2(context.Context, *s3.ListObjectsV2Input, ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) + DeleteObjects(context.Context, *s3.DeleteObjectsInput, ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) } // S3Factory builds an S3API for a resolved Target (the 127.0.0.1 overlay endpoint @@ -198,6 +200,13 @@ func (m MinIO) Drop(ctx context.Context, t Target, r Resource) error { } return nil } + // `rb --force` (Params["force"]): recursively purge every object first so a + // non-empty bucket can be removed (a plain DeleteBucket fails with BucketNotEmpty). + if boolParam(r.Params, "force") { + if err := m.emptyBucket(ctx, c, bucket); err != nil { + return err + } + } if _, err := c.DeleteBucket(ctx, &s3.DeleteBucketInput{Bucket: aws.String(bucket)}); err != nil { if isNotFound(err) { return nil @@ -207,6 +216,41 @@ func (m MinIO) Drop(ctx context.Context, t Target, r Resource) error { return nil } +// emptyBucket recursively deletes every object in the bucket via paginated +// ListObjectsV2 + batched DeleteObjects, so a `rb --force` can remove a non-empty +// bucket. Idempotent: an already-empty or missing bucket is a no-op. +func (MinIO) emptyBucket(ctx context.Context, c S3API, bucket string) error { + var token *string + for { + out, err := c.ListObjectsV2(ctx, &s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + ContinuationToken: token, + }) + if err != nil { + if isNotFound(err) { + return nil + } + return fmt.Errorf("list objects in %q: %w", bucket, err) + } + if len(out.Contents) > 0 { + ids := make([]s3types.ObjectIdentifier, 0, len(out.Contents)) + for _, o := range out.Contents { + ids = append(ids, s3types.ObjectIdentifier{Key: o.Key}) + } + if _, err := c.DeleteObjects(ctx, &s3.DeleteObjectsInput{ + Bucket: aws.String(bucket), + Delete: &s3types.Delete{Objects: ids, Quiet: aws.Bool(true)}, + }); err != nil { + return fmt.Errorf("delete objects in %q: %w", bucket, err) + } + } + if out.IsTruncated == nil || !*out.IsTruncated { + return nil + } + token = out.NextContinuationToken + } +} + // Preflight verifies the endpoint is reachable and the creds are valid (a // ListBuckets round-trip). Absence degrades only the s3 verbs, never `up`. func (m MinIO) Preflight(ctx context.Context, t Target) error { diff --git a/internal/resource/minio_test.go b/internal/resource/minio_test.go index e0307f6..f198c13 100644 --- a/internal/resource/minio_test.go +++ b/internal/resource/minio_test.go @@ -2,16 +2,19 @@ package resource import ( "context" + "strings" "testing" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" + smithy "github.com/aws/smithy-go" ) // fakeS3 is an in-memory S3API for the MinIO provisioner tests (no live endpoint). type fakeS3 struct { buckets map[string]bool + objects map[string][]string // bucket → object keys versioning map[string]string lifecycle map[string][]s3types.LifecycleRule policy map[string]string @@ -22,6 +25,7 @@ type fakeS3 struct { func newFakeS3() *fakeS3 { return &fakeS3{ buckets: map[string]bool{}, + objects: map[string][]string{}, versioning: map[string]string{}, lifecycle: map[string][]s3types.LifecycleRule{}, policy: map[string]string{}, @@ -47,13 +51,47 @@ func (f *fakeS3) HeadBucket(_ context.Context, in *s3.HeadBucketInput, _ ...func func (f *fakeS3) DeleteBucket(_ context.Context, in *s3.DeleteBucketInput, _ ...func(*s3.Options)) (*s3.DeleteBucketOutput, error) { f.calls = append(f.calls, "DeleteBucket:"+aws.ToString(in.Bucket)) - if !f.buckets[aws.ToString(in.Bucket)] { + name := aws.ToString(in.Bucket) + if !f.buckets[name] { return nil, &s3types.NoSuchBucket{} } - delete(f.buckets, aws.ToString(in.Bucket)) + // Match S3/MinIO: a bucket with objects cannot be removed without emptying it. + if len(f.objects[name]) > 0 { + return nil, &smithy.GenericAPIError{Code: "BucketNotEmpty", Message: "bucket not empty"} + } + delete(f.buckets, name) return &s3.DeleteBucketOutput{}, nil } +func (f *fakeS3) ListObjectsV2(_ context.Context, in *s3.ListObjectsV2Input, _ ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { + name := aws.ToString(in.Bucket) + if !f.buckets[name] { + return nil, &s3types.NoSuchBucket{} + } + out := &s3.ListObjectsV2Output{IsTruncated: aws.Bool(false)} + for _, k := range f.objects[name] { + out.Contents = append(out.Contents, s3types.Object{Key: aws.String(k)}) + } + return out, nil +} + +func (f *fakeS3) DeleteObjects(_ context.Context, in *s3.DeleteObjectsInput, _ ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) { + name := aws.ToString(in.Bucket) + f.calls = append(f.calls, "DeleteObjects:"+name) + del := map[string]bool{} + for _, o := range in.Delete.Objects { + del[aws.ToString(o.Key)] = true + } + var keep []string + for _, k := range f.objects[name] { + if !del[k] { + keep = append(keep, k) + } + } + f.objects[name] = keep + return &s3.DeleteObjectsOutput{}, nil +} + func (f *fakeS3) ListBuckets(_ context.Context, _ *s3.ListBucketsInput, _ ...func(*s3.Options)) (*s3.ListBucketsOutput, error) { out := &s3.ListBucketsOutput{} for name := range f.buckets { @@ -273,3 +311,42 @@ func TestMinIODropBucket(t *testing.T) { t.Errorf("drop of missing bucket must be idempotent: %v", err) } } + +func TestMinIODropForceEmptiesBucket(t *testing.T) { + f := newFakeS3() + f.buckets["web-uploads"] = true + f.objects["web-uploads"] = []string{"a.txt", "nested/b.txt"} + m := fakeMinIO(f) + + // Without --force, a non-empty bucket cannot be removed (BucketNotEmpty). + if err := m.Drop(context.Background(), minioTarget(), + Resource{Engine: "minio", Kind: "bucket", Name: "web-uploads"}); err == nil { + t.Fatal("removing a non-empty bucket without --force must fail") + } + if !f.buckets["web-uploads"] { + t.Fatal("bucket must survive a failed non-force drop") + } + + // With --force, every object is deleted first, then the bucket. + if err := m.Drop(context.Background(), minioTarget(), Resource{ + Engine: "minio", Kind: "bucket", Name: "web-uploads", + Params: map[string]any{"force": true}, + }); err != nil { + t.Fatalf("force drop: %v", err) + } + if f.buckets["web-uploads"] { + t.Error("bucket not removed by force drop") + } + if len(f.objects["web-uploads"]) != 0 { + t.Errorf("objects not purged: %v", f.objects["web-uploads"]) + } + sawDeleteObjects := false + for _, c := range f.calls { + if strings.HasPrefix(c, "DeleteObjects:") { + sawDeleteObjects = true + } + } + if !sawDeleteObjects { + t.Errorf("force drop must call DeleteObjects: %v", f.calls) + } +} diff --git a/internal/secrets/cred.go b/internal/secrets/cred.go index 071a64e..880576a 100644 --- a/internal/secrets/cred.go +++ b/internal/secrets/cred.go @@ -2,31 +2,46 @@ package secrets import ( "crypto/rand" - "encoding/base64" "fmt" ) // This file is the credential generator behind the `generated` resource // credential policy (spec 27 §Credential surfacing) — distinct from -// GenerateAgeKey (age/SOPS key material). It produces a random, URL-safe secret -// via crypto/rand (pure-Go, no new dependency) that a provisioner pushes to a -// secrets provider (the Pusher) and injects as a valueless env key; the value is -// never written to a generated file. +// GenerateAgeKey (age/SOPS key material). It produces a random, ALPHANUMERIC +// secret via crypto/rand (pure-Go, no new dependency) that a provisioner routes +// through a secrets Pusher and injects as a valueless env key; the value is never +// written to a generated file. -// RandomPassword returns a cryptographically-random, URL-safe password with at -// least n characters (n must be positive). It draws from crypto/rand and encodes -// with base64 raw-url (no padding), so the result is safe in a DSN and free of -// shell-special characters. +// pwAlphabet is the alphanumeric alphabet (62 symbols) RandomPassword draws from. +// It is deliberately free of shell/DSN-hostile characters (no +/-_=), so a value +// is safe unquoted in a DSN, a URL, or an env var. +const pwAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789" + +// RandomPassword returns a cryptographically-random, alphanumeric password of +// exactly n characters (n must be positive). It draws from crypto/rand with +// rejection sampling so every symbol is uniform (no modulo bias). func RandomPassword(n int) (string, error) { if n <= 0 { return "", fmt.Errorf("RandomPassword: length %d must be positive", n) } - // base64 raw-url yields ~4 chars per 3 bytes; request enough bytes to cover n. - nbytes := (n*3 + 3) / 4 - buf := make([]byte, nbytes) - if _, err := rand.Read(buf); err != nil { - return "", fmt.Errorf("RandomPassword: read random bytes: %w", err) + // Reject bytes at/above the largest multiple of len(alphabet) so the modulo + // mapping is unbiased (256 % 62 != 0 would otherwise favour the first symbols). + const maxUnbiased = 256 - (256 % len(pwAlphabet)) + out := make([]byte, 0, n) + buf := make([]byte, n) + for len(out) < n { + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("RandomPassword: read random bytes: %w", err) + } + for _, b := range buf { + if int(b) >= maxUnbiased { + continue // reject to keep the distribution uniform + } + out = append(out, pwAlphabet[int(b)%len(pwAlphabet)]) + if len(out) == n { + break + } + } } - s := base64.RawURLEncoding.EncodeToString(buf) - return s[:n], nil + return string(out), nil } diff --git a/internal/secrets/cred_test.go b/internal/secrets/cred_test.go index d96bb6a..f7d6fb0 100644 --- a/internal/secrets/cred_test.go +++ b/internal/secrets/cred_test.go @@ -12,11 +12,11 @@ func TestRandomPasswordLengthAndUniqueness(t *testing.T) { t.Errorf("RandomPassword(%d) len = %d, want %d (%q)", n, len(p), n, p) } } - // URL-safe alphabet only (no shell/DSN-hostile characters). + // Alphanumeric alphabet only (no shell/DSN-hostile characters, not even -/_). p, _ := RandomPassword(128) for _, r := range p { - if !(r == '-' || r == '_' || (r >= '0' && r <= '9') || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z')) { - t.Fatalf("RandomPassword produced non-url-safe rune %q in %q", r, p) + if !((r >= '0' && r <= '9') || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z')) { + t.Fatalf("RandomPassword produced non-alphanumeric rune %q in %q", r, p) } } // Two draws must differ (astronomically likely).