Fix PushSecret lookup in keepersecurity provider (#4077)

* Fixed Keeper Security custom record type name in docs

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

* Fixed Keeper records lookup in PushSecret

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

* Improved Keeper record lookup to search only for records of the expected type
Improved PushSecret and DeleteSecret
Fixed "nil pointer dereference" errors

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

* Fixed tests

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

* chore(helm): Add extra labels to the validating webhooks (#4074)

It should add a bunch of app.kubernetes.io labels

Signed-off-by: Miguel Sacristán Izcue <miguel_tete17@hotmail.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

* Added tests for secrets with multiple matches

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>

---------

Signed-off-by: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com>
Signed-off-by: Miguel Sacristán Izcue <miguel_tete17@hotmail.com>
Co-authored-by: Tete17 <miguel_tete17@hotmail.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

docs/provider/keeper-security.md

@@ -80,7 +80,7 @@ There are some limitations using this provider.
 
 ## Push Secrets
 
-Push Secret will only work with a custom KeeperSecurity Record type `ExternalSecret`
+Push Secret will only work with a custom KeeperSecurity Record type `externalSecrets`
 
 ### Behavior
 * `selector`:

pkg/provider/keepersecurity/client.go

@@ -46,7 +46,7 @@ const (
 	errInvalidJSONSecret                        = "invalid Secret. Secret %s can not be converted to JSON. %w"
 	errInvalidRegex                             = "find.name.regex. Invalid Regular expresion %s. %w"
 	errInvalidRemoteRefKey                      = "match.remoteRef.remoteKey. Invalid format. Format should match secretName/key got %s"
-	errInvalidSecretType                        = "ESO can only push/delete %s record types. Secret %s is type %s"
+	errInvalidSecretType                        = "ESO can only push/delete records of type %s. Secret %s is type %s"
 	errFieldNotFound                            = "secret %s does not contain any custom field with label %s"
 
 	externalSecretType = "externalSecrets"
@@ -66,6 +66,7 @@ type Client struct {
 type SecurityClient interface {
 	GetSecrets(filter []string) ([]*ksm.Record, error)
 	GetSecretByTitle(recordTitle string) (*ksm.Record, error)
+	GetSecretsByTitle(recordTitle string) (records []*ksm.Record, err error)
 	CreateSecretWithRecordData(recUID, folderUID string, recordData *ksm.RecordCreate) (string, error)
 	DeleteSecrets(recrecordUids []string) (map[string]string, error)
 	Save(record *ksm.Record) error
@@ -172,24 +173,22 @@ func (c *Client) PushSecret(_ context.Context, secret *corev1.Secret, data esv1b
 	if err != nil {
 		return err
 	}
+
 	record, err := c.findSecretByName(parts[0])
 	if err != nil {
-		_, err = c.createSecret(parts[0], parts[1], value)
-		if err != nil {
-			return err
-		}
+		return err
 	}
+
 	if record != nil {
-		if record.Type() != externalSecretType {
+		if record.Type() == externalSecretType {
+			return c.updateSecret(record, parts[1], value)
+		} else {
 			return fmt.Errorf(errInvalidSecretType, externalSecretType, record.Title(), record.Type())
 		}
-		err = c.updateSecret(record, parts[1], value)
-		if err != nil {
-			return err
-		}
+	} else {
+		_, err = c.createSecret(parts[0], parts[1], value)
+		return err
 	}
-
-	return nil
 }
 
 func (c *Client) DeleteSecret(_ context.Context, remoteRef esv1beta1.PushSecretRemoteRef) error {
@@ -200,16 +199,15 @@ func (c *Client) DeleteSecret(_ context.Context, remoteRef esv1beta1.PushSecretR
 	secret, err := c.findSecretByName(parts[0])
 	if err != nil {
 		return err
+	} else if secret == nil {
+		return nil // not found == already deleted (success)
 	}
+
 	if secret.Type() != externalSecretType {
 		return fmt.Errorf(errInvalidSecretType, externalSecretType, secret.Title(), secret.Type())
 	}
 	_, err = c.ksmClient.DeleteSecrets([]string{secret.Uid})
-	if err != nil {
-		return nil
-	}
-
-	return nil
+	return err
 }
 
 func (c *Client) SecretExists(_ context.Context, _ esv1beta1.PushSecretRemoteRef) (bool, error) {
@@ -325,12 +323,32 @@ func (c *Client) findSecretByID(id string) (*ksm.Record, error) {
 }
 
 func (c *Client) findSecretByName(name string) (*ksm.Record, error) {
-	record, err := c.ksmClient.GetSecretByTitle(name)
+	records, err := c.ksmClient.GetSecretsByTitle(name)
 	if err != nil {
 		return nil, err
 	}
 
-	return record, nil
+	// filter in-place, preserve only records of type externalSecretType
+	n := 0
+	for _, record := range records {
+		if record.Type() == externalSecretType {
+			records[n] = record
+			n++
+		}
+	}
+	records = records[:n]
+
+	// record not found is not an error - handled differently:
+	// PushSecret will create new record instead
+	// DeleteSecret will consider record already deleted (no error)
+	if len(records) == 0 {
+		return nil, nil
+	} else if len(records) == 1 {
+		return records[0], nil
+	}
+
+	// len(records) > 1
+	return nil, fmt.Errorf(errKeeperSecuritySecretNotUnique, name)
 }
 
 func (s *Secret) validate() error {

pkg/provider/keepersecurity/client_test.go

@@ -70,8 +70,8 @@ func TestClientDeleteSecret(t *testing.T) {
 							record0: record0,
 						}, nil
 					},
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
-						return generateRecords()[0], nil
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return generateRecords()[:1], nil
 					},
 				},
 				folderID: folderID,
@@ -85,11 +85,16 @@ func TestClientDeleteSecret(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Delete invalid secret type",
+			name: "Delete secret with multiple matches by Name",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
-						return generateRecords()[1], nil
+					DeleteSecretsFn: func(recrecordUids []string) (map[string]string, error) {
+						return map[string]string{
+							record0: record0,
+						}, nil
+					},
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return []*ksm.Record{generateRecords()[0], generateRecords()[0]}, nil
 					},
 				},
 				folderID: folderID,
@@ -106,7 +111,7 @@ func TestClientDeleteSecret(t *testing.T) {
 			name: "Delete non existing secret",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
 						return nil, errors.New("failed")
 					},
 				},
@@ -304,6 +309,24 @@ func TestClientGetSecret(t *testing.T) {
 			wantErr: false,
 		},
 		{
+			name: "Get secret with multiple matches by ID",
+			fields: fields{
+				ksmClient: &fake.MockKeeperClient{
+					GetSecretsFn: func(filter []string) ([]*ksm.Record, error) {
+						return []*ksm.Record{generateRecords()[0], generateRecords()[0]}, nil
+					},
+				},
+				folderID: folderID,
+			},
+			args: args{
+				ctx: context.Background(),
+				ref: v1beta1.ExternalSecretDataRemoteRef{
+					Key: record0,
+				},
+			},
+			wantErr: true,
+		},
+		{
 			name: "Get non existing secret",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
@@ -511,8 +534,8 @@ func TestClientPushSecret(t *testing.T) {
 			name: "Push new valid secret",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
-						return nil, errors.New("NotFound")
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return generateRecords()[0:0], nil
 					},
 					CreateSecretWithRecordDataFn: func(recUID, folderUid string, recordData *ksm.RecordCreate) (string, error) {
 						return "record5", nil
@@ -533,8 +556,8 @@ func TestClientPushSecret(t *testing.T) {
 			name: "Push existing valid secret",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
-						return generateRecords()[0], nil
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return generateRecords()[0:1], nil
 					},
 					SaveFn: func(record *ksm.Record) error {
 						return nil
@@ -552,14 +575,11 @@ func TestClientPushSecret(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Push existing invalid secret",
+			name: "Unable to push new valid secret with multiple matches by Name",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
-						return generateRecords()[1], nil
-					},
-					SaveFn: func(record *ksm.Record) error {
-						return nil
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return []*ksm.Record{generateRecords()[0], generateRecords()[0]}, nil
 					},
 				},
 				folderID: folderID,
@@ -569,7 +589,7 @@ func TestClientPushSecret(t *testing.T) {
 					SecretKey: secretKey,
 					RemoteKey: validExistingRecord,
 				},
-				value: []byte("foo2"),
+				value: []byte("foo"),
 			},
 			wantErr: true,
 		},
@@ -577,7 +597,7 @@ func TestClientPushSecret(t *testing.T) {
 			name: "Unable to push new valid secret",
 			fields: fields{
 				ksmClient: &fake.MockKeeperClient{
-					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
 						return nil, errors.New("NotFound")
 					},
 					CreateSecretWithRecordDataFn: func(recUID, folderUID string, recordData *ksm.RecordCreate) (string, error) {
@@ -602,6 +622,9 @@ func TestClientPushSecret(t *testing.T) {
 					GetSecretByTitleFn: func(recordTitle string) (*ksm.Record, error) {
 						return generateRecords()[0], nil
 					},
+					GetSecretsByTitleFn: func(recordTitle string) (records []*ksm.Record, err error) {
+						return generateRecords()[0:1], nil
+					},
 					SaveFn: func(record *ksm.Record) error {
 						return errors.New("Unable to save")
 					},

pkg/provider/keepersecurity/fake/fake.go

@@ -19,6 +19,7 @@ import ksm "github.com/keeper-security/secrets-manager-go/core"
 type MockKeeperClient struct {
 	GetSecretsFn                 func([]string) ([]*ksm.Record, error)
 	GetSecretByTitleFn           func(recordTitle string) (*ksm.Record, error)
+	GetSecretsByTitleFn          func(recordTitle string) (records []*ksm.Record, err error)
 	CreateSecretWithRecordDataFn func(recUID, folderUID string, recordData *ksm.RecordCreate) (string, error)
 	DeleteSecretsFn              func(recrecordUids []string) (map[string]string, error)
 	SaveFn                       func(record *ksm.Record) error
@@ -47,6 +48,10 @@ func (mc *MockKeeperClient) GetSecretByTitle(recordTitle string) (*ksm.Record, e
 	return mc.GetSecretByTitleFn(recordTitle)
 }
 
+func (mc *MockKeeperClient) GetSecretsByTitle(recordTitle string) (records []*ksm.Record, err error) {
+	return mc.GetSecretsByTitleFn(recordTitle)
+}
+
 func (mc *MockKeeperClient) CreateSecretWithRecordData(recUID, folderUID string, recordData *ksm.RecordCreate) (string, error) {
 	return mc.CreateSecretWithRecordDataFn(recUID, folderUID, recordData)
 }