Fixes #28384 - Allow hiding stacktraces in the UI #7220
Conversation
|
Issues: #28384 |
There was a problem hiding this comment.
I am wondering how useful this really is. We still want to make it easy to collect failures and the Something went wrong isn't very helpful.
It is common to generate some error code that you can also look up in the log (as an admin). That at least gives some way of finding out what happened.
Other than that: it's an open source project. The source is available and the version you're on is exposed anyway. Then it's trivial to get an equivalent install. That obfuscation isn't very useful IMHO.
|
@ekohl while I agree the value is very small given the traceback does not tell anything people wouldn't know, if we add this setting (default is keeping current behavior), we'll no longer need to argue with users who disagree. We'll only tell them, switch the setting. Given this is a minor change and it will make life easier for users with strict (and sometimes useless) security audits, plus it does not change it for majority of users, I'm fine with it. |
|
I consider this more of a UX thing than a security thing. Also something we should solve. The traceability is something I'd like to keep. In smart-proxy we generate error codes based on the exception. I'd propose we do something similar here: show the user some error code. They can then go to their Foreman admin and say 'while doing x and y I got error e'. The admin can go to the log, search for the error code and report a bug. For the end user there's the added value that it feels much more concrete than only a generic 'Something went wrong'. However, I can't really find how we generate those codes now so I can't link it. Thoughts? |
|
The problem with generic error can be solved with showing only with numeric code like 500 or 404 but I think it's not a good UX way. But I think it will be desirable solution to show a error message (like User with id='' not found) but not show stacktrace (when is disabled). Is it a good idea? @ekohl |
|
With error codes I don't mean HTTP error codes. I'm referring to https://projects.theforeman.org/projects/foreman/wiki/ErrorCodes. Looks like it's defined in foreman.git: foreman/lib/foreman/exception.rb Lines 8 to 14 in ac1f161 |
|
I agree with @ekohl :
We have rejected multiple such requests in the past. users who want to adhere with useless security requirements are free to edit this file on their own. Security through obscurity is not security. Foreman is always installed in the same folder structure, there is no information disclosed in the stack trace that can't be gained by simply installing foreman. If I'm not mistaken, it is even not possible to get to a stack trace without being logged in. 👎 in my opinion to "fixing" the issue rather than rejecting it. |
|
Let me chip in here. Both arguments that Foreman is an open source project are irrelevant - we've accepted several patches in the past to make harder for attacker to find out Foreman version. Stacktrace is a valuable asset for attackers mainly to determine which version of the software is installed, you can usually tell from the paths of the gem dependencies in the stacktrace. This is, without a doubt, a security issue that must be fixed no matter if we are open source project or not. Now, I don't like another setting honestly. I believe this should work "out of box", therefore I believe the best user experience is to adopt one of the golden standards we see for other projects, services or products sorted from worse to best IMHO:
I think the last one is the best one, also easy to implement because nothing needs to be actually stored. We have a (secure) request ID stored in our logging MDC, this can be shown and administrators can easily look this up in the Foreman log. Plans are also to show these in the Audits page in the future so experience is also good for the operator. Maybe even combination (request id + super users do see whole backtrace immediately) could be a nice way too. Hope this helps to put some light into the problem. |
Is that the same as I was describing or are you suggesting to generate a unique per per request (UUID or similar)? |
Both would work, I slightly lean towards simply displaying the request id as it's safe to publish we generate it randomly and it's easy for admins to work with (just grep production.log or elasticsearch). |
|
I'm happy with either generic error code or request ID or both. The current implementation only shows "something failed" which is IMHO insufficient because it makes tracking very hard when it does go wrong. |
|
I'd like to see both there, @domitea how does that sound? |
|
I'd show whole message (the error) that's fine. What should be kept in the secret is just the stack trace. Request ID is randomized so no problem showing that. If you want to take it to the extreme, you could also create hash of backtrace and show that too - that should be safe as well. However, you'd need to convert absolute filenames to relative to Rails.root because people tend to install Foreman also from gems to various locations. |
|
I guess i can live with having the message give clear instructions on how to get the stack trace from the log instead of printing it out. but I'm not sure it would be so easy - iirc the request id is only displayed on the first line of the stack, not all lines. If we could show a massage such as "to see the full details run I still don't agree that this is an information disclosure as there is no sensitive information in the stack trace. Any user can go to the about page and see what foreman version they are on and what plugins are enabled, and from that it is trivial to know which gems are installed where. |
|
I'd also still show track traces when the rails environment is not production. |
This occurs anyways, the 500 page is only rendered in production: https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L9 |
This is a good suggestion, let's tell the user how to get the details.
I do agree it's not serious problem, however this PR exists because many security auditors do believe, it was exploited many times in the past. If we would not show Foreman version on the login page, attacker would not know which version was deployed at all and stacktrace can possibly give good hints. I think it's not good practice to reveal version to public via login page but nobody complained yet. |
To review:
In development mode, you can also show whole stacktrace of course. Do not introduce any setting please. I think this is what we all agree on, correct me if I am wrong. |
I'm afraid some auditors might also see mentioning the log location as an information leak. It also doesn't help the user who sees that error message. From the principle that you should only show information to the user that they can act on, I'd leave it out. Additionally, we should write documentation on dealing with error reports. We can't tell users to deploy it, but sentry is normally a proper solution to this. It can group exceptions and gather a lot of context from the request making it easier to dive into the bug. Since it's all automated on the back end, it doesn't require the end users to report bugs to the admins. |
I beg to differ here. |
I agree, I also don't understand how log file location adds any information to black hats. Also we made sure the request id differs from the Rails request id (or session id) so it's safe (just a random nonsense). |
|
Fair enough, I just wanted to raise the concern. On a related note, just yesterday I had a chat with @iNecas about getting useful bug reports with sufficient context. Out of scope for this PR, but an area we have a lot of potential to improve. |
|
Summary of today discussion on the scope with @tbrisker, @domitea and myself:
We also discussed the way how to get the backtrace. Backtrace has request id only on the first line. It would be great to add the request id to every line or do something like |
domitea
force-pushed
the
28384_added-hide-stacktrace
branch
from
f6ef03f
to
e5ac11c
Compare
app/views/common/500.html.erb
Outdated
| <%= _("You would probably need to attach the") %> | ||
| <%= link_to_function(_("Full trace"), "$('#backtrace').toggle()")%> | ||
| <%= _(", relevant log entries, and it is highly recommended to also attach the foreman-debug output.") %> | ||
| <%= link_to _("Foreman ticketing system"), "https://projects.theforeman.org/", :rel => "external" %>, |
There was a problem hiding this comment.
This still applies, please change.
lib/foreman/logging.rb
Outdated
| @@ -86,7 +86,7 @@ def exception(context_message, exception, options = {}) | |||
| options.assert_valid_keys :level, :logger | |||
| logger_name = options[:logger] || 'app' | |||
| level = options[:level] || :warn | |||
| backtrace_level = options[:backtrace_level] || :debug | |||
| backtrace_level = options[:backtrace_level] || :warn | |||
There was a problem hiding this comment.
Whatever, it makes more sense to show them as we advice for searching them.
domitea
force-pushed
the
28384_added-hide-stacktrace
branch
from
e5ac11c
to
77e8b4a
Compare
|
@domitea please rebase this on latest develop to see if test failures are related |
domitea
force-pushed
the
28384_added-hide-stacktrace
branch
from
77e8b4a
to
e4c3062
Compare
lib/foreman/logging.rb
Outdated
| @@ -86,7 +86,7 @@ def exception(context_message, exception, options = {}) | |||
| options.assert_valid_keys :level, :logger | |||
| logger_name = options[:logger] || 'app' | |||
| level = options[:level] || :warn | |||
| backtrace_level = options[:backtrace_level] || :debug | |||
| backtrace_level = options[:backtrace_level] || :warn | |||
There was a problem hiding this comment.
Please make this info and not warn so users can filter out backtraces if needed.
domitea
force-pushed
the
28384_added-hide-stacktrace
branch
from
e4c3062
to
b8cb00a
Compare
|
@tbrisker Code updated. |
| <%= link_to _("Foreman ticketing system"), "https://projects.theforeman.org/projects/foreman/issues", :rel => "external" %>, | ||
| <% if Foreman::Logging.config[:type] == "file" %> | ||
| <%= _("Please include in your report the full error log that can be acquired by running: ") %> | ||
| <strong> foreman-rake errors:fetch_log request_id=<%= request_id %></strong> |
There was a problem hiding this comment.
Wouldn't a <pre> block be better for a code sample?
| ex_message = exception.message | ||
| Foreman::Logging.exception(ex_message, exception) | ||
| full_request_id = request.request_id | ||
| render :template => "common/500", :layout => !request.xhr?, :status => :internal_server_error, :locals => { :exception_message => ex_message, request_id: full_request_id.split('-').first, full_request_id: full_request_id} |
There was a problem hiding this comment.
full_request_id.split('-').first logic is duplicated here with MultilineRequestPatternLayout. I wonder if there's a way to avoid that and have a request.short_request_id.
| puts "You can search the logs in #{type} for request_id #{request_id}" | ||
| exit(1) | ||
| end | ||
| file_path = File.join(Foreman::Logging.log_directory, logfile) |
There was a problem hiding this comment.
Does it make sense to check if logfile is an absolute path and not join then?
|
@ekohl thanks for the feedback, since this PR has been open for so long I'd rather get it in and reiterate on resolving these issues in follow-up prs |
|
When I get a HTTP 500 from the API, can I also find the short request ID? It could help to provide instructions in https://projects.theforeman.org/issues/30284. |
It can probably be added to: |
Added ability in setting to set to allow to show a stacktrace at error pages