Re: The future of commit access policy for core Firefox

On Mon, Mar 13, 2017 at 11:41 AM, Eric Rescorla <[email protected]> wrote:

> On Mon, Mar 13, 2017 at 11:35 AM, Ehsan Akhgari <[email protected]>
> wrote:
>
> > 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>
> >
>
> Yes.
>
>
>  I don't understand how any of this is useful without this assumption.
>
>
> I'm not following. The reason that this is not necessary is because (by
> hypothesis)
> you are trusted, and this proposal is not intended to prevent against
> attack by
> trusted people. In order to do that, we would need to ensure signoff by
> *two*
> trusted people.
>
>
> >     , 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?
> >
>
> Yes. As noted above, we would need to do something more aggressive
> than mconnor is proposing to address this threat.
>


I think we're going the right direction in this thread: moving from less
secure to more secure.  But if the threat is compromised L3 creds, what
sort of solutions might help mitigate that?  (Assume one full set of
compromised L3 creds.  Prevent, or mitigate, shipping malicious code.)  Can
that solution be designed in such a way that it can coexist with at least
close-to-current levels of developer productivity?

I would guess addressing review and commit policy, as we are discussing in
this thread, would be at least part of that solution.  Requiring 2 L3
committers to sign off on certain types of commits may be a solution.
Pre-merge-day tree-review may be a solution that requires less day-to-day
overhead, but adds a significant task every N weeks, and doesn't protect
Nightly users.  A pre-Nightly branch (or set of branches) where changes can
accumulate until they are given another round of cumulative review may be a
solution.  Any solution that leverages automation to lessen manual overhead
would be worth investigating, as long as we have some trust in that
automation.

aki



>
>
> (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.)
> >
>
> Yes.
>
>
> > 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.
> >
>
> I agree that it does not. It merely attempts to ensure that the code comes
> from
> L3 committers.
>
> -Ekr
> _______________________________________________
> dev-planning mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-planning
>