diff --git a/Jenkinsfile b/Jenkinsfile index c14ae1ef9..15157b7eb 100644 --- a/Jenkinsfile +++ b/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 } diff --git a/ROTATION.md b/ROTATION.md index 7ee614481..afdec7053 100644 --- a/ROTATION.md +++ b/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: Defaults to `init`.
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: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 - diff --git a/deploy/policy/templates/conjur-delete-secret.template.sh.yml b/deploy/policy/templates/conjur-delete-secret.template.sh.yml new file mode 100755 index 000000000..37d3f7d73 --- /dev/null +++ b/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 diff --git a/deploy/teardown_resources.sh b/deploy/teardown_resources.sh index 2ef0c62b3..eb6b71d22 100755 --- a/deploy/teardown_resources.sh +++ b/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" diff --git a/deploy/test/test_cases/TEST_ID_28_secrets_rotation.sh b/deploy/test/test_cases/TEST_ID_28_secrets_rotation.sh index 9d854900a..9bdb883fa 100755 --- a/deploy/test/test_cases/TEST_ID_28_secrets_rotation.sh +++ b/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 diff --git a/deploy/test/test_in_docker.sh b/deploy/test/test_in_docker.sh index c3ceab42b..d94a55994 100755 --- a/deploy/test/test_in_docker.sh +++ b/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 diff --git a/deploy/utils.sh b/deploy/utils.sh index a95278743..73b5ef9e3 100644 --- a/deploy/utils.sh +++ b/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 +} diff --git a/pkg/secrets/config/config.go b/pkg/secrets/config/config.go index 36bc174ee..ecc05fe18 100644 --- a/pkg/secrets/config/config.go +++ b/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) diff --git a/pkg/secrets/config/config_test.go b/pkg/secrets/config/config_test.go index 38aa02a01..2647d626e 100644 --- a/pkg/secrets/config/config_test.go +++ b/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,11 +420,12 @@ 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", @@ -424,7 +433,7 @@ var newConfigTestCases = []newConfigTestCase{ RequiredK8sSecrets: []string{"secret-1", "secret-2", "secret-3"}, RetryCountLimit: 10, RetryIntervalSec: 20, - SanitizeEnabled: DefaultSanitizeEnabled, + SanitizeEnabled: false, }), }, { diff --git a/pkg/secrets/pushtofile/provide_conjur_secrets.go b/pkg/secrets/pushtofile/provide_conjur_secrets.go index 3ae043a48..3b4bce460 100644 --- a/pkg/secrets/pushtofile/provide_conjur_secrets.go +++ b/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) diff --git a/pkg/secrets/pushtofile/provide_conjur_secrets_test.go b/pkg/secrets/pushtofile/provide_conjur_secrets_test.go index 5712bc427..2f0555949 100644 --- a/pkg/secrets/pushtofile/provide_conjur_secrets_test.go +++ b/pkg/secrets/pushtofile/provide_conjur_secrets_test.go @@ -3,6 +3,7 @@ package pushtofile import ( "context" "fmt" + "os" "testing" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur" @@ -18,6 +19,31 @@ func retrieve(variableIDs []string, ctx context.Context) (map[string][]byte, err return masterMap, nil } +func retrieveWith403(variableIDs []string, ctx context.Context) (map[string][]byte, error) { + return nil, fmt.Errorf("403") +} + +func retrieveWithGenericError(variableIDs []string, ctx context.Context) (map[string][]byte, error) { + return nil, fmt.Errorf("generic error") +} + +func secretGroups(filePath string) []*SecretGroup { + return []*SecretGroup{ + { + Name: "groupname", + FilePath: filePath, + FileFormat: "yaml", + FilePermissions: 123, + SecretSpecs: []SecretSpec{ + { + Alias: "password", + Path: "path1", + }, + }, + }, + } +} + func TestNewProvider(t *testing.T) { TestCases := []struct { description string @@ -70,29 +96,19 @@ func TestNewProvider(t *testing.T) { func TestProvideWithDeps(t *testing.T) { TestCases := []struct { - description string - provider fileProvider - assert func(*testing.T, fileProvider, error, *ClosableBuffer, pushToWriterSpy, openWriteCloserSpy) + description string + provider fileProvider + createFileName string + sanitizeEnabled bool + assert func(*testing.T, fileProvider, error, *ClosableBuffer, pushToWriterSpy, openWriteCloserSpy) }{ { description: "happy path", provider: fileProvider{ retrieveSecretsFunc: retrieve, - secretGroups: []*SecretGroup{ - { - Name: "groupname", - FilePath: "/path/to/file", - FileFormat: "yaml", - FilePermissions: 123, - SecretSpecs: []SecretSpec{ - { - Alias: "password", - Path: "path1", - }, - }, - }, - }, + secretGroups: secretGroups("/path/to/file"), }, + sanitizeEnabled: true, assert: func( t *testing.T, p fileProvider, @@ -106,6 +122,69 @@ func TestProvideWithDeps(t *testing.T) { assert.Nil(t, err) }, }, + { + description: "403 error", + createFileName: "path_to_file.yaml", + provider: fileProvider{ + retrieveSecretsFunc: retrieveWith403, + secretGroups: secretGroups("path_to_file.yaml"), + }, + sanitizeEnabled: true, + assert: func( + t *testing.T, + p fileProvider, + err error, + closableBuf *ClosableBuffer, + spyPushToWriter pushToWriterSpy, + spyOpenWriteCloser openWriteCloserSpy, + ) { + assert.Error(t, err) + // File should be deleted because of 403 error + assert.NoFileExists(t, "path_to_file.yaml") + }, + }, + { + description: "generic error", + createFileName: "path_to_file.yaml", + provider: fileProvider{ + retrieveSecretsFunc: retrieveWithGenericError, + secretGroups: secretGroups("path_to_file.yaml"), + }, + sanitizeEnabled: true, + assert: func( + t *testing.T, + p fileProvider, + err error, + closableBuf *ClosableBuffer, + spyPushToWriter pushToWriterSpy, + spyOpenWriteCloser openWriteCloserSpy, + ) { + assert.Error(t, err) + // File should not be deleted because of generic error + assert.FileExists(t, "path_to_file.yaml") + }, + }, + { + description: "403 error with sanitize disabled", + createFileName: "path_to_file.yaml", + provider: fileProvider{ + retrieveSecretsFunc: retrieveWith403, + secretGroups: secretGroups("path_to_file.yaml"), + }, + sanitizeEnabled: false, + assert: func( + t *testing.T, + p fileProvider, + err error, + closableBuf *ClosableBuffer, + spyPushToWriter pushToWriterSpy, + spyOpenWriteCloser openWriteCloserSpy, + ) { + assert.Error(t, err) + // File shouldn't be deleted because sanitize is disabled + assert.FileExists(t, "path_to_file.yaml") + }, + }, } for _, tc := range TestCases { @@ -117,10 +196,16 @@ func TestProvideWithDeps(t *testing.T) { writeCloser: closableBuf, } + if tc.createFileName != "" { + _, err := os.Create(tc.createFileName) + assert.NoError(t, err) + defer os.Remove(tc.createFileName) + } + err := provideWithDeps( context.Background(), tc.provider.secretGroups, - true, + tc.sanitizeEnabled, fileProviderDepFuncs{ retrieveSecretsFunc: tc.provider.retrieveSecretsFunc, depOpenWriteCloser: spyOpenWriteCloser.Call,