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

skip allow_superuser in Windows OS #16629

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

kaisecheng
Copy link
Contributor

Release notes

[rn:skip]

What does this PR do?

As Process.euid() is always zero in Windows,
this commit excluded the checking of running as root in Windows.

Why is it important/What is the impact to the user?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

As euid() is always zero in Windows,
this commit excluded the checking of running as root in Windows.
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@kaisecheng
Copy link
Contributor Author

windows buildkite is green

@@ -320,7 +320,7 @@
#
# ------------ Other Settings --------------
#
# Allow or block running Logstash as superuser (default: true)
# Allow or block running Logstash as superuser (default: true). Windows are excluded from the checking
# allow_superuser: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we agree that we are not going to support this feature on Windows?

Copy link
Member

Choose a reason for hiding this comment

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

If there is a desire to generally determine if a process is running as an admin or with elevated status on windows there is a pattern in Puppet for doing so https://github.com/puppetlabs/puppet/blob/e758d5c969403631810fa6385057ef0eaf03974f/lib/puppet/util/windows/user.rb#L11 though it is fairly complex and carries a dependency on ffi which requires native extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mashhurs My goal here is to unblock the exhaustive test, not to bring the feature to windows
I believe it is another PR to fix the original handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow up issue to fix windows handling #16632

@kaisecheng kaisecheng changed the title handle allow_superuser in Windows OS skip allow_superuser in Windows OS Nov 4, 2024
@kaisecheng kaisecheng requested a review from mashhurs November 4, 2024 23:28
@kaisecheng kaisecheng linked an issue Nov 5, 2024 that may be closed by this pull request
Copy link
Contributor

@mashhurs mashhurs left a comment

Choose a reason for hiding this comment

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

LGTM!
Feature bug will be fixed/handlied with #16632

@kaisecheng kaisecheng merged commit 5847d77 into elastic:main Nov 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Fix exhaustive test and DRA build
4 participants