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.

Sign up for GitHub

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

Jump to bottom

add deprecation warning for allow_superuser: true #16555

Open
wants to merge 2 commits into
base: 8.x
Choose a base branch
from
Open

add deprecation warning for allow_superuser: true #16555

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e0397a
add deprecation warning for `allow_superuser: true`
kaisecheng Oct 15, 2024
8451aeb
log warning instead of deprecation
kaisecheng Oct 25, 2024
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
11 changes: 10 additions & 1 deletion logstash-core/lib/logstash/runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,17 @@ def execute

def running_as_superuser
if Process.euid() == 0
unless @settings.set?("allow_superuser")
deprecation_logger.deprecated("WARNING: You are currently running Logstash with superuser privileges. " +
"Starting from version 9, this will be disabled by default. " +
"To avoid disruption during the upgrade, set 'allow_superuser' to true now if you wish to continue running as superuser temporarily after the upgrade. " +
"Note that this is not recommended for security reasons.")
Comment on lines +486 to +489
Copy link
Contributor

Choose a reason for hiding this comment

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

I would a bit simplify the statement.

Suggested change
deprecation_logger.deprecated("WARNING: You are currently running Logstash with superuser privileges. " +
"Starting from version 9, this will be disabled by default. " +
"To avoid disruption during the upgrade, set 'allow_superuser' to true now if you wish to continue running as superuser temporarily after the upgrade. " +
"Note that this is not recommended for security reasons.")
deprecation_logger.deprecated("WARNING: You are currently running Logstash with superuser privileges which is not recommended for security reasons. " +
"Starting from version 9, this will not be allowed by default unless 'allow_superuser' is intentionally set to true.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone could you suggest how to do the deprecation communication? We want to suggest user to set 'allow_superuser' to true now for preparing v9 upgrade. The setting is optional at this moment, I am not sure how to deliver the message.

Copy link
Member

Choose a reason for hiding this comment

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

@karenzone We are changing the behaviour when someone attempts to run Logstash as a superuser.

Running Logstash as a superuser is not recommended for security reasons, as it breaks the "Principle of Least Privilege", and increases any potential attack surface.

The current behaviour is to allow users to run as as a superuser by default, adding a setting allow_superuser:false to lock that down that has to be done specifically, potentially leading to a situation where Logstash could be run as root inadvertently.

In 9.0, we are going to switch the behaviour such that it has to be a conscious decision to allow Logstash to be run as a "superuser" that the user has to allow explicitly by setting allow_superuser to true

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the ping and clarification, @kaisecheng and @robbavey. I'll take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the question and the additional clarification. It seems like @mashhurs is on the right track with his suggestion. I started with his suggestion and iterated on it.

Does this achieve your goals:

"Starting from version 9, running with superuser privileges is not permitted unless you explicitly set 'allow_superuser' to true, thereby acknowledging the possible security risks."

If not, please let me know and we'll get it right.

end

if setting("allow_superuser")
deprecation_logger.deprecated("NOTICE: Running Logstash as superuser is not recommended and won't be allowed in the future. Set 'allow_superuser' to 'false' to avoid startup errors in future releases.")
deprecation_logger.deprecated("NOTICE: Running Logstash as superuser will be completely disallowed in future versions. " +
"To prepare for this and avoid startup errors in future releases, it is strongly recommended to set 'allow_superuser' to false now " +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone please also consider this message. Both deprecation logs can print together.

"and adjust your configuration accordingly.")
else
raise(RuntimeError, "Logstash cannot be run as superuser.")
end
Expand Down
4 changes: 2 additions & 2 deletions logstash-core/spec/logstash/runner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@
it "runs successfully with warning message" do
LogStash::SETTINGS.set("allow_superuser", true)
expect(logger).not_to receive(:fatal)
expect(deprecation_logger_stub).to receive(:deprecated).with(/NOTICE: Running Logstash as superuser is not recommended and won't be allowed in the future. Set 'allow_superuser' to 'false' to avoid startup errors in future releases./)
expect(deprecation_logger_stub).to receive(:deprecated).with(/NOTICE: Running Logstash as superuser will be completely disallowed in future versions./)
expect { subject.run(args) }.not_to raise_error
end
end
Expand All @@ -716,7 +716,7 @@
it "runs successfully without any messages" do
LogStash::SETTINGS.set("allow_superuser", false)
expect(logger).not_to receive(:fatal)
expect(deprecation_logger_stub).not_to receive(:deprecated).with(/NOTICE: Running Logstash as superuser is not recommended and won't be allowed in the future. Set 'allow_superuser' to 'false' to avoid startup errors in future releases./)
expect(deprecation_logger_stub).not_to receive(:deprecated).with(/NOTICE: Running Logstash as superuser will be completely disallowed in future versions./)
expect { subject.run(args) }.not_to raise_error
end
end
Expand Down