880354 - implement whitelist of domains that updates from balrog can point to

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

At the Balrog security review we talked about having a whitelist of hosts that Balrog updates may point at. Having this reduces the attack surface in the event that a bad person gains access to the Balrog admin interface. Eg, they could only serve people a malicious update if they also have access to download.mozilla.org or ftp.

This whitelist will have to live outside of the database (presumably in the public facing app's config file) for it to be effective.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: domain whitelisting

This follows the implementation of the special hosts a little bit, but it's consumed createSnippet and createXML. Pretty simple patch, I think.

I also changed the test_AUS.py tests to use an in-memory database, because AFAICT they have no need to use a slow, real file one.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 762327 [details] [diff] [review]
domain whitelisting

I think this look fine, but wonder if we should also have something on the API side that refuses to update blobs with domains not in the whitelist.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #2)
> Comment on attachment 762327 [details] [diff] [review]
> domain whitelisting
> 
> I think this look fine, but wonder if we should also have something on the
> API side that refuses to update blobs with domains not in the whitelist.

I thought a little bit about that too. My brain came up with some vague scenario in which we'd want to add a release with a non-whitelisted URL ahead of time, and then whitelist it later, before we ship it. I don't have a concrete use case for that, though. It would be pretty easy/cheap to add that check I think...

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

I think we'd probably prefer to deploy a whitelist change away from the stress of deploying updates, and I can't think of a downside to enabling early. An API check would also prevent serving no updates when random bad dude tries to update blob to their own server.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #4)
> I think we'd probably prefer to deploy a whitelist change away from the
> stress of deploying updates, and I can't think of a downside to enabling
> early. An API check would also prevent serving no updates when random bad
> dude tries to update blob to their own server.

Good points. Adding a release not in the whitelist would also be a good candidate for notifying on (see bug 880354). I'll take a stab at this.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: support domain whitelisting on admin+public interfaces

Here's what I think is a complete patch. Changes since the original include:
* Read whitelists from the configs, and allow for them to be set by admin.py/AUS-serve.py.
* Make AUSDatabase the holder of the whitelist (the Releases table also has one too, but AUSDatabase makes sure it stays up to date if it changes).
* Reject any changes that set an URL to a non-whitelisted domain. I think it makes sense to do this at the Releases table level rather than the View level, in case we have multiple views that manipulate the Releases table.

I looked a little bit at trying to generalize the checking of domains against the whitelist, but because AUS3 doesn't have release blobs to check (it just has the URLs for the specific update it wants to serve). If we did that check in evaluateRules instead, we could check the entire release and bail early if we find a forbidden domain -- but then we'd be checking the entire blob during every request, which seems too expensive to do.

In addition to the unit tests, I did my own simple testing against admin.py + both wsgi apps.

Comment 7

[[e-mail] (:bhearsum)]

Need to get the config files in the dev environment updated before I can land this patch. Filed bug 884656 for that.

Comment 8

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/58eda7b667b1229ce0643bcedea0919637918962
bug 880354: implement whitelist of domains that updates from balrog can point to. r=nthomas

Comment 9

[[e-mail] (:bhearsum)]

http://10.134.48.37:8080/job/Balrog/30/ tests pass.

I just noticed that I didn't request ftp.mozilla.org to be in the whitelist...updates won't work for awhile while until bug 884656 is re-resolved.

Comment 10

[[e-mail] (:bhearsum)]

This appears to be working fine. Before the correct config was in place I wasn't able to get nightly updates, and I also got an error when I tried to upload a blob with "evil.mozilla.org" in one of its fileUrls.