Re: The future of commit access policy for core Firefox

On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[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]> wrote:
> >
> >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[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:


Hm, maybe I'm missing something obvious, but I don't follow.


> for example, there is still the issue of whether
> reviewers actually treat code coming from someone named Ehsan Akhgari as
> potentially malicious code,


I assumed they wouldn't.


> what this means for rebases, who will be
> responsible for the final sign off on the patch in the r+-with-nits
> situation, etc.
>

The point is that r+-with-nits is only allowed in the case where the person
doing the nit fixup is trusted.


>
> 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?
>