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.

Sign up for GitHub

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

Feature/additional secrets improvements #80

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from
Open

Feature/additional secrets improvements #80

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5843f1d
add message redaction
ElijahSwiftIBM Feb 14, 2024
945bf92
Clean up message redaction string
ElijahSwiftIBM Feb 14, 2024
9d1e0cb
Update logger.py
ElijahSwiftIBM Feb 14, 2024
1ea6dd4
Added a unit test to verify new functions work
ElijahSwiftIBM Feb 14, 2024
354e407
Update pyproject.toml
ElijahSwiftIBM Feb 20, 2024
e796e09
Move Secrets Redaction Unit Testing To Common
ElijahSwiftIBM Feb 21, 2024
f25153c
Add Unit Testing for Additional Secrets Redaction
ElijahSwiftIBM Feb 21, 2024
d96978f
Remove Experimental Tag
ElijahSwiftIBM Feb 21, 2024
ff53162
Reorginazation
ElijahSwiftIBM Feb 28, 2024
a2e6cef
Addressed PR Comments
ElijahSwiftIBM Mar 1, 2024
6a3be62
Update secrets_redaction_error.py
ElijahSwiftIBM Mar 1, 2024
a61fe8e
Address latest round of PR comments
ElijahSwiftIBM Mar 4, 2024
58e81c1
Add Unit Testing for ValueError
ElijahSwiftIBM Mar 4, 2024
f01f477
Add more unit tests
ElijahSwiftIBM Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

[tool.poetry]
name="pyracf"
version="1.0b5"
version="1.0b6"
description="Python interface to RACF using IRRSMO00 RACF Callable Service."
license = "Apache-2.0"
authors = [
Expand Down
59 changes: 44 additions & 15 deletions pyracf/common/utilities/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,19 +149,33 @@ def __redact_request_dictionary(
def __redact_string(
self,
input_string: Union[str, bytes],
start_ind: int,
end_pattern: Union[str, bytes],
):
key: str,
) -> Union[str, bytes]:
"""
Redacts characters in a string between a starting index and ending pattern.
Redacts characters in a string between a starting index and ending tag.
Replaces the identified characters with '********' regardless of the original length.
"""
asterisks = "********"
is_bytes = False
if isinstance(input_string, bytes):
asterisks = asterisks.encode("cp1047")
pre_keyword = input_string[:start_ind]
post_keyword = end_pattern.join(input_string[start_ind:].split(end_pattern)[1:])
return pre_keyword + asterisks + end_pattern + post_keyword
input_string = input_string.decode("cp1047")
is_bytes = True
quoted = re.search(rf"{key.upper()}( +)\(\'.*?(?<!\')\'\)", input_string)
if quoted is not None:
input_string = re.sub(
rf"{key.upper()}( +)\(\'.*?(?<!\')\'\)",
rf"{key.upper()}\1('{asterisks}')",
input_string,
)
else:
input_string = re.sub(
rf"{key.upper()}( +)\(.*?(?<!\\)\)",
rf"{key.upper()}\1({asterisks})",
input_string,
)
if is_bytes:
return input_string.encode("cp1047")
return input_string

def redact_request_xml(
self,
Expand Down Expand Up @@ -191,7 +205,11 @@ def redact_request_xml(
# <tag operation="del" />
if f"</{xml_key}>" not in xml_string:
continue
xml_string = self.__redact_string(xml_string, match.end(), f"</{xml_key}")
xml_string = re.sub(
rf"{xml_key}(.*)>.*<\/{xml_key}",
rf"{xml_key}\1>********</{xml_key}",
xml_string,
)
if is_bytes:
xml_string = xml_string.encode("utf-8")
return xml_string
Expand All @@ -203,27 +221,38 @@ def redact_result_xml(
) -> str:
"""
Redacts a list of specific secret traits in a result xml string.
Based on the following RACF command pattern:
Based on the following RACF command patterns:
'TRAIT (value)'
"TRAIT ('value')"
This function also accounts for varied amounts of whitespace in the pattern.
"""
if isinstance(security_result, list):
return security_result
for xml_key in secret_traits.values():
racf_key = xml_key.split(":")[1] if ":" in xml_key else xml_key
end_pattern = ")"
if isinstance(security_result, bytes):
match = re.search(
rf"{racf_key.upper()} +\(", security_result.decode("cp1047")
)
end_pattern = end_pattern.encode("cp1047")
else:
match = re.search(rf"{racf_key.upper()} +\(", security_result)
if not match:
continue
security_result = self.__redact_string(
security_result, match.end(), end_pattern
)
security_result = self.__redact_string(security_result, racf_key)
if isinstance(security_result, bytes):
security_result = re.sub(
rf"<message>([A-Z]*[0-9]*[A-Z]) [^<>]*{racf_key.upper()}[^<>]*<\/message>",
rf"<message>REDACTED MESSAGE CONCERNING {racf_key.upper()}, "
+ r"REVIEW DOCUMENTATION OF \1 FOR MORE INFORMATION</message>",
security_result.decode("cp1047"),
).encode("cp1047")
else:
security_result = re.sub(
rf"<message>([A-Z]*[0-9]*[A-Z]) [^<>]*{racf_key.upper()}[^<>]*<\/message>",
rf"<message>REDACTED MESSAGE CONCERNING {racf_key.upper()}, "
+ r"REVIEW DOCUMENTATION OF \1 FOR MORE INFORMATION</message>",
security_result,
)
Copy link
Member

Choose a reason for hiding this comment

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

Also explain how these regexes work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

return security_result

def __colorize_json(self, json_text: str) -> str:
Expand Down
10 changes: 10 additions & 0 deletions tests/user/test_user_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ def get_sample(sample_file: str) -> Union[str, bytes]:
TEST_ALTER_USER_RESULT_ERROR_UID_SECRET_DICTIONARY = get_sample(
"alter_user_result_error_uid_secret.json"
)
TEST_ALTER_USER_RESULT_INST_DATA_SUCCESS_XML = get_sample(
"alter_user_result_success_inst_data.xml"
)
TEST_ALTER_USER_RESULT_SUCCESS_INST_DATA_SECRET_DICTIONARY = get_sample(
"alter_user_result_success_inst_data_secret.json"
)

# Extract User
TEST_EXTRACT_USER_RESULT_BASE_OMVS_SUCCESS_XML = get_sample(
Expand Down Expand Up @@ -218,6 +224,10 @@ def get_sample(sample_file: str) -> Union[str, bytes]:
)
TEST_ALTER_USER_REQUEST_TRAITS_UID_ERROR = dict(TEST_ALTER_USER_REQUEST_TRAITS_EXTENDED)
TEST_ALTER_USER_REQUEST_TRAITS_UID_ERROR["omvs:uid"] = 90000000000
TEST_ALTER_USER_REQUEST_TRAITS_INST_DATA = dict(TEST_ALTER_USER_REQUEST_TRAITS_EXTENDED)
TEST_ALTER_USER_REQUEST_TRAITS_INST_DATA["base:installation_data"] = (
"Test = Value; Other(stuff goes here)'')"
)

# Extract User
TEST_EXTRACT_USER_REQUEST_BASE_OMVS_XML = get_sample(
Expand Down
24 changes: 24 additions & 0 deletions tests/user/test_user_result_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,3 +523,27 @@ def test_user_admin_custom_secret_redacted_on_error(
f"({TestUserConstants.TEST_ALTER_USER_REQUEST_TRAITS_UID_ERROR['omvs:uid']})",
exception.exception.result,
)

def test_user_admin_custom_secret_redacted_when_complex_characters(
self,
call_racf_mock: Mock,
):
user_admin = UserAdmin(additional_secret_traits=["base:installation_data"])
call_racf_mock.side_effect = [
TestUserConstants.TEST_EXTRACT_USER_RESULT_BASE_ONLY_SUCCESS_XML,
TestUserConstants.TEST_ALTER_USER_RESULT_INST_DATA_SUCCESS_XML,
]
result = user_admin.alter(
"squidwrd",
traits=TestUserConstants.TEST_ALTER_USER_REQUEST_TRAITS_INST_DATA,
)
self.assertEqual(
result,
TestUserConstants.TEST_ALTER_USER_RESULT_SUCCESS_INST_DATA_SECRET_DICTIONARY,
)
self.assertNotIn(
TestUserConstants.TEST_ALTER_USER_REQUEST_TRAITS_INST_DATA[
"base:installation_data"
],
result,
)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there should be an extract test too for the complex installation data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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".

Copy link
Member

@lcarcaramo lcarcaramo Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just to confirm, redaction for profile extract is not within the scope of this pull request?

12 changes: 6 additions & 6 deletions tests/user/user_log_samples/alter_user_additional_secret_added_error.log
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@
<returncode>16</returncode>
<reasoncode>8</reasoncode>
<image>ALTUSER SQUIDWRD NOSPECIAL OMVS (HOME ('/u/clarinet') NOPROGRAM UID (********))</image>
<message>IKJ56702I INVALID UID, 90000000000</message>
<message>IKJ56701I MISSING OMVS UID+</message>
<message>IKJ56701I MISSING OMVS USER ID (UID), 1-10 NUMERIC DIGITS</message>
<message>REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56702I FOR MORE INFORMATION</message>
<message>REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION</message>
<message>REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION</message>
</command>
</user>
<returncode>4</returncode>
Expand Down Expand Up @@ -271,9 +271,9 @@
"reasonCode": 8,
"image": "ALTUSER SQUIDWRD NOSPECIAL OMVS (HOME ('/u/clarinet') NOPROGRAM UID (********))",
"messages": [
"IKJ56702I INVALID UID, 90000000000",
"IKJ56701I MISSING OMVS UID+",
"IKJ56701I MISSING OMVS USER ID (UID), 1-10 NUMERIC DIGITS"
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56702I FOR MORE INFORMATION",
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION",
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION"
]
}
]
Expand Down
6 changes: 3 additions & 3 deletions tests/user/user_result_samples/alter_user_result_error_uid_secret.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
"reasonCode": 8,
"image": "ALTUSER SQUIDWRD NOSPECIAL OMVS (HOME ('/u/clarinet') NOPROGRAM UID (********))",
"messages": [
"IKJ56702I INVALID UID, 90000000000",
"IKJ56701I MISSING OMVS UID+",
"IKJ56701I MISSING OMVS USER ID (UID), 1-10 NUMERIC DIGITS"
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56702I FOR MORE INFORMATION",
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION",
"REDACTED MESSAGE CONCERNING UID, REVIEW DOCUMENTATION OF IKJ56701I FOR MORE INFORMATION"
]
}
]
Expand Down
14 changes: 14 additions & 0 deletions tests/user/user_result_samples/alter_user_result_success_inst_data.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version="1.0" encoding="IBM-1047"?>
<securityresult xmlns="http://www.ibm.com/systems/zos/saf/IRRSMO00Result1">
<user name="SQUIDWRD" operation="set" requestid="UserRequest">
<info>Definition exists. Add command skipped due to precheck option</info>
<command>
<safreturncode>0</safreturncode>
<returncode>0</returncode>
<reasoncode>0</reasoncode>
<image>ALTUSER SQUIDWRD NAME ('Squidward') OWNER (leonard) SPECIAL DATA ('Test = Value; Other(stuff goes here)'')') OMVS (UID (2424) HOME ('/u/squidwrd') PROGRAM ('/bin/sh'))</image>
</command>
</user>
<returncode>0</returncode>
<reasoncode>0</reasoncode>
</securityresult>
23 changes: 23 additions & 0 deletions tests/user/user_result_samples/alter_user_result_success_inst_data_secret.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"securityResult": {
"user": {
"name": "SQUIDWRD",
"operation": "set",
"requestId": "UserRequest",
"info": [
"Definition exists. Add command skipped due to precheck option"
],
"commands": [
{
"safReturnCode": 0,
"returnCode": 0,
"reasonCode": 0,
"image": "ALTUSER SQUIDWRD NAME ('Squidward') OWNER (leonard) SPECIAL DATA ('********') OMVS (UID (2424) HOME ('/u/squidwrd') PROGRAM ('/bin/sh'))"
}
]
},
"returnCode": 0,
"reasonCode": 0,
"runningUserid": "testuser"
}
}
Loading