arachnys / cabot

Self-hosted, easily-deployable monitoring and alerts service - like a lightweight PagerDuty
MIT License
5.59k stars 594 forks source link

Cabot sends `None` string in HTTP status checks in auth header for username or password #590

Closed davidjb closed 6 years ago

davidjb commented 6 years ago

Due to non-obvious behaviour in requests (https://github.com/requests/requests/issues/4465), the way in which Cabot currently constructs its auth for HTTP status checks results in the string of None being sent as part of Authorization header.

In short, because the Python object None gets passed into the auth tuple in https://github.com/arachnys/cabot/blob/master/cabot/cabotapp/models/base.py#L760, requests takes this and converts it to a string, resulting in an Authorization header that decodes to my_api_key_orusername:None, which then breaks authentication for URLs/APIs (like GitHub) that only require a username.

In this case, the solution is to ensure requests.get is passed an a empty string for the password if one isn't specified. To get to this point, I can see two options: either this PR, which is a basic check for the object of None, or otherwise modify the Django models & database to use an empty string (ala https://docs.djangoproject.com/en/2.0/ref/models/fields/#null).

The former is simplest as it doesn't change the data model and/or require migration; but the latter is probably the cleaner of the two options.

codecov[bot] commented 6 years ago

Codecov Report

Merging #590 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #590   +/-   ##
=======================================
  Coverage   80.99%   80.99%           
=======================================
  Files          45       45           
  Lines        2920     2920           
  Branches      177      177           
=======================================
  Hits         2365     2365           
  Misses        497      497           
  Partials       58       58
Impacted Files Coverage Δ
cabot/cabotapp/models/base.py 79.52% <0%> (ø) :arrow_up:

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 9ba585d...bca6ee1. Read the comment docs.

hartwork commented 6 years ago

Can you rebase against latest master? It may also get the CI back to green. Thanks!

davidjb commented 6 years ago

@hartwork Rebased accordingly! CI is in progress...

dbuxton commented 6 years ago

I guess my question is: are there likely to be cases where the username is None but the password is used as the API key? In which case do we want to do the same for self.username?

davidjb commented 6 years ago

@dbuxton A fine plan. Looking at the source code for requests (https://github.com/requests/requests/blob/master/requests/auth.py#L38) shows that non-string usernames and passwords will be no longer supported in Requests 3.0.0. Cabot may not use that version yet but this updated PR avoids the same problem with empty usernames.