Re: The future of commit access policy for core Firefox

On 2017-03-13 2:19 PM, Eric Rescorla wrote:
> 
> 
> On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[email protected]
> <mailto:[email protected]>> wrote:
> 
>     On 2017-03-13 1:40 PM, Bobby Holley wrote:
>     > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[email protected]
>     <mailto:[email protected]>> wrote:
>     >
>     >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[email protected]
>     <mailto:[email protected]>> wrote:
>     >>
>     >>> On 3/12/17 9:55 PM, Eric Rescorla wrote:
>     >>>
>     >>>> Alternately, you can create a patch which gets r+ with nits, and
>     >>>> then update with some malicious code and  r=<committer>.
>     >>>>
>     >>>
>     >>> Speaking as a reviewer, for people I don't trust I pretty much
>     never give
>     >>> "r+ with nits", because I don't trust them to address the nits
>     correctly.
>     >>>
>     >>
>     >> Actually, I wish I had written this differently. Say I get an r+
>     w/o nits,
>     >> I suspect
>     >> that the sheriffs will accept an updated patch (e.g., ostensibly
>     with a
>     >> comment
>     >> fix) that is marked r=<foo>.
>     >>
>     >>
>     >> So, I would ask: do people believe that this is an acceptable
>     state of
>     >>>> affairs or should the minimum number of 'trusted' people
>     required to
>     >> land
>     >>>> a patch be 1
>     >>>> or more?
>     >>>>
>     >>>
>     >>> Obviously the latter.  ;)
>     >>
>     >>
>     >> Me too. And I think "trust" in this case at least arguably should be
>     >> defined as
>     >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility
>     would be
>     >> have a
>     >> policy like the following.
>     >>
>     >> - Every CL must either be written by someone trusted OR r+ed by
>     someone
>     >> trusted.
>     >> - If a patch is r+ with nits, then the final patch must be posted by
>     >> someone
>     >> trusted.
>     >>
>     >> This would ensure that every CL that lands was signed off on in
>     its final
>     >> form
>     >> by a someone trusted.
>     >>
>     >> Does this seem crazy?
>     >>
>     >
>     > This seems like a very reasonable compromise to me. It gets the
>     overhead
>     > out of the way in the hot paths but still gives us some
>     organization-wide
>     > guarantees.
> 
>     I still think this has mostly the same issues as has been raised in the
>     thread so far, even though at first glance it may seem a bit nicer than
>     the original proposal: for example, there is still the issue of whether
>     reviewers actually treat code coming from someone named Ehsan Akhgari as
>     potentially malicious code
> 
> 
> I was assuming that they did not.

Did you see my response here?
<https://groups.google.com/d/msg/mozilla.dev.planning/nUP-j72I9bE/7OFmSMuqDgAJ>
 I don't understand how any of this is useful without this assumption.

>     , what this means for rebases, who will be
>     responsible for the final sign off on the patch in the r+-with-nits
>     situation, etc.
> 
> 
> This would depend on the original patch author. Specifically:
> 
> - If the original patch author (or really the person posting the final
> version)
>   was trusted, then they would be able to (once they had r+) make final
>   changes, rebase, etc.
> 
> - If the original patch author was untrusted, the they would need final
> signoff
>   on rebases, nits, etc. from someone trusted.

OK, to me, this essentially reads as if an L3 committer is malicious,
we're screwed, which is basically the status quo.  Am I understanding
the pre-and post- picture correctly under your suggestion?

(I agree on the other part of this proposal about *non-L3 committers*
being malicious, this proposal would inject a hook into the review
process where we can scan those commits for malicious code, but we would
need to do a good job somehow at highlighting people's commit access
rights in the UI of the tool we use to do code reviews so that the
reviewer can quickly assess whether they're supposed to treat the path
author as potentially malicious.)

>     Note that it seems now we have modified our goal slightly.  Originally
>     it seems we were trying to ensure we don't get incoming security bugs by
>     ensuring that any commit that ultimately gets checked in gets an r+.
>     Now we are talking about every commit needs to be r+ed or authored by
>     someone with L3 access.  Are we still protecting against incoming
>     security vulnerabilities?
> 
> 
> That's certainly the intent of what I'm suggesting. Note that this is
> intended
> to enforce a weaker security condition than the one that Mike's proposal
> claimed to be trying to enforce (specifically that compromise of a single
> trusted person's credentials would not allow them to land malicious code).
> However, as I noted originally, I do not believe that that proposal actually
> achieved those objectives.

Yes, this much we can agree on.  :-)

> The intent of this proposal is, rather, that at
> least one trusted person must sign off on any code before it lands.

I still don't think this proposal achieves any meaningful security
benefits where code is coming from L3 committers.  With this proposal,
we're as screwed in case an L3 committer starts to act maliciously as we
are today.

(FWIW, if I may interject some personal opinions here, if I were to redo
our commit access policy I would probably approach things from a
different angle.  I don't think there is anything we can do to remove
the requirement around trust in our L3 committers without basically
removing commit access and hence the need to trust humans which IMHO
would be the wrong end of the security/usability spectrum.  Instead I
would probably focus a bit on verification measures, for example around
whether the L3 committers actually keep acting on good faith and
investigations on our disclosed security vulnerabilities, if we aren't
already.)