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

Gevent ssl traceback #1298

Merged
merged 3 commits into from
Jul 6, 2021
Merged

Gevent ssl traceback #1298

merged 3 commits into from
Jul 6, 2021

Conversation

mssalvatore
Copy link
Collaborator

What does this PR do?

Fixes #859

The gevent hub has 2 mechanisms for controlling how errors (tracebacks) are handled.

  1. The exception_stream controls where tracebacks are written (default=stderr): https://www.gevent.org/api/gevent.hub.html#gevent.hub.Hub.exception_stream
  2. The handle_error() method controls what happens when an error is encountered: https://www.gevent.org/api/gevent.hub.html#gevent.hub.Hub.handle_error

If exception_stream is set, the user may not get any indication that a gevent error occurred. To solve this, the handle_error() functionality is wrapped and the exception (not traceback) is logged to WARNING.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running monkey island

  • If applicable, add screenshots or log transcripts of the feature working

Screenshots

image

@mssalvatore mssalvatore mentioned this pull request Jul 5, 2021
1 task
@mssalvatore mssalvatore force-pushed the gevent-ssl-traceback branch from 7684120 to 16de12b Compare July 5, 2021 16:26
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #1298 (16de12b) into develop (19e9fe5) will decrease coverage by 0.04%.
The diff coverage is 4.54%.

❗ Current head 16de12b differs from pull request most recent head 96fc330. Consider uploading reports for the commit 96fc330 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1298      +/-   ##
===========================================
- Coverage    30.68%   30.63%   -0.05%     
===========================================
  Files          449      450       +1     
  Lines        13467    13489      +22     
===========================================
+ Hits          4132     4133       +1     
- Misses        9335     9356      +21     
Impacted Files Coverage Δ
monkey/monkey_island/cc/server_setup.py 0.00% <0.00%> (ø)
...monkey_island/cc/setup/gevent_hub_error_handler.py 0.00% <0.00%> (ø)
monkey/monkey_island/cc/server_utils/consts.py 92.85% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19e9fe5...96fc330. Read the comment docs.

@@ -54,6 +58,7 @@ def run_monkey_island():

connect_to_mongodb()

_configure_gevent_exception_handling(logger, Path(config_options.data_dir))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass logger as an argument here? Won't it just be able to use logger because of logger = logging.getLogger(__name__) above?

# Send gevent's exception output to a log file.
# https://www.gevent.org/api/gevent.hub.html#gevent.hub.Hub.exception_stream
hub.exception_stream = gevent_exception_log
hub.handle_error = GeventHubErrorHandler(hub, logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass logger as an argument here? Why not have logger = logging.getLogger(__name__) in monkey_island/cc/setup/gevent_hub_error_handler.py?

Copy link
Collaborator Author

@mssalvatore mssalvatore Jul 6, 2021

Choose a reason for hiding this comment

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

So that all of the gevent objects (i.e. Hub and WSGIServer) have the same logger. It also has the added benefit of making the dependencies of GeventHubErrorHandler explicit. Unlike other classes, this class doesn't use a logger to log information about itself. Rather, it uses a logger object as part of an exception handling routine on behalf of another component. In other words, logging is part of this component's primary function, so it should be explicit in its interface/dependencies.

By default, gevent prints exceptions and tracebacks to stderr. This is
obnoxious as it results in large tracebacks intermixed with the output
that the logger prints to the console. This commit redirects this data
to {DATA_DIR}/gevent_exceptions.log. Unfortunately, this would mean that
the user might be left without any indication these exceptions had
occurred, unless they take the time to inspect the
gevent_exceptions.log. Therefore, when an excepion occurs, a message
with just the exception (not the traceback) is logged to WARNING.

Fixes #859
@mssalvatore mssalvatore force-pushed the gevent-ssl-traceback branch from 16de12b to 96fc330 Compare July 6, 2021 12:39
@mssalvatore mssalvatore merged commit 832704d into develop Jul 6, 2021
@mssalvatore mssalvatore deleted the gevent-ssl-traceback branch July 29, 2021 14:14
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.

SSL warnings
2 participants