AWS SM PushSecret creates new version w/ every refresh cycle with secretPushFormat "string"

[gtfortytwo]

Describe the bug
When using PushSecret with the AWS Secrets Manager provider and the secretPushFormat is set to string, ESO calls PutSecretValueWithContext to create a new version of the secret on every refresh cycle, even when the desired value has not changed since the previous refresh. This eventually results in hitting AWS API limits (on version update frequency) and creates numerous extraneous revisions of the AWS secret. This appears to have been introduced in v0.9.14 with #3189 which initially enabled the "string" value format.

Steps to reproduce the behavior:

1. Create a PushSecret resource against a SecretStore using the AWS Secrets Manager provider and set spec.data[].metadata.secretPushFormat to string. To make the failure more evident, use a short spec.refreshInterval like 10s
2. The incorrect behavior can be observed in several ways:

• Watch the "Versions" tab of the secret in the AWS console and note the version IDs continue to change for the AWSCURRENT and AWSPREVIOUS labels.
• Get the secret values with the AWS CLI (aws secretsmanager list-secret-version-ids)
• Watch CloudTrail logs for repeated calls to PutSecretValueWithContext for the same secret resource.
• Watch externalsecret_provider_api_calls_count ESO metrics for PutSecretValueWithContext

Expected behavior
ESO should only create new AWS secret versions when the desired secret value is different from the value observed in the destination secret.

Additional context
It appears that the logic is intended to behave as described in "expected behavior". In line 572 of secretsmanager.go, having read the remote value from the provider, the code checks that the remote secret exists and compares its observed value to the desired value. If those values are the same, it exits the reconcile loop taking no further action:

	if awsSecret != nil && bytes.Equal(awsSecret.SecretBinary, value) {
		return nil
	}

Unfortunately, it only compares the SecretBinary field. If the user elected to use secretPushFormat: string, then the API docs indicate SecretBinary will have no value because the value was set using SecretString as in line 590:

	if secretPushFormat == SecretPushFormatString {
		input.SetSecretBinary(nil).SetSecretString(string(value))
	}

Presumably, the fix would be fairly straightforward: if the user elected secretPushFormat: string (like it checks in line 589), then instead of comparing value to SecretBinary, we'd want to compare string(value) to SecretString (with appropriate safety checks). I wish I felt confident enough in my golang skills to submit a PR for this, buuuut I don't. :/

Sub-optimal workarounds
A few less-than-ideal workarounds are available while awaiting this fix:

• Use binary format. Users can use binary format for the secret, if the consumer of the secret can tolerate that and do any necessary conversion. It also makes it impossible to view the secret value with the AWS console.
• Set long refreshInterval to avoid excessive writes. This can help avoid hitting API limits, but it's no guarantee.

[gusfcarvalho]

this issue indeed seems to be simple (an if clause) when checking for data validity. It does seems that right now we're comparing binary versions always - leading to error when secretString is used (as the binary is empty)

[vsantos]

Just a heads-up, this issue is also affecting ParameterStore's PushSecret.

[Skarlso]

Ah bummer. Thanks for the heads up.

[Skarlso]

@vsantos Do you have an example please?

[Skarlso]

@vsantos Also, what version are you running?

[Skarlso]

Okay, I think I was able to reproduce it with a unit test.

	require.NoError(t, ps.PushSecret(context.TODO(), fakeSecret, psd))

	// call twice, secret push should only be called once.
	require.NoError(t, ps.PushSecret(context.TODO(), fakeSecret, psd))

	assert.Equal(t, 1, client.PutParameterWithContextCalledN)

This should have been 1, but actually was 2.

[Skarlso]

Err ^ no. :) Because I haven't configured a second call to the mock client. :D

[Skarlso]

Okay, I have this now #3584 but this is working, so @vsantos gonna need an example please. :)

[vsantos]

Sorry @Skarlso, my initial report was wrong. I believe I mixed up things a little since I indeed have a "new version every refresh cycle" bug but for AWS Parameter Store instead.

What I'm facing was introduced after this #3576 but only for parameters with type SecureString, both String and StringList(property introduced within this same #3576) do not present such an issue.

I was able to reproduce it using your PR #3584 (commit 727c74eb) as well.

Since my report does not relate to this one, I will simply move this thread to a specific issue. Thanks for your support!