Add integration & UT for file deletion

Jenkinsfile

@@ -113,7 +113,7 @@ pipeline {
             script {
               def tasks = [:]
               tasks["Kubernetes GKE, DAP"] = {
-                sh "./bin/start --docker --dap --gke"
+                sh "./bin/start --docker --dap --gke --test-prefix=TEST_ID_28"
               }
               parallel tasks
             }

ROTATION.md

@@ -51,6 +51,11 @@ how Push to File works.
    For example:
    If the duration is set to two seconds, but retrieving the secrets takes three second then the
    secrets will be updated every three seconds.
+
+   Note:
+   If one or more of the secrets have been removed from Conjur or have had access revoked, the Secrets Provider
+   will remove the secrets files from the shared volume. To disable this feature, set the
+   `conjur.org/remove-deleted-secrets-enabled` annotation to `false`.
 4. The Secrets Provider renders secret files for each secret group, and
    writes the resulting files to a volume that is shared with your application
    container. The Secrets Provider will rewrite the secret files if there are any changes.
@@ -110,6 +115,7 @@ below is a list of annotations that are needed for secrets rotation.
 | `conjur.org/container-mode`         | Allowed values: <ul><li>`init`</li><li>`application`</li><li>`sidecar`</li></ul>Defaults to `init`.<br>Must be set (or default) to `init` or `sidecar`for Push to File mode.|
 | `conjur.org/secrets-refresh-enabled`  | Set to `true` to enable Secrets Rotation. Defaults to `false` unless `conjur.org/secrets-rotation-interval` is explicitly set. Secrets Provider will exit with error if this is set to `false` and `conjur.org/secrets-rotation-interval` is set. |
 | `conjur.org/secrets-refresh-interval` | Set to a valid duration string as defined [here](https://pkg.go.dev/time#ParseDuration). Setting a time implicitly enables refresh. Valid time units are `s`, `m`, and `h` (for seconds, minutes, and hours, respectively). Some examples of valid duration strings:<ul><li>`5m`</li><li>`2h30m`</li><li>`48h`</li></ul>The minimum refresh interval is 1 second. A refresh interval of 0 seconds is treated as a fatal configuration error. The default refresh interval is 5 minutes. The maximum refresh interval is approximately 290 years. |
+| `conjur.org/remove-deleted-secrets-enabled` | Set to `false` to disable deletion of secrets files from the shared volume when a secret is removed or access is revoked in Conjur. Defaults to `true`. |
 
 ## Troubleshooting
 
@@ -128,9 +134,8 @@ To enable the debug logs, See [enable-logs](PUSH_TO_FILE.md#enable-logs)
 This feature is a **Community** level project that is still under development.
 There could be changes to the documentation so please check back often for updates and additions.
 Future enhancements to this feature will include:
+
 - Support for secrets rotation of Kubernetes secrets
 - atomic writes for multiple Conjur secret values
-- reporting of multiple errored secret variables for bulk Conjur secret retrieval
-- secret file deletion upon secret deleted / access being revoked
+- reporting of multiple errored secret variables for bulk Conjur secret retrieval and selective deletion of secret values from files
 - atomic write of secret files
-

deploy/policy/templates/conjur-delete-secret.template.sh.yml

@@ -0,0 +1,11 @@
+#!/bin/bash
+
+set -euo pipefail
+cat << EOL
+# Should be loaded into root
+- !policy
+  id: secrets
+  body:
+    - !delete
+      record: !variable test_secret
+EOL

deploy/teardown_resources.sh

@@ -25,8 +25,6 @@ fi
 
 set_namespace $CONJUR_NAMESPACE_NAME
 
-$cli_with_timeout "exec $(get_conjur_cli_pod_name) -- conjur variable values add secrets/test_secret \"supersecret\""
-
 set_namespace $APP_NAMESPACE_NAME
 
 $cli_with_timeout "delete secret dockerpullsecret --ignore-not-found=true"

deploy/test/test_cases/TEST_ID_28_secrets_rotation.sh

@@ -1,6 +1,27 @@
 #!/bin/bash
 set -euxo pipefail
 
+delete_test_secret() {
+  set_namespace "$CONJUR_NAMESPACE_NAME"
+  configure_cli_pod
+
+  pushd "../../policy"
+    mkdir -p ./generated
+    ./templates/conjur-delete-secret.template.sh.yml > ./generated/$APP_NAMESPACE_NAME.conjur-delete-secret.yml
+  popd
+
+  conjur_cli_pod=$(get_conjur_cli_pod_name)
+  $cli_with_timeout "exec $conjur_cli_pod -- rm -rf /policy"
+  $cli_with_timeout "cp ../../policy $conjur_cli_pod:/policy"
+
+  $cli_with_timeout "exec $(get_conjur_cli_pod_name) -- \
+    conjur policy load --delete root \"/policy/generated/$APP_NAMESPACE_NAME.conjur-delete-secret.yml\""
+
+  $cli_with_timeout "exec $conjur_cli_pod -- rm -rf ./policy"
+
+  set_namespace $APP_NAMESPACE_NAME
+}
+
 echo "Deploying Secrets rotation tests"
 set_conjur_secret secrets/test_secret secret1
 
@@ -75,3 +96,23 @@ if "$test_failed"; then
     exit 1
 fi
 
+# Delete a secret from conjur
+delete_test_secret
+
+# Check if the value is deleted from secrets provider
+sleep 10
+
+test_failed=false
+for f in $FILES; do
+    echo "Checking if file $f exists"
+    if [[ "$($cli_with_timeout exec "$pod_name" -c test-app -- bash -c "\"if [[ -f /opt/secrets/conjur/"$f" ]] ; then echo true; else echo false; fi\"")" == "true" ]] ; then
+        echo "Secrets Rotation file deletion FAILED for file $f"
+        echo "Expected file to be deleted."
+        test_failed=true
+    else
+        echo "Secrets Rotation file deletion PASSED for $f!"
+    fi
+done
+if "$test_failed"; then
+    exit 1
+fi

deploy/test/test_in_docker.sh

@@ -5,6 +5,9 @@ set -xeuo pipefail
 
 # Clean up when script completes and fails
 finish() {
+  dump_conjur_namespace_upon_error
+  dump_application_namespace_upon_error
+
   announce 'Wrapping up and removing test environment'
 
   # Stop the running processes

deploy/utils.sh

@@ -600,3 +600,45 @@ deploy_push_to_file() {
     $cli_with_timeout "get pods --namespace=$APP_NAMESPACE_NAME --selector app=$deployment_name --no-headers | wc -l | grep $expected_num_replicas"
   fi
 }
+
+function dump_kubernetes_resources() {
+  namespace="$1"
+  echo "Status of pods in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" pods
+  echo "Display pods in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" pods -o yaml
+  echo "Describe pods in namespace $namespace:"
+  "$cli_without_timeout" describe -n "$namespace" pods
+  echo "Services:in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" svc
+  echo "ServiceAccounts:in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" serviceaccounts
+  echo "Deployments in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" deployments
+  if [[ "$PLATFORM" == "openshift" ]]; then
+    echo "DeploymentConfigs in namespace $namespace:"
+    "$cli_without_timeout" get -n "$namespace" deploymentconfigs
+  fi
+  echo "Roles in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" roles
+  echo "RoleBindings in namespace $namespace:"
+  "$cli_without_timeout" get -n "$namespace" rolebindings
+  echo "ClusterRoles in the cluster:"
+  "$cli_without_timeout" get clusterroles
+  echo "ClusterRoleBindings in the cluster:"
+  "$cli_without_timeout" get clusterrolebindings
+}
+
+function dump_conjur_namespace_upon_error {
+  if [ $? -ne 0 ]; then
+    announce "Test FAILED!!!! Displaying resources in Conjur Namespace"
+    dump_kubernetes_resources "$CONJUR_NAMESPACE_NAME"
+  fi
+}
+
+function dump_application_namespace_upon_error {
+  if [ $? -ne 0 ]; then
+    announce "Test FAILED!!!! Displaying resources in application Namespace"
+    dump_kubernetes_resources "$APP_NAMESPACE_NAME"
+  fi
+}

pkg/secrets/config/config.go

@@ -284,7 +284,6 @@ func NewConfig(settings map[string]string) *Config {
 	// ignore errors here, if the interval string is null, zero is returned
 	refreshInterval, _ := time.ParseDuration(refreshIntervalStr)
 
-	// TODO: Should we only enable this when rotation is enabled?
 	sanitizeEnableStr := settings[RemoveDeletedSecretsKey]
 	sanitizeEnable := parseBoolFromStringOrDefault(sanitizeEnableStr, DefaultSanitizeEnabled)
 

pkg/secrets/config/config_test.go

@@ -94,6 +94,13 @@ var validateAnnotationsTestCases = []validateAnnotationsTestCase{
 		},
 		assert: assertErrorInList(fmt.Errorf(messages.CSPFK042E, retryCountLimitKey, "seven", "Integer")),
 	},
+	{
+		description: "when an annotation expects a bool but is given a non-bool value, an error is returned",
+		annotations: map[string]string{
+			RemoveDeletedSecretsKey: "10",
+		},
+		assert: assertErrorInList(fmt.Errorf(messages.CSPFK042E, RemoveDeletedSecretsKey, "10", "Boolean")),
+	},
 	{
 		description: "when an annotation expects a boolean but is given a non-boolean value, an error is returned",
 		annotations: map[string]string{
@@ -195,6 +202,7 @@ var validateSecretsProviderSettingsTestCases = []validateSecretsProviderSettings
 			retryCountLimitKey:                    "10",
 			retryIntervalSecKey:                   "20",
 			k8sSecretsKey:                         "- secret-1\n- secret-2\n- secret-3\n",
+			RemoveDeletedSecretsKey:               "true",
 			"conjur.org/container-mode":           "sidecar",
 			"conjur.org/secrets-refresh-interval": "5m",
 			"conjur.org/secrets-refresh-enabled":  "true",
@@ -412,19 +420,20 @@ var newConfigTestCases = []newConfigTestCase{
 	{
 		description: "a valid map of annotation-based Secrets Provider settings returns a valid Config",
 		settings: map[string]string{
-			"MY_POD_NAMESPACE":    "test-namespace",
-			SecretsDestinationKey: "k8s_secrets",
-			k8sSecretsKey:         "- secret-1\n- secret-2\n- secret-3\n",
-			retryCountLimitKey:    "10",
-			retryIntervalSecKey:   "20",
+			"MY_POD_NAMESPACE":      "test-namespace",
+			SecretsDestinationKey:   "k8s_secrets",
+			k8sSecretsKey:           "- secret-1\n- secret-2\n- secret-3\n",
+			retryCountLimitKey:      "10",
+			retryIntervalSecKey:     "20",
+			RemoveDeletedSecretsKey: "false",
 		},
 		assert: assertGoodConfig(&Config{
 			PodNamespace:       "test-namespace",
 			StoreType:          "k8s_secrets",
 			RequiredK8sSecrets: []string{"secret-1", "secret-2", "secret-3"},
 			RetryCountLimit:    10,
 			RetryIntervalSec:   20,
-			SanitizeEnabled:    DefaultSanitizeEnabled,
+			SanitizeEnabled:    false,
 		}),
 	},
 	{

pkg/secrets/pushtofile/provide_conjur_secrets.go

@@ -3,6 +3,7 @@ package pushtofile
 import (
 	"context"
 	"os"
+	"strings"
 
 	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
 	"github.com/cyberark/conjur-opentelemetry-tracer/pkg/trace"
@@ -76,9 +77,8 @@ func provideWithDeps(
 	spanCtx, span := tr.Start(traceContext, "Fetch Conjur Secrets")
 	secretsByGroup, err := FetchSecretsForGroups(depFuncs.retrieveSecretsFunc, groups, spanCtx)
 	if err != nil {
-		if sanitizeEnabled {
-			// Delete secret files for variables that no longer exist or the user no longer has permissions to
-			// TODO: Should we check the error message to see if it's a 404 or 403?
+		// Delete secret files for variables that no longer exist or the user no longer has permissions to
+		if (strings.Contains(err.Error(), "403") || strings.Contains(err.Error(), "404")) && sanitizeEnabled {
 			for _, group := range groups {
 				os.Remove(group.FilePath)
 				log.Info(messages.CSPFK019I)