Add Regorus policy engine framework #3187
Conversation
mgunnala
requested review from
narrieta,
ZhidongPeng,
nagworld9 and
maddieford
as code owners
| log_policy("Policy enforcement is disabled via configuration file.") | ||
| return | ||
|
|
||
| if not self._is_policy_supported(): |
There was a problem hiding this comment.
In this case, we want to allow all extensions to be installed, but log that customer is trying to enable policy on unsupported distro.
There was a problem hiding this comment.
Discussed offline: all extensions should be blocked. I've updated the code to return an empty query result in this case. The next PR will use the empty query result to return an empty allow list, so that no extensions will be installed (ADO task 29144116, added TODO to code).
There was a problem hiding this comment.
I think empty list is not the right way to represent those errors. Both of them block extension execution, but empty list should just communicate the user that no extensions are allowed to execute, while errors should be very explicit about what is happening (try to enable on unsupported distro, invalid policy file, etc) so that the user can take the appropriate action
There was a problem hiding this comment.
+1, the extension status message should be clear that extension processing is blocked because this is an unsupported distro (or whatever the reason is)
There was a problem hiding this comment.
Updated error handling to raise exceptions
|
|
||
| # Define support matrix for Regorus and policy engine feature. | ||
| # Dict in the format: { distro:min_supported_version } | ||
| POLICY_SUPPORTED_DISTROS = { |
There was a problem hiding this comment.
Minor: suggest POLICY_SUPPORTED_DISTROS_MIN_VERSIONS
| # Dict in the format: { distro:min_supported_version } | ||
| POLICY_SUPPORTED_DISTROS = { | ||
| 'ubuntu': FlexibleVersion('16.04'), | ||
| 'mariner': FlexibleVersion('2') |
| 'ubuntu': FlexibleVersion('16.04'), | ||
| 'mariner': FlexibleVersion('2') | ||
| } | ||
| POLICY_SUPPORTED_ARCHITECTURE = ['x86_64', 'amd64'] |
There was a problem hiding this comment.
double-check this, specially for arm64. it can also be aarch64
There was a problem hiding this comment.
We should have more telemetry on this now. I'll share with Manu
There was a problem hiding this comment.
We're not supporting arm64 until public preview. Added a TODO to update this list once we enable support.
There was a problem hiding this comment.
I think we should remove 'amd64' from this list. We don't have telemetry for this arch type:
cluster("azcore.centralus.kusto.windows.net").database("Fa").GuestAgentExtensionEvents
| where PreciseTimeStamp >= ago(30d)
| where OSVersion startswith "linux"
| summarize dcount(ContainerId) by KeywordName
| KeywordName | dcount_ContainerId |
|---|---|
| {"CpuArchitecture": "x86_64"} | 739,107,991 |
| 639,940,276 | |
| {"CpuArchitecture": "aarch64"} | 3,092,416 |
| {"CpuArchitecture": "i686"} | 280 |
| Log information to console and telemetry. | ||
| """ | ||
| if is_success: | ||
| logger.info("[Policy] " + formatted_string) |
There was a problem hiding this comment.
Minor: not sure we need the "[Policy]" prefix
There was a problem hiding this comment.
I thought it made the log file a little easier to read/parse, can remove if you think it adds too much clutter.
| PolicyEngineConfigurator._initialized = True | ||
|
|
||
| @staticmethod | ||
| def is_policy_configured(): |
There was a problem hiding this comment.
renamed to is_policy_enforcement_enabled( ), to avoid confusion with the policy_engine_enabled property.
| log_policy("Policy enforcement is disabled via configuration file.") | ||
| return | ||
|
|
||
| if not self._is_policy_supported(): |
There was a problem hiding this comment.
I think empty list is not the right way to represent those errors. Both of them block extension execution, but empty list should just communicate the user that no extensions are allowed to execute, while errors should be very explicit about what is happening (try to enable on unsupported distro, invalid policy file, etc) so that the user can take the appropriate action
| return | ||
|
|
||
| # Regorus import should only be attempted after completing the above checks within | ||
| # the configurator, but the module itself needs to be accessible outside this class. |
There was a problem hiding this comment.
by PolicyEngine? shouldn't we move the initialization there then?
There was a problem hiding this comment.
I think importing here, only after checking that the feature is enabled, etc, used to make sense when the interface with regorus was going to be a set of Python bindings that may crash on some distros. Not sure what the motivation is with the current implementation.
There was a problem hiding this comment.
makes sense. I've moved the initialization to policy engine.
| Use: PolicyEngineConfigurator.get_instance() | ||
| """ | ||
| _instance = None # configurator is implemented as a singleton | ||
| _initialized = False # set to true on first init, even if Regorus import fails, we don't want to retry. |
There was a problem hiding this comment.
I don't think this state should be static, since we are trying to initialize only once per waagent service start.
Under some scenarios, for example the user tries to use the feature on an unsupported distro, the user should not need to restart the agent to fix the issue.
This initialization should probably be done on a per-goal state basis, unless it is very expensive.
Alternatively, some initialization could be done on a per-service-start basis, and some on a per-goal-state basis, but those should be clearly defined.
There was a problem hiding this comment.
Moved initialization to PolicyEngine. Exthandlers.py will initialize the PolicyEngine on a per-goal state basis.
| if arch not in POLICY_SUPPORTED_ARCHITECTURE: | ||
| return False | ||
| distro_info = get_distro() | ||
| distro_name = distro_info[0] |
There was a problem hiding this comment.
prefer DISTRO_NAME and DISTRO_VERSION over distro_info[0], [1]. Makes the code easier to read.
they are initialized like this
__distro__ = get_distro()
DISTRO_NAME = __distro__[0]
DISTRO_VERSION = __distro__[1]
| import azurelinuxagent.ga.policy.regorus as regorus | ||
| PolicyEngineConfigurator._policy_enabled = True | ||
|
|
||
| except (ImportError, NameError) as ex: |
There was a problem hiding this comment.
How could ImportError and NameError happen?
There was a problem hiding this comment.
Removed these errors, not relevant since we're no longer using a python wrapper
| return | ||
|
|
||
| # Regorus import should only be attempted after completing the above checks within | ||
| # the configurator, but the module itself needs to be accessible outside this class. |
There was a problem hiding this comment.
I think importing here, only after checking that the feature is enabled, etc, used to make sense when the interface with regorus was going to be a set of Python bindings that may crash on some distros. Not sure what the motivation is with the current implementation.
| self._policy_engine_enabled = False | ||
| self._engine = None | ||
| try: | ||
| if PolicyEngineConfigurator.get_instance().get_policy_enabled(): |
There was a problem hiding this comment.
This usage is weird, because get_policy_enabled() is an static method, so one does not need an instance to invoke it...
Why the call to get_instance()?
This is related to one of my comments above about all methods/properties of PolicyEngineConfigurator being static, so there is no need for a singleton instance
There was a problem hiding this comment.
Removed PolicyEngineConfigurator
azurelinuxagent/ga/policy/regorus.py
Outdated
| def add_policy(self, policy_path): | ||
| """Policy_path is expected to point to a valid Regorus file.""" | ||
| if not os.path.exists(policy_path) or not policy_path.endswith('.rego'): | ||
| raise Exception("Policy path {0} is not a valid .rego file.".format(policy_path)) |
There was a problem hiding this comment.
this check seems extremely weak... seems like we accept as valid any file, no matter its contents, as long as its name ends with ".rego"
There was a problem hiding this comment.
i see, the actual validation would be done by regorus and then we would need to handle the error reporting here.
a TODO comment would clarify thin
| log_policy("Error: Failed to add data to Regorus policy engine. '{0}'".format(ex), is_success=False) | ||
|
|
||
| def set_input(self, input_json): | ||
| """Input_json should be a JSON object.""" |
There was a problem hiding this comment.
can you describe what is in that JSON object?
azurelinuxagent/ga/policy/regorus.py
Outdated
|
|
||
| def eval_query(self, query): | ||
| missing_files = [] | ||
| if self._policy_file is None: |
There was a problem hiding this comment.
this is weird. callers to eval_query() need to know that they must call add_policy(), set_input() and add_data() f first. How do they know that? why if the caller invokes those 3 methods to evaluate a query and then forgets to call one of them when trying to evaluate another query?
why not
def eval_query(self, policy_path, input_json, data, query):
There was a problem hiding this comment.
Discussed offline - this is how the regorus library implements it, we're keeping the functions the same in case we want to change the underlying implementation in the future (ex switch to a python library). PolicyEngine init function calls add_policy() and add_data(), PolicyEngine.evaluate_query() takes input as a parameter and calls set_input().
azurelinuxagent/ga/policy/regorus.py
Outdated
| self._policy_file = policy_path | ||
|
|
||
| def set_input(self, input_json): | ||
| """Input_path is expected to point to a valid JSON object.""" |
There was a problem hiding this comment.
can you describe what is in that JSON object?
azurelinuxagent/ga/policy/regorus.py
Outdated
| raise Exception("Input to policy engine is not a valid JSON object.") | ||
|
|
||
| def add_data(self, data): | ||
| """Data parameter is expected to point to a valid json file.""" |
There was a problem hiding this comment.
can you describe what is in that JSON file?
There was a problem hiding this comment.
Added, though I haven't included full details here, more explanation in the spec doc. Let me know if more detail is needed here specifically.
azurelinuxagent/ga/policy/regorus.py
Outdated
| "-i", input_file.name, query] | ||
|
|
||
| # use subprocess.Popen instead of subprocess.run for Python 2 compatibility | ||
| process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
use shellutil.run_command() instead of Popen()
| @@ -0,0 +1,125 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
should this be under the normal Agent license instead?
There was a problem hiding this comment.
updated to normal agent license since this will be packaged with agent
| @staticmethod | ||
| def is_policy_enforcement_supported(): | ||
| """Check that both platform architecture and distro/version are supported.""" | ||
| arch = platform.machine().lower() |
There was a problem hiding this comment.
We have an osutil function for this: osutil.get_vm_arch()
| if arch not in POLICY_SUPPORTED_ARCHITECTURE: | ||
| return False | ||
| __distro__ = get_distro() | ||
| DISTRO_NAME = __distro__[0] |
There was a problem hiding this comment.
Sorry, what I meant was to use the distro name and version already defined in azurelinuxagent.common.version
There was a problem hiding this comment.
Ah, that makes sense. Updated.
| DISTRO_VERSION = __distro__[1] | ||
| try: | ||
| distro_version = DistroVersion(DISTRO_VERSION) | ||
| except ValueError: |
There was a problem hiding this comment.
I changed this function implementation based on the other comments, removed this block too.
| """This property tracks whether the feature is enabled and Regorus engine has been successfully initialized""" | ||
| return self._policy_engine_enabled | ||
|
|
||
| def evaluate_query(self, input_dict, query): |
There was a problem hiding this comment.
can we use a better name than "input_dict"?
There was a problem hiding this comment.
"input_to_query" or "input_to_check" maybe?
| }, ... | ||
| } | ||
|
|
||
| Expected format for query: "data.agent_extension_policy.extensions_to_download" |
There was a problem hiding this comment.
what does "data.agent_extension_policy.extensions_to_download"? an extension name?
There was a problem hiding this comment.
This is the value we're querying for in the regorus engine. We can query for "data", "data.agent_extension_policy", "data.agent_extension_policy.extensions_to_download", etc.
"data.agent_extension_policy.extensions_to_download" will return output in the format:
"extensions_to_download": {
"Microsoft.Azure.ActiveDirectory.AADSSHLoginForLinux": {
"downloadAllowed": true
},
"Microsoft.Azure.Extensions.CustomScript": {
"downloadAllowed": false
}
}
...
I've put a full example in the spec.
| try: | ||
| full_result = self._engine.eval_query(input_dict, query) | ||
| if not full_result.get('result') or not isinstance(full_result['result'], list): | ||
| raise PolicyError("query returned unexpected output with no 'result' list. Please validate rule file.") |
There was a problem hiding this comment.
could you add the full_result to the message? it would be needed for debugging.
while doing that, adding the path to the rule file would also be helpful (it was mentioned before that there may be multiple fules files)
There was a problem hiding this comment.
similar comment for other messages
tests/ga/test_policy_engine.py
Outdated
| with patch('azurelinuxagent.ga.policy.policy_engine.get_osutil') as mock_get_osutil: | ||
| with patch('azurelinuxagent.ga.policy.policy_engine.conf.get_extension_policy_enabled', return_value=True): | ||
| with self.assertRaises(PolicyError, msg="Policy should not be enabled on unsupported architecture ARM64, should have raised exception."): | ||
| mock_osutil = MagicMock() |
There was a problem hiding this comment.
why mocking all of osutil instead of just mocking get_vm_arch? (if the code ends up doing other calls to utilities in osutil, the test may break)
There was a problem hiding this comment.
Good catch, thanks. I've fixed it.
| all methods will be no-ops. | ||
|
|
||
| If any errors are thrown in regorus.py, they will be caught and handled here. add_policy, add_data, | ||
| and set_input will be no-ops, eval_query will return an empty dict |
There was a problem hiding this comment.
Is this comment still true?
add_policy, add_data, and set_input will be no-ops, eval_query will return an empty dict
Seems like we updated to raise exc instead of no-op
| Example format for query: "data.agent_extension_policy.extensions_to_download" | ||
| """ | ||
| # This method should never be called if policy is not enabled, this would be a developer error. | ||
| if not self.policy_engine_enabled: |
There was a problem hiding this comment.
| if not self.policy_engine_enabled: | |
| if not self._policy_engine_enabled: |
or
| if not self.policy_engine_enabled: | |
| if not self.policy_engine_enabled(): |
There was a problem hiding this comment.
Whoa! great catch @maddieford!
pylint disappoints me more often than I'd like. But if the linter fails, the next step would be unit tests, I guess
There was a problem hiding this comment.
@maddieford not sure I'm understanding the issue - this came from a previous comment to make policy_engine_enabled( ) into a @Property method. My understanding is we can just access "self.policy_engine_enabled" since it's acting as a property. Is this not the case?
There was a problem hiding this comment.
@mgunnala - the parenthesis are missing from the function call. As written currently, that condition is always True
maddieford points out that you should add the parenthesis or use the private property
There was a problem hiding this comment.
This is a bad bug, easy to miss. The linter (or the unit tests) didn't point it out.
There was a problem hiding this comment.
I thought @Property methods in python didn't require the parentheses.
I have a unit test looking for exactly this scenario - "test_eval_query_should_throw_error_when_disabled()". It passes, and when I step through it, the function is being called as expected.
There was a problem hiding this comment.
yes, i think we misread the code
There was a problem hiding this comment.
but, related to this property.. is this needed at all? see my other comment. looks like a left-over
| try: | ||
| full_result = self._engine.eval_query(input_to_check, query) | ||
| debug_info = "Rule file is located at '{0}'. \nFull query output: {1}".format(self._engine.rule_file, full_result) | ||
| if not full_result.get('result') or not isinstance(full_result['result'], list): |
There was a problem hiding this comment.
not full_result.get('result')
Are we checking for None here? Looks like 'result' value type is list so we should probably do explicit check for None here instead of 'not full_result...'
| if not full_result.get('result') or not isinstance(full_result['result'], list): | |
| if full_result.get('result') is None or not isinstance(full_result['result'], list): |
There was a problem hiding this comment.
+1
@mgunnala We avoid those implicit comparisons. We check "if foo/if not foo" only for actual bools. Comparisons against None, empty list, empty string, etc, should be explicit
There was a problem hiding this comment.
Added explicit None checks for full_result, result, expression and value
| if not full_result.get('result') or not isinstance(full_result['result'], list): | ||
| raise PolicyError("query returned unexpected output with no 'result' list. Please validate rule file. {0}".format(debug_info)) | ||
| expressions = full_result['result'][0].get('expressions') | ||
| if not expressions or not isinstance(expressions, list) or len(expressions) == 0: |
There was a problem hiding this comment.
same comment here, check if expressions is None explicitly
| if not expressions or not isinstance(expressions, list) or len(expressions) == 0: | ||
| raise PolicyError("query returned unexpected output with no 'expressions' list. {0}".format(debug_info)) | ||
| value = expressions[0].get('value') | ||
| if not value: |
azurelinuxagent/common/conf.py
Outdated
|
|
||
| def get_extension_policy_enabled(conf=__conf__): | ||
| """ | ||
| Determine whether extension policy is enabled. If true, Regorus will be installed on the VM and |
There was a problem hiding this comment.
nit: missed this one before: "If true, Regorus will be installed on the VM". Regorus is always installed on the VM, no matter the value of the flag
|
|
||
| class PolicyEngine(object): | ||
| """ | ||
| Implements policy engine API. Class will always be initialized, but if the Regorus import fails, |
There was a problem hiding this comment.
left-over comment; we removed the try/except of ImportError in a previous iteration
| Implements policy engine API. Class will always be initialized, but if the Regorus import fails, | ||
| all methods will be no-ops. | ||
|
|
||
| If any errors are thrown in regorus.py, they will be caught and handled here. add_policy, add_data, |
There was a problem hiding this comment.
left-over comment -- i think those methods are now gone
| Implements policy engine API. Class will always be initialized, but if the Regorus import fails, | ||
| all methods will be no-ops. | ||
|
|
||
| If any errors are thrown in regorus.py, they will be caught and handled here. add_policy, add_data, |
There was a problem hiding this comment.
there are some related left-over comments in the unit tests as well
| try: | ||
| self._engine = regorus.Engine(policy_file=policy_file, rule_file=rule_file) | ||
| self._policy_engine_enabled = True | ||
| except Exception as ex: |
There was a problem hiding this comment.
is this try/catch a leftover of a previous iteration? at this point the initialization is just
self._engine = None
self._rule_file = rule_file
self._policy_file = policy_file
| Example format for query: "data.agent_extension_policy.extensions_to_download" | ||
| """ | ||
| # This method should never be called if policy is not enabled, this would be a developer error. | ||
| if not self.policy_engine_enabled: |
There was a problem hiding this comment.
Whoa! great catch @maddieford!
pylint disappoints me more often than I'd like. But if the linter fails, the next step would be unit tests, I guess
| try: | ||
| full_result = self._engine.eval_query(input_to_check, query) | ||
| debug_info = "Rule file is located at '{0}'. \nFull query output: {1}".format(self._engine.rule_file, full_result) | ||
| if not full_result.get('result') or not isinstance(full_result['result'], list): |
There was a problem hiding this comment.
+1
@mgunnala We avoid those implicit comparisons. We check "if foo/if not foo" only for actual bools. Comparisons against None, empty list, empty string, etc, should be explicit
|
|
||
| @property | ||
| def policy_engine_enabled(self): | ||
| """This property tracks whether the feature is enabled and Regorus engine has been successfully initialized""" |
There was a problem hiding this comment.
left-over comment: "and Regorus engine has been successfully initialized""
| raise PolicyError(msg) | ||
|
|
||
| @classmethod | ||
| def log_policy(cls, msg, is_success=True, op=WALAEventOperation.Policy, send_event=True): |
Description
This is PR 1 for policy enforcement within the guest agent. This PR adds the test Regorus executable and initial policy engine framework to call the executable. Guest agent functionality is not yet affected.
PR information
Quality of Code and Contribution Guidelines