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

Replace Regorus with Python-based policy engine #3234

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

mgunnala
Copy link

@mgunnala mgunnala commented Sep 25, 2024

Description

Issue #


Add Python-based policy handing logic to replace Regorus.
Extension handling code is not changed in this PR, so agent functionality is not affected. No e2e tests were added.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

@narrieta
Copy link
Member

Did not go thru the implementation details, but approach looks good to me

@mgunnala mgunnala marked this pull request as ready for review September 25, 2024 21:57
@mgunnala mgunnala changed the title Add policy engine Replace Regorus with Python-based policy engine Sep 25, 2024

# Customer-defined policy is expected to be located at this path.
# If there is no file at this path, default policy will be used.
CUSTOM_POLICY_PATH = "/etc/waagent_policy.json"
Copy link
Member

Choose a reason for hiding this comment

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

let's do the same we do for waagent.conf

class DefaultOSUtil(object):
    def __init__(self):
        self.agent_conf_file_path = '/etc/waagent.conf'

Copy link
Author

Choose a reason for hiding this comment

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

Added get_custom_policy_file_path() to DefaultOSUtil and called it here

# If there is no file at this path, default policy will be used.
CUSTOM_POLICY_PATH = "/etc/waagent_policy.json"
# Default policy values to be used when no custom policy is present.
DEFAULT_ALLOW_LISTED_EXTENSIONS_ONLY = False
Copy link
Member

Choose a reason for hiding this comment

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

should be private

Copy link
Author

Choose a reason for hiding this comment

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

updated

azurelinuxagent/ga/policy/policy_engine.py Outdated Show resolved Hide resolved
azurelinuxagent/ga/policy/policy_engine.py Show resolved Hide resolved
self._log_policy("Custom policy found at {0}. Using custom policy.".format(CUSTOM_POLICY_PATH))
with open(CUSTOM_POLICY_PATH, 'r') as f: # Open file in read-only mode.
custom_policy = json.load(f)
return custom_policy
Copy link
Member

Choose a reason for hiding this comment

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

add TODO for validation

Copy link
Author

Choose a reason for hiding this comment

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

added

should_allow = not allow_listed_extension_only or extension_allowlist.get(self.extension_to_check.name.lower()) is not None
return should_allow

def should_enforce_signature(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def should_enforce_signature(self):
def should_enforce_signature_validation(self):

Copy link
Author

Choose a reason for hiding this comment

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

Updated


def tearDown(self):
# Clean up any custom policy file we created
if os.path.exists(self.custom_policy_path):
Copy link
Member

Choose a reason for hiding this comment

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

isn't this deleting the data file from the source code?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe you meant to use self.tmp_dir

Copy link
Author

Choose a reason for hiding this comment

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

We don't actually store a test policy file in the source code, each test creates its own file and cleans it up. But I was looking for a better place to create this file, so I've changed it to use self.tmp_dir.

Copy link
Member

Choose a reason for hiding this comment

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

tearDown cleans up self.tmp_dir, no need for explicit deletion

Copy link
Author

Choose a reason for hiding this comment

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

Removed

}
}
}
with open(self.custom_policy_path, mode='w') as policy_file:
Copy link
Member

Choose a reason for hiding this comment

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

        with open(self.custom_policy_path, mode='w') as policy_file:
            json.dump(policy, policy_file, indent=4)
            policy_file.flush()

should probably be refactored into a helper method

Copy link
Author

Choose a reason for hiding this comment

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

good point, I added this helper

tests/ga/test_policy_engine.py Show resolved Hide resolved
tests/ga/test_policy_engine.py Outdated Show resolved Hide resolved
azurelinuxagent/ga/policy/policy_engine.py Outdated Show resolved Hide resolved
return custom_policy
else:
self._log_policy("No custom policy found at {0}. Using default policy.".format(CUSTOM_POLICY_PATH))
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom_policy_path may exist, but have no contents. In that case this function would return an empty dict as well. If we return default policy in the else condition, I think we should also return defaults for any prop the customer did not include in their custom policy path

self.policy = self.__get_policy()

@staticmethod
def _log_policy(msg, is_success=True, op=WALAEventOperation.Policy, send_event=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest _log_policy_info or something similar.
_log_policy() sounds like we're logging the policy attribute

Copy link
Author

Choose a reason for hiding this comment

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

how about _log_policy_event()? _log_policy_info might make it seem like we're just writing to logger.info, but we're also writing to logger.error

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, _log_policy_message? event is also ok with me

Copy link
Author

Choose a reason for hiding this comment

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

changed to _log_policy_event


class ExtensionPolicyEngine(_PolicyEngine):

def __init__(self, extension_to_check):
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we should remove this parameter since we're initializing on a goal state basis.

"""
super(ExtensionPolicyEngine, self).__init__()
self._extension_to_check = extension_to_check
if not self.is_policy_enforcement_enabled():
Copy link
Contributor

Choose a reason for hiding this comment

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

This check already happens in base class init

Copy link
Author

Choose a reason for hiding this comment

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

Originally, we needed this check because policy is None if the feature isn't enabled - but since we've removed the extension_to_check parameter, we don't need this constructor at all. I've removed it.


self.extension_policy = extension_policy

def should_allow_extension(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this function will be called for every ext, in every goal state? Even if policy is not enabled?

Copy link
Author

Choose a reason for hiding this comment

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

This function will be called any time we try to enable an extension (right before the handle_enable call in handle_ext_handler()). It will be called even if policy isn't enabled, but if policy is not enabled, it will just return True.


# Customer-defined policy is expected to be located at this path.
# If there is no file at this path, default policy will be used.
CUSTOM_POLICY_PATH = DefaultOSUtil().get_custom_policy_file_path()
Copy link
Member

Choose a reason for hiding this comment

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

private

Copy link
Author

Choose a reason for hiding this comment

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

Updated



class PolicyError(AgentError):
"""
Error raised during agent policy enforcement.
"""
# TODO: split into two error classes for internal/dev errors and user errors.
def __init__(self, msg=None, inner=None, code=-1):
Copy link
Member

Choose a reason for hiding this comment

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

how are we planning to use 'code'?

should probably not have a default value

Copy link
Author

Choose a reason for hiding this comment

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

I set the code here to eventually use in create_status_file_if_not_exist(), when we block the extension. But I've removed it for now, I can add in the next PR if needed.

TEST_EXTENSION_NAME = "Microsoft.Azure.ActiveDirectory.AADSSHLoginForLinux"


class TestExtensionPolicyEngine(AgentTestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should separate these unit tests into separate classes.
TestPolicyEngine
TestExtensionPolicyEngine

policy enabled/parsing logic should be in TestPolicyEngine

Copy link
Author

Choose a reason for hiding this comment

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

I didn't initially split it this way because PolicyEngine is a private class and functionality is actually tested through ExtensionPolicyEngine. But I'm neutral on the change so went ahead and refactored it - let me know if the new split makes sense to you.

Copy link
Contributor

@maddieford maddieford left a comment

Choose a reason for hiding this comment

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

Changes in the agent look good. I reviewed the tests in more detail with this review

__validate_attribute_type("extensions", extensions, dict)
for ext_name, ext_policy in extensions.items():
__validate_attribute_type(ext_name, ext_policy, dict, "extensions")
if ext_policy is not None:
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 still need this None check?

Copy link
Author

Choose a reason for hiding this comment

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

Removed it - validation will now raise an error if ext_policy is None.

tests/ga/test_policy_engine.py Show resolved Hide resolved
"""
def test_should_allow_and_should_not_enforce_signature_for_default_policy(self):
"""
Default policy should allow all extensions and not enforce signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test name sounds like its testing default policy:

# Default policy values to be used when no custom policy is present.
_DEFAULT_ALLOW_LISTED_EXTENSIONS_ONLY = False
_DEFAULT_SIGNATURE_REQUIRED = False

but its actual testing the case where no policy exists/is in use

Copy link
Author

Choose a reason for hiding this comment

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

I realized there are a few overlapping tests for testing default policy / custom policy empty / no custom policy exists. I've refactored them into:
test_should_allow_and_should_not_enforce_signature_if_no_custom_policy_file
test_should_allow_and_should_not_enforce_signature_if_conf_flag_false
test_should_use_default_policy_if_no_custom_extension_policy_specified

tests/ga/test_policy_engine.py Show resolved Hide resolved
self.assertTrue(should_allow,
msg="No custom policy is present, so use default policy. Should allow all extensions.")

def test_should_allow_if_extension_policy_section_missing(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a case to test that we don't enforce signature if extension policy section is missing

Copy link
Author

Choose a reason for hiding this comment

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

Added

should_allow = engine.should_allow_extension(test_extension)
self.assertTrue(should_allow)

def test_should_allow_if_policy_disabled(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

__get_policy_enforcement_enabled() checks if the policy file exists first. Since policy file doesn't exist, the function is returning false without checking the conf flag. Consider creating a policy file for this test and test_should_not_enforce_signature_if_policy_disabled

Copy link
Author

Choose a reason for hiding this comment

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

Refactored these into two different tests:
test_should_allow_and_should_not_enforce_signature_if_no_custom_policy_file
test_should_allow_and_should_not_enforce_signature_if_conf_flag_false

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 94.89796% with 5 lines in your changes missing coverage. Please review.

Project coverage is 72.40%. Comparing base (3aebcdd) to head (e038560).
Report is 314 commits behind head on develop.

Files with missing lines Patch % Lines
azurelinuxagent/ga/policy/policy_engine.py 94.68% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3234      +/-   ##
===========================================
+ Coverage    71.97%   72.40%   +0.42%     
===========================================
  Files          103      113      +10     
  Lines        15692    16956    +1264     
  Branches      2486     2452      -34     
===========================================
+ Hits         11295    12277     +982     
- Misses        3881     4121     +240     
- Partials       516      558      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@maddieford maddieford left a comment

Choose a reason for hiding this comment

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

Added two very minor comments

expected_extension_policy = expected_policy.get("extensionPolicies")
self.assertEqual(actual_extension_policy.get("allowListedExtensionsOnly"), expected_extension_policy.get("allowListedExtensionsOnly"))
self.assertEqual(actual_extension_policy.get("signatureRequired"), expected_extension_policy.get("signatureRequired"))
self.assertEqual(actual_extension_policy.get("signatureRequired"), expected_extension_policy.get("signatureRequired"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 227 is a duplicate of 226

Copy link
Author

Choose a reason for hiding this comment

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

Fixed!

tests/ga/test_policy_engine.py Show resolved Hide resolved
@@ -143,6 +143,7 @@ def get_firewall_delete_conntrack_drop_command(wait, destination):
class DefaultOSUtil(object):
def __init__(self):
self.agent_conf_file_path = '/etc/waagent.conf'
self.custom_policy_file_path = '/etc/waagent_policy.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Today, some distros place the waagent.conf in different path, do we want to take that responsibility and update those distros policy path based on waagent.conf path in respective osutils?

Copy link
Author

Choose a reason for hiding this comment

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

I think we should leave it up to individual distros if they want to update the policy path, but will defer to you on this. PolicyEngine just uses get_osutil().get_custom_policy_file_path().

Copy link
Member

Choose a reason for hiding this comment

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

@mgunnala - @nagworld9 is right. Initially I asked you to add it here, and then I realized it actually needed to be in waagent.conf, but did not have time to post my message. Thanks for pointing this out, Nag.

Copy link
Author

@mgunnala mgunnala Oct 7, 2024

Choose a reason for hiding this comment

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

To make sure I'm understanding correctly, is this the change we would want to make?

  • remove custom_policy_file_path instance variable from DefaultOSUtil
  • get_custom_policy_file_path() should return <conf_file_directory>/waagent_policy.json
  • policy_engine.py calls get_osutil().get_custom_policy_file_path() (as currently implemented)

Copy link
Member

Choose a reason for hiding this comment

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

Almost. Move the path from osutil to conf (we already define other paths there)

def get_lib_dir(conf=__conf__):
    return conf.get("Lib.Dir", "/var/lib/waagent")

def get_ext_log_dir(conf=__conf__):
    return conf.get("Extension.LogDir", "/var/log/azure")

BTW, let's remove "custom" from it. Maybe Policy.PolicyFile?

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

with open(_CUSTOM_POLICY_PATH, 'r') as f: # Open file in read-only mode.
self._log_policy_event("Custom policy file found at {0}. Using custom policy.".format(_CUSTOM_POLICY_PATH))
try:
custom_policy = json.load(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

customer can change json file while we read, so we may get jsondecode error. Do we want to add few retries for json load?

Copy link
Author

Choose a reason for hiding this comment

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

We read the policy on a per goal state basis. If the customer does change the file during goal state processing, we don't necessarily want to support that so it would make sense to raise an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

If user is modifying policy file while the agent is processing extensions, what message will be surfaced to the user? Will it be useful/clear?

Copy link
Author

Choose a reason for hiding this comment

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

The policy file is only read once during extension processing. If the user successfully modifies the file to a valid JSON before json.load is called, no error message is raised and we apply the modified version of the policy.

If the file is actively being modified and incomplete/invalid when json.load is called, JSONDecodeError will be thrown, we catch and re-raise it as an InvalidPolicyError with an error message like "policy file does not conform to valid json syntax". This might be a case where it makes sense to dump the policy file in the error message.

azurelinuxagent/ga/policy/policy_engine.py Outdated Show resolved Hide resolved
azurelinuxagent/ga/policy/policy_engine.py Show resolved Hide resolved
maddieford
maddieford previously approved these changes Oct 4, 2024
@@ -369,6 +369,9 @@ def is_current_instance_id(self, id_that):
def get_agent_conf_file_path(self):
return self.agent_conf_file_path

def get_custom_policy_file_path(self):
Copy link
Member

Choose a reason for hiding this comment

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

left-over

Copy link
Author

Choose a reason for hiding this comment

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

removed

@@ -27,12 +39,31 @@ class PolicyError(AgentError):
"""


class PolicyEngine(object):
class PolicyInvalidError(AgentError):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class PolicyInvalidError(AgentError):
class InvalidPolicyError(AgentError):

Copy link
Author

Choose a reason for hiding this comment

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

Updated this


# Customer-defined policy is expected to be located at this path.
# If there is no file at this path, default policy will be used.
_CUSTOM_POLICY_PATH = conf.get_policy_file_path()
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this module-global variable?

Copy link
Author

Choose a reason for hiding this comment

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

I kept it to easily accommodate changes to where we keep the policy path (for example, moving from osutil to conf). Can remove if you think it's not needed.

Copy link
Member

Choose a reason for hiding this comment

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

I thought it was a left-over from a previous iteration, when we defining the value here.

Seems odd that we define this global for this conf parameter, while we don't for conf.get_extension_policy_enabled (and other parameters in the rest of the code)

Copy link
Author

Choose a reason for hiding this comment

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

removed

# Policy should only be enabled if conf flag is true AND policy file is present.
return conf.get_extension_policy_enabled()
policy_file_exists = os.path.exists(_CUSTOM_POLICY_PATH)
Copy link
Member

Choose a reason for hiding this comment

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

     policy_file_exists = os.path.exists(_CUSTOM_POLICY_PATH)
     return conf.get_extension_policy_enabled() and policy_file_exists

The enabled flag should be checked first:

return conf.get_extension_policy_enabled() and os.path.exists(conf.get_policy_file_path())

Copy link
Author

Choose a reason for hiding this comment

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

updated

# The same policy should be enforced even if the policy file is deleted/changed during a single goal state processing.
self._policy_enforcement_enabled = self.__get_policy_enforcement_enabled()
if not self.policy_enforcement_enabled:
self._log_policy_event(msg="Policy enforcement is not enabled.")
Copy link
Member

Choose a reason for hiding this comment

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

On most VMs, the policy file will not exist and this message won't make much sense. Maybe log when enforcing, rather than when not enforcing? (and include the path to the policy file in the message)

def policy_enforcement_enabled(self):
return self._policy_enforcement_enabled

def __get_policy(self):
Copy link
Member

Choose a reason for hiding this comment

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

Should be static. It is called from init and, as the code evolves, it may end up referencing an uninitialized instance member

Copy link
Author

Choose a reason for hiding this comment

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

not sure I understand - what's an example of when this could happen, and would it not be caught by unit tests? I've changed the code in __parse_policy to directly update self._policy, trying to understand if this implementation is problematic.

if not isinstance(attribute_value, expected_type):
# Error message should refer to "JSON" not "dict".
if expected_type == dict:
expected_type_in_msg = "JSON"
Copy link
Member

Choose a reason for hiding this comment

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

"object", rather than "JSON"

Copy link
Author

Choose a reason for hiding this comment

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

added a __parse_dict method and changed the error message to "object"


# Agent supports up to this version of the policy file ("policyVersion" in schema).
# Increment this number when any new attributes are added to the policy schema.
_MAX_SUPPORTED_POLICY_VERSION = "0.1.0"
Copy link
Author

Choose a reason for hiding this comment

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

Do we want this to be "0.1.0" to accommodate any changes we might make before GA? Or set to "1.0.0" now?

if version is None:
return

pattern = r'^\d+\.\d+\.\d+$'
Copy link
Author

Choose a reason for hiding this comment

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

is something like "1" or "1.0" acceptable? or should this be a strict major.minor.patch format?

@@ -170,7 +170,8 @@ def load_conf_from_file(conf_file_path, conf=__conf__):
"ResourceDisk.MountPoint": "/mnt/resource",
"ResourceDisk.MountOptions": None,
"ResourceDisk.Filesystem": "ext3",
"AutoUpdate.GAFamily": "Prod"
"AutoUpdate.GAFamily": "Prod",
"Policy.PolicyFile": "/etc/waagent_policy.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Policy.PolicyFile": "/etc/waagent_policy.json"
"Policy.PolicyFilePath": "/etc/waagent_policy.json"

with open(conf.get_policy_file_path(), 'r') as f:
_PolicyEngine._log_policy_event(
"Policy enforcement is enabled. Enforcing policy using policy file found at '{0}'.".format(
conf.get_policy_file_path()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth logging/sending telemetry with the provided policy (either before or after parsing)?

We don't want to flood the log, but we only check policy when there's a new extensions goal state to process, so I think it may be useful to add this logging/telemetry to aid in debugging. Right now, if a customer raises an incident that their policy was incorrectly applied, we won't know what the state of the policy file was at the time of the issue.

@narrieta, @mgunnala what do you think?

Copy link
Author

@mgunnala mgunnala Oct 14, 2024

Choose a reason for hiding this comment

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

I think it could be worth logging the policy immediately after reading it (before parsing), it would be helpful especially in cases where the customer unexpectedly changes the file during goal state processing.

I was also thinking if we should include it in InvalidPolicyError messages, so the user knows exactly what policy file contents caused that error. But this seems like potentially overkill and might make error messages quite difficult to read.

with open(_CUSTOM_POLICY_PATH, 'r') as f: # Open file in read-only mode.
self._log_policy_event("Custom policy file found at {0}. Using custom policy.".format(_CUSTOM_POLICY_PATH))
try:
custom_policy = json.load(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

If user is modifying policy file while the agent is processing extensions, what message will be surfaced to the user? Will it be useful/clear?

return None

if not isinstance(value, (str, ustr)):
raise InvalidPolicyError("invalid type '{0}' for attribute '{1}', please change to object.".format(type(value).__name__, key))
Copy link
Contributor

Choose a reason for hiding this comment

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

please change to object

should this be str?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thanks.

I was thinking about merging __parse_bool, __parse_dict, __parse_string into a single function, so we have a common error message between all of them. I ended up keeping them separate because I thought it made the code more readable but let me know if you have a preference.

Copy link
Contributor

@maddieford maddieford Oct 14, 2024

Choose a reason for hiding this comment

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

I think it would make sense to merge them.
I also think the type validation could instead be done in __validate_schema by using the value of the attribute in _POLICY_SCHEMA as the expected type

Copy link
Contributor

Choose a reason for hiding this comment

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

As is, if we add a new attribute to the extensionPolicies section, we would need to add it to POLICY_SCHEMA, and add a separate call to _parse_bool for the new attribute.


# Validate "extensions" dict against the schema
extensions_schema = _POLICY_SCHEMA["extensionPolicies"]["extensions"]
if not isinstance(extensions, dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a repeat of the type check that happens in __parse_dict on line 212

parsed_extensions_dict = {}
for (extension_name, individual_policy) in extensions.items():
self.__validate_schema(individual_policy, extensions_schema["<extensionName>"])
if not isinstance(individual_policy, dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this type check should happen before __validate_schema on line 225. Validate schema iterates over keys in the individual_policy, before validating the type of the individual_policy

Copy link
Author

Choose a reason for hiding this comment

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

I changed the order, also added a unit test to check the case where individual_policy is not iterable.

# Parse individual extension policies
self.__parse_extensions(extension_policies)

return extension_policies
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this return is necessary currently

Copy link
Author

Choose a reason for hiding this comment

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

removed

}
}
custom_policy = self.__get_policy()
self.__parse_policy(custom_policy)
Copy link
Contributor

Choose a reason for hiding this comment

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

__parse_policy may update self._policy with a partially parsed policy in the case that an exception is raised during parsing.

I don't think self._policy should be in a 'partially parsed' state. For now, I understand why this may be ok, but it could be risky in future iterations of the code.

I think self._policy should either be -> default policy, or fully parsed custom policy

the error message to avoid ambiguity.
"""
value = policy.get(key)
if value is None:
Copy link
Author

Choose a reason for hiding this comment

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

what if the policy file specifies a null value (like { "signatureRequired": None }) - what is the expected behavior? with this implementation, we don't raise an error and use the default value

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.

4 participants