From 181ad4e4faae82e540b9cafe021fff8aab7bd286 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Fri, 12 Apr 2024 11:44:26 -0400 Subject: [PATCH] Don't error out on secret create --replace on missing secret Consolidate all errors between the different secret drivers. Fixes: https://github.com/containers/podman/issues/22000 Fixes: https://github.com/containers/podman/issues/21952 Signed-off-by: Daniel J Walsh --- pkg/secrets/define/secrets.go | 16 ++++++++++++ pkg/secrets/filedriver/filedriver.go | 13 +++------- pkg/secrets/passdriver/passdriver.go | 27 ++++++++------------- pkg/secrets/passdriver/passdriver_test.go | 11 +++++---- pkg/secrets/secrets.go | 13 ++++++---- pkg/secrets/secrets_test.go | 19 +++++++++++++++ pkg/secrets/secretsdb.go | 14 ++++++----- pkg/secrets/shelldriver/shelldriver.go | 25 +++++++------------ pkg/secrets/shelldriver/shelldriver_test.go | 11 +++++---- 9 files changed, 86 insertions(+), 63 deletions(-) create mode 100755 pkg/secrets/define/secrets.go diff --git a/pkg/secrets/define/secrets.go b/pkg/secrets/define/secrets.go new file mode 100755 index 000000000..6fb8c1020 --- /dev/null +++ b/pkg/secrets/define/secrets.go @@ -0,0 +1,16 @@ +package define + +import ( + "errors" +) + +var ( + // ErrNoSuchSecret indicates that the secret does not exist + ErrNoSuchSecret = errors.New("no such secret") + + // ErrSecretIDExists indicates that there is secret data already associated with an id + ErrSecretIDExists = errors.New("secret data with ID already exists") + + // ErrInvalidKey indicates that something about your key is wrong + ErrInvalidKey = errors.New("invalid key") +) diff --git a/pkg/secrets/filedriver/filedriver.go b/pkg/secrets/filedriver/filedriver.go index 48817d0e6..54df4a69e 100644 --- a/pkg/secrets/filedriver/filedriver.go +++ b/pkg/secrets/filedriver/filedriver.go @@ -9,6 +9,7 @@ import ( "path/filepath" "sort" + "github.com/containers/common/pkg/secrets/define" "github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/lockfile" "golang.org/x/exp/maps" @@ -17,12 +18,6 @@ import ( // secretsDataFile is the file where secrets data/payload will be stored var secretsDataFile = "secretsdata.json" -// errNoSecretData indicates that there is not data associated with an id -var errNoSecretData = errors.New("no secret data with ID") - -// errNoSecretData indicates that there is secret data already associated with an id -var errSecretIDExists = errors.New("secret data with ID already exists") - // Driver is the filedriver object type Driver struct { // secretsDataFilePath is the path to the secretsfile @@ -75,7 +70,7 @@ func (d *Driver) Lookup(id string) ([]byte, error) { if data, ok := secretData[id]; ok { return data, nil } - return nil, fmt.Errorf("%s: %w", id, errNoSecretData) + return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } // Store stores the bytes associated with an ID. An error is returned if the ID already exists @@ -88,7 +83,7 @@ func (d *Driver) Store(id string, data []byte) error { return err } if _, ok := secretData[id]; ok { - return fmt.Errorf("%s: %w", id, errSecretIDExists) + return fmt.Errorf("%s: %w", id, define.ErrSecretIDExists) } secretData[id] = data marshalled, err := json.MarshalIndent(secretData, "", " ") @@ -113,7 +108,7 @@ func (d *Driver) Delete(id string) error { if _, ok := secretData[id]; ok { delete(secretData, id) } else { - return fmt.Errorf("%s: %w", id, errNoSecretData) + return fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } marshalled, err := json.MarshalIndent(secretData, "", " ") if err != nil { diff --git a/pkg/secrets/passdriver/passdriver.go b/pkg/secrets/passdriver/passdriver.go index d6a4f63a6..b99541faa 100644 --- a/pkg/secrets/passdriver/passdriver.go +++ b/pkg/secrets/passdriver/passdriver.go @@ -12,20 +12,10 @@ import ( "sort" "strings" + "github.com/containers/common/pkg/secrets/define" "github.com/containers/storage/pkg/fileutils" ) -var ( - // errNoSecretData indicates that there is not data associated with an id - errNoSecretData = errors.New("no secret data with ID") - - // errNoSecretData indicates that there is secret data already associated with an id - errSecretIDExists = errors.New("secret data with ID already exists") - - // errInvalidKey indicates that something about your key is wrong - errInvalidKey = errors.New("invalid key") -) - type driverConfig struct { // Root contains the root directory where the secrets are stored Root string @@ -128,10 +118,10 @@ func (d *Driver) Lookup(id string) ([]byte, error) { return nil, err } if err := d.gpg(context.TODO(), nil, out, "--decrypt", key); err != nil { - return nil, fmt.Errorf("%s: %w", id, errNoSecretData) + return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } if out.Len() == 0 { - return nil, fmt.Errorf("%s: %w", id, errNoSecretData) + return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } return out.Bytes(), nil } @@ -139,7 +129,7 @@ func (d *Driver) Lookup(id string) ([]byte, error) { // Store saves the bytes associated with an ID. An error is returned if the ID already exists func (d *Driver) Store(id string, data []byte) error { if _, err := d.Lookup(id); err == nil { - return fmt.Errorf("%s: %w", id, errSecretIDExists) + return fmt.Errorf("%s: %w", id, define.ErrSecretIDExists) } in := bytes.NewReader(data) key, err := d.getPath(id) @@ -156,7 +146,10 @@ func (d *Driver) Delete(id string) error { return err } if err := os.Remove(key); err != nil { - return fmt.Errorf("%s: %w", id, errNoSecretData) + if errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) + } + return fmt.Errorf("%s: %w", id, err) } return nil } @@ -176,10 +169,10 @@ func (d *Driver) gpg(ctx context.Context, in io.Reader, out io.Writer, args ...s func (d *Driver) getPath(id string) (string, error) { path, err := filepath.Abs(filepath.Join(d.Root, id)) if err != nil { - return "", errInvalidKey + return "", define.ErrInvalidKey } if !strings.HasPrefix(path, d.Root) { - return "", errInvalidKey + return "", define.ErrInvalidKey } return path + ".gpg", nil } diff --git a/pkg/secrets/passdriver/passdriver_test.go b/pkg/secrets/passdriver/passdriver_test.go index 43413ebd3..86dffcc1f 100644 --- a/pkg/secrets/passdriver/passdriver_test.go +++ b/pkg/secrets/passdriver/passdriver_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/containers/common/pkg/secrets/define" "github.com/stretchr/testify/require" ) @@ -54,7 +55,7 @@ func TestStoreAndLookup(t *testing.T) { name: "storing into a sneaky key fails", key: "../../../sneaky", value: []byte("abc"), - expStoreErr: errInvalidKey, + expStoreErr: define.ErrInvalidKey, }, } @@ -102,12 +103,12 @@ func TestLookup(t *testing.T) { { name: "lookup of a non-existing key fails", key: "invalid", - expErr: fmt.Errorf("invalid: %w", errNoSecretData), + expErr: fmt.Errorf("invalid: %w", define.ErrNoSuchSecret), }, { name: "lookup of a sneaky key fails", key: "../../../etc/shadow", - expErr: errInvalidKey, + expErr: define.ErrInvalidKey, }, } @@ -151,12 +152,12 @@ func TestDelete(t *testing.T) { { name: "deleting an non-existing item fails", key: "wrong", - expErr: fmt.Errorf("wrong: %w", errNoSecretData), + expErr: fmt.Errorf("wrong: %w", define.ErrNoSuchSecret), }, { name: "using a sneaky path fails", key: "../../../etc/shadow", - expErr: errInvalidKey, + expErr: define.ErrInvalidKey, }, } diff --git a/pkg/secrets/secrets.go b/pkg/secrets/secrets.go index 7092c8f2b..8ffcc738b 100644 --- a/pkg/secrets/secrets.go +++ b/pkg/secrets/secrets.go @@ -7,6 +7,7 @@ import ( "path/filepath" "time" + "github.com/containers/common/pkg/secrets/define" "github.com/containers/common/pkg/secrets/filedriver" "github.com/containers/common/pkg/secrets/passdriver" "github.com/containers/common/pkg/secrets/shelldriver" @@ -26,7 +27,7 @@ const secretIDLength = 25 var errInvalidPath = errors.New("invalid secrets path") // ErrNoSuchSecret indicates that the secret does not exist -var ErrNoSuchSecret = errors.New("no such secret") +var ErrNoSuchSecret = define.ErrNoSuchSecret // errSecretNameInUse indicates that the secret name is already in use var errSecretNameInUse = errors.New("secret name in use") @@ -151,7 +152,7 @@ func (s *SecretsManager) newID() (string, error) { newID = newID[0:secretIDLength] _, err := s.lookupSecret(newID) if err != nil { - if errors.Is(err, ErrNoSuchSecret) { + if errors.Is(err, define.ErrNoSuchSecret) { return newID, nil } return "", err @@ -217,12 +218,14 @@ func (s *SecretsManager) Store(name string, data []byte, driverType string, opti } if options.Replace { - if err := driver.Delete(secr.ID); err != nil { + if err := driver.Delete(secr.ID); err != nil && !errors.Is(err, define.ErrNoSuchSecret) { return "", fmt.Errorf("deleting secret %s: %w", secr.ID, err) } - if err := s.delete(secr.ID); err != nil { - return "", fmt.Errorf("deleting secret %s: %w", secr.ID, err) + if err == nil { + if err := s.delete(secr.ID); err != nil && !errors.Is(err, define.ErrNoSuchSecret) { + return "", fmt.Errorf("deleting secret %s: %w", secr.ID, err) + } } } diff --git a/pkg/secrets/secrets_test.go b/pkg/secrets/secrets_test.go index 395eab227..f68763ff8 100644 --- a/pkg/secrets/secrets_test.go +++ b/pkg/secrets/secrets_test.go @@ -147,6 +147,25 @@ func TestAddSecretDupName(t *testing.T) { _, err = manager.Store("mysecret", []byte("mydata"), drivertype, storeOpts) require.Error(t, err) + + storeOpts.Replace = true + _, err = manager.Store("mysecret", []byte("mydata"), drivertype, storeOpts) + require.NoError(t, err) +} + +func TestAddReplaceSecretName(t *testing.T) { + manager, opts := setup(t) + + storeOpts := StoreOptions{ + DriverOpts: opts, + Replace: true, + } + + _, err := manager.Store("mysecret", []byte("mydata"), drivertype, storeOpts) + require.NoError(t, err) + + _, err = manager.Store("mysecret", []byte("mydata.diff"), drivertype, storeOpts) + require.NoError(t, err) } func TestAddSecretPrefix(t *testing.T) { diff --git a/pkg/secrets/secretsdb.go b/pkg/secrets/secretsdb.go index cc86c4501..fb8201f59 100644 --- a/pkg/secrets/secretsdb.go +++ b/pkg/secrets/secretsdb.go @@ -8,6 +8,8 @@ import ( "os" "strings" "time" + + "github.com/containers/common/pkg/secrets/define" ) type db struct { @@ -70,14 +72,14 @@ func (s *SecretsManager) getNameAndID(nameOrID string) (name, id string, err err name, id, err = s.getExactNameAndID(nameOrID) if err == nil { return name, id, nil - } else if !errors.Is(err, ErrNoSuchSecret) { + } else if !errors.Is(err, define.ErrNoSuchSecret) { return "", "", err } // ID prefix may have been given, iterate through all IDs. // ID and partial ID has a max length of 25, so we return if its greater than that. if len(nameOrID) > secretIDLength { - return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, ErrNoSuchSecret) + return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, define.ErrNoSuchSecret) } exists := false var foundID, foundName string @@ -95,7 +97,7 @@ func (s *SecretsManager) getNameAndID(nameOrID string) (name, id string, err err if exists { return foundName, foundID, nil } - return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, ErrNoSuchSecret) + return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, define.ErrNoSuchSecret) } // getExactNameAndID takes a secret's name or ID and returns both its name and full ID. @@ -114,7 +116,7 @@ func (s *SecretsManager) getExactNameAndID(nameOrID string) (name, id string, er return name, id, nil } - return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, ErrNoSuchSecret) + return "", "", fmt.Errorf("no secret with name or id %q: %w", nameOrID, define.ErrNoSuchSecret) } // exactSecretExists checks if the secret exists, given a name or ID @@ -122,7 +124,7 @@ func (s *SecretsManager) getExactNameAndID(nameOrID string) (name, id string, er func (s *SecretsManager) exactSecretExists(nameOrID string) (bool, error) { _, _, err := s.getExactNameAndID(nameOrID) if err != nil { - if errors.Is(err, ErrNoSuchSecret) { + if errors.Is(err, define.ErrNoSuchSecret) { return false, nil } return false, err @@ -157,7 +159,7 @@ func (s *SecretsManager) lookupSecret(nameOrID string) (*Secret, error) { return &secret, nil } - return nil, fmt.Errorf("no secret with name or id %q: %w", nameOrID, ErrNoSuchSecret) + return nil, fmt.Errorf("no secret with name or id %q: %w", nameOrID, define.ErrNoSuchSecret) } // Store creates a new secret in the secrets database. diff --git a/pkg/secrets/shelldriver/shelldriver.go b/pkg/secrets/shelldriver/shelldriver.go index c903ca749..262ee5cfc 100644 --- a/pkg/secrets/shelldriver/shelldriver.go +++ b/pkg/secrets/shelldriver/shelldriver.go @@ -9,20 +9,13 @@ import ( "os/exec" "sort" "strings" -) - -var ( - - // errMissingConfig indicates that one or more of the external actions are not configured - errMissingConfig = errors.New("missing config value") - // errNoSecretData indicates that there is not data associated with an id - errNoSecretData = errors.New("no secret data with ID") - - // errInvalidKey indicates that something about your key is wrong - errInvalidKey = errors.New("invalid key") + "github.com/containers/common/pkg/secrets/define" ) +// errMissingConfig indicates that one or more of the external actions are not configured +var errMissingConfig = errors.New("missing config value") + type driverConfig struct { // DeleteCommand contains a shell command that deletes a secret. // The secret id is provided as environment variable SECRET_ID @@ -111,7 +104,7 @@ func (d *Driver) List() (secrets []string, err error) { // Lookup returns the bytes associated with a secret ID func (d *Driver) Lookup(id string) ([]byte, error) { if strings.Contains(id, "..") { - return nil, errInvalidKey + return nil, define.ErrInvalidKey } cmd := exec.CommandContext(context.TODO(), "/bin/sh", "-c", d.LookupCommand) @@ -124,7 +117,7 @@ func (d *Driver) Lookup(id string) ([]byte, error) { err := cmd.Run() if err != nil { - return nil, fmt.Errorf("%s: %w", id, errNoSecretData) + return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } return buf.Bytes(), nil } @@ -132,7 +125,7 @@ func (d *Driver) Lookup(id string) ([]byte, error) { // Store saves the bytes associated with an ID. An error is returned if the ID already exists func (d *Driver) Store(id string, data []byte) error { if strings.Contains(id, "..") { - return errInvalidKey + return define.ErrInvalidKey } cmd := exec.CommandContext(context.TODO(), "/bin/sh", "-c", d.StoreCommand) @@ -149,7 +142,7 @@ func (d *Driver) Store(id string, data []byte) error { // Delete removes the secret associated with the specified ID. An error is returned if no matching secret is found. func (d *Driver) Delete(id string) error { if strings.Contains(id, "..") { - return errInvalidKey + return define.ErrInvalidKey } cmd := exec.CommandContext(context.TODO(), "/bin/sh", "-c", d.DeleteCommand) @@ -161,7 +154,7 @@ func (d *Driver) Delete(id string) error { err := cmd.Run() if err != nil { - return fmt.Errorf("%s: %w", id, errNoSecretData) + return fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) } return nil diff --git a/pkg/secrets/shelldriver/shelldriver_test.go b/pkg/secrets/shelldriver/shelldriver_test.go index 42391296f..6c5f1468d 100644 --- a/pkg/secrets/shelldriver/shelldriver_test.go +++ b/pkg/secrets/shelldriver/shelldriver_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/containers/common/pkg/secrets/define" "github.com/stretchr/testify/require" ) @@ -46,7 +47,7 @@ func TestStoreAndLookup(t *testing.T) { name: "storing into a sneaky key fails", key: "../../../sneaky", value: []byte("abc"), - expStoreErr: errInvalidKey, + expStoreErr: define.ErrInvalidKey, }, } @@ -94,12 +95,12 @@ func TestLookup(t *testing.T) { { name: "lookup of a non-existing key fails", key: "invalid", - expErr: fmt.Errorf("invalid: %w", errNoSecretData), + expErr: fmt.Errorf("invalid: %w", define.ErrNoSuchSecret), }, { name: "lookup of a sneaky key fails", key: "../../../etc/shadow", - expErr: errInvalidKey, + expErr: define.ErrInvalidKey, }, } @@ -143,12 +144,12 @@ func TestDelete(t *testing.T) { { name: "deleting an non-existing item fails", key: "wrong", - expErr: fmt.Errorf("wrong: %w", errNoSecretData), + expErr: fmt.Errorf("wrong: %w", define.ErrNoSuchSecret), }, { name: "using a sneaky path fails", key: "../../../etc/shadow", - expErr: errInvalidKey, + expErr: define.ErrInvalidKey, }, }