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

Fix #33: Add X-Watchman-Version header to responses #35

Merged
merged 2 commits into from
Jul 2, 2015

Conversation

mwarkentin
Copy link
Owner

  • Add response header
  • Add tests for dashboard / watchman json views
  • Update history.rst

Response headers (json)

Connection:keep-alive
Content-Encoding:gzip
Content-Type:application/json
Date:Thu, 02 Jul 2015 19:06:48 GMT
Server:nginx/1.0.15
Transfer-Encoding:chunked
Vary:Accept-Encoding
X-Watchman-Version:0.6.0a0

Response headers (dashboard)

Connection:keep-alive
Content-Encoding:gzip
Content-Type:text/html; charset=utf-8
Date:Thu, 02 Jul 2015 19:31:34 GMT
Server:nginx/1.0.15
Transfer-Encoding:chunked
Vary:Accept-Encoding
X-Watchman-Version:0.6.0a0

* Add response header
* Add tests for dashboard / watchman json views
* Update history.rst
'checks': check_types,
'overall_status': overall_status
})

response[WATCHMAN_VERSION_HEADER] = __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

that feels so hacky but I know of no better way 🌟

Copy link
Owner Author

Choose a reason for hiding this comment

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

@JBKahn
Copy link
Contributor

JBKahn commented Jul 2, 2015

👍

Conflicts:
	HISTORY.rst
	watchman/views.py
mwarkentin added a commit that referenced this pull request Jul 2, 2015
Fix #33: Add `X-Watchman-Version` header to responses
@mwarkentin mwarkentin merged commit 2985ba7 into master Jul 2, 2015
@mwarkentin mwarkentin deleted the 33-watchman-metadata branch July 2, 2015 20:35
@blag
Copy link
Contributor

blag commented Jul 2, 2015

I'm curious - what was the motivation for this?

A long, long time ago [in a galaxy far away] PHPBB included its version number in the footer at the bottom of it's pages. Then arose a character [aligned with the dark side], who wrote a novel piece of malware that would infect vulnerable versions of PHPBB by automatically Googling for the vulnerable version strings, and then attack them [with lightsabers]. Eventually [the prophecy of] a more secure PHPBB version came to pass, which removed its version string from the footer and fixed the exploit, and the malware was stopped [by a well-aimed shot into one of its exhaust ports].

So, if an attacker can get the version of watchman, they can limit their known range of versions of Django (eg: version x.y.z of watchman only supports version a.b.c through d.e.f of Django). Given this information, an attacker can refine their attack strategy, allowing them to attempt fewer exploits than they would otherwise. That, in turn, makes it easier to evade IDS systems.

Basically: version information disclosure to malicious parties violates defense-in-depth guidelines.

Now, this is only an issue when either of the following is true:

  1. there is no authentication done on the status or dashboard endpoints
  2. a malicious party has access to the status or dashboard endpoints

The first is unlikely yet possible with PR #30 if the authentication/authorization decorator is flawed, and the second is a misplaced trust issue to being with.

There are also a few ameliorating factors:

  1. Python is leaps and bounds more readable and debuggable than PHP.
  2. Django is leaps and bounds more readable, debuggable, and secure than PHPBB.
  3. PHP sucks. Python rocks.
  4. PHP developers tend to suck.
  5. Exactly 0% of this project is written in PHP.
  6. Did I mention that PHP is a terrible, bug-ridden, exploit-prone language that makes it difficult to understand due to its minutiae and impossible with which to write secure code from the outset?

Given all of that, this isn't a huge deal, I just want to recognize that this PR creates a small, mild defense-in-depth violation for essentially what are misconfigured systems already. Making a note about this issue in the documentation for watchman might be a good idea to help users avoid this potential pitfall. But I think "secure by default" should be the accepted practice, so I would also suggest turning off this feature by default, and letting users turn it on if they wish.

This comment kind of got away from me and I apologize for the terrible timing and the length. 😄

Your thoughts?

@mwarkentin
Copy link
Owner Author

Welp, that was quite a read. 😄

Basically, this feature came up as a result of managing about 10 or so systems, and those systems running 3 different versions of watchman - there were some questions about the names of the checks to use in the GET query params, which had changed between 0.4.0 and 0.5.0. It was pretty annoying to figure out which version of watchman was running where - basically came down to a github search, or ssh-ing into 10 different boxes to check.

I figured this was something safe to enable as watchman is a read-only service, and I figured most people would've used the token auth to restrict access.

However, you bring up a good point - I'll open an issue to track it / continue discussion.

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.

3 participants