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

Bug fixes for Issue 57 #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Bug fixes for Issue 57 #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a17dd22
checking memberToReplace in service_to_replacement
saurabhtalreja Oct 6, 2023
5e5089e
BugFix - static-ip doesn't have nested member object
saurabhtalreja Oct 6, 2023
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
7 changes: 6 additions & 1 deletion src/palo_alto_firewall_analyzer/validators/consolidatable_addresses_and_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,13 @@ def replace_policy_contents(policies_needing_replacement, address_to_replacement
object_policy_dict[translation]['translated-address'], replacements_made = replace_member_contents(object_policy_dict[translation]['translated-address'], address_to_replacement, replacements_made)
if object_policy_dict[translation].get('dynamic-ip-and-port', {}).get('translated-address', {}).get('member'):
object_policy_dict[translation]['dynamic-ip-and-port']['translated-address']['member'], replacements_made = replace_member_contents(object_policy_dict[translation]['dynamic-ip-and-port']['translated-address']['member'], address_to_replacement, replacements_made)
if object_policy_dict[translation].get('static-ip', {}).get('translated-address', {}).get('member'):
# when static-ip --> translated-address doesn't have member object, and IP comes directly as string
if isinstance(object_policy_dict[translation].get('static-ip', {}).get('translated-address', {}),str):
translatedAddress = object_policy_dict[translation]['static-ip']['translated-address']
object_policy_dict[translation]['static-ip']['translated-address']={'member':translatedAddress}
object_policy_dict[translation]['static-ip']['translated-address']['member'], replacements_made = replace_member_contents(object_policy_dict[translation]['static-ip']['translated-address']['member'], address_to_replacement, replacements_made)
elif object_policy_dict[translation].get('static-ip', {}).get('translated-address', {}).get('member'):
object_policy_dict[translation]['static-ip']['translated-address']['member'], replacements_made = replace_member_contents(object_policy_dict[translation]['static-ip']['translated-address']['member'], address_to_replacement, replacements_made)
text = f"Replace the following Address members in {policy_dg}'s {policy_type} {policy_entry.get('name')}: {sorted([k + ' with ' + v for k, v in replacements_made.items()])}"
badentries.append(BadEntry(data=[policy_entry, object_policy_dict], text=text, device_group=policy_dg, entry_type=policy_type))
return badentries
Expand Down
2 changes: 1 addition & 1 deletion src/palo_alto_firewall_analyzer/validators/consolidatable_service_and_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def consolidate_service_like_objects(profilepackage, object_type, object_friendl
replacements_made[member_to_replace] = service_to_replacement[member_to_replace]
object_policy_dict['service'] = service_to_replacement[member_to_replace]
# If it's a policy with only one member, it'll be parsed as a string, not a list
elif isinstance(object_policy_dict['service']['member'], str):
elif isinstance(object_policy_dict['service']['member'], str) and object_policy_dict['service']['member'] in service_to_replacement:
Copy link
Owner

Choose a reason for hiding this comment

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

Re: and object_policy_dict['service']['member'] in service_to_replacement

I'm concerned for a scenario where there is only a single member, so the value is a string, but not present in the service_to_replacement mapping, and so it wouldn't pass the second condition. What would then happen is that it would then execute the else statement, which would iterate through the value as a string (meaning, iterating through one character at a time).

As such, this seems like this change would introduce a bug. Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not aware of functionality end-to-end, I got issue in a case where object_policy_dict['service']['member'] was not present in service_to_replacement and the code was breaking on line 146

Copy link
Owner

@moshekaplan moshekaplan Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've created a new release with additional debugging. Could you re-run pan_analyzer with --debug appended and paste the object's information from the bottom of the debug file, after the crash that this is intended to fix, so I can see the problematic object?

member_to_replace = object_policy_dict['service']['member']
replacements_made[member_to_replace] = service_to_replacement[member_to_replace]
object_policy_dict['service']['member'] = service_to_replacement[member_to_replace]
Expand Down