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 [#8647]: mark linux integrations requiring root #8917

Merged
merged 1 commit into from Mar 26, 2024

Conversation

pkoutsovasilis
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis commented Jan 17, 2024

This PR marks the appropriate linux integrations of [#8647] as requiring or not root

Related Issues

@pkoutsovasilis pkoutsovasilis requested review from a team as code owners January 17, 2024 17:16
@elasticmachine
Copy link

elasticmachine commented Jan 17, 2024

🚀 Benchmarks report

Package auditd_manager 👍(0) 💚(0) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
auditd 10989.01 7092.2 -3896.81 (-35.46%) 💔

Package system_audit 👍(0) 💚(0) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
package 58823.53 47619.05 -11204.48 (-19.05%) 💔

To see the full report comment with /test benchmark fullreport

@norrietaylor norrietaylor requested review from a team and andrewkroh January 18, 2024 15:12
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.

Each package with changes will need a changelog entry. You can do it with the CLI via elastic-package change add.

@@ -60,6 +60,9 @@ policy_templates:
responses:
- match: [executableChanges]
actions: [alert]
agent:
privileges:
root: false
Copy link
Member

Choose a reason for hiding this comment

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

With false being the default this could be entirely omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure my aim was explicitness and also kinda a notification to the sec-linux-platform team that this is getting marked as not requiring root. But what you say makes sense and I will omit that entirely

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 do suggest changing the description field to start with a capital and end with punctuation because these become user facing in our documentation (example https://docs.elastic.co/en/integrations/crowdstrike#changelog).

@botelastic
Copy link

botelastic bot commented Mar 2, 2024

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Mar 2, 2024
@pkoutsovasilis pkoutsovasilis force-pushed the pkoutsovasilis/root_linux_integrations branch from c110ae1 to 435f240 Compare March 26, 2024 16:03
@botelastic botelastic bot removed the Stalled label Mar 26, 2024
@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Mar 26, 2024

@andrewkroh please one more review 🙂

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

@pkoutsovasilis pkoutsovasilis merged commit 16b2650 into main Mar 26, 2024
5 checks passed
@pkoutsovasilis pkoutsovasilis deleted the pkoutsovasilis/root_linux_integrations branch March 26, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark appropriate linux integrations as requiring root.
4 participants