New issue
Advanced search Search tips

Issue 894541 link

Starred by 1 user
Status: Fixed
Owner:
qyears...@chromium.org
Closed: Nov 8
Cc:
no...@chromium.org
vadimsh@chromium.org
mar...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature



Sign in to add a comment

Only automatically run Tricium in chromium/src on CLs from trusted users

Project Member Reported by qyearsley@google.com, Oct 11 
The main specific change here would be: before starting an analyze request for a new CL, Tricium would have to: (1) check the project settings to see whether it should run for all CLs or just CLs from authors belonging to certain groups, (2) if the latter, then send a request to Gerrit asking if the author is in a trusted group. This can likely be done with one extra request to Gerrit per CL, maybe using the accounts/list-groups endpoint. (In order to avoid the extra request per CL, owners->permissions map may be cached somehow).

Additional requirement: If tricium is not run immediately for all CLs, then we need to make it clear in the UI whether it has been run, and allow committers to trigger runs. Ideally this UI should be consistent with the tryjob or CQ-related UI.

Comment 1 by qyearsley@google.com, Oct 23

Cc: vadimsh@chromium.org no...@chromium.org mar...@chromium.org
Owner: qyears...@chromium.org
Status: Assigned (was: Available)
Julie pointed out: we already have a way to do this: the per-repo whitelisted_group field in the project config.

It's already the case that when polling for new changes, the owner of the change is checked against a chrome-infra-auth group, which can be set to committers, or accounts with tryjob access, etc.:

https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/poll.go?l=401

I believe that this way is just as robust as adding a separate check for Gerrit group membership, since either way there is a check in Tricium on polling, and enabling/configuring this check is done in the project config.

CL which would add a whitelisted group for chromium/src:
https://chromium-review.googlesource.com/c/chromium/src/+/1295389

Does this seem like it would resolve the main security concern for Tricium for now, since committing that CL would make it so Tricium isn't run for people wihtout tryjob access?

Comment 2 by qyears...@chromium.org, Oct 24

Status: Started (was: Assigned)
Just tested this approach out with a quick CL in the playground repo (https://chromium-review.googlesource.com/c/playground/gerrit-tricium/+/1297705) and no analysis was started for a user outside the whitelisted group.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2fa251dfc8e8536702fea6e1dd69b0e7f82c530

commit e2fa251dfc8e8536702fea6e1dd69b0e7f82c530
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Wed Oct 24 20:43:11 2018

Only trigger tricium for authors with tryjob access

This is a change to the chromium/src tricium project config,
which would make it so that Tricium jobs are only run for
members of the given group (project-chrome-tryjob-access).

Others could potentially manually run jobs; this would affect
the behavior for analysis run when polling gerrit for new
patchsets. Related code:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/poll.go?l=401

Bug:  894541 
Change-Id: I1da729a7dd64ac2862c8057b32ced35063bfcf6e
Reviewed-on: https://chromium-review.googlesource.com/c/1295389
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602434}
[modify] https://crrev.com/e2fa251dfc8e8536702fea6e1dd69b0e7f82c530/infra/config/global/tricium-prod.cfg

Comment 4 by qyears...@chromium.org, Nov 8

Status: Fixed (was: Started)
►

Sign in to add a comment