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: 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.


, 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.


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. The intent of this proposal is, rather, that at
least one trusted person must sign off on any code before it lands.

-Ekr