Skip to content

Integration of Microsoft SQL Server - Audit Events #2009

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

Merged
merged 66 commits into from
Dec 14, 2021

Conversation

r00tu53r
Copy link
Contributor

@r00tu53r r00tu53r commented Oct 25, 2021

What does this PR do?

Build a new integration package for Microsoft SQL Server Audit Events. (#1635).

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

Author's Checklist

Still working on adding visualizations and saved searches etc.

How to test this PR locally

elastic-package test pipeline

Related issues

Screenshots

MSSQL-dashboard-v2

@r00tu53r r00tu53r added enhancement New feature or request draft Draft labels Oct 25, 2021
@r00tu53r r00tu53r requested a review from P1llus October 25, 2021 05:43
@r00tu53r r00tu53r self-assigned this Oct 25, 2021
@elasticmachine
Copy link

elasticmachine commented Oct 25, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-08T02:33:16.161+0000

  • Duration: 12 min 23 sec

  • Commit: ce2bc9a

Test stats 🧪

Test Results
Failed 0
Passed 4
Skipped 0
Total 4

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@P1llus
Copy link
Member

P1llus commented Oct 25, 2021

There was an initial discussion on this on slack, awaiting further changes to be pushed before doing another review.

- Change field types to numbers for those which can be queried by range
- Set kv target directly to sqlserver.audit and remove _temp
- Update README
- Add event_id as configurable input
- Handle tags and preserve original event
- Add processors
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@andrewkroh andrewkroh requested a review from a team November 30, 2021 16:11
target_field: process
ignore_missing: true
- rename:
field: winlog.user
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this might be the process that wrote the event log rather than the SQL user that performed the action. So I would leave it unmodified.

I would expect user.* to be based on the user that's logged into the DB. I think that means server_principal_* is used to populate user.* and target_server_principal_* gets copied into user.target.*.

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 have addressed these too. From what I understand the server_principal_sid for a domain account is stored in the encoded binary form in SQL Server so it probably isn't sending target_server_principal_sid (I haven't seen it in my tests) as they are the same. I've set it anyway in case we receive it, we'll set it.

Copy link
Member

Choose a reason for hiding this comment

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

So these are not the standard Windows SID strings (S-R-I-S-S format), but are a SQS server SID in varbinary format? Perhaps it we should retain the original "principal" keys to have some traceability on where the values of user.* came from.

If we never retain the original principal fields then they should be removed from the mappings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value in these depend on the type of login / account. SQL Server can be configured to allow logins in Mixed Mode or Windows Authentication Mode. Microsoft recommends to use Windows authentication as it is more secure. In mixed mode the user could be a SQL server user in which case the SID is a 16 byte varbinary specific to SQL Server version. But if the login is tied to Windows authentication then SQL server hands off the authentication to Windows domain controller. For this case the SID is windows SID but converted into binary format. There seems to be functions available to convert them back to string representation (S-1-x-x-x) in SQL server.

From what I understand, the value could be the original principal SID but in a different format. It could be converted between varbinary and text formats in SQL Server.

Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

LGTM! Great work on the package, sorry it tok so long. Did a lot of the reviewing outside of Github as well

* Set SPN and Target SPN to user.* and user.target.* ECS fields
* Cleanup
@r00tu53r
Copy link
Contributor Author

r00tu53r commented Dec 8, 2021

@andrewkroh Thank you for your comments. I've addressed them all. I'll wait for you to take another look.

@r00tu53r
Copy link
Contributor Author

r00tu53r commented Dec 8, 2021

LGTM! Great work on the package, sorry it tok so long. Did a lot of the reviewing outside of Github as well

Thank you @P1llus for your inputs and help with this integration! No worries about the timelines.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I think the organization of the pipeline needs changed if events other than 33205 are expected to be processed. Everything else LGTM.

@r00tu53r
Copy link
Contributor Author

r00tu53r commented Dec 14, 2021

I think the organization of the pipeline needs changed if events other than 33205 are expected to be processed. Everything else LGTM.

Once the SQL Server has been configured to log audit events to event log and has no issues w.r.t permissions etc all events are logged with id 33205 starting from MS SQL Server 2008 as stated here.

There are some audit related events reaching other IDs like 33203, 33204, 33210 etc (stated here). These are for cases where audit could not be performed due to permission issues etc.

@r00tu53r r00tu53r merged commit f1edb32 into elastic:master Dec 14, 2021
@andrewkroh andrewkroh added Integration:microsoft_sqlserver Microsoft SQL Server New Integration Issue or pull request for creating a new integration package. labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.1 candidate enhancement New feature or request Integration:microsoft_sqlserver Microsoft SQL Server New Integration Issue or pull request for creating a new integration package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSSQL Audit Integration
6 participants