Page MenuHomePhabricator

[land-bot] Only operate on trusted patches
ClosedPublic

Authored by jasonbcox on May 8 2020, 23:13.

Details

Summary

Even though the land bot will be running in a jail, so the results of the
build do not matter even if they are horribly clobbered, it's possible for the build
to leak sensitive information such as the SSH private key that will be used to push
changes upstream. To ensure this is not possible, all revisions going through land
bot will at least need to be already greened by folks in the Bitcoin ABC reviewer group.

Test Plan
CONDUIT_TOKEN=<your-token> ./land-patch.sh --dry-run -r Dxxxx

Where xxxx point to revisions with different statuses:

  • Accepted by Bitcoin ABC reviewers (patch is applied, smoke tests run, etc.)
  • Rejected by Bitcoin ABC reviewers (fails as expected)
  • Not reviewed by Bitcoin ABC reviewers (fails as expected)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
land-bot-check-accepted
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10704
Build 19204: Default Diff Build & Tests
Build 19203: arc lint + arc unit

Event Timeline

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.

I don't understand why you need to do the check in this script. Isn't it already checked by arc land ?

deadalnix requested changes to this revision.May 11 2020, 13:50
deadalnix added a subscriber: deadalnix.

You are reimplementing rules that already exist and are checked by arc land. Just do what arc land does.

This revision now requires changes to proceed.May 11 2020, 13:50

I don't understand why you need to do the check in this script. Isn't it already checked by arc land ?

It's necessary to check before doing arc land because arc land behavior can be changed by the patch by modifying .arcconfig or any of the libraries loaded by .arcconfig.

Make the review checking more similar to arc land's implementation so
as not to have to check for the ABC reviewer group specifically.

This revision is now accepted and ready to land.May 12 2020, 17:03