Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete secrets files when retrieval fails #447

Merged
merged 3 commits into from
Mar 16, 2022
Merged

Delete secrets files when retrieval fails #447

merged 3 commits into from
Mar 16, 2022

Conversation

szh
Copy link
Contributor

@szh szh commented Mar 9, 2022

Desired Outcome

Delete secret file if Conjur returns a 403 or 404 status code.

Implemented Changes

When secrets rotation is enabled, if a batch fetch call to Conjur fails during a refresh due to a deleted secret or revoked access, delete the secrets files from the shared volume to avoid leaving unauthorized secrets on the file system. Currently there's no support for specifying which secret(s) failed, so we need to delete all secrets included in the fetch, which includes all secret groups. A future effort will improve this.
This feature can be disabled using an annotation, conjur.org/remove-deleted-secrets-enabled: "false".

Connected Issue/Story

CyberArk internal issue link: ONYX-17938

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

  • Secret files are deleted if a secret doesn't exist or isn't authorized
  • An annotation exists to disable this functionality
  • UT exists
  • Integration tests exist

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: insert issue ID
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@szh szh self-assigned this Mar 9, 2022
@szh szh changed the title Delete file if secret deleted or revoked Delete secrets files when retrieval fails Mar 9, 2022
@szh szh force-pushed the delete-file-if-revoked branch from b519ffe to fc3dc3d Compare March 10, 2022 14:04
@szh szh force-pushed the delete-file-if-revoked branch 10 times, most recently from 75cf39e to 436b1ac Compare March 11, 2022 15:30
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good! Thanks for all of the gratuitous, style-related cleanup, and for adding in the Namespace resource dumps on E2E test failure.

@@ -5,6 +5,9 @@ set -xeuo pipefail

# Clean up when script completes and fails
finish() {
dump_conjur_namespace_upon_error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this in!!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing it because I was getting "kubectl: command not found" errors... Not sure what that's about but I don't want it to hold this up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, the 'kubectl' commands would need to be run via a test client container.

pkg/secrets/config/config.go Outdated Show resolved Hide resolved
@szh szh force-pushed the delete-file-if-revoked branch 13 times, most recently from a1f59e3 to 2a890a2 Compare March 14, 2022 17:54
@szh szh force-pushed the delete-file-if-revoked branch 3 times, most recently from 578f0cd to 3a0e82d Compare March 14, 2022 19:53
@szh szh marked this pull request as ready for review March 14, 2022 20:23
@szh szh requested a review from a team as a code owner March 14, 2022 20:23
@szh szh force-pushed the delete-file-if-revoked branch from 3a0e82d to 231ac94 Compare March 14, 2022 20:42
@szh szh requested a review from diverdane March 14, 2022 20:45
@codeclimate
Copy link

codeclimate bot commented Mar 14, 2022

Code Climate has analyzed commit 231ac94 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 93.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 90.0% (-3.7% change).

View more on Code Climate.

@szh szh mentioned this pull request Mar 16, 2022
17 tasks
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants