diff --git a/apps/cinc/cmd/client.go b/apps/cinc/cmd/client.go index 368ee9c..75ce323 100644 --- a/apps/cinc/cmd/client.go +++ b/apps/cinc/cmd/client.go @@ -1,7 +1,6 @@ package cmd import ( - "encoding/json" "fmt" "os" @@ -111,12 +110,8 @@ cinc client edit worker-01`, var updated cinc.APIClient if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.APIClient](inputFile); err != nil { + return err } } else { current, _, err := c.Clients.Get(cmd.Context(), name) diff --git a/apps/cinc/cmd/common.go b/apps/cinc/cmd/common.go index 4551b25..c72c016 100644 --- a/apps/cinc/cmd/common.go +++ b/apps/cinc/cmd/common.go @@ -133,19 +133,37 @@ func resolveSecret(cmd *cobra.Command, profile config.Profile) ([]byte, error) { return nil, errors.New("we need an encrypted data bag secret but couldn't find one. Pass --secret-file (or --secret ), set $CINC_SECRET_FILE, or add a secret_file key to your credentials profile.") } -// readSecretFile reads a secret file the way Chef does: its contents with -// leading and trailing whitespace stripped. A read error, or a file with -// nothing left once stripped, becomes a conversational message. +// readSecretFile reads a secret file the way Chef's load_secret does (see +// cinc.LoadDataBagSecret: the ends are stripped, the contents must be +// UTF-8). A file we can't read, or one with nothing left once stripped, +// becomes a conversational message. func readSecretFile(path string) ([]byte, error) { + secret, err := cinc.LoadDataBagSecret(path) + var pathErr *fs.PathError + switch { + case errors.Is(err, cinc.ErrEmptyDataBagSecret): + return nil, fmt.Errorf("the data bag secret file at %s is empty. Put the shared secret in it, or point --secret-file at the right file.", path) + case errors.As(err, &pathErr): + return nil, fmt.Errorf("we couldn't read the data bag secret at %s: %w", path, pathErr.Err) + case err != nil: + return nil, fmt.Errorf("we can't use the data bag secret at %s: %w", path, err) + } + return secret, nil +} + +// readJSONFile reads the JSON document a --file flag points at into a T. +// Every `create`/`edit --file` path goes through it, so they all report a +// missing file or bad JSON the same way. +func readJSONFile[T any](path string) (T, error) { + var v T data, err := os.ReadFile(path) if err != nil { - return nil, fmt.Errorf("can't read the data bag secret at %s: %w", path, err) + return v, fmt.Errorf("we couldn't read %s: %w", path, err) } - secret := bytes.TrimSpace(data) - if len(secret) == 0 { - return nil, fmt.Errorf("the data bag secret file at %s is empty. Put the shared secret in it, or point --secret-file at the right file.", path) + if err := json.Unmarshal(data, &v); err != nil { + return v, fmt.Errorf("%s isn't valid JSON: %w", path, err) } - return secret, nil + return v, nil } // resolveClient builds a server client from the --config and --profile @@ -155,6 +173,13 @@ func resolveClient(cmd *cobra.Command) (*cinc.Client, error) { if err != nil { return nil, err } + return clientForProfile(cmd, profile) +} + +// clientForProfile builds a server client for an already-resolved profile, +// for commands that need the profile for something else too (the data bag +// secret, say) and shouldn't load the credentials file twice. +func clientForProfile(cmd *cobra.Command, profile config.Profile) (*cinc.Client, error) { c, err := client.New(profile) if errors.Is(err, config.ErrMissingServerURL) { return nil, missingServerURLError(cmd) diff --git a/apps/cinc/cmd/cookbook.go b/apps/cinc/cmd/cookbook.go index 30f8131..996ddf3 100644 --- a/apps/cinc/cmd/cookbook.go +++ b/apps/cinc/cmd/cookbook.go @@ -28,8 +28,8 @@ func newCookbookCmd() *cobra.Command { } // newCookbookDownloadCmd builds the `cinc cookbook download [version]` -// command. With no version it resolves the "_latest" sentinel the Chef Server -// exposes for the highest semver. Every file in the version's manifest is +// command. With no version it asks the server for cinc.LatestVersion, which +// resolves to the highest version. Every file in the version's manifest is // written under /-/ (recreating the cookbook layout), // where defaults to the current directory and is overridable with // --dir, matching knife's `cookbook download`. @@ -49,18 +49,19 @@ cinc cookbook download nginx 1.2.0 --dir ./cookbooks`, return err } name := args[0] - version := "_latest" + version := cinc.LatestVersion if len(args) == 2 { version = args[1] } - // Resolve the concrete version first so "_latest" never leaks into - // the destination directory name. + // Fetch the manifest first so the directory is named after the + // concrete version, never "_latest", then download from that same + // manifest. cb, _, err := c.Cookbooks.Get(cmd.Context(), name, version) if err != nil { return err } destDir := filepath.Join(dir, name+"-"+cb.Version) - if err := c.Cookbooks.Download(cmd.Context(), name, cb.Version, destDir); err != nil { + if err := c.Cookbooks.DownloadFiles(cmd.Context(), cb, destDir); err != nil { return err } fmt.Fprintf(cmd.OutOrStdout(), "Downloaded cookbook %q version %s to %s\n", name, cb.Version, destDir) @@ -72,8 +73,8 @@ cinc cookbook download nginx 1.2.0 --dir ./cookbooks`, } // newCookbookShowCmd builds the `cinc cookbook show [version]` -// command. With no version the command resolves the special "_latest" -// sentinel that the Chef Server exposes for the highest semver. +// command. With no version it shows cinc.LatestVersion, which the server +// resolves to the highest version. func newCookbookShowCmd() *cobra.Command { return &cobra.Command{ Use: "show [version]", @@ -92,7 +93,7 @@ cinc cookbook show nginx 1.2.0`, if err != nil { return err } - version := "_latest" + version := cinc.LatestVersion if len(args) == 2 { version = args[1] } diff --git a/apps/cinc/cmd/cookbook_download_test.go b/apps/cinc/cmd/cookbook_download_test.go index dea47d8..510467a 100644 --- a/apps/cinc/cmd/cookbook_download_test.go +++ b/apps/cinc/cmd/cookbook_download_test.go @@ -16,8 +16,8 @@ import ( // bodies, since cinc-api verifies every download. The manifest's file URLs point back at this // same server, mirroring how the real server hands out bookshelf URLs. // requestedVersions records, in order, the version segments the manifest was -// fetched under, so tests can assert "_latest" resolution (the command fetches -// once to resolve the concrete version, then the download re-fetches it). +// fetched under, so tests can assert "_latest" resolution and that the +// manifest is fetched only once. func cookbookDownloadServer(t *testing.T, requestedVersions *[]string) *httptest.Server { t.Helper() var base string @@ -63,10 +63,11 @@ func TestCookbookDownloadWritesFilesUnderNameVersionDir(t *testing.T) { t.Fatalf("cinc cookbook download: %v", err) } - // No explicit version => the first fetch resolves the "_latest" sentinel - // server-side, and "_latest" never appears in the destination dir name. - if len(requested) == 0 || requested[0] != "_latest" { - t.Errorf("first manifest fetch under %v, want it to start with _latest", requested) + // No explicit version => one fetch resolves the "_latest" sentinel + // server-side, the files come from that same manifest, and "_latest" + // never appears in the destination dir name. + if len(requested) != 1 || requested[0] != "_latest" { + t.Errorf("manifest fetched under %v, want exactly one fetch, of _latest", requested) } cbDir := filepath.Join(destParent, "nginx-1.2.0") @@ -97,10 +98,8 @@ func TestCookbookDownloadAcceptsExplicitVersion(t *testing.T) { if err := root.Execute(); err != nil { t.Fatalf("cinc cookbook download nginx 1.2.0: %v", err) } - for _, v := range requested { - if v != "1.2.0" { - t.Errorf("manifest fetched under version %q, want only 1.2.0", v) - } + if len(requested) != 1 || requested[0] != "1.2.0" { + t.Errorf("manifest fetched under %v, want exactly one fetch, of 1.2.0", requested) } if _, err := os.Stat(filepath.Join(destParent, "nginx-1.2.0", "metadata.rb")); err != nil { t.Errorf("expected cookbook downloaded to nginx-1.2.0/: %v", err) diff --git a/apps/cinc/cmd/databag.go b/apps/cinc/cmd/databag.go index d9db634..24541f1 100644 --- a/apps/cinc/cmd/databag.go +++ b/apps/cinc/cmd/databag.go @@ -2,10 +2,8 @@ package cmd import ( "context" - "encoding/json" "errors" "fmt" - "os" "slices" cinc "github.com/cinc-project/cinc-api" @@ -41,21 +39,7 @@ func newDataBagShowCmd() *cobra.Command { Example: `Show the item IDs in a data bag. cinc databag show passwords`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - format, err := resolveFormat(cmd) - if err != nil { - return err - } - c, err := resolveClient(cmd) - if err != nil { - return err - } - ids, err := fetchDataBagItemIDs(cmd.Context(), c, args[0]) - if err != nil { - return err - } - return printer.New(cmd.OutOrStdout(), format).List(ids) - }, + RunE: runListDataBagItems, } } @@ -120,7 +104,6 @@ cinc databag create passwords mysql`, } id := args[1] - item["id"] = id if _, _, err := c.DataBags.Items(bag).Create(cmd.Context(), item); err != nil { return err } @@ -132,27 +115,37 @@ cinc databag create passwords mysql`, return cmd } -// loadOrEditNewItem produces a DataBagItem either by reading the -// given file or by opening the editor on a stub item carrying just -// the id field. +// loadOrEditNewItem produces the new item id, either from --file (see +// readDataBagItemFile) or by opening the editor on a stub carrying just +// the id. func loadOrEditNewItem(id, inputFile string) (cinc.DataBagItem, error) { if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return nil, fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := validateDataBagItem(data); err != nil { - return nil, err - } - var item cinc.DataBagItem - if err := json.Unmarshal(data, &item); err != nil { - return nil, fmt.Errorf("cinc: parse %s: %w", inputFile, err) - } - return item, nil + return readDataBagItemFile(inputFile, id) } return editDataBagItem(cinc.DataBagItem{"id": id}) } +// readDataBagItemFile reads the item id from the JSON file at path. The +// file has to be a whole item, "id" included, and that id has to be the one +// on the command line. We refuse a mismatch rather than quietly storing the +// file under the argument's id: a file naming another item is almost always +// the wrong file (or one copied from another item), and for an edit that +// would overwrite one item with the content meant for another. Nothing has +// been sent yet, so refusing costs the user one fix. +func readDataBagItemFile(path, id string) (cinc.DataBagItem, error) { + item, err := readJSONFile[cinc.DataBagItem](path) + if err != nil { + return nil, err + } + if item.Validate() != nil { + return nil, fmt.Errorf("%s doesn't say which item it is. Add \"id\": %q to it.", path, id) + } + if item.ID() != id { + return nil, fmt.Errorf("%s is the item %q, but you asked for %q. Change the file's id to %q, or name %q on the command line.", path, item.ID(), id, id, item.ID()) + } + return item, nil +} + // newDataBagItemCmd builds the `cinc databag item` command group. // Data bag items are arbitrary JSON documents (always carrying an // "id" key) so the item-level verbs live under their own subgroup. @@ -171,8 +164,8 @@ func newDataBagItemCmd() *cobra.Command { // newDataBagItemCreateCmd builds `cinc databag item create `. The // bag must already exist (use `cinc databag create` to make one). Without -// --file the built-in JSON editor opens on a stub carrying just the id; the -// path arg's id always pins the item identifier. This is the item-scoped +// --file the built-in JSON editor opens on a stub carrying just the id; a +// file or edit naming a different id is refused. This is the item-scoped // equivalent of the two-arg `databag create ` form. func newDataBagItemCreateCmd() *cobra.Command { var inputFile string @@ -192,7 +185,6 @@ cinc databag item create passwords mysql`, if err != nil { return err } - item["id"] = id if _, _, err := c.DataBags.Items(bag).Create(cmd.Context(), item); err != nil { return err } @@ -267,22 +259,26 @@ func newDataBagItemListCmd() *cobra.Command { Example: `List the item IDs in a data bag. cinc databag item list passwords`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - format, err := resolveFormat(cmd) - if err != nil { - return err - } - c, err := resolveClient(cmd) - if err != nil { - return err - } - ids, err := fetchDataBagItemIDs(cmd.Context(), c, args[0]) - if err != nil { - return err - } - return printer.New(cmd.OutOrStdout(), format).List(ids) - }, + RunE: runListDataBagItems, + } +} + +// runListDataBagItems prints the item IDs in the bag args[0]. It backs both +// `databag show` and `databag item list`, which are the same question. +func runListDataBagItems(cmd *cobra.Command, args []string) error { + format, err := resolveFormat(cmd) + if err != nil { + return err + } + c, err := resolveClient(cmd) + if err != nil { + return err } + ids, err := fetchDataBagItemIDs(cmd.Context(), c, args[0]) + if err != nil { + return err + } + return printer.New(cmd.OutOrStdout(), format).List(ids) } // fetchDataBagItemIDs returns the sorted item IDs within a single bag. @@ -303,8 +299,8 @@ func fetchDataBagItemIDs(ctx context.Context, c *cinc.Client, bag string) ([]str // It fetches the item, opens its JSON in the built-in editor (same // engine as `cinc client edit`), validates on save, and PUTs the // result back. `--file` reads the updated JSON from disk for -// scripted use. The path arg's id pins the item identifier so an -// edit can never accidentally rename the item out from under itself. +// scripted use. A file or edit naming a different id is refused, so an +// edit can never rename the item or overwrite it with another's content. func newDataBagItemEditCmd() *cobra.Command { var inputFile string cmd := &cobra.Command{ @@ -323,14 +319,7 @@ cinc databag item edit passwords mysql`, var updated cinc.DataBagItem if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) - } - if err := validateDataBagItem(data); err != nil { + if updated, err = readDataBagItemFile(inputFile, id); err != nil { return err } } else { @@ -348,7 +337,6 @@ cinc databag item edit passwords mysql`, } updated = edited } - updated["id"] = id if _, _, err := items.Update(cmd.Context(), updated); err != nil { return err @@ -361,19 +349,6 @@ cinc databag item edit passwords mysql`, return cmd } -// validateDataBagItem checks that b is valid JSON and contains a -// non-empty string "id" key. It is exported via the editor seam. -func validateDataBagItem(b []byte) error { - var item cinc.DataBagItem - if err := json.Unmarshal(b, &item); err != nil { - return err - } - if item.ID() == "" { - return errors.New("data bag item is missing a non-empty \"id\" field") - } - return nil -} - // newDataBagDeleteCmd builds the `cinc databag delete ` command. func newDataBagDeleteCmd() *cobra.Command { return &cobra.Command{ diff --git a/apps/cinc/cmd/databag_create_validate_test.go b/apps/cinc/cmd/databag_create_validate_test.go index 7f4f49a..fdaa442 100644 --- a/apps/cinc/cmd/databag_create_validate_test.go +++ b/apps/cinc/cmd/databag_create_validate_test.go @@ -92,3 +92,53 @@ func TestDataBagCreateRejectsFileWithoutItem(t *testing.T) { t.Errorf("error should explain that --file needs an item: %v", err) } } + +// TestDataBagFileWithAnotherItemsIDIsRefused pins what happens when --file +// names a different item than the command line: every command that reads an +// item from a file refuses, naming both ids, before it sends anything. It +// used to store the file under the argument's id, which for an edit +// overwrites one item with the content meant for another. +func TestDataBagFileWithAnotherItemsIDIsRefused(t *testing.T) { + secret := writeSecretFile(t, "s3cr3t") + for name, args := range map[string][]string{ + "databag create": {"databag", "create", "users", "alice"}, + "databag item create": {"databag", "item", "create", "users", "alice"}, + "databag item edit": {"databag", "item", "edit", "users", "alice"}, + "secret create": {"databag", "secret", "create", "users", "alice", "--secret-file", secret}, + "secret edit": {"databag", "secret", "edit", "users", "alice", "--secret-file", secret}, + } { + t.Run(name, func(t *testing.T) { + srv := databagNoRequestServer(t) + file := writeItemFile(t, cinc.DataBagItem{"id": "bob", "role": "admin"}) + root := newRootCmd() + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs(append(args, "--file", file, "--config", writeDataBagConfig(t, srv.URL))) + err := root.Execute() + if err == nil { + t.Fatal("expected the mismatched id to be refused") + } + if !strings.Contains(err.Error(), `"bob"`) || !strings.Contains(err.Error(), `"alice"`) { + t.Errorf("error should name both ids: %v", err) + } + }) + } +} + +// TestDataBagItemEditorKeepsTheID checks the editor's save validation: an +// edit that drops or changes the id is refused in the editor, where the user +// can fix it, rather than renaming the item. +func TestDataBagItemEditorKeepsTheID(t *testing.T) { + validate := dataBagItemValidator("alice") + for body, ok := range map[string]bool{ + `{"id": "alice", "role": "admin"}`: true, + `{"id": "bob", "role": "admin"}`: false, + `{"role": "admin"}`: false, + `{"id": "alice",`: false, + `null`: false, + } { + if err := validate([]byte(body)); (err == nil) != ok { + t.Errorf("validate(%s) = %v, want ok=%v", body, err, ok) + } + } +} diff --git a/apps/cinc/cmd/databag_secret.go b/apps/cinc/cmd/databag_secret.go index 0b9b7a2..de9a8b8 100644 --- a/apps/cinc/cmd/databag_secret.go +++ b/apps/cinc/cmd/databag_secret.go @@ -1,10 +1,8 @@ package cmd import ( - "encoding/json" "errors" "fmt" - "os" cinc "github.com/cinc-project/cinc-api" "github.com/spf13/cobra" @@ -54,6 +52,25 @@ func addSecretFlags(cmd *cobra.Command) { cmd.Flags().String("secret", "", "the encrypted data bag secret as a literal string (mutually exclusive with --secret-file)") } +// resolveSecretClient resolves the profile once and returns both the data +// bag secret and a server client for it. The secret comes first, so a +// missing or unusable secret fails before anything talks to the server. +func resolveSecretClient(cmd *cobra.Command) ([]byte, *cinc.Client, error) { + profile, err := resolveProfile(cmd) + if err != nil { + return nil, nil, err + } + secret, err := resolveSecret(cmd, profile) + if err != nil { + return nil, nil, err + } + c, err := clientForProfile(cmd, profile) + if err != nil { + return nil, nil, err + } + return secret, c, nil +} + // newDataBagSecretCreateCmd builds `cinc databag secret create `. // The bag must already exist (use `cinc databag create`). Without --file // the built-in JSON editor opens on a stub carrying just the id; every @@ -67,15 +84,7 @@ func newDataBagSecretCreateCmd() *cobra.Command { cinc databag secret create passwords mysql --secret-file ~/.cinc/secret`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - profile, err := resolveProfile(cmd) - if err != nil { - return err - } - secret, err := resolveSecret(cmd, profile) - if err != nil { - return err - } - c, err := resolveClient(cmd) + secret, c, err := resolveSecretClient(cmd) if err != nil { return err } @@ -84,13 +93,8 @@ cinc databag secret create passwords mysql --secret-file ~/.cinc/secret`, if err != nil { return err } - item["id"] = id - encrypted, err := item.Encrypt(secret) - if err != nil { - return err - } - if _, _, err := c.DataBags.Items(bag).Create(cmd.Context(), encrypted); err != nil { - return err + if _, _, err := c.DataBags.Items(bag).CreateEncrypted(cmd.Context(), item, secret); err != nil { + return encryptItemError(err, bag, id) } fmt.Fprintf(cmd.OutOrStdout(), "Created encrypted item %q in data bag %q\n", id, bag) return nil @@ -117,24 +121,12 @@ cinc databag secret show passwords mysql --secret-file ~/.cinc/secret`, if err != nil { return err } - profile, err := resolveProfile(cmd) - if err != nil { - return err - } - secret, err := resolveSecret(cmd, profile) - if err != nil { - return err - } - c, err := resolveClient(cmd) + secret, c, err := resolveSecretClient(cmd) if err != nil { return err } bag, id := args[0], args[1] - item, _, err := c.DataBags.Items(bag).Get(cmd.Context(), id) - if err != nil { - return err - } - plain, err := item.Decrypt(secret) + plain, _, err := c.DataBags.Items(bag).GetDecrypted(cmd.Context(), id, secret) if err != nil { return decryptItemError(err, bag, id) } @@ -149,7 +141,8 @@ cinc databag secret show passwords mysql --secret-file ~/.cinc/secret`, // fetches and decrypts the item, opens its plaintext in the built-in JSON // editor (same engine as `cinc databag item edit`), re-encrypts the saved // result, and PUTs it back. `--file` reads the updated plaintext JSON from -// disk for scripted use. The path arg's id pins the identifier. +// disk for scripted use; like `databag item edit`, a file or edit naming a +// different id is refused. func newDataBagSecretEditCmd() *cobra.Command { var inputFile string cmd := &cobra.Command{ @@ -159,15 +152,7 @@ func newDataBagSecretEditCmd() *cobra.Command { cinc databag secret edit passwords mysql --secret-file ~/.cinc/secret`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - profile, err := resolveProfile(cmd) - if err != nil { - return err - } - secret, err := resolveSecret(cmd, profile) - if err != nil { - return err - } - c, err := resolveClient(cmd) + secret, c, err := resolveSecretClient(cmd) if err != nil { return err } @@ -176,22 +161,11 @@ cinc databag secret edit passwords mysql --secret-file ~/.cinc/secret`, var updated cinc.DataBagItem if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := validateDataBagItem(data); err != nil { + if updated, err = readDataBagItemFile(inputFile, id); err != nil { return err } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) - } } else { - current, _, err := items.Get(cmd.Context(), id) - if err != nil { - return err - } - plain, err := current.Decrypt(secret) + plain, _, err := items.GetDecrypted(cmd.Context(), id, secret) if err != nil { return decryptItemError(err, bag, id) } @@ -205,13 +179,8 @@ cinc databag secret edit passwords mysql --secret-file ~/.cinc/secret`, } updated = edited } - updated["id"] = id - encrypted, err := updated.Encrypt(secret) - if err != nil { - return err - } - if _, _, err := items.Update(cmd.Context(), encrypted); err != nil { - return err + if _, _, err := items.UpdateEncrypted(cmd.Context(), updated, secret); err != nil { + return encryptItemError(err, bag, id) } fmt.Fprintf(cmd.OutOrStdout(), "Updated encrypted item %q in bag %q\n", id, bag) return nil @@ -222,6 +191,18 @@ cinc databag secret edit passwords mysql --secret-file ~/.cinc/secret`, return cmd } +// encryptItemError explains an item that is already encrypted, which is +// what you get by handing `databag secret create` or `edit` an item copied +// from the server rather than its plaintext. Encrypting it again would +// store ciphertext that decrypts to more ciphertext. Anything else passes +// through unchanged. +func encryptItemError(err error, bag, id string) error { + if errors.Is(err, cinc.ErrAlreadyEncrypted) { + return fmt.Errorf("item %q for data bag %q already holds encrypted values, and encrypting them again would make it unreadable. Give us the plaintext values instead (`cinc databag secret show %s %s` prints them), or store the encrypted item as-is with `cinc databag item create` or `edit`.", id, bag, bag, id) + } + return err +} + // decryptItemError turns a Decrypt failure into a conversational message. // A not-encrypted item points the user at the plaintext command; an // authentication failure suggests a wrong secret. Anything else passes diff --git a/apps/cinc/cmd/databag_secret_file_test.go b/apps/cinc/cmd/databag_secret_file_test.go index 9a1578a..e1b1feb 100644 --- a/apps/cinc/cmd/databag_secret_file_test.go +++ b/apps/cinc/cmd/databag_secret_file_test.go @@ -143,3 +143,46 @@ func writeItemFile(t *testing.T, item cinc.DataBagItem) string { } return path } + +// TestDataBagSecretCreateRefusesEncryptedItem covers feeding `secret create` +// an item copied from the server, already encrypted. Encrypting it again +// would store an item that decrypts to ciphertext, so it's refused before +// anything is sent, with a pointer at what to do instead. +func TestDataBagSecretCreateRefusesEncryptedItem(t *testing.T) { + srv := databagNoRequestServer(t) + enc := encryptedItem(t, cinc.DataBagItem{"id": "mysql", "password": "p"}, "s3cr3t") + root := newRootCmd() + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"databag", "secret", "create", "passwords", "mysql", + "--file", writeItemFile(t, enc), + "--secret-file", writeSecretFile(t, "s3cr3t"), "--config", writeDataBagConfig(t, srv.URL)}) + err := root.Execute() + if err == nil { + t.Fatal("expected an already-encrypted item to be refused") + } + if !strings.Contains(err.Error(), "already holds encrypted values") || !strings.Contains(err.Error(), "plaintext") { + t.Errorf("error should explain the item is already encrypted: %v", err) + } +} + +// TestDataBagSecretFileRejectsInvalidUTF8 follows Chef, which reads a secret +// file as UTF-8 text and can't use one that isn't, so knife and chef-client +// could never decrypt what such a secret encrypted. +func TestDataBagSecretFileRejectsInvalidUTF8(t *testing.T) { + srv := databagNoRequestServer(t) + secretPath := writeSecretFile(t, "\xff\xfesecret") + root := newRootCmd() + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"databag", "secret", "create", "passwords", "mysql", + "--file", writeItemFile(t, cinc.DataBagItem{"id": "mysql", "password": "p"}), + "--secret-file", secretPath, "--config", writeDataBagConfig(t, srv.URL)}) + err := root.Execute() + if err == nil { + t.Fatal("expected a non-UTF-8 secret file to be refused") + } + if !strings.Contains(err.Error(), secretPath) || !strings.Contains(err.Error(), "UTF-8") { + t.Errorf("error should name the secret file and say it isn't UTF-8: %v", err) + } +} diff --git a/apps/cinc/cmd/databag_secret_test.go b/apps/cinc/cmd/databag_secret_test.go index 2282f95..324a336 100644 --- a/apps/cinc/cmd/databag_secret_test.go +++ b/apps/cinc/cmd/databag_secret_test.go @@ -117,7 +117,7 @@ func TestDataBagSecretCreateReadsSecretFromFile(t *testing.T) { }) filePath := filepath.Join(t.TempDir(), "item.json") - body, _ := json.Marshal(cinc.DataBagItem{"id": "ignored", "password": "from-file"}) + body, _ := json.Marshal(cinc.DataBagItem{"id": "mysql", "password": "from-file"}) if err := os.WriteFile(filePath, body, 0o600); err != nil { t.Fatal(err) } @@ -132,7 +132,7 @@ func TestDataBagSecretCreateReadsSecretFromFile(t *testing.T) { t.Fatalf("databag secret create --secret-file: %v", err) } if gotItem["id"] != "mysql" { - t.Errorf("POST body id = %v, want mysql (path arg wins over file)", gotItem["id"]) + t.Errorf("POST body id = %v, want mysql", gotItem["id"]) } plain, err := gotItem.Decrypt([]byte("file-secret")) if err != nil { diff --git a/apps/cinc/cmd/databag_test.go b/apps/cinc/cmd/databag_test.go index 031b90e..917e90d 100644 --- a/apps/cinc/cmd/databag_test.go +++ b/apps/cinc/cmd/databag_test.go @@ -273,7 +273,7 @@ func TestDataBagCreateCommandWithItemReadsFromFile(t *testing.T) { }) filePath := filepath.Join(t.TempDir(), "item.json") - body, _ := json.Marshal(cinc.DataBagItem{"id": "ignored-in-file", "password": "from-file"}) + body, _ := json.Marshal(cinc.DataBagItem{"id": "db-password", "password": "from-file"}) if err := os.WriteFile(filePath, body, 0o600); err != nil { t.Fatal(err) } @@ -287,7 +287,7 @@ func TestDataBagCreateCommandWithItemReadsFromFile(t *testing.T) { t.Fatalf("cinc databag create --file: %v", err) } if gotItem["id"] != "db-password" { - t.Errorf("PUT body id = %v, want db-password (path arg must win over file)", gotItem["id"]) + t.Errorf("PUT body id = %v, want db-password", gotItem["id"]) } if gotItem["password"] != "from-file" { t.Errorf("PUT body password = %v, want from-file", gotItem["password"]) @@ -511,7 +511,7 @@ func TestDataBagItemEditCommandReadsFromFile(t *testing.T) { }) filePath := filepath.Join(t.TempDir(), "item.json") - body, err := json.Marshal(cinc.DataBagItem{"id": "ignored-in-file", "role": "editor"}) + body, err := json.Marshal(cinc.DataBagItem{"id": "alice", "role": "editor"}) if err != nil { t.Fatal(err) } @@ -530,7 +530,7 @@ func TestDataBagItemEditCommandReadsFromFile(t *testing.T) { t.Errorf("PUT body role = %v, want editor", gotPut["role"]) } if gotPut["id"] != "alice" { - t.Errorf("PUT body id = %v, want alice (path arg must win over file)", gotPut["id"]) + t.Errorf("PUT body id = %v, want alice", gotPut["id"]) } } @@ -688,7 +688,7 @@ func TestDataBagItemCreateCommandReadsFromFile(t *testing.T) { }) filePath := filepath.Join(t.TempDir(), "item.json") - body, _ := json.Marshal(cinc.DataBagItem{"id": "ignored-in-file", "password": "from-file"}) + body, _ := json.Marshal(cinc.DataBagItem{"id": "db-password", "password": "from-file"}) if err := os.WriteFile(filePath, body, 0o600); err != nil { t.Fatal(err) } @@ -701,7 +701,7 @@ func TestDataBagItemCreateCommandReadsFromFile(t *testing.T) { t.Fatalf("cinc databag item create --file: %v", err) } if gotItem["id"] != "db-password" { - t.Errorf("POST body id = %v, want db-password (path arg must win over file)", gotItem["id"]) + t.Errorf("POST body id = %v, want db-password", gotItem["id"]) } if gotItem["password"] != "from-file" { t.Errorf("POST body password = %v, want from-file", gotItem["password"]) diff --git a/apps/cinc/cmd/editor.go b/apps/cinc/cmd/editor.go index 67fa7b8..1a66348 100644 --- a/apps/cinc/cmd/editor.go +++ b/apps/cinc/cmd/editor.go @@ -3,6 +3,7 @@ package cmd import ( "bytes" "encoding/json" + "fmt" cinc "github.com/cinc-project/cinc-api" @@ -62,8 +63,10 @@ func openObjectJSONEditor[T any](in *T) (*T, error) { } // editDataBagItem opens a data bag item in the shared JSON editor. -// Validation rejects malformed JSON and items missing an "id" key. It -// is a package variable so tests can stub it. +// Validation rejects malformed JSON, and an item whose "id" is missing or +// no longer the one it opened with (the id names the item, so changing it +// would rename or overwrite another). It is a package variable so tests can +// stub it. var editDataBagItem = openDataBagItemJSONEditor func openDataBagItemJSONEditor(in cinc.DataBagItem) (cinc.DataBagItem, error) { @@ -71,7 +74,7 @@ func openDataBagItemJSONEditor(in cinc.DataBagItem) (cinc.DataBagItem, error) { if err != nil { return nil, err } - edited, err := jsoneditor.Run(initial, validateDataBagItem) + edited, err := jsoneditor.Run(initial, dataBagItemValidator(in.ID())) if err != nil { return nil, err } @@ -81,3 +84,19 @@ func openDataBagItemJSONEditor(in cinc.DataBagItem) (cinc.DataBagItem, error) { } return out, nil } + +// dataBagItemValidator returns the editor's save check for the item id: the +// saved JSON must be an item (see cinc.DataBagItem.Validate) that still +// carries that id. +func dataBagItemValidator(id string) func([]byte) error { + return func(b []byte) error { + var item cinc.DataBagItem + if err := json.Unmarshal(b, &item); err != nil { + return err + } + if item.Validate() != nil || item.ID() != id { + return fmt.Errorf("the item's \"id\" has to stay %q, since it names the item", id) + } + return nil + } +} diff --git a/apps/cinc/cmd/environment.go b/apps/cinc/cmd/environment.go index 86e1157..1761e8d 100644 --- a/apps/cinc/cmd/environment.go +++ b/apps/cinc/cmd/environment.go @@ -1,9 +1,7 @@ package cmd import ( - "encoding/json" "fmt" - "os" cinc "github.com/cinc-project/cinc-api" "github.com/spf13/cobra" @@ -47,12 +45,8 @@ cinc environment edit prod`, var updated cinc.Environment if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.Environment](inputFile); err != nil { + return err } } else { current, _, err := c.Environments.Get(cmd.Context(), name) @@ -132,12 +126,8 @@ cinc environment create prod`, } env := cinc.Environment{Name: args[0]} if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &env); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if env, err = readJSONFile[cinc.Environment](inputFile); err != nil { + return err } env.Name = args[0] } diff --git a/apps/cinc/cmd/group.go b/apps/cinc/cmd/group.go index b95108f..3fc485b 100644 --- a/apps/cinc/cmd/group.go +++ b/apps/cinc/cmd/group.go @@ -1,9 +1,7 @@ package cmd import ( - "encoding/json" "fmt" - "os" "strings" cinc "github.com/cinc-project/cinc-api" @@ -51,12 +49,8 @@ cinc group edit admins`, var updated cinc.Group if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.Group](inputFile); err != nil { + return err } } else { current, _, err := c.Groups.Get(cmd.Context(), name) diff --git a/apps/cinc/cmd/keys.go b/apps/cinc/cmd/keys.go index 13200f2..3aae56d 100644 --- a/apps/cinc/cmd/keys.go +++ b/apps/cinc/cmd/keys.go @@ -1,7 +1,6 @@ package cmd import ( - "encoding/json" "fmt" "os" "slices" @@ -192,12 +191,8 @@ cinc %[1]s key edit %[2]s rotation --file regenerate.json --key-file rotation.pe var updated cinc.Key if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.Key](inputFile); err != nil { + return err } } else { current, _, err := scope.Get(cmd.Context(), keyName) diff --git a/apps/cinc/cmd/node.go b/apps/cinc/cmd/node.go index 4ed8c13..9aa1528 100644 --- a/apps/cinc/cmd/node.go +++ b/apps/cinc/cmd/node.go @@ -2,9 +2,7 @@ package cmd import ( "bufio" - "encoding/json" "fmt" - "os" "strings" "text/tabwriter" "time" @@ -81,12 +79,8 @@ cinc node create web01 --file web01.json`, } node := cinc.Node{Name: args[0]} if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &node); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if node, err = readJSONFile[cinc.Node](inputFile); err != nil { + return err } node.Name = args[0] } @@ -155,13 +149,9 @@ cinc node edit web01`, return nil } - data, err := os.ReadFile(inputFile) + updated, err := readJSONFile[cinc.Node](inputFile) if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - var updated cinc.Node - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + return err } updated.Name = name if _, _, err := c.Nodes.Update(cmd.Context(), &updated); err != nil { diff --git a/apps/cinc/cmd/org.go b/apps/cinc/cmd/org.go index 8144e54..044858b 100644 --- a/apps/cinc/cmd/org.go +++ b/apps/cinc/cmd/org.go @@ -1,10 +1,8 @@ package cmd import ( - "encoding/json" "errors" "fmt" - "os" "slices" cinc "github.com/cinc-project/cinc-api" @@ -206,12 +204,8 @@ cinc org edit acme`, var updated cinc.Org if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.Org](inputFile); err != nil { + return err } } else { current, _, err := c.Orgs.Get(cmd.Context(), name) diff --git a/apps/cinc/cmd/policy.go b/apps/cinc/cmd/policy.go index cc7038d..f919fc9 100644 --- a/apps/cinc/cmd/policy.go +++ b/apps/cinc/cmd/policy.go @@ -2,6 +2,8 @@ package cmd import ( "fmt" + "maps" + "slices" cinc "github.com/cinc-project/cinc-api" @@ -56,8 +58,10 @@ cinc policy delete appserver`, } } -// newPolicyShowCmd builds the `cinc policy show ` command. It -// shows every revision of the named policy, keyed by revision ID. +// newPolicyShowCmd builds the `cinc policy show ` command. It lists +// the revision IDs of the named policy, sorted. The server's answer is +// {"revisions": {"": {}}}, whose values are always empty, so the human +// form prints just the IDs; --format json keeps that wire shape as-is. func newPolicyShowCmd() *cobra.Command { return &cobra.Command{ Use: "show ", @@ -78,7 +82,11 @@ cinc policy show appserver`, if err != nil { return err } - return printer.New(cmd.OutOrStdout(), format).Value(policy) + p := printer.New(cmd.OutOrStdout(), format) + if format == printer.FormatJSON { + return p.Value(policy) + } + return p.List(slices.Sorted(maps.Keys(policy.Revisions))) }, } } diff --git a/apps/cinc/cmd/policy_archive_test.go b/apps/cinc/cmd/policy_archive_test.go index 1f2c01a..2723ae1 100644 --- a/apps/cinc/cmd/policy_archive_test.go +++ b/apps/cinc/cmd/policy_archive_test.go @@ -114,6 +114,36 @@ func TestPolicyPushArchiveCommandTarballInput(t *testing.T) { } } +// TestPolicyPushArchiveUploadsUnderLockName covers a bundled cookbook whose +// metadata.rb names it with a non-literal the static parser can't read, so +// the only name on hand is its directory, base-. The artifact +// must still land under the lock's name, base. +func TestPolicyPushArchiveUploadsUnderLockName(t *testing.T) { + const identifier = "0000000000000000000000000000000000000004" + var uploadedArtifact, associated bool + var associateBody []byte + srv := pushArchiveServer(t, identifier, &uploadedArtifact, &associated, &associateBody) + + bundleDir, _ := exportBundle(t, identifier) + dirs, err := filepath.Glob(filepath.Join(bundleDir, "cookbooks", "base-*")) + if err != nil || len(dirs) != 1 { + t.Fatalf("bundle cookbook dirs = %v, %v; want one base-* dir", dirs, err) + } + if err := os.WriteFile(filepath.Join(dirs[0], "metadata.rb"), []byte("name File.basename(__dir__)\nversion '1.0.0'\n"), 0o644); err != nil { + t.Fatal(err) + } + + root := newRootCmd() + root.SetOut(&bytes.Buffer{}) + root.SetArgs([]string{"policy", "push-archive", "prod", bundleDir, "--config", writeCreateConfig(t, srv.URL)}) + if err := root.Execute(); err != nil { + t.Fatalf("cinc policy push-archive: %v", err) + } + if !uploadedArtifact { + t.Error("cookbook artifact was not uploaded under the lock's cookbook name") + } +} + // TestPolicyPushArchiveDefaultsToCwdArchive omits the archive argument; the // command should discover the single .tar.gz in the working directory. func TestPolicyPushArchiveDefaultsToCwdArchive(t *testing.T) { diff --git a/apps/cinc/cmd/policy_deploy.go b/apps/cinc/cmd/policy_deploy.go index 472590d..851118c 100644 --- a/apps/cinc/cmd/policy_deploy.go +++ b/apps/cinc/cmd/policy_deploy.go @@ -210,9 +210,6 @@ func resolvePushArchivePath(arg string) (string, error) { } } -// newFetcher builds a policyfile.Fetcher rooted at the default cinc cookbook -// cache, resolving path sources relative to lockDir and using chef for -// chef_server sources. // lockNeedsServer reports whether any cookbook the lock pins is fetched from // the Cinc/Chef server rather than a path, git, or artifact source. func lockNeedsServer(lock *cinc.PolicyRevision) bool { @@ -224,6 +221,9 @@ func lockNeedsServer(lock *cinc.PolicyRevision) bool { return false } +// newFetcher builds a policyfile.Fetcher rooted at the default cinc cookbook +// cache, resolving path sources relative to lockDir and using chef for +// chef_server sources. func newFetcher(lockDir string, chef *cinc.Client) (*policyfile.Fetcher, error) { cacheRoot, err := policyfile.DefaultCacheRoot() if err != nil { @@ -245,47 +245,41 @@ func fetchLockCookbooks(ctx context.Context, fetcher *policyfile.Fetcher, lock * if err != nil { return nil, fmt.Errorf("cinc: load cookbook %q from %s: %w", name, dir, err) } - // Upload under the name the lock records. The directory is often a - // cache entry (--), and cinc-api falls back to - // it when metadata.rb names the cookbook with a non-literal. - cb.Name = name - cb.Metadata.Name = name cookbooks[name] = cb } return cookbooks, nil } -// pushRevision deploys lockJSON to group and reports what it did. PushRevision -// uploads only the artifacts the server lacks, so the upload count is read -// from the server first rather than assumed to be every cookbook. +// pushRevision deploys lockJSON to group and reports what it did, including +// how many of the lock's cookbook artifacts the server already held. func pushRevision(cmd *cobra.Command, format printer.Format, c *cinc.Client, lock *cinc.PolicyRevision, lockJSON []byte, group string, cookbooks map[string]*cinc.LocalCookbook) error { - uploads, err := policyfile.ArtifactsToUpload(cmd.Context(), c, lock) - if err != nil { - return err - } - rev, _, err := c.Policies.PushRevision(cmd.Context(), lockJSON, group, cookbooks) + res, _, err := c.Policies.PushRevision(cmd.Context(), lockJSON, group, cookbooks) if err != nil { return err } - return emitPushResult(cmd, format, lock.Name, group, rev.Revision, uploads, len(cookbooks)) + return emitPushResult(cmd, format, lock.Name, group, res) } -func emitPushResult(cmd *cobra.Command, format printer.Format, policy, group string, rev *cinc.PolicyRevision, uploaded, total int) error { +func emitPushResult(cmd *cobra.Command, format printer.Format, policy, group string, res *cinc.PushResult) error { + uploaded := len(res.Uploaded) + total := uploaded + len(res.AlreadyPresent) if format == printer.FormatJSON { return printer.New(cmd.OutOrStdout(), format).Value(map[string]any{ "policy": policy, "group": group, - "revision_id": rev.RevisionID, + "revision_id": res.Revision.RevisionID, "cookbooks": total, "cookbooks_uploaded": uploaded, + "uploaded": res.Uploaded, + "already_present": res.AlreadyPresent, }) } if uploaded == total { fmt.Fprintf(cmd.OutOrStdout(), "Pushed policy %q (revision %s) to group %q with %d cookbook(s)\n", - policy, rev.RevisionID, group, total) + policy, res.Revision.RevisionID, group, total) return nil } fmt.Fprintf(cmd.OutOrStdout(), "Pushed policy %q (revision %s) to group %q with %d cookbook(s) (%d uploaded, %d already on the server)\n", - policy, rev.RevisionID, group, total, uploaded, total-uploaded) + policy, res.Revision.RevisionID, group, total, uploaded, total-uploaded) return nil } diff --git a/apps/cinc/cmd/policy_deploy_test.go b/apps/cinc/cmd/policy_deploy_test.go index 2cc2c00..40e9d9b 100644 --- a/apps/cinc/cmd/policy_deploy_test.go +++ b/apps/cinc/cmd/policy_deploy_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "slices" "strings" "testing" ) @@ -197,8 +198,18 @@ func TestPolicyPushReportsOnlyWhatItUploaded(t *testing.T) { if !strings.Contains(buf.String(), tc.want) { t.Errorf("output = %q, want it to contain %q", buf.String(), tc.want) } - if tc.format == "json" && !strings.Contains(buf.String(), `"cookbooks": 1`) { - t.Errorf("json output = %q, want the lock's cookbook count too", buf.String()) + if tc.format == "json" { + var got struct { + Cookbooks int `json:"cookbooks"` + Uploaded []string `json:"uploaded"` + AlreadyPresent []string `json:"already_present"` + } + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("json output not valid JSON: %v\n%s", err, buf.String()) + } + if got.Cookbooks != 1 || len(got.Uploaded) != 0 || !slices.Equal(got.AlreadyPresent, []string{"base"}) { + t.Errorf("json output = %+v, want 1 cookbook, none uploaded, base already present", got) + } } }) } diff --git a/apps/cinc/cmd/policy_test.go b/apps/cinc/cmd/policy_test.go index a6ae98a..964e42f 100644 --- a/apps/cinc/cmd/policy_test.go +++ b/apps/cinc/cmd/policy_test.go @@ -113,6 +113,29 @@ client_key = %q } } +// TestPolicyShowHumanListsRevisionIDs pins the human form: the revision IDs, +// sorted, one per line, rather than the {"revisions":{id:{}}} wire shape. +func TestPolicyShowHumanListsRevisionIDs(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/organizations/acme/policies/appserver", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"revisions":{"bbb222":{},"aaa111":{}}}`)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + root := newRootCmd() + var buf bytes.Buffer + root.SetOut(&buf) + root.SetArgs([]string{"policy", "show", "appserver", "--config", writePolicyConfig(t, srv.URL)}) + if err := root.Execute(); err != nil { + t.Fatalf("cinc policy show: %v", err) + } + if got, want := buf.String(), "aaa111\nbbb222\n"; got != want { + t.Errorf("policy show = %q, want %q", got, want) + } +} + func TestPolicyDeleteCommandEndToEnd(t *testing.T) { var deleted string mux := http.NewServeMux() diff --git a/apps/cinc/cmd/policylifecycle.go b/apps/cinc/cmd/policylifecycle.go index bf6b8d8..01b525f 100644 --- a/apps/cinc/cmd/policylifecycle.go +++ b/apps/cinc/cmd/policylifecycle.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "os" "reflect" @@ -130,8 +131,9 @@ func computePolicyDiff(policy, fromRef, toRef string, a, b *cinc.PolicyRevision) } } -// cookbookRefs returns the value to show for a lock when versions differ -// (the version) versus when only the identifier differs (the identifier). +// diffCookbooks reports each cookbook added, removed, or changed between two +// revisions' locks, sorted by name. A changed cookbook shows its versions, +// or its identifiers when only the content changed under the same version. func diffCookbooks(a, b map[string]cinc.CookbookLock) []cookbookDelta { names := map[string]struct{}{} for n := range a { @@ -249,8 +251,10 @@ func newPolicyDiffCmd() *cobra.Command { cmd := &cobra.Command{ Use: "diff ", Short: "Compare two revisions of a policy", - Example: `Compare two revisions of a policy. -cinc policy diff appserver 1.0.0 1.1.0`, + Example: `Compare the appserver revisions active in the staging and production groups. +cinc policy diff appserver staging production +Compare two revisions by their revision ids. +cinc policy diff appserver --revisions 1a2b3c4d 5e6f7a8b`, Long: "Compare two revisions of a policy.\n\n" + "By default ref1 and ref2 name policy groups and the comparison is\n" + "between the revision active in each. Pass --revisions to treat them as\n" + @@ -276,21 +280,18 @@ cinc policy diff appserver 1.0.0 1.1.0`, return err } - fromRev, toRev := fromRef, toRef - if !revisionsForm { - if fromRev, err = activeRevision(cmd.Context(), c, fromRef, name); err != nil { - return err - } - if toRev, err = activeRevision(cmd.Context(), c, toRef, name); err != nil { - return err + fetch := func(ref string) (*cinc.PolicyRevision, error) { + if revisionsForm { + rev, _, err := c.Policies.GetRevision(cmd.Context(), name, ref) + return rev, err } + return activeRevision(cmd.Context(), c, ref, name) } - - a, _, err := c.Policies.GetRevision(cmd.Context(), name, fromRev) + a, err := fetch(fromRef) if err != nil { return err } - b, _, err := c.Policies.GetRevision(cmd.Context(), name, toRev) + b, err := fetch(toRef) if err != nil { return err } @@ -307,17 +308,19 @@ cinc policy diff appserver 1.0.0 1.1.0`, return cmd } -// activeRevision returns the revision id of policy active in group. -func activeRevision(ctx context.Context, c *cinc.Client, group, policy string) (string, error) { - g, _, err := c.PolicyGroups.Get(ctx, group) - if err != nil { - return "", err +// activeRevision returns the revision of policy active in group, in one +// request. The server answers 404 both for a group that doesn't exist and +// for one that doesn't pin the policy, so only then do we look at the group +// to say which. +func activeRevision(ctx context.Context, c *cinc.Client, group, policy string) (*cinc.PolicyRevision, error) { + rev, _, err := c.PolicyGroups.GetPolicy(ctx, group, policy) + if !errors.Is(err, cinc.ErrNotFound) { + return rev, err } - assignment, ok := g.Policies[policy] - if !ok { - return "", fmt.Errorf("policy %q is not assigned to group %q", policy, group) + if _, _, err := c.PolicyGroups.Get(ctx, group); err != nil { + return nil, err } - return assignment.RevisionID, nil + return nil, fmt.Errorf("policy %q is not assigned to group %q", policy, group) } // renderPolicyDiff prints the human form of a diff. @@ -503,27 +506,17 @@ cinc policy clean-cookbooks --dry-run`, } // referencedArtifacts returns the set of "@" artifacts -// pinned by any revision of any policy. It walks every policy, every revision, -// and every cookbook lock so an artifact is considered orphaned only when no -// revision anywhere still points at it. +// pinned by any revision of any policy. The policy list already names every +// revision, so it fetches each revision once and reads its cookbook locks; an +// artifact is orphaned only when no revision anywhere still points at it. func referencedArtifacts(ctx context.Context, c *cinc.Client) (map[string]struct{}, error) { index, _, err := c.Policies.List(ctx) if err != nil { return nil, err } - names := make([]string, 0, len(index)) - for name := range index { - names = append(names, name) - } - slices.Sort(names) - referenced := map[string]struct{}{} - for _, name := range names { - revs, _, err := c.Policies.Get(ctx, name) - if err != nil { - return nil, err - } - for revID := range revs.Revisions { + for name, entry := range index { + for revID := range entry.Revisions { rev, _, err := c.Policies.GetRevision(ctx, name, revID) if err != nil { return nil, err diff --git a/apps/cinc/cmd/policylifecycle_test.go b/apps/cinc/cmd/policylifecycle_test.go index 7d451ec..a19dfe0 100644 --- a/apps/cinc/cmd/policylifecycle_test.go +++ b/apps/cinc/cmd/policylifecycle_test.go @@ -192,35 +192,46 @@ func TestPolicyCreateCommandForceOverwrites(t *testing.T) { // --- diff (command) ----------------------------------------------------- // policyDiffServer serves policy revisions and, optionally, policy-group -// pinnings for the diff command tests. -func policyDiffServer(t *testing.T, revisions map[string]cinc.PolicyRevision, groups map[string]string) *httptest.Server { +// pinnings for the diff command tests. It counts the requests it answers, so +// a test can pin how many a diff takes. +func policyDiffServer(t *testing.T, revisions map[string]cinc.PolicyRevision, groups map[string]string) (*httptest.Server, *int) { t.Helper() + requests := new(int) mux := http.NewServeMux() - for rev, doc := range revisions { - mux.HandleFunc("/organizations/acme/policies/appserver/revisions/"+rev, func(w http.ResponseWriter, _ *http.Request) { + handle := func(path string, body any) { + mux.HandleFunc(path, func(w http.ResponseWriter, _ *http.Request) { + *requests++ w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(doc) + _ = json.NewEncoder(w).Encode(body) }) } + for rev, doc := range revisions { + handle("/organizations/acme/policies/appserver/revisions/"+rev, doc) + } for group, rev := range groups { - mux.HandleFunc("/organizations/acme/policy_groups/"+group, func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(cinc.PolicyGroup{ - Policies: map[string]cinc.PolicyAssignment{"appserver": {RevisionID: rev}}, - }) + if rev == "" { + // A group that exists but doesn't pin appserver. + handle("/organizations/acme/policy_groups/"+group, cinc.PolicyGroup{Policies: map[string]cinc.PolicyAssignment{}}) + continue + } + handle("/organizations/acme/policy_groups/"+group, cinc.PolicyGroup{ + Policies: map[string]cinc.PolicyAssignment{"appserver": {RevisionID: rev}}, }) + handle("/organizations/acme/policy_groups/"+group+"/policies/appserver", revisions[rev]) } srv := httptest.NewServer(mux) t.Cleanup(srv.Close) - return srv + return srv, requests } +// TestPolicyDiffCommandGroupsForm diffs the revisions active in two groups. +// Each side is one request for the group's active revision of the policy. func TestPolicyDiffCommandGroupsForm(t *testing.T) { revisions := map[string]cinc.PolicyRevision{ "1.1.0": {RevisionID: "1.1.0", CookbookLocks: map[string]cinc.CookbookLock{"web": {Version: "1.3.0"}}}, "1.0.0": {RevisionID: "1.0.0", CookbookLocks: map[string]cinc.CookbookLock{"web": {Version: "1.2.0"}}}, } - srv := policyDiffServer(t, revisions, map[string]string{"staging": "1.1.0", "prod": "1.0.0"}) + srv, requests := policyDiffServer(t, revisions, map[string]string{"staging": "1.1.0", "prod": "1.0.0"}) root := newRootCmd() var buf bytes.Buffer @@ -237,9 +248,32 @@ func TestPolicyDiffCommandGroupsForm(t *testing.T) { if d.From.Ref != "staging" || d.To.Ref != "prod" { t.Errorf("refs = %+v / %+v", d.From, d.To) } + if d.From.RevisionID != "1.1.0" || d.To.RevisionID != "1.0.0" { + t.Errorf("revision ids = %+v / %+v", d.From, d.To) + } if len(d.Cookbooks) != 1 || d.Cookbooks[0].From != "1.3.0" || d.Cookbooks[0].To != "1.2.0" { t.Errorf("cookbooks = %+v", d.Cookbooks) } + if *requests != 2 { + t.Errorf("diff made %d requests, want 2 (one per group)", *requests) + } +} + +// TestPolicyDiffCommandGroupWithoutPolicy names the problem when a group +// exists but doesn't pin the policy, rather than a bare not-found. +func TestPolicyDiffCommandGroupWithoutPolicy(t *testing.T) { + revisions := map[string]cinc.PolicyRevision{"1.0.0": {RevisionID: "1.0.0"}} + srv, _ := policyDiffServer(t, revisions, map[string]string{"staging": "1.0.0", "empty": ""}) + + root := newRootCmd() + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"policy", "diff", "appserver", "staging", "empty", "--config", writePolicyConfig(t, srv.URL)}) + + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), `policy "appserver" is not assigned to group "empty"`) { + t.Errorf("err = %v, want it to say the policy isn't assigned to the group", err) + } } func TestPolicyDiffCommandRevisionsForm(t *testing.T) { @@ -247,7 +281,7 @@ func TestPolicyDiffCommandRevisionsForm(t *testing.T) { "1.0.0": {RevisionID: "1.0.0", RunList: []string{"recipe[base]"}}, "1.1.0": {RevisionID: "1.1.0", RunList: []string{"recipe[base]", "recipe[web::ssl]"}}, } - srv := policyDiffServer(t, revisions, nil) + srv, _ := policyDiffServer(t, revisions, nil) root := newRootCmd() var buf bytes.Buffer @@ -270,7 +304,7 @@ func TestPolicyDiffCommandRevisionsForm(t *testing.T) { } func TestPolicyDiffCommandRejectsMixedForms(t *testing.T) { - srv := policyDiffServer(t, map[string]cinc.PolicyRevision{"1.0.0": {RevisionID: "1.0.0"}}, nil) + srv, _ := policyDiffServer(t, map[string]cinc.PolicyRevision{"1.0.0": {RevisionID: "1.0.0"}}, nil) root := newRootCmd() root.SetOut(&bytes.Buffer{}) @@ -403,12 +437,9 @@ func cleanCookbooksServer(t *testing.T, deleted *[]string) *httptest.Server { }) for policy, revs := range revisions { mux.HandleFunc("/organizations/acme/policies/"+policy, func(w http.ResponseWriter, _ *http.Request) { - out := cinc.PolicyRevisions{Revisions: map[string]json.RawMessage{}} - for rev := range revs { - out.Revisions[rev] = json.RawMessage(`{}`) - } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(out) + // The policy list already names every revision. + t.Errorf("clean-cookbooks fetched /policies/%s, which the policy list makes redundant", policy) + w.WriteHeader(http.StatusInternalServerError) }) mux.HandleFunc("/organizations/acme/policies/"+policy+"/revisions/", func(w http.ResponseWriter, r *http.Request) { rev := strings.TrimPrefix(r.URL.Path, "/organizations/acme/policies/"+policy+"/revisions/") diff --git a/apps/cinc/cmd/role.go b/apps/cinc/cmd/role.go index 5ce3f87..8f07ed3 100644 --- a/apps/cinc/cmd/role.go +++ b/apps/cinc/cmd/role.go @@ -1,9 +1,7 @@ package cmd import ( - "encoding/json" "fmt" - "os" cinc "github.com/cinc-project/cinc-api" "github.com/spf13/cobra" @@ -48,12 +46,8 @@ cinc role create webserver`, } role := cinc.Role{Name: args[0]} if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &role); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if role, err = readJSONFile[cinc.Role](inputFile); err != nil { + return err } role.Name = args[0] } @@ -93,12 +87,8 @@ cinc role edit webserver`, var updated cinc.Role if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.Role](inputFile); err != nil { + return err } } else { current, _, err := c.Roles.Get(cmd.Context(), name) diff --git a/apps/cinc/cmd/user.go b/apps/cinc/cmd/user.go index 8bc1c0f..968b7a3 100644 --- a/apps/cinc/cmd/user.go +++ b/apps/cinc/cmd/user.go @@ -1,7 +1,6 @@ package cmd import ( - "encoding/json" "fmt" "os" "strings" @@ -52,12 +51,8 @@ cinc user edit alice`, var updated cinc.User if inputFile != "" { - data, err := os.ReadFile(inputFile) - if err != nil { - return fmt.Errorf("cinc: read %s: %w", inputFile, err) - } - if err := json.Unmarshal(data, &updated); err != nil { - return fmt.Errorf("cinc: parse %s: %w", inputFile, err) + if updated, err = readJSONFile[cinc.User](inputFile); err != nil { + return err } } else { current, _, err := c.Users.Get(cmd.Context(), name) diff --git a/cli/explore/kind_cookbook.go b/cli/explore/kind_cookbook.go index c10b8d8..55e33a7 100644 --- a/cli/explore/kind_cookbook.go +++ b/cli/explore/kind_cookbook.go @@ -18,6 +18,8 @@ func (cookbookKind) Title() string { return "Cookbooks" } func (cookbookKind) Columns() []string { return []string{"NAME", "VERSIONS"} } func (cookbookKind) List(ctx context.Context, c *cinc.Client) ([]Row, error) { + // A bare List returns only each cookbook's latest version on erchef, so + // ask for all of them to count them. index, _, err := c.Cookbooks.ListVersions(ctx, "all") if err != nil { return nil, err @@ -41,16 +43,15 @@ func (cookbookKind) Child(parent string) Kind { return cookbookVersionsKind{name // metadata lives on the per-version manifest, so we fetch the latest version to // read it; the JSON carried along for the detail view is that same manifest. func (cookbookKind) Summary(ctx context.Context, c *cinc.Client, name string) (summaryView, error) { - index, _, err := c.Cookbooks.ListVersions(ctx, "all") + entry, _, err := c.Cookbooks.GetVersions(ctx, name, "all") if err != nil { return summaryView{}, err } - entry, ok := index[name] - if !ok { - return summaryView{}, fmt.Errorf("cookbook %q not found", name) - } versions := len(entry.Versions) - latest := latestCookbookVersion(entry.Versions) + latest := "" + if versions > 0 { + latest = entry.Versions[0].Version // newest-first + } return summarize(ctx, c, latest, func(ctx context.Context, c *cinc.Client, v string) (*cinc.Cookbook, error) { @@ -71,19 +72,6 @@ func (cookbookKind) Summary(ctx context.Context, c *cinc.Client, name string) (s }) } -// latestCookbookVersion returns the newest version string, picking the -// greatest by string comparison to match the newest-first ordering the -// version list itself uses. -func latestCookbookVersion(versions []cinc.CookbookVersion) string { - latest := "" - for _, v := range versions { - if v.Version > latest { - latest = v.Version - } - } - return latest -} - // cookbookVersionsKind lists the versions of one cookbook. A version // can be viewed (its manifest), downloaded, or deleted. type cookbookVersionsKind struct{ name string } @@ -92,20 +80,15 @@ func (k cookbookVersionsKind) Title() string { return k.name } func (cookbookVersionsKind) Columns() []string { return []string{"VERSION"} } func (k cookbookVersionsKind) List(ctx context.Context, c *cinc.Client) ([]Row, error) { - index, _, err := c.Cookbooks.ListVersions(ctx, "all") + entry, _, err := c.Cookbooks.GetVersions(ctx, k.name, "all") if err != nil { return nil, err } - entry, ok := index[k.name] - if !ok { - return nil, fmt.Errorf("cookbook %q not found", k.name) - } + // The versions come newest-first, so the latest is at the top. rows := make([]Row, 0, len(entry.Versions)) for _, v := range entry.Versions { rows = append(rows, Row{Name: v.Version, Cells: []string{v.Version}}) } - // Versions sort newest-first so the latest is at the top. - sort.Slice(rows, func(i, j int) bool { return rows[i].Name > rows[j].Name }) return rows, nil } diff --git a/cli/explore/kind_cookbook_test.go b/cli/explore/kind_cookbook_test.go index fc58a5e..b680358 100644 --- a/cli/explore/kind_cookbook_test.go +++ b/cli/explore/kind_cookbook_test.go @@ -10,24 +10,16 @@ import ( cinc "github.com/cinc-project/cinc-api" ) -func TestLatestCookbookVersion(t *testing.T) { - versions := []cinc.CookbookVersion{{Version: "1.0.0"}, {Version: "1.2.0"}, {Version: "1.1.5"}} - if got := latestCookbookVersion(versions); got != "1.2.0" { - t.Errorf("latestCookbookVersion = %q, want 1.2.0", got) - } - if got := latestCookbookVersion(nil); got != "" { - t.Errorf("latestCookbookVersion(nil) = %q, want empty", got) - } -} - // A cookbook shows its version count and newest version on hover, plus the // latest version's identity metadata so you needn't drill in to see it. +// "Latest" is the highest version by Chef's numeric ordering, so 1.10.0 +// beats 1.9.0 whatever order the server lists them in. func TestCookbookKindSummary(t *testing.T) { mux := http.NewServeMux() - jsonHandler(mux, "/organizations/acme/cookbooks", - `{"nginx":{"versions":[{"version":"1.0.0"},{"version":"1.2.0"}]}}`) - jsonHandler(mux, "/organizations/acme/cookbooks/nginx/1.2.0", - `{"cookbook_name":"nginx","version":"1.2.0", + jsonHandler(mux, "/organizations/acme/cookbooks/nginx", + `{"nginx":{"versions":[{"version":"1.9.0"},{"version":"1.10.0"},{"version":"1.0.0"}]}}`) + jsonHandler(mux, "/organizations/acme/cookbooks/nginx/1.10.0", + `{"cookbook_name":"nginx","version":"1.10.0", "metadata":{"description":"Installs and configures nginx","maintainer":"Sous Chefs","license":"Apache-2.0", "dependencies":{"apt":">= 7.0"}}}`) srv := httptest.NewServer(mux) @@ -42,8 +34,8 @@ func TestCookbookKindSummary(t *testing.T) { got[f.Label] = f.Value } want := map[string]string{ - "Versions": "2", - "Latest": "1.2.0", + "Versions": "3", + "Latest": "1.10.0", "Description": "Installs and configures nginx", "Maintainer": "Sous Chefs", "License": "Apache-2.0", diff --git a/cli/explore/kinds_test.go b/cli/explore/kinds_test.go index a170524..1bae661 100644 --- a/cli/explore/kinds_test.go +++ b/cli/explore/kinds_test.go @@ -51,10 +51,19 @@ func TestKindCapabilities(t *testing.T) { } } +// TestCookbookKindListReportsVersionCount counts every version. erchef lists +// only a cookbook's latest version unless asked for num_versions=all, which +// this server mimics. func TestCookbookKindListReportsVersionCount(t *testing.T) { mux := http.NewServeMux() - jsonHandler(mux, "/organizations/acme/cookbooks", - `{"apache":{"url":"u","versions":[{"version":"1.0.0","url":"u"},{"version":"2.0.0","url":"u"}]}}`) + mux.HandleFunc("/organizations/acme/cookbooks", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("num_versions") != "all" { + _, _ = w.Write([]byte(`{"apache":{"url":"u","versions":[{"version":"2.0.0","url":"u"}]}}`)) + return + } + _, _ = w.Write([]byte(`{"apache":{"url":"u","versions":[{"version":"1.0.0","url":"u"},{"version":"2.0.0","url":"u"}]}}`)) + }) srv := httptest.NewServer(mux) t.Cleanup(srv.Close) @@ -70,10 +79,12 @@ func TestCookbookKindListReportsVersionCount(t *testing.T) { } } +// TestCookbookVersionsSortNewestFirst drills into one cookbook. Versions +// order numerically, as Chef orders them: 1.10.0 is newer than 1.9.0. func TestCookbookVersionsSortNewestFirst(t *testing.T) { mux := http.NewServeMux() - jsonHandler(mux, "/organizations/acme/cookbooks", - `{"apache":{"url":"u","versions":[{"version":"1.0.0","url":"u"},{"version":"2.0.0","url":"u"}]}}`) + jsonHandler(mux, "/organizations/acme/cookbooks/apache", + `{"apache":{"url":"u","versions":[{"version":"1.9.0","url":"u"},{"version":"2.0.0","url":"u"},{"version":"1.10.0","url":"u"}]}}`) srv := httptest.NewServer(mux) t.Cleanup(srv.Close) @@ -81,7 +92,7 @@ func TestCookbookVersionsSortNewestFirst(t *testing.T) { if err != nil { t.Fatal(err) } - if got := names(rows); !equal(got, []string{"2.0.0", "1.0.0"}) { + if got := names(rows); !equal(got, []string{"2.0.0", "1.10.0", "1.9.0"}) { t.Errorf("versions = %v, want newest first", got) } } diff --git a/cli/policyfile/archive.go b/cli/policyfile/archive.go index 4f7a468..ffc787c 100644 --- a/cli/policyfile/archive.go +++ b/cli/policyfile/archive.go @@ -52,16 +52,13 @@ func OpenBundle(path string) (string, func(), error) { // LoadBundleCookbooks reads each cookbook the lock pins from a bundle's // cookbooks/-/ tree into an uploadable LocalCookbook, keyed by -// cookbook name. The directory layout matches what Export writes; the cookbook's -// upload name is taken from the lock (not the on-disk directory, which carries -// the identifier suffix). +// cookbook name. The directory layout matches what Export writes. PushRevision +// uploads each one under its lock name, so the identifier suffix on the +// directory never reaches the server. func LoadBundleCookbooks(dir string, lock *cinc.PolicyRevision) (map[string]*cinc.LocalCookbook, error) { cookbooks := make(map[string]*cinc.LocalCookbook, len(lock.CookbookLocks)) for name, cl := range lock.CookbookLocks { - ddi := cl.DottedDecimalIdentifier - if ddi == "" { - ddi = cl.Identifier - } + ddi := cl.DottedIdentifier() // name and ddi come from the (untrusted) lock; keep the lookup from // escaping the bundle's cookbooks directory. cbDir, err := safeJoin(dir, "cookbooks", name+"-"+ddi) @@ -72,12 +69,6 @@ func LoadBundleCookbooks(dir string, lock *cinc.PolicyRevision) (map[string]*cin if err != nil { return nil, fmt.Errorf("policyfile: load cookbook %q from %s: %w", name, cbDir, err) } - // The on-disk directory is "-"; the upload name is - // the bare cookbook name the lock records. The manifest's metadata - // must agree with it, and cinc-api falls back to the directory name - // there when metadata.rb names the cookbook with a non-literal. - cb.Name = name - cb.Metadata.Name = name cookbooks[name] = cb } return cookbooks, nil diff --git a/cli/policyfile/archive_test.go b/cli/policyfile/archive_test.go index dbc9da0..639d488 100644 --- a/cli/policyfile/archive_test.go +++ b/cli/policyfile/archive_test.go @@ -58,27 +58,6 @@ func TestLoadBundleCookbooksUsesLockNameNotDirName(t *testing.T) { } } -// TestLoadBundleCookbooksNamesMetadataFromLock covers a metadata.rb whose name -// is not a literal: cinc-api then falls back to the directory name -// (base-1.2.3) for the manifest's metadata, which disagrees with the lock name -// and makes the upload fail. The lock name has to reach the metadata too. -func TestLoadBundleCookbooksNamesMetadataFromLock(t *testing.T) { - dir, lock := writeBundle(t) - md := filepath.Join(dir, "cookbooks", "base-1.2.3", "metadata.rb") - if err := os.WriteFile(md, []byte("name File.basename(__dir__)\nversion '1.0.0'\n"), 0o644); err != nil { - t.Fatal(err) - } - - cookbooks, err := LoadBundleCookbooks(dir, lock) - if err != nil { - t.Fatalf("LoadBundleCookbooks: %v", err) - } - cb := cookbooks["base"] - if cb.Name != "base" || cb.Metadata.Name != "base" { - t.Errorf("name = %q, metadata name = %q, want both %q", cb.Name, cb.Metadata.Name, "base") - } -} - func TestOpenBundleAcceptsDirectory(t *testing.T) { dir, _ := writeBundle(t) got, cleanup, err := OpenBundle(dir) diff --git a/cli/policyfile/export.go b/cli/policyfile/export.go index 3d062c1..475a727 100644 --- a/cli/policyfile/export.go +++ b/cli/policyfile/export.go @@ -43,10 +43,7 @@ func Export(ctx context.Context, fetcher *Fetcher, lock *cinc.PolicyRevision, lo if err != nil { return ExportResult{}, err } - ddi := cl.DottedDecimalIdentifier - if ddi == "" { - ddi = cl.Identifier - } + ddi := cl.DottedIdentifier() // name and ddi come from the (untrusted) lock; keep them from escaping // the export directory. dst, err := safeJoin(destDir, "cookbooks", name+"-"+ddi) diff --git a/cli/policyfile/fetch.go b/cli/policyfile/fetch.go index 685ce73..338b424 100644 --- a/cli/policyfile/fetch.go +++ b/cli/policyfile/fetch.go @@ -141,7 +141,7 @@ func (f *Fetcher) fetchChefServer(ctx context.Context, name, version, dest strin return fmt.Errorf("chef_server source requires a configured server connection") } if version == "" { - version = "_latest" + version = cinc.LatestVersion } return f.Chef.Cookbooks.Download(ctx, name, version, dest) } @@ -151,12 +151,9 @@ func (f *Fetcher) fetchChefServer(ctx context.Context, name, version, dest strin // subdirectory) into dest, omitting the .git directory. It shells out to the // `git` binary, which must be installed. func (f *Fetcher) fetchGit(ctx context.Context, lock cinc.CookbookLock, repoURL, dest string) error { - revision := stringOption(lock.SourceOptions, "revision") + revision := lock.GitRef() if revision == "" { - revision = stringOption(lock.SourceOptions, "branch") - } - if revision == "" { - return fmt.Errorf("git source has no revision or branch to check out") + return fmt.Errorf("git source has no revision, ref, tag or branch to check out") } // repoURL and revision come from the (potentially untrusted) lock. Reject // values that begin with "-" so they cannot be smuggled in as git flags @@ -188,15 +185,9 @@ func (f *Fetcher) fetchGit(ctx context.Context, lock cinc.CookbookLock, repoURL, root := clone // chef (cookbook-omnifetch) records a cookbook in a subdirectory of its - // repository as "rel". "path" is accepted too, though a lock carrying it - // is classified as a path source before it gets here. - key := "rel" - sub := stringOption(lock.SourceOptions, key) - if sub == "" { - key = "path" - sub = stringOption(lock.SourceOptions, key) - } - if sub != "" { + // repository as "rel". A lock that also carries "path" never gets here: + // CookbookLock.Origin classifies it as a path source first. + if sub := lock.GitSubdir(); sub != "" { // The subdirectory comes from the untrusted lock, so it has to stay // inside the clone. Nested paths are legitimate (the // "cookbooks/" monorepo layout), so this is a containment @@ -204,7 +195,7 @@ func (f *Fetcher) fetchGit(ctx context.Context, lock cinc.CookbookLock, repoURL, // names like cache keys. root = filepath.Join(clone, sub) if !withinDir(clone, root) { - return fmt.Errorf("refusing git source %s %q: it escapes the repository", key, sub) + return fmt.Errorf("refusing git source rel %q: it escapes the repository", sub) } } if !hasCookbookMetadata(root) { @@ -281,13 +272,6 @@ func hasCookbookMetadata(dir string) bool { return false } -func stringOption(opts map[string]any, key string) string { - if v, ok := opts[key].(string); ok { - return v - } - return "" -} - func baseURL(raw string) (string, error) { u, err := url.Parse(raw) if err != nil || u.Scheme == "" || u.Host == "" { diff --git a/cli/policyfile/fetch_test.go b/cli/policyfile/fetch_test.go index 02669e8..69ff23e 100644 --- a/cli/policyfile/fetch_test.go +++ b/cli/policyfile/fetch_test.go @@ -209,6 +209,28 @@ func TestEnsureCookbookGitClonesAndCaches(t *testing.T) { } } +// TestEnsureCookbookGitChecksOutRefOrTag covers a lock that pins its git +// cookbook with "ref" or "tag" rather than "revision" or "branch". chef-cli +// accepts all four, so the checkout does too. +func TestEnsureCookbookGitChecksOutRefOrTag(t *testing.T) { + repo, sha := initGitCookbookRepo(t) + for _, key := range []string{"ref", "tag"} { + t.Run(key, func(t *testing.T) { + f := &Fetcher{CacheRoot: t.TempDir(), LockDir: t.TempDir()} + dir, err := f.EnsureCookbook(context.Background(), "gitcb", cinc.CookbookLock{ + CacheKey: "gitcb-" + key, + SourceOptions: map[string]any{"git": repo, key: sha}, + }) + if err != nil { + t.Fatalf("EnsureCookbook (git, %s): %v", key, err) + } + if _, err := os.Stat(filepath.Join(dir, "metadata.rb")); err != nil { + t.Errorf("git cookbook not cached: %v", err) + } + }) + } +} + func TestEnsureCookbookGitRejectsFlagInjection(t *testing.T) { f := &Fetcher{CacheRoot: t.TempDir(), LockDir: t.TempDir()} cases := []struct { @@ -281,79 +303,6 @@ func TestEnsureCookbookRejectsCacheKeyTraversal(t *testing.T) { } } -// TestFetchGitRejectsPathEscapingTheClone covers the git source's "path" -// sub-option, which selects a subdirectory of the clone and comes from the -// untrusted lock. Today EnsureCookbook cannot deliver such a lock here, -// because cinc-api's CookbookLock.Origin prefers the "path" key over "git" -// and classifies the lock as a path source before fetchGit is reached. That -// makes this containment check defence in depth rather than a live fix: the -// guard belongs next to the join, not in another module's key precedence. -// -// Without it, a path of "../.." reaches any directory on the machine holding -// a metadata.rb, and copyTree would pull it into the cookbook cache. -func TestFetchGitRejectsPathEscapingTheClone(t *testing.T) { - repo, sha := initGitCookbookRepo(t) - f := &Fetcher{CacheRoot: t.TempDir(), LockDir: t.TempDir()} - - for _, sub := range []string{"../outside", "nested/../../outside"} { - t.Run(sub, func(t *testing.T) { - lock := cinc.CookbookLock{ - SourceOptions: map[string]any{"git": repo, "revision": sha, "path": sub}, - } - err := f.fetchGit(context.Background(), lock, repo, t.TempDir()) - if err == nil { - t.Fatalf("path %q was accepted, want it rejected for escaping the clone", sub) - } - if !contains(err.Error(), "escapes") { - t.Errorf("error = %v, want it to say the path escapes the repository", err) - } - }) - } -} - -// TestFetchGitAcceptsNestedPath is the other half: the common -// "cookbooks/" monorepo layout still resolves. -func TestFetchGitAcceptsNestedPath(t *testing.T) { - if _, err := exec.LookPath("git"); err != nil { - t.Skip("git not installed") - } - repo := t.TempDir() - nested := filepath.Join(repo, "cookbooks", "nested") - if err := os.MkdirAll(nested, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(nested, "metadata.rb"), []byte("name 'nested'\nversion '1.0.0'\n"), 0o644); err != nil { - t.Fatal(err) - } - for _, args := range [][]string{ - {"init", "--quiet"}, - {"config", "user.email", "t@example.test"}, - {"config", "user.name", "Test"}, - {"add", "-A"}, - {"commit", "--quiet", "-m", "cookbook"}, - } { - if out, err := runGit(context.Background(), repo, args...); err != nil { - t.Fatalf("git %v: %v: %s", args, err, out) - } - } - out, err := runGit(context.Background(), repo, "rev-parse", "HEAD") - if err != nil { - t.Fatalf("git rev-parse: %v: %s", err, out) - } - - f := &Fetcher{CacheRoot: t.TempDir(), LockDir: t.TempDir()} - dest := t.TempDir() - lock := cinc.CookbookLock{ - SourceOptions: map[string]any{"git": repo, "revision": strings.TrimSpace(out), "path": "cookbooks/nested"}, - } - if err := f.fetchGit(context.Background(), lock, repo, dest); err != nil { - t.Fatalf("fetchGit with a nested path: %v", err) - } - if _, err := os.Stat(filepath.Join(dest, "metadata.rb")); err != nil { - t.Errorf("nested cookbook not fetched: %v", err) - } -} - // TestEnsureCookbookGitHonoursRel covers the lock chef writes for a git // cookbook in a subdirectory of its repository: cookbook-omnifetch records // the subdirectory as source_options "rel", not "path" (a "path" key would diff --git a/cli/policyfile/push.go b/cli/policyfile/push.go deleted file mode 100644 index 53d4d31..0000000 --- a/cli/policyfile/push.go +++ /dev/null @@ -1,41 +0,0 @@ -package policyfile - -import ( - "context" - "fmt" - - cinc "github.com/cinc-project/cinc-api" -) - -// ArtifactsToUpload counts the lock's cookbooks the server does not hold yet -// as cookbook artifacts under the identifiers the lock records. That is how -// many cookbooks cinc-api's PushRevision uploads: it skips an artifact the -// server already has, so a lock pushed to a second group uploads none. -// -// The count is read just before the push, so a concurrent push of the same -// artifacts can make it high by the ones the other push stored first. -func ArtifactsToUpload(ctx context.Context, c *cinc.Client, lock *cinc.PolicyRevision) (int, error) { - if len(lock.CookbookLocks) == 0 { - return 0, nil - } - remote, _, err := c.CookbookArtifacts.List(ctx) - if err != nil { - return 0, fmt.Errorf("cinc: list cookbook artifacts: %w", err) - } - missing := 0 - for name, cl := range lock.CookbookLocks { - if !hasArtifact(remote[name], cl.Identifier) { - missing++ - } - } - return missing, nil -} - -func hasArtifact(entry cinc.CookbookArtifactListEntry, identifier string) bool { - for _, v := range entry.Versions { - if v.Identifier == identifier { - return true - } - } - return false -} diff --git a/cli/policyfile/push_test.go b/cli/policyfile/push_test.go deleted file mode 100644 index 4b35b8e..0000000 --- a/cli/policyfile/push_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package policyfile - -import ( - "context" - "crypto/rand" - "crypto/rsa" - "io" - "net/http" - "net/http/httptest" - "testing" - - cinc "github.com/cinc-project/cinc-api" -) - -func TestArtifactsToUpload(t *testing.T) { - mux := http.NewServeMux() - mux.HandleFunc("/organizations/acme/cookbook_artifacts", func(w http.ResponseWriter, _ *http.Request) { - // The server holds base@aaa and an older app@old, but not app@new. - _, _ = io.WriteString(w, `{ - "base": {"url": "u", "versions": [{"identifier": "aaa", "url": "u"}]}, - "app": {"url": "u", "versions": [{"identifier": "old", "url": "u"}]} - }`) - }) - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal(err) - } - c, err := cinc.NewClient(cinc.Config{ServerURL: srv.URL, Org: "acme", ClientName: "tim", Key: key}) - if err != nil { - t.Fatal(err) - } - - lock := &cinc.PolicyRevision{CookbookLocks: map[string]cinc.CookbookLock{ - "base": {Identifier: "aaa"}, - "app": {Identifier: "new"}, - "db": {Identifier: "ddd"}, - }} - got, err := ArtifactsToUpload(context.Background(), c, lock) - if err != nil { - t.Fatalf("ArtifactsToUpload: %v", err) - } - if got != 2 { - t.Errorf("ArtifactsToUpload = %d, want 2 (app@new and db@ddd)", got) - } - - // A lock with no cookbooks needs no listing at all. - if got, err := ArtifactsToUpload(context.Background(), c, &cinc.PolicyRevision{}); err != nil || got != 0 { - t.Errorf("ArtifactsToUpload(no cookbooks) = %d, %v; want 0, nil", got, err) - } -} diff --git a/docs/commands/cinc_policy_diff.md b/docs/commands/cinc_policy_diff.md index f411eff..c0193df 100644 --- a/docs/commands/cinc_policy_diff.md +++ b/docs/commands/cinc_policy_diff.md @@ -16,10 +16,16 @@ cinc policy diff [flags] ### Examples -Compare two revisions of a policy. +Compare the appserver revisions active in the staging and production groups. + +```bash +cinc policy diff appserver staging production +``` + +Compare two revisions by their revision ids. ```bash -cinc policy diff appserver 1.0.0 1.1.0 +cinc policy diff appserver --revisions 1a2b3c4d 5e6f7a8b ``` ### Options diff --git a/integration/go.mod b/integration/go.mod index 3c918a6..350474e 100644 --- a/integration/go.mod +++ b/integration/go.mod @@ -15,6 +15,8 @@ require ( golang.org/x/crypto v0.57.0 ) +require github.com/kevinburke/ssh_config v1.6.0 // indirect + require ( github.com/BurntSushi/toml v1.6.0 // indirect github.com/alecthomas/chroma/v2 v2.27.0 // indirect @@ -37,7 +39,6 @@ require ( github.com/google/uuid v1.6.0 // indirect github.com/goruby/goruby v0.0.0-20210827060341-983436007185 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect - github.com/kevinburke/ssh_config v1.6.0 // indirect github.com/lucasb-eyer/go-colorful v1.4.1 // indirect github.com/mattn/go-isatty v0.0.24 // indirect github.com/mattn/go-localereader v0.0.1 // indirect diff --git a/integration/suite/databags.go b/integration/suite/databags.go index f66dbfb..0cde544 100644 --- a/integration/suite/databags.go +++ b/integration/suite/databags.go @@ -18,7 +18,7 @@ var databagFamily = family{cases: []testCase{ {"databags/create-with-item", []string{"databag create", "databag item show"}, testDatabagCreateWithItem}, {"databags/create-with-item-bad-file", []string{"databag create", "databag list"}, testDatabagCreateWithItemBadFile}, {"databags/item-show-exact", []string{"databag item create", "databag item show", "databag item edit"}, testDatabagItemShowExact}, - {"databags/item-id-from-argument", []string{"databag item create", "databag item edit", "databag item list"}, testDatabagItemIDFromArgument}, + {"databags/item-id-mismatch", []string{"databag item create", "databag item edit", "databag item list"}, testDatabagItemIDMismatch}, {"databags/special-names", []string{"databag create", "databag item create", "databag item show", "databag item delete"}, testDatabagSpecialNames}, {"databags/invalid-names", []string{"databag create", "databag item create"}, testDatabagInvalidNames}, {"databags/already-exists", []string{"databag create", "databag item create", "databag secret create"}, testDatabagAlreadyExists}, @@ -194,21 +194,19 @@ func testDatabagItemShowExact(t *testing.T, _ Target, c *cli) { } } -// testDatabagItemIDFromArgument checks that the id argument names the item, -// whatever id the file carries: create stores it under the argument, and -// edit updates the argument's item rather than creating or renaming one. -func testDatabagItemIDFromArgument(t *testing.T, _ Target, c *cli) { +// testDatabagItemIDMismatch checks that a --file naming another item is +// refused, naming both ids: create stores nothing, and edit leaves the +// argument's item as it was rather than overwriting it with the file. +func testDatabagItemIDMismatch(t *testing.T, _ Target, c *cli) { bag := databagNew(c) - c.run("databag", "item", "create", bag, "foo", "--file", writeJSON(t, map[string]any{"id": "bar", "v": "1"})) - wantSlice(t, "items after create", databagItemIDs(c, bag), []string{"foo"}) - if got := databagItemShow(c, bag, "foo"); !reflect.DeepEqual(got, map[string]any{"id": "foo", "v": "1"}) { - t.Errorf("created item = %v, want id foo", got) - } + databagWantStderr(t, c.fail("databag", "item", "create", bag, "foo", "--file", writeJSON(t, map[string]any{"id": "bar", "v": "1"})), `"bar"`, `"foo"`) + wantSlice(t, "items after a refused create", databagItemIDs(c, bag), []string{}) - c.run("databag", "item", "edit", bag, "foo", "--file", writeJSON(t, map[string]any{"id": "baz", "v": "2"})) - wantSlice(t, "items after edit", databagItemIDs(c, bag), []string{"foo"}) - if got := databagItemShow(c, bag, "foo"); !reflect.DeepEqual(got, map[string]any{"id": "foo", "v": "2"}) { - t.Errorf("edited item = %v, want id foo", got) + databagItemCreate(c, bag, map[string]any{"id": "foo", "v": "1"}) + databagWantStderr(t, c.fail("databag", "item", "edit", bag, "foo", "--file", writeJSON(t, map[string]any{"id": "baz", "v": "2"})), `"baz"`, `"foo"`) + wantSlice(t, "items after a refused edit", databagItemIDs(c, bag), []string{"foo"}) + if got := databagItemShow(c, bag, "foo"); !reflect.DeepEqual(got, map[string]any{"id": "foo", "v": "1"}) { + t.Errorf("item after a refused edit = %v, want it unchanged", got) } } @@ -550,11 +548,11 @@ func testDatabagSecretEdit(t *testing.T, _ Target, c *cli) { } } - // The id argument pins the item, whatever the file says. - c.run("databag", "secret", "edit", bag, "api-key", - "--file", writeJSON(t, map[string]any{"id": "other", "token": "v3"}), "--secret-file", secret) + // A file naming another item is refused, and the item is left alone. + databagWantStderr(t, c.fail("databag", "secret", "edit", bag, "api-key", + "--file", writeJSON(t, map[string]any{"id": "other", "token": "v3"}), "--secret-file", secret), `"other"`, `"api-key"`) wantSlice(t, "items", databagItemIDs(c, bag), []string{"api-key"}) - wantEqual(t, "token", databagSecretShow(c, nil, bag, "api-key", "--secret-file", secret)["token"], any("v3")) + wantEqual(t, "token", databagSecretShow(c, nil, bag, "api-key", "--secret-file", secret)["token"], any("v2-rotated")) } // testDatabagSecretWrongSecret checks that the wrong secret gives a clear