Closed
Bug 880354
Opened 11 years ago
Closed 11 years ago
implement whitelist of domains that updates from balrog can point to
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bhearsum, Assigned: bhearsum)
References
Details
(Whiteboard: [balrog])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
nthomas
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → bhearsum
Assignee | ||
Comment 1•11 years ago
|
||
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.
Status: NEW → ASSIGNED
Attachment #762327 -
Flags: feedback?(nthomas)
Comment 2•11 years ago
|
||
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.
Attachment #762327 -
Flags: feedback?(nthomas) → feedback+
Assignee | ||
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Assignee | ||
Comment 6•11 years ago
|
||
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.
Attachment #762327 -
Attachment is obsolete: true
Attachment #764441 -
Flags: review?(nthomas)
Updated•11 years ago
|
Attachment #764441 -
Flags: review?(nthomas) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Need to get the config files in the dev environment updated before I can land this patch. Filed bug 884656 for that.
Comment 8•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #764441 -
Flags: checked-in+
Assignee | ||
Comment 9•11 years ago
|
||
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.
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•11 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•