-
Notifications
You must be signed in to change notification settings - Fork 3.3k
SQL database audit and threat detection commands #2536
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
Conversation
Use ARM lookup instead. This is a lot of custom code, but without it the customer experience is bad.
|
@jaredmoo, |
Codecov Report
@@ Coverage Diff @@
## master #2536 +/- ##
==========================================
+ Coverage 72.16% 72.35% +0.19%
==========================================
Files 362 363 +1
Lines 19798 19942 +144
Branches 2920 2943 +23
==========================================
+ Hits 14288 14430 +142
- Misses 4593 4596 +3
+ Partials 917 916 -1
Continue to review full report at Codecov.
|
| factory=self._client_factory, | ||
| custom_function_op=custom_function_op) | ||
| custom_function_op=custom_function_op, | ||
| **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following is my personal preference, so I don't intend to block this PR here. I usually advise avoiding **kwargs if the arguments can be explicit defined. The keyworded arguments are powerful but it also hurts the readability and maintainability. It is not obvious through the glance at the signature to make out what's the additional arguments here. Can we instead of using keyworded arguments define the arguments explicitly?
| with ServiceGroup(__name__, | ||
| get_sql_database_threat_detection_policies_operations, | ||
| database_threat_detection_policy_operations) as s: | ||
| with s.group('sql db threat-detection-policy') as c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very long command name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, understood. Does threat-policy make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
| # to determine the account's keys and endpoint. Why isn't this just a command line parameter: | ||
| # because if it was a command line parameter then the customer would need to specify storage | ||
| # resource group just to update some unrelated property, which is annoying and makes no sense to | ||
| # the customer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
troydai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I left as few comments but none of them are blocking. You can decide whether you want to address them.
| def test_sql_db_security_mgmt(self, resource_group, resource_group_2, | ||
| resource_group_location, server, | ||
| storage_account, storage_account_2): | ||
| database_name = "cliautomationdb01" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random name?
|
Thanks Troy. I'll take a look. Please don't merge until I get signoff from SQL data security team on the design & implementation. |
|
Got it. I'll put a do not merge label. Please take it down when your team OKed this change. |
| storage_account_access_key must be specified. | ||
| examples: | ||
| - name: Enable by specifying storage account name | ||
| text: az db audit-policy update -g mygroup -s myserver -n mydb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not az sql db here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Well spotted.
| get_sql_database_operations, | ||
| get_sql_database_blob_auditing_policies_operations, | ||
| get_sql_database_threat_detection_policies_operations, | ||
| get_sql_elasticpools_operations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are those related to the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes these functions are used below
| # resource group just to update some unrelated property, which is annoying and makes no sense to | ||
| # the customer. | ||
| def _find_storage_account(name): | ||
| resource_type = 'Microsoft.Storage/storageAccounts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about classic storage accounts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure CLI doesn't support creating classic storage accounts, so I have no way to create them for automated test. On top of that, there is no python client library, so I would need to handwrite http request - exactly the kind of thing you would want to have automated. The customer can still use classic storage by specifying storage endpoint and key instead of account name, so classic storage isn't totally blocked, just inconvenient. I'm ok with this for the first release. I'm adding explicit checks.
| account_name=storage_account) | ||
|
|
||
| # Get endpoint | ||
| return account.primary_endpoints.blob # pylint: disable=no-member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are storage accounts without blob parameter.
we need to check that and return a meaningful error asking the user to choose a different storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'm not sure how to create such a storage account, so I'm taking your word for it ;) I tested by replacing primary_endpoints.blob with primary_endpoints.potato and then handling that exception.
| if storage_endpoint is not None: | ||
| instance.storage_endpoint = storage_endpoint | ||
| if storage_account is not None: | ||
| storage_resource_group = _find_storage_account(storage_account) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"_find_storage_account_resource_group"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
| retention_days=None): | ||
|
|
||
| # Apply state | ||
| if state is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have parameters value validation? (e.g. in the "state" case the valid set is "Enabled" and "Disabled")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will be validated due to enum_choice_list in params.py
(env) D:\git\azure-cli [datasecurity ≡]> az sql db audit-policy update --state adf
az sql db audit-policy update: error: argument --state: invalid choice: 'adf' (choose from 'Enabled', 'Disabled')
|
@troydai I have now addressed @yaakoviyun 's feedback so you can now merge. (I don't have perms to remove the label). @yaakoviyun & @nathannfan if you have any more feedback I can do follow-up PR. Thanks all |
db group:
audit-policy subgroup:
threat-policy subgroup: