Open Bug 1158178 Opened 9 years ago Updated 3 years ago

Create Hg sec-approval hook

Categories

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Product:
Developer Services
Component:
Mercurial: hg.mozilla.org
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

People

(Reporter: lmandel, Unassigned)

References

Details

sec-critical and sec-high bugs require sec-approval before landing on any branch. We do occasionally make a mistake and land a bugs with these designations without sec-approval. Given the sensitive nature of these bugs, I would like to see whether it is feasible to create an Hg pre-commit hook to catch landings of sec-crit and sec-high bugs that do not have sec-approval.

Challenges:
- the hook will need sec access
- we don't want severely adverse performance of either Hg or Bugzilla from needing to poll Bugzilla for every commit
- to be effective, the hook should be on all branches - specifically, it should be on m-c

Dan/Al - Do you think the risk of granting sec access for a hook is worth the reward in this case?
It is feasible. However, I have concerns with server-side hooks connecting to external services because it creates an operational dependency on the external service being up. It also adds latency to pushes. While BMO rarely goes down, I'd rather not introduce a dependency.

I think this kind of filtering is something better suited for the Autoland system. But opening up Autoland to deal with sec sensitive bugs is its own can of worms...
Dan and I were discussing this yesterday and we were concerned that we'd have to give this whole thing security access to see the bugs and that this might be a vector for attack. It is still worth discussing though.

This would have to have security bug access and be able to read keywords.
As an idea, maybe a periodic dump of json bug #'s that are considered this level of sec. That is only accessible to hg.m.o's server (hook). This dump can be generated once an hour, or once a day depending, and it would be all bugs that are both private and sec-high/sec-crit

Said json could then be easily parsed by hg.m.o (or even pulled and stored/cached locally). This removes the need to give hg.m.o the sec access and removes some of the latency.
Yeah, that was my thought too. It seems to me that the set of relevant bug numbers is not particularly sensitive. So as long as you a component that delivers that set to a less-secure location that the hook can feed off of, then the hook doesn't need any special access.
An offline dump of bug #s would remove my objection about 3rd party network access at push time. However, I /think/ for this to be effective, that list would need to be updated very quickly after sec approval is granted in the bug, otherwise the list seen by Mercurial is stale and ineffective.

What exactly is the workflow in Bugzilla? Could we maintain 2 lists: one of sec-sensitive bugs and one with sec-approval and look for entries in the former but not the latter? You still have syncing problems there, of course...
There are also times where release management gets an uplift request for a patch, and I might accidentally approve it for uplift without it having sec approval.  So, don't let anyone set a tracking flag to +, if sec-approval isn't already set to +. 

Preventing that mistake would help, and we could do that without sending anything to hg.
I could file that separately as a bugzilla.mozilla.org bug. What do you think?
Works for me.
Liz, could you fill the bug if you haven't done it? Thanks
Flags: needinfo?(lhenry)
There's some discussion of a related idea in bug 1164063, so I added a comment there.  The idea in that bug is for a warning on sec-high and sec-critical marked bugs.  But maybe we could also block the uplift approval being set unless sec-approval is set.
Flags: needinfo?(lhenry)
We should really make progress on this. I have two recent examples of sec patches landing without sec-approval.
I'm not willing to have the Mercurial server make additional HTTP requests to Bugzilla for every push. Too much latency and it may prevent pushes from going through if Bugzilla or the network connectivity to Bugzilla is down.

If the Mercurial server had a local file containing a list of bugs we could cross-reference to perform additional security checks only for that small subset of bugs, I'd be fine with a hook checking against that list. I'd possibly even be fine with an HTTP request to Bugzilla in that limited circumstance.

We'll need a file available to the Mercurial server. There are tons of ways to do this. They all start with mcote and the BMO team producing said file. So needinfo mcote on feasibility.

Also, hooks run on on the Mercurial server as the user who is pushing. This means that the bug list file will be readable by anyone who has SSH access to hg.mozilla.org. We'll not expose the file content to the user, of course. But there is a risk of a security vulnerability on the server leading to the disclosure of the file.
Flags: needinfo?(mcote)
glob may have ideas on what's best from BMO's perspective.
Flags: needinfo?(mcote) → needinfo?(glob)
i agree that adding a bmo ping for every push may be problematic, however having bmo update a file on the hg server feels clumsy at best (and i share similar connectivity concerns for when hg is down).


perhaps it's better to do bug 1164063 initially and see if that makes a difference before we tackle more complicated solutions.
Flags: needinfo?(glob)
So as I understand it, what we want to do here is have an hg hook that rejects commits for bugs that are sec-high or sec-crit, and do not have sec-approval. Except that sec-approval is for individual patches, not bugs. So to make this well-defined, let's say we allow landings when the bug has at least one non-obsolete patch attached with sec-approval+.

But we also have the a= comment on the patch to use. So let's say we reject sec-high/sec-crit landings if there's no a= in the commit message, and otherwise, we'll still only accept the changeset if it has at least one non-obsolete sec-approval+.

I'm just going to wave my hands about a bit here.

The list of sec-anything bugs is public. You could iterate through bug numbers if nothing else. The subset of those that are sec-high/sec-crit is moderately sensitive, I'm guessing.

I personally would not want the hg server to do a mandatory HTTPS round trip on every push. More points of failure, and a perf nightmare. I also would not want bugzilla to be updating some extremely special-purpose file on a large number of actions. On the bright side, it seems like it would be ok for this to fail open: if the system is unsure whether a patch is clear to land, it's ok if it just lets it through. We're no worse off than we are now.

On the other hand, it wouldn't bother me if bugzilla were to log a record of all of its changes via pulse. Or at least, it wouldn't bother me if pulse could provide a secure channel, and I have no clue whether it can.

What I'm getting at is the following: in the simplest version, bugzilla posts a pulse message for any security status change on bugs (not on attachments for now). A pulse client running alongside the hg server, or at least with write access to shared space with the hg server, maintains the set of sec-high/sec-crit bugs. It could even do it lazily -- partition all bugs into known-secure, known-insecure, and unknown. It can do synchronous fetches for unknown bugs. And it can timeout those requests and reply "dunno yet, but go ahead and let it through." When the hg server gets a push, it checks the shared table (might be a file on disk, might be a custom protocol to the pulse client, might be one of those key/value stores that people like to use these days). If the bug is labeled secure, then it talks to bugzilla to figure out whether there's a non-obsolete patch attached with sec-approval+.

So all pushes for sec-high bugs would be slower. If you get too lazy with initializing the table, some pushes for public bugs would be slower too, but only during warmup and the laziness is not required (you read in the last known state on startup). But if the pulse client is down or the connection to the bugzilla server is busted, it'll still let things through.

You can represent a boolean for every one of our bugs in 150KB, btw.

Also, if the bugzilla roundtrips really bother you, you could have the pulse client maintain a cache of attachment state as well. (It might need to start doing bugzilla roundtrips, eg if it sees sec-approval revoked on a patch then it would need to query bugzilla for the other attachments on the same bug.)

Alternatively, you can think of all of this as either a caching proxy layer in front of bugzilla, or as a partial online replica of bugzilla's database. And the usual ways of implementing that could work -- you could just do it with a local squid proxy in front of the bugzilla's REST API. configured to be allowed to aggressively cache even at the cost of correctness (because we can fail open.) Or perhaps bugzilla's database server supports partial replication? An issue with those generic approaches is that you *would* end up storing secure data.

Food for thought. It would be easy to make this really complex, but a pulse client that is queriable from the hg server seems fairly straightforward. If it maintained a little more state, it might also be reusable for detecting incorrect bug numbers. (main tree pushes to RESOLVED bugs would require a CLOSEDBUG token in the commit message or something.)
Pulse is a non-starter right now; we have not done the requisite investigations to use it with confidential data.

That said, BMO has its own push service (a generic event-queuing system) that could be adapted for this purpose.
Pulse for Bugzilla broadcasts is not compatible with the future architecture of hg.mozilla.org. I'd much prefer some out-of-band process for installing a file that the push hook can quickly read and check against. Given the security implications, I'm guessing this isn't a public API on BMO but more a backchannel CRON somewhere.
if we're going to do the "save a copy of all sec-{high,critical} bugs" then my preferred implementation is:

- a bmo push connector
  - listens for sec-{high,critical} keyword and sec-approval changes
  - determine if the bug's current state should prevent commits
  - calls a new hg.m.o webservice (eg. "add bug 12346", "remove bug 98765")

- a hg endpoint
  - authenticated call(s) only for bmo
  - handles updating hg's file/db

- hg hook
  - checks file, yadda yadda


this separates the concerns, and simplifies the security requirements around getting bmo to update/manage a remote file on hg directly.  our push system is queue based, so events generated when hg is down will not be lost, and will be delivered in order once connectivity is restored.  generally messages are delivered immediately after the bug is updated.
I really like the idea of throwing up an obvious warning, i.e. bug 1164063, before going into the hideous labyrinth sfink describes. We should give it a try!
in all seriousness, there's some complexity in determining the exact rules, so it makes a lot of sense to start off with a warning.  once that's bedded down then we can look at allocating resources to make this hook work.
We can't easily have HTTP pingbacks to https://hg.mozilla.org/ because port 443 is load balanced across a number of servers that intentionally don't have shared state (because single point of failure). There is, however, a single master. But it is only open to the world over SSH. And we don't want to expose it on HTTP (to the world at least) for reliability and security reasons.

I could make a hacky web service on hg.mozilla.org that talks back to the master behind the scenes. But it seems like a lot of work compared to the requested feature.

Let's improve the UI in Bugzilla before we start talking about complex server side foo.
See Also: → 1250769
QA Contact: hwine → klibby
You need to log in before you can comment on or make changes to this bug.