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

feat: Update SQL Server auditing filter to be more flexible #8314

Merged
merged 2 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 38 additions & 21 deletions tools/c7n_azure/c7n_azure/resources/sqlserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
from c7n_azure.filters import FirewallRulesFilter, FirewallBypassFilter
from c7n_azure.provider import resources
from c7n_azure.resources.arm import ArmResourceManager
from c7n_azure.utils import ThreadHelper, StringUtils
from c7n_azure.utils import ThreadHelper
from netaddr import IPRange, IPSet, IPNetwork, IPAddress

from c7n.exceptions import PolicyValidationError
from c7n.utils import type_schema
from c7n.filters.core import ValueFilter, Filter
from c7n.filters.core import ValueFilter

AZURE_SERVICES = IPRange('0.0.0.0', '0.0.0.0') # nosec
log = logging.getLogger('custodian.azure.sql-server')
Expand Down Expand Up @@ -315,7 +315,7 @@


@SqlServer.filter_registry.register('auditing')
class AuditingFilter(Filter):
class AuditingFilter(ValueFilter):
"""
Filter by the current auditing
policy for this sql server.
Expand All @@ -335,52 +335,69 @@

"""

cache_key = 'c7n:auditing-settings'
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like we have a consistent way to name this type of variable for the azure provider. In AWS it's most commonly annotation_key, but in Azure I see a pretty even mix of cache_key, annotation_key and <attribute>_key, and <attribute>_property. It's probably worth converging on (and documenting) an Azure convention going forward.

cache_key seems clear enough in this context, though I can also see that causing confusion with the cache_key used with the local disk cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is closed now, but the idea of annotations is something that I don't fully understand. My assumption has been that it is meant as an addition to the resource data that would show up in the final output for a matched resource. However, I'm not aware of any documentation around their use, and the confusing part is that the annotation also tends to end up being a local cache for filter data which is separate from the more generic cache you mention.

All that to say I agree there should be a standard for handling what is a pretty common pattern across filters.


schema = type_schema(
'auditing',
required=['type', 'enabled'],
**{
'enabled': {"type": "boolean"},
}
rinherit=ValueFilter.schema,
enabled=dict(type='boolean')
)

log = logging.getLogger('custodian.azure.sqlserver.auditing-filter')

def __init__(self, data, manager=None):
super(AuditingFilter, self).__init__(data, manager)
self.enabled = self.data['enabled']
super().__init__(data, manager)

self.enabled = self.data.get('enabled')
# track if we are using the legacy behavior
self.is_legacy = 'enabled' in self.data

def validate(self):
# only allow legacy behavior or new ValueFilter behavior, not both
# when in "legacy" mode the only entries should be "type" (required by schema) and
# "enabled" (required by is_legacy)
if self.is_legacy:
if len(self.data) > 2:
raise PolicyValidationError(

Check warning on line 361 in tools/c7n_azure/c7n_azure/resources/sqlserver.py

View check run for this annotation

Codecov / codecov/patch

tools/c7n_azure/c7n_azure/resources/sqlserver.py#L361

Added line #L361 was not covered by tests
"When using 'enabled', ValueFilter properties are not allowed")
# only validate value filter when not in "legacy" mode
else:
super().validate()

def process(self, resources, event=None):
resources, exceptions = ThreadHelper.execute_in_parallel(
_, exceptions = ThreadHelper.execute_in_parallel(
resources=resources,
event=event,
execution_method=self._process_resource_set,
executor_factory=self.executor_factory,
log=log
)

if exceptions:
raise exceptions[0]
return resources

return super().process(resources, event)

def _process_resource_set(self, resources, event=None):
client = self.manager.get_client()
result = []
for resource in resources:
if 'auditingSettings' not in resource['properties']:
if self.cache_key not in resource['properties']:
auditing_settings = client.server_blob_auditing_policies.get(
resource['resourceGroup'],
resource['name'])

resource['properties']['auditingSettings'] = \
resource['properties'][self.cache_key] = \
auditing_settings.serialize(True).get('properties', {})

required_status = 'Enabled' if self.enabled else 'Disabled'

if StringUtils.equal(
resource['properties']['auditingSettings'].get('state'),
required_status):
result.append(resource)
def __call__(self, resource):
auditing_enabled = resource['properties'][self.cache_key].get('state') == 'Enabled'

return result
# Apply filter based on legacy behavior which only checks against enablement
if self.is_legacy:
return auditing_enabled == self.enabled
# otherwise process the auditing settings using ValueFilter logic for full flexibility
else:
return super().__call__(resource['properties'][self.cache_key])
Comment on lines +396 to +400
Copy link
Member

Choose a reason for hiding this comment

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

I like this clear/explicit handling of the previous behavior 👍



@SqlServer.action_registry.register('set-firewall-rules')
Expand Down
19 changes: 19 additions & 0 deletions tools/c7n_azure/tests_azure/tests_resources/test_sqlserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,25 @@ def test_auditing_filter_enabled(self):
resources = p.run()
self.assertEqual(1, len(resources))

@cassette_name('auditing')
def test_auditing_filter_value(self):
p = self.load_policy({
'name': 'test-azure-sql-server',
'resource': 'azure.sqlserver',
'filters': [
{'type': 'value',
'key': 'name',
'op': 'glob',
'value_type': 'normalize',
'value': 'cctestsqlserver*'},
{'type': 'auditing',
'key': "auditActionsAndGroups[?@=='FAILED_DATABASE_AUTHENTICATION_GROUP']" +
" | length(@)",
'value': 1}],
}, validate=True)
resources = p.run()
self.assertEqual(1, len(resources))


class SQLServerFirewallFilterTest(BaseTest):

Expand Down