-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[AI-5055] DDS: OpenVPN Integration V1.0.0 #19811
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
Review from iliakur is dismissed. Related teams and files:
- agent-integrations
- openvpn/assets/dashboards/openvpn_overview.json
openvpn/assets/logs/openvpn.yaml
Outdated
rule3 %{regex("(.*?)(?=:)")}:(\s+)?\'%{notSpace} %{notSpace} %{regex("Web login authentication failed"):log_type}: \{\\'status\\': %{integer:status}, \\'user\\': \\'%{regex("(.*)(?=\\\\')"):user}\\', \\'reason\\': "%{regex("(.*)(?=\")"):reason}", \\'auth method\\': \\'%{regex("(.*?)(?=\\\\')"):auth_method}\\'}' | ||
|
||
|
||
rule4 %{regex("(.*?)(?=:)")}:(\s+)?\'%{notSpace} %{notSpace} %{regex("Web login authentication failed"):log_type}: \{\\'status\\': %{integer:status}, \\'user\\': \\'%{regex("(.*)(?=\\\\')"):user}\\', \\'reason\\': "%{regex("(.*)(?=\")"):reason}"}' |
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.
These rules have some significant overlap and could lead to fully parsing the logs multiple times. Can you reduce this redundancy?
Also, some of these seem to be JSON or keyvalue based, for example the status
and user
fields in rule3. Could those be extracted and remapped into the expected log fields rather than using complex parsers and regex?
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.
We are checking on this and will update soon.
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.
- We have created two Grok parsers to handle the parsing of web authentication logs—one for the headers and another for key-value format. This separation helps eliminate overlap and reduces redundant parsing.
- For
auth method
field, we observed that keys containing spaces were not correctly extracted by thedata::keyvalue
parser. To address this, we created static parsing rules. - Additionally, fields such as
user
andstatus
were escaped as 'user', which prevented them from being parsed correctly by thedata::keyvalue
parser. To handle this, we added static rules specifically to extract these escaped fields.
matchRules: >- | ||
rule1 \[%{regex("(.*?)(?=])")}\](\s+)?%{regex("AUTH | ||
SUCCESS"):log_type}\s+\{%{data::keyvalue(": ",", ")}, 'auth | ||
method': '%{regex("(.*?)(?=\\')"):auth_method}', |
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.
Similar here - it looks like 'auth method'
is a key in a JSON-like structure. Could this be parsed out into an object and then extract the value for the key you're interested in?
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.
Can you please help us understand this comment better?
@gunterd We were not able to access this PR yesterday. Let me have a look at the comments and get back to you. |
openvpn/assets/logs/openvpn.yaml
Outdated
grok: | ||
supportRules: "" | ||
matchRules: >- | ||
rule1 \[%{regex("(.*?)(?=])")}\](\s+)?%{regex("AUTH |
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.
To reduce some of the regex burden here, can you replace \[%{regex("(.*?)(?=])")}\]
with (\[\-\])
?
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.
We have used [%{regex("(.*?)(?=])")}] as a safekeeping to ensure that grok parser doesn't fail if different value comes in between [].
Based on our testing, we have only observed [-] hence we have updated regex to ([-]) for better efficiency.
authentication failed: {'status': 1, 'user': 'openvpn', 'reason': | ||
'local auth failed: password verification failed'}\"" | ||
- "[-] [WEB] OUT: '2025-03-11T12:59:46+0000 [stdout#info] Web login | ||
authentication failed: {\\'status\\': 2, \\'user\\': \\'abc\\', |
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 these logs come in with the backslashes escaped?
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, we are getting those logs with backslashes escaped.
PR Security UpdateAll commits in this PR up to and including b9cc37b have been reviewed and marked safe by SDLC security. For any questions, please reach out to #ci-for-external-contributors-collab on Slack. |
* Add: OpenVPN integration * Update: labeler for openvpn * Updated tests yaml file * updated test yaml file * Updated changelog number * Updated minor changes * Updated as per PR comments * Update: address review comments * Update: address review comments * Update: minor changes and move openvpn logo * Update: remove filter from Datadog Cloud SIEM group * Update: CODEOWNERS * Update: CODEOWNER add tag * Update: address review comments * Update: logs sample * Update: Address review comments --------- Co-authored-by: manan-crest <[email protected]> 0b4ffe8
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.
Need an update on the copy of the dashboard. Comment added.
"id": 6980405278649262, | ||
"definition": { | ||
"type": "note", | ||
"content": "OpenVPN is a free, open-source protocol that creates secure connections between devices over the internet. It's used to create virtual private networks (VPNs).\n\n\nThe OpenVPN Overview dashboard provides an overall insights of the logs generated by OpenVPN.\n\n\nFor more information, see the [OpenVPN Integration Documentation](https://docs.datadoghq.com/integrations/openvpn/).\n\n**Tips**\n- Use the timeframe selector in the top right of the dashboard to change the default timeframe.\n- Clone this dashboard to rearrange, modify and add widgets and visualizations. ", |
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.
Please update copy here.
Current: The OpenVPN Overview dashboard provides an overall insights of the logs generated by OpenVPN.
Proposed: The OpenVPN Overview dashboard provides insights into the logs generated by OpenVPN.
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.
@jnhunsberger Have made the suggested changes in below raised PR.
PR Link: <link>
What does this PR do?
PR for a new integration OpenVPN 1.0.0
Additional Notes
-- OOTB detection rules JSON would be shared separately with the required teams as a part of separate repository .
-- Since during the standard attribute remapping we are not preserving the source attributes as per suggested best practices, it would result in filters using these standard attributes populating the values of other integrations as well as per current datadog behavior.
Review checklist (to be filled by reviewers)