Chromium

Added this into hotlist:available to remove from the trooper queue.

The need for trusted deps makes sense, but meeting all submit requirements is rather extreme. It effectively means each CL must be reviewed one-at-a-time, bringing all the way to ready-to-submit before you can even find out if the child passes tests.

A unprivileged committer can trigger a dry-run only if the CL is submittable in Gerrit, which means that a probably-Googler reviewed and approved the CL. So, even if we make a change for your proposal, the CL has to be approved by privileged committers first.

Then, could the reviewer (privileged committer) trigger a dry-run on the CL at the time of reviewing the CLs? privileged committers can trigger dry-runs regardless of open-deps. If the UX is an issue, then we could consider updating the UI such that it will ask the reviewer if they want to trigger dry-runs when the approval button is clicked.

Our unpriviledged users actually cannot ever trigger dry-runs, even on approved CLs [1]. This is actually about priviledged users triggering dry-runs of CLs owned by unpriviledged users.

Then, could the reviewer (privileged committer) trigger a dry-run on the CL at the time of reviewing the CLs? privileged committers can trigger dry-runs regardless of open-deps.

This does not seem to be the case, at least for us. Am I misunderstanding? Or maybe this is a configuration issue? For example, see https://go.dev/cl/595564. Filippo Valsorda is a fully-privileged user that voted Commit-Queue+1 and was denied due to open deps: "CV cannot start a Run because of the following dependencies. They must be submittable (please check the submit requirement) because their owners are not committers or dry-runners. Alternatively, you can ask the owner of this CL to trigger a dry-run."

[1] Perhaps we could consider changing that, but that is not relevant here.

My apologies. You are right. It's required to have all deps approved in order for a committer to trigger a dry-run on someone else' CL.

• https://source.chromium.org/chromium/infra/infra_superproject/+/main:infra/go/src/go.chromium.org/luci/cv/internal/acls/run_create.go;l=251-254

I believe that we ACL-ed the dry-run privileges to minimize the chance of malicious code getting compiled and executed in tryjobs. For instance, in this flow, it's possible that

1. an unprivileged external contributor uploads CL-1 with malicious code, and sends a review request to someone else.
2. the contributor uploads CL-2, having CL-1 as a dep CL.
3. the contributor sends a review request for CL-2 to a committer, and the committer triggers a dry-run without realizing that CL-1 has malicious code.

It's not perfect, but by enforcing all deps fully-reviewed by a committer with a stamp, we effectively minimize the chance of such an attack. However, I acknowledge that it effectively blocks code reviews on a CL until its dep CLs are approved, as no one can trigger a dry-run on the CL, uploaded by an external contributors, with open-deps.

We can introduce a new config option to relax this contraint. It means that a fully privileged user can trigger a dry-run on someone else' CL, even if it has open deps. In chrome, I'd route this topic to chops-security to check the security concerns, but I'm not sure about golang. If the option to relax the constraint sounds good to you, we can add the option. WDYT?

I agree that avoiding accidentally executing malicious code from a dep you failed to look at is useful in general. I'm trying to weigh the usefulness vs the pain.

We can introduce a new config option to relax this contraint. It means that a fully privileged user can trigger a dry-run on someone else' CL, even if it has open deps. In chrome, I'd route this topic to chops-security to check the security concerns, but I'm not sure about golang. If the option to relax the constraint sounds good to you, we can add the option. WDYT?

I would want to check with our security team as well. Our old CI system allowed this, so I suspect it would be OK (though perhaps just because no one thought about it).

I'm curious if you have thoughts on my somewhat "middle ground" proposal in #1: "Instead, I propose that a dep CL is trusted if it has an inflight or completed dry/full. This informs us that a privileged user must have reviewed that dep and deemed it safe to test. The resulting workflow would require a reviewer to vote Commit-Queue+1 in the proper order, which is a bit annoying but much less than the status quo."