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

Secrets Rotation: only write secret files if changed #432

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Conversation

szh
Copy link
Contributor

@szh szh commented Feb 2, 2022

Desired Outcome

Overview:

For conjur secrets rotation functionality for push-to-file mode, the SP's pushtofile package needs to be able to detect changes in secret file content, and conditionally write secret files only if content has been changed since the last write of the file.

Notes:

  • For security reasons, the previous content of the secret files should not be cached directly. Instead, we should keep a SHA-256 checksum of the file content to compare against.
  • The SHA-256 checksum should not be updated until the secret file has been successfully written (i.e. updated).
  • The processing of the pushToWriter() function can be modified to be:
    1. Parse the template for a secret group
    2. Render the secret file content to a bytes.Buffer (using tpl.Execute())
    3. Caclulate the SHA-256 checksum for the bytes.Buffer data
    4. If checksum differs from prefiously cached checksum, write the file
    5. If writing the file is successful, update the cached checksum with the new checksum

Implemented Changes

  • Modified push to file logic to store the hash of each group's rendered content
  • Before writing to the files, checks if the new content matches the previous hash and only writes if it's different

Connected Issue/Story

CyberArk internal issue link: ONYX-15541

Definition of Done

  • P2F does not perform a file write if the content of the group hasn't changed
  • P2F performs a file write if the rendered content changes, whether due to a secret or template change

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 Feb 2, 2022
@szh szh force-pushed the ONYX-15541 branch 6 times, most recently from 201d2f7 to 8a8aff0 Compare February 2, 2022 21:34
@szh szh changed the title Secrets Rotation: Add logic to detect changes in secret files and only write if changed Secrets Rotation: only write secret files if changed Feb 2, 2022
@szh szh force-pushed the ONYX-15541 branch 3 times, most recently from 4b3b6e8 to 5ada5a9 Compare February 3, 2022 15:40
@szh szh requested review from diverdane and rpothier February 3, 2022 15:41
@szh szh marked this pull request as ready for review February 3, 2022 15:41
@szh szh requested a review from a team as a code owner February 3, 2022 15:41
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.

Looks great, very nice job on the UT!!!

if _, err := writer.Write(fileContent.Bytes()); err != nil {
return err
}
prevFileChecksums[groupName] = checksum
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to log an INFO level message here that an individual file has been written?

Or do we assume that the user will deduce that the secret file has been written if we don't see the No change in secret files, no secret files written for this file, but we do see the DAP/Conjur Secrets pushed to shared volume successfully at the very end of the refresh cycle?

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 don't think it's necessary to add a new log statement for this functionality. Nothing has changed in that scenario and the "Secrets pushed to shared volume" log at the end should be sufficient. I think it's more important to log if they aren't written since this could be a source of problems if, say, the secrets file was deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. The only weird aspect is that the logs will continually show "Secrets pushed to shared volume" when they really aren't (after the first write). But then again, we don't want to make the logs too chatty either.

@codeclimate
Copy link

codeclimate bot commented Feb 4, 2022

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

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

This pull request will bring the total coverage in the repository to 92.5% (0.0% change).

View more on Code Climate.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants