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 redaction #9

Merged
merged 18 commits into from
Aug 30, 2023
Merged

Secrets redaction #9

merged 18 commits into from
Aug 30, 2023

Conversation

ElijahSwiftIBM
Copy link
Collaborator

💡 Issue Reference

Issue: #6

💻 What does this address?

Leverages existing IRRSMO00 functionality to better redact passwords, password phrases and other fields. Also better cleans up references to secrets in the python code, and allows for adding to the list of redacted secrets (password and password phrase). Also adds UserAdmin.set_passphrase function.

📟 Implementation Details

Changed options to always include option 8 in IRRSMO00. Drop references to parameter lists and strings containing input information when no longer used. Redact using regex rather than character replacement.

📋 Is there a test case?

Added a test case and made adjustments to existing test case suite for password and password phrase redaction

Either include the path to the test case file, or details on a manual test

Change all calls to SMO to include option 8 for secrets redaction.
Drop methods that contain potentially sensitive information as soon as they are no longer necessary.

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Check to ensure new redaction eliminates password and password phrase information within the result

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-changes the way secrets are redacted on input
-no longer stores copy of secrets for replacement

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-customers can add new secrets to redact
-cannot overwrite existing secrets for security

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-add set_passphrase settr to UserAdmin
-add unit testing for this feature

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-add functionality to redact secrets beyond default
-cannot change defaults as this is security concern, once something is added, a new instance would be necessary
- unit test for this must be run after all other user tests as it redacts UID's and this cannot be reversed for the life of the SecurityAdmin object

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
pyracf/common/logger.py Outdated Show resolved Hide resolved
pyracf/common/logger.py Outdated Show resolved Hide resolved
@lcarcaramo
Copy link
Member

One other thing to note is that now that IRRSMO00 handles redacting sensitive data from results, the password and passphrase redaction tests are actually now redundant. We only really need to verify that all secrets in debug logging are redacted and that request xml returned when using generate_requests_only is redacted. One thing that might be good to validate though is the clear state functionality if possible. Either way, I think we can get rid of the password and passphrase redaction tests in test_user_result_parser.py along with corresponding constants and xml samples since they don't really test anything anymore. Again, also note that anything that redacts results in logger.py is also now unnecessary since irrsmo00 handles redacting results for us as I have noted in feedback I provided earlier.

@ElijahSwiftIBM
Copy link
Collaborator Author

One other thing to note is that now that IRRSMO00 handles redacting sensitive data from results, the password and passphrase redaction tests are actually now redundant. We only really need to verify that all secrets in debug logging are redacted and that request xml returned when using generate_requests_only is redacted. One thing that might be good to validate though is the clear state functionality if possible. Either way, I think we can get rid of the password and passphrase redaction tests in test_user_result_parser.py along with corresponding constants and xml samples since they don't really test anything anymore. Again, also note that anything that redacts results in logger.py is also now unnecessary since irrsmo00 handles redacting results for us as I have noted in feedback I provided earlier.

I would like to keep the redaction tests as they serve as validation of the redaction of the result xml. While we now rely on IRRSMO00's redaction somewhat, ours is a bit more comprehensive, and testing us allows to ensure that this functionality persists with future releases.

-minor typo fixes and name changes
-convert options parameter to precheck keyword.

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-Fix docstrings
-Clean up positional/keyword arguments
-Clean unit testing and code bits

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
pyracf/common/logger.py Outdated Show resolved Hide resolved
Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Re-introduce assertNotIn and assertIn unit testing
Add simplified unit testing for "simple" password redaction
Fix docstrings and minor errors

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
pyracf/common/logger.py Outdated Show resolved Hide resolved
@lcarcaramo lcarcaramo linked an issue Aug 30, 2023 that may be closed by this pull request
@lcarcaramo lcarcaramo added this to the Alpha 1.0a2 milestone Aug 30, 2023
@lcarcaramo lcarcaramo added the documentation Improvements or additions to documentation label Aug 30, 2023
-clean type hints
-added assertnotin for additional secrets unit testing

Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
@lcarcaramo lcarcaramo added enhancement New feature or request and removed enhancement New feature or request labels Aug 30, 2023
@lcarcaramo lcarcaramo merged commit 7bdd9d8 into dev Aug 30, 2023
1 check passed
@lcarcaramo lcarcaramo deleted the secrets-redaction branch August 30, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Improved Secrets redaction
2 participants