Feature/additional secrets improvements #80
Conversation
-Add code to redact messages that may contain additionally redacted secrets based on the `racf_key`. -Update unit testing to account for this Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-Update redaction logic for request and result xml data.
-Check for Value () or Value ('') (the single quotes are for full strings that can contain "delimiters" like () otherwise this would mess with IKJPARSE. This works well for our logic too) patterns and use non-greedy regular expressions to replace the values specified in the patterns
-This passes all existing unit tests. New unit tests will be built for possible commands not seen (would have failed on previous logic)
Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Added a unit test to verify installation data can be redacted when contains characters like () (on requests) Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
| rf"<message>REDACTED MESSAGE CONCERNING {racf_key.upper()}, " | ||
| + r"REVIEW DOCUMENTATION OF \1 FOR MORE INFORMATION</message>", | ||
| security_result, | ||
| ) |
There was a problem hiding this comment.
Also explain how these regexes work.
There was a problem hiding this comment.
Added the following:
This function employs the following regular expression:
<message>([A-Z]*[0-9]*[A-Z]) [^<>]*{racf_key.upper()}[^<>]*<\/message>" -
Designed to match the above pattern by starting and ending with the message xml tag
string as shown, the value of the message is targeted based on the racf_key. This
should capture only messages that contain information about a redacted key.
The message identifier is "captured" as a variable and preserved by the substitution
operation through the use of the \1 supplied in the replacement string.
There was a problem hiding this comment.
Just a slight working change suggestion:
Designed to match any message xml element that contains the provided racf_key.
Instead of
Designed to match the above pattern by starting and ending with the message xml tag string as shown, the value of the message is targeted based on the racf_key.
| "base:installation_data" | ||
| ], | ||
| result, | ||
| ) |
There was a problem hiding this comment.
I wonder if there should be an extract test too for the complex installation data?
There was a problem hiding this comment.
I actually am working on stabilizing this more fully in my RAS branch. There is a new test but it required new extract logic to actually work.
There was a problem hiding this comment.
I would still not consider INSTALLATION_DATA "Stable" at the end of this branch, but once this is approved, I would consider additional_secrets_redaction "stable".
There was a problem hiding this comment.
Just to confirm, redaction for profile extract is not within the scope of this pull request?
-Move Secrets Redaction Unit Testing To Common -Generalize Secrets Redaction Unit testing to other applicable function groups -Add mapping in logger functions for RACF Keys that do not match values in the trait dictionary -Add mapping in logger functions for RACF Message keys that do not match the value in the RACF key Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-Unit Test for Redacting traits that map based on logger dictionary (Audit rules) -Unit test for traits that have different message strings (UACC) Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-Remove Experimental Tag for logging of additional secret traits -Update lots of docstrings Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
-Add back experimental tag (likely true at release) -Trim down Regex comments -Rearchitect racf_key_map and racf_message_key_map to belong to individual administration objects -Move Unit test samples used ONLY for additional secrets testing to Common Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
| ) | ||
|
|
||
| @patch("pyracf.common.irrsmo00.IRRSMO00.call_racf") | ||
| def test_resource_admin_custom_mapped_secret_redacted_on_success( |
There was a problem hiding this comment.
Does it make sense to test an error scenario for audit rules?
There was a problem hiding this comment.
Again, the redaction of secrets is functionally no different on success cases or error cases with the potential exception of messages concerning the field. If you would like me to add the tests as well, I can, but they don't take us through any additional code paths.
There was a problem hiding this comment.
If you get error messages with audit rules when there is a failure, it might make sense to add a test case for that. I guess I will leave that up to you though since none of the traits are intended to be fully stabilized in this release.
-Moved Debug logging testing of secret traits to common -Re-architected racf_key_map and racf_message_key_map under 1 dictionary -changed comments -established SecretRedactionError Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Change Secrets Redaction Error message for invalid admin function Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
|
Dealt with many of the comments here, but there is still more to do.
I will deal with these next week, but the rest is ready for review. |
pyracf/common/utilities/logger.py
Outdated
| matching the TRAIT name, a variable (potentially zero) amount of spaces, then the | ||
| ( subtrait1(value) subtrait2(value)) portion which must start and end with ( and ), | ||
| but must also contain a nested set of open and close parentheses rather than a direct | ||
| seqence of them. The pattern looks for these nested open parenthesis as a sequence would |
There was a problem hiding this comment.
The wording of the last couple sentences here is still a little confusing to me.
There was a problem hiding this comment.
I changed to the following, please let me know if this is still unclear:
Regex 2 ("nested") - {trait_key.upper()}( +){{0,}}\([^)]*\(.*\)( +){{0,}}\)
This is designed to match the pattern TRAIT( subtrait1(value) subtrait2(value)) by
matching the TRAIT name, a variable (potentially zero) amount of spaces, then the
( subtrait1(value) subtrait2(value)) portion which must start and end with ( and ),
but must also contain a'(' before the ')'. This indicates that there is a "nested"
structure rather than a sequential one. In the example, this portion of the pattern
matches '( subtrait1(', but would not match '(value) subtrait2(' because of the ')'
character between the two '(' characters. The pattern looks for these nested open
parenthesis as a sequence would have a ')' character between them. Then the expression
allows any non-newline characters to handle all possible trait values and subtrait names
followed by the "end pattern" of ')' and ')' separated by a variable (potentially zero)
whitespace.
There was a problem hiding this comment.
I think this is ok now. There is just a typo that I think needs to be fixed:
change
a'(' before the ')'.
to
a '(' before the ')'.
-Change error type to ValueError -Address typos and comment fixes -Ensure AssertNotIn tests use proper strings Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Add Unit Testing for ValueError in Secrets Redaction Tweak ValueError Text Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
Add debug logging redaction unit testing for other function groups Fix some underlying bugs/inconsistencies in other unit testing. Signed-off-by: Elijah Swift <elijah.swift@ibm.com>
| for secret in additional_secret_traits: | ||
| if secret in self.__secret_traits: | ||
| continue | ||
| if ":" not in secret: | ||
| bad_secret_traits.append( | ||
| f"\nCould not map {secret} to a valid segment trait." |
There was a problem hiding this comment.
I think it is sufficient to say "\n'{secret}' is not a valid trait."
|
|
||
| # ============================================================================ | ||
| # Secrets Redaction | ||
| # ============================================================================ | ||
| def __add_additional_secret_traits(self, additional_secret_traits: list) -> None: | ||
| """Add additional fields to be redacted in logger output.""" | ||
| unsupported_profile_types = ["permission", "groupConnection", "systemSettings"] | ||
| error_message = ( | ||
| f"Cannot add specified additional secrets to '{self._profile_type}' object." |
There was a problem hiding this comment.
You should be able to use inspect.stack() here to get the name of the "admin" class (i.e., UserAdmin):
pyracf/pyracf/common/utilities/logger.py
Line 116 in dd23a06
Also change the word object to type and change Cannot to Unable.
You should get an exception message like this when you are done:
Unable to add specified additional secrets to 'UserAdmin' type.Make similar corresponding changes to the unsupported profile type exception message.
| else: | ||
| match = re.search(rf"{racf_key.upper()} +\(", security_result) | ||
| match = re.search(rf"{racf_key.upper()}( +){{0,}}\(", security_result) |
There was a problem hiding this comment.
match = re.search(racf_command_argument_regex, security_result)
| security_result = self.__redact_string( | ||
| security_result, match.end(), end_pattern | ||
| security_result = self.__redact_string(security_result, racf_key) | ||
| if racf_key in racf_trait_and_message_key_map.get("message_map", {}): |
There was a problem hiding this comment.
Comments to indicate which stage of processing we are at? Looks like there are two steps. Redact the "RACF command" and redact any corresponding "RACF messages"
| @@ -35,7 +35,7 @@ def test_alter_group_request_debug_log_works_on_success( | |||
| stdout = io.StringIO() | |||
| with contextlib.redirect_stdout(stdout): | |||
| self.group_admin.alter( | |||
| "TESTGRP0", traits=TestGroupConstants.TEST_ALTER_GROUP_REQUEST_TRAITS | |||
| "testgrp0", traits=TestGroupConstants.TEST_ALTER_GROUP_REQUEST_TRAITS | |||
There was a problem hiding this comment.
What is the nature of these changes?
💡 Issue Reference
Issue: #68
💻 What does this address?
Additional Secrets Redaction was marked experimental due to 2 key weaknesses. 1) not redacting secret material in messages in the security response object and 2) the method of secret redaction opened the possibility of incomplete redaction of "additional" secrets.
📟 Implementation Details
Additional Secrets redaction now uses more streamlined regex pattern matching to exhaustively redact secrets in the command images. Additional features were added to redact messages by their RACF tag to ensure that no "false positives" would give away redacted fields.
📋 Is there a test case?
Additional test cases were added/existing test cases were changed to test the new behavior of redacting more complex and varied fields as well as messages.