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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/logstash.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

#
# Where to find custom plugins
Expand Down
2 changes: 2 additions & 0 deletions logstash-core/lib/logstash/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,8 @@ def execute
end # def self.main

def running_as_superuser
return if LogStash::Environment.windows? # windows euid always returns 0, skip checking

if Process.euid() == 0
if setting("allow_superuser")
logger.warn("NOTICE: Allowing Logstash to run as superuser is heavily discouraged as it poses a security risk. " +
Expand Down
68 changes: 39 additions & 29 deletions logstash-core/spec/logstash/runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -574,41 +574,51 @@
let(:deprecation_logger_stub) { double("DeprecationLogger").as_null_object }
before(:each) { allow(runner).to receive(:deprecation_logger).and_return(deprecation_logger_stub) }

context "unintentionally running logstash as superuser" do
before do
expect(Process).to receive(:euid).and_return(0)
if LogStash::Environment.windows?
context "unintentionally running logstash as superuser" do
it "runs successfully" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).not_to receive(:fatal)
expect { subject.run(args) }.not_to raise_error
end
end
it "fails with bad exit" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).to receive(:fatal) do |msg, hash|
expect(msg).to eq("An unexpected error occurred!")
expect(hash[:error].to_s).to match("Logstash cannot be run as superuser.")
else
context "unintentionally running logstash as superuser" do
before do
expect(Process).to receive(:euid).and_return(0)
end
it "fails with bad exit" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).to receive(:fatal) do |msg, hash|
expect(msg).to eq("An unexpected error occurred!")
expect(hash[:error].to_s).to match("Logstash cannot be run as superuser.")
end
expect(subject.run(args)).to eq(1)
end
expect(subject.run(args)).to eq(1)
end
end

context "intentionally running logstash as superuser " do
before do
expect(Process).to receive(:euid).and_return(0)
end
it "runs successfully with warning message" do
LogStash::SETTINGS.set("allow_superuser", true)
expect(logger).not_to receive(:fatal)
expect(logger).to receive(:warn).with(/NOTICE: Allowing Logstash to run as superuser is heavily discouraged as it poses a security risk./)
expect { subject.run(args) }.not_to raise_error
context "intentionally running logstash as superuser " do
before do
expect(Process).to receive(:euid).and_return(0)
end
it "runs successfully with warning message" do
LogStash::SETTINGS.set("allow_superuser", true)
expect(logger).not_to receive(:fatal)
expect(logger).to receive(:warn).with(/NOTICE: Allowing Logstash to run as superuser is heavily discouraged as it poses a security risk./)
expect { subject.run(args) }.not_to raise_error
end
end
end

context "running logstash as non-root " do
before do
expect(Process).to receive(:euid).and_return(100)
end
it "runs successfully without any messages" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).not_to receive(:fatal)
expect(logger).not_to receive(:warn).with(/NOTICE: Allowing Logstash to run as superuser is heavily discouraged as it poses a security risk./)
expect { subject.run(args) }.not_to raise_error
context "running logstash as non-root " do
before do
expect(Process).to receive(:euid).and_return(100)
end
it "runs successfully without any messages" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).not_to receive(:fatal)
expect(logger).not_to receive(:warn).with(/NOTICE: Allowing Logstash to run as superuser is heavily discouraged as it poses a security risk./)
expect { subject.run(args) }.not_to raise_error
end
end
end
end
Expand Down