Commit into repository directly from differential
Open, NormalPublic

Tokens
"Like" token, awarded by amartin33."Love" token, awarded by austinstoker."Orange Medal" token, awarded by turadg."Like" token, awarded by tycho.tatitscheff."Like" token, awarded by halfsea."Like" token, awarded by hwinkel."Like" token, awarded by igorgatis.
Assigned To
Authored By
tuomaspelkonen, May 26 2011

Description

Just opening up a conversation about how to do this. This would be a really nice feature for deffirential.

From Facebook Tasks:

D255317 fixes (or at least should fix) the latest branch, which means we can get a working build out for testing.

Right now though, we need to get it reviewed, committed, and requested for merging in to latest.

I'd like to be able to request a diff to be merged, prior to it actually being committed. This is a valid thing to do as long as we're not using shadow branches in production (and only for testing), though I guess that might be debatable.

Details

Blocks
T6008: Editing files and contributing changes via web
Blocked By
T9952: Let "Land Revision" target branches other than master
T9252: Unprototype Drydock (v1)
Restricted Maniphest Task
T603: Support permissions/policies in all Phabricator applications
Differential Revisions
D7534: Add support for landing to hosted Mercurial repos.
D7555: Land to GitHub + support stuff
D7486: Land Revision button for hosted git repos
Commits
D14350 / rP1c7443f8f242: Make "Land Revision" button state consistent, prevent non-accepted lands
D14349 / rPa763f9510e76: Add some Drydock documentation plus "Test Configuration" for repository…
D14348 / rPcea633f698da: Don't show error operations after a successful land operation
D14347 / rPbbf4ce79e341: Provide a username and email when running `git merge --squash`
D14346 / rP5a35dd233b19: Don't use --ff-only inside "Land Revision"
D14343 / rP0b24a6e2006e: Make "Land Revision" show merge conflicts more clearly
D14341 / rP2326d5f8d090: Show lease on Repository Operation detail view and awaken on failures
D14338 / rPa0fba642b382: Show the oldest non-failing revision land operation, or the newest failure
D14337 / rP9c3949379617: Make WorkingCopyBlueprint responsible for performing merges
D14314 / rP5b619862cb06: Show a more reasonable status element for pull requests
D14295 / rP92a626fc1ca3: Add a basic list view for repository operations
D14280 / rPf3f3d957021b: When landing revisions via repository automation, use better metadata
D14270 / rP43bee4562c72: If the stars align, make "Land Revision" kind of work
D14266 / rPb4af57ec51e1: Rough cut of DrydockRepositoryOperation
D14259 / rPdf5a031b54a5: Allow "Repository Automation" to be configured for repositories
D14258 / rP3f3626c11a04: Write some documentation about Drydock security and repository automation
D13022 / rP08a9e0f22aff: Hide Land to GitHub
D8719 / rPe8e62f82ceae: Hide "Land to hosted git" button for now
There are a very large number of changes, so older changes are hidden. Show Older Changes
sophie added a subscriber: sophie.Sep 1 2015, 8:14 PM
epriestley moved this task from Backlog to Preflight on the Prioritized board.

I think we're ready to start laying in some of the groundwork for this. Per T9252, we now have hosted builds of commits and revisions in the upstream. This code still needs additional work and is missing a lot of generality, but the fundamentals seem to be in reasonable shape and I think we're on the path to stability.

Some of the specific pieces we need to build are:

  • Blueprint selection, with some discussion in T9252. Administrators must be able to configure hosts which only run trusted code, and currently there's no way to prevent semi-trusted code (unit tests in revisions) and trusted code (merge operations from Phabricator) from running on the same tier. We need this anyway to tackle a bunch of other problems, but it's particularly important in this case.
  • Configuration. This is probably straightforward and can just live in Diffusion, although configuring it per-repository may be cumbersome. At a minimum we need to select a blueprint to use to perform merge operations, but we probably need a bunch of additional options eventually.
  • Representation of requests. The current DifferentialLandingStrategy classes do everything synchronously, but this stuff needs to move into objects + workers. Perhaps these should be generic and part of Drydock itself? Releeph may also want to write directly to repositories some time in 2025.
  • UI and stuff like dependencies, but I think this is all fairly straightforward. (Do we want to let you queue a merge request that depends on a dependency landing? Probably not in v0.)
  • Mergeability tests? This feels like not-a-v0-feature, but it would be nice to tell the user if we expect a clean merge or not before they press the button (like GitHub). This may not really be too difficult.

At least one install wants to require tests run and pass on the merged code before it is published. I think the scaling challenges and high latency this implies make it a questionable approach for most projects, but that we should be powerful and flexible enough to accommodate it in some form. An implication of this is that a merge request probably has to be a Harbormaster Buildable, and that the actual logic to build a merged state likely needs to live in the Drydock WorkingCopy blueprint.

I think this set of responsibilities (WorkingCopy blueprints perform merges; Drydock owns merge requests; merge requests are Buildables) ultimately makes sense, even though it's not immediately obvious that Differential should be a thin client on top of services provided by other applications since almost 100% of the UI will be surfaced there.

One potential challenge is that this makes it somewhat difficult to do the pre-merge tests in an external system, but I think a Harbormaster build plan like this can actually accomplish that:

  • Lease a working copy with the merge request Buildable in it.
  • Run a git push to push it to a staging area tag somewhere.
  • Call Jenkins (or whatever) to build the tag.

We could plausibly even formalize that, although in the general case we should not trust that the merge is good since it might have happened in a lower-trust environment. If we wanted to do this in the upstream the tag push would happen from sbuild (low-trust) so we could not use it later on saux (high-trust).

We are also likely to build some sort of consensus framework (primarily to solve problems in T7639 / T7148) which provides generic support for "N of M users must approve this". This is a cleaner fit for having peers reset your MFA than putting a second weird review step on the merge pipeline, but maybe applicable at some point down the line.

I think we can accommodate this by putting a "needs consensus" state on the request in the future, though, and don't specifically need to worry about this in v0.

Defining the v0 of this in greater detail -- largely just me thinking out loud:

Blueprint Selection: Currently, Drydock satisfies lease requests using any available matching blueprint. This creates a trust problem because a compromised user can potentially create a Harbormaster build plan which allocates a similar lease and ends up executing unit tests on the same host in a way that isn't isolated and can interact with other processes on the host, including the merge process. Examples of ways that an attacker can interfere with the merge process include:

  • Write a "unit test" which runs forever and watches for merge processes. Between the merge and push, win a race and rewrite HEAD so the merge process pushes a different commit. You might be able to renice or suspend + resume the merge process to make this easier, but even if you can't figure out any dirty tricks you can use to make success more likely you can sometimes just win a race. Execute this unit test by creating a private revision. Or you can probably just push whatever you want yourself since you essentially have access to the same credentials.
  • A subtler version of this is that you write a "unit test" which adds a commit hook to the repository. (Maybe WorkingCopy resources should just wipe hooks to prevent this? But this is just a slightly easier version of other attacks in this vein.)
  • Write a "unit test" which changes $PATH in ~/.profile to replace git with a wrapper that does evil things.

Basically, Drydock -- by design -- allows processes with similar trust levels to share resources because it's hugely more efficient in many common cases. It's generally OK that malicious unit tests can mess with benign unit tests since there's no real cost to this in most situations. You accomplish nothing by making passing tests fail, and an attacker accomplishes very little by making failing tests pass. The best attack I can come up with for this is:

  • Attacker writes evil revision A, which they send for review. This does something evil that would normally be caught by a unit test. They don't want to touch the test to avoid arousing suspicion.
  • The attacker also writes evil revision B, which has a "unit test" that looks for the process running tests for "A" and makes them pass instead of fail. They submit this but use policy settings which hide it from other users.
  • Revision A's tests run and are turned green by revision B's "tests".
  • The evil thing that A does isn't caught in review, but hypothetically would have been if the tests had gone red.

This is also currently academic because the attacker can just call the API to pass the test, although we can lock this down eventually.

Very paranoid installs might eventually build Drydock blueprints that construct completely clean environments for every test to prevent interaction between processes, and I would expect to eventually deliver a set of capabilities which allows test results to be trusted, but I think it is reasonable to expect many installs not to consider this complexity vs paranoia tradeoff worthwhile. This attack is very difficult to execute, runs a high risk of detection, and depends on a lot of luck to have a small chance of increasing the possibility that other uncompromised actors make an error.

Broadly, I'm modeling unit tests as a correctness mechanism, not a defense-in-depth mechanism. In general it isn't possible to write unit tests which assert that there are no security holes. An attacker who can submit an arbitrary revision for review can almost certainly evade unit tests more easily than they can rig up a series of coordinating processes to compromise tests. Even if damaging software correctness is sufficient (e.g., to introduce a correctness issue in missile guidance software) it is hard for me to imagine that this is ever the weakest link in the chain.

We will eventually support trusted unit tests because it's reasonable for installs to want to be able to make this tradeoff, but I suspect that for 99% of installs it is acceptable for unit tests to be theoretically compromisable by an attacker that is sufficiently sophisticated and lucky, as long as the review and merge processes are fully trusted.

Anyway, the upshot here is that installs must be able to configure multiple build pools, like our sbuild (secure build, low trust) and saux (secure auxiliary, high trust) and run trusted code only on trusted tiers. In particular, this means:

  • Merge processes must know how to request high-trust leases.
  • High-trust leases must always acquire high-trust resources.
  • Low-trust leases must never be allowed to acquire high-trust resources.

I think there are two approaches to this, and we might pursue both:

  • More General Approach: When configuring Harbormaster (for builds) or Diffusion (for merges), you select which blueprints are acceptable. The first version of this can be a tokenizer, but we'll outscale this at some point. It's likely that we eventually need a way to select groups of resources in a flexible way -- so maybe we let you select Query filters instead? But we also have to prevent anyone from just selecting "All Blueprints" for a build and getting high-trust blueprints some of the time -- we need to keep builds off the high-trust tiers, not just keep merges on those tiers. I currently believe this is a problem we need a high-power solution to eventually, and don't have an approach in mind yet. Notably, this is tricky because most processes do not execute as users, so we're limited in our ability to apply standard policy controls. I'm confident I'll arrive at something reasonable after giving this some more thought, but don't have something implementable yet.
  • Less General Approach: Give Drydock an explicit "Trust" flag which every level of the stack understands. This is kind of a bad hard-codey approach but it's possible that "trusted code" vs "untrusted code" is a reasonable flag to make top-level. This would provide an easier solution to the immediate problem and isn't necessarily terrible, although I'd like to figure out what the long-term general approach is going to be before pursuing this, and have more confidence that this flag really is exceptional enough to justify creating.

Broadly, solving this problem is really part of T9252 since Drydock's power is severely limited as long as we don't have an approach here. Whatever the v0 of this ends up being probably is not actually hard to implement, but it needs a v1/v2 plan that handles all these cases and scales into eventual high-trust builds and other complex scenarios where some code is executing at different effective user-defined trust levels.

Configuration: I think we can just shove a "Blueprint Selection" control into Diffusion for v0 and hard code everything else for now, once the blueprint selection part works. Even if we move configuration elsewhere in the long run this isn't hard to migrate. Some installs will care about the details here soon, but it's perfectly reasonable to hard-code stuff like rebase strategies and require the build tier to have the right credentials already configured for v0. We get into a little bit of trouble with "who can press the button for non-hosted repositories" but v0 could just require edit capability on the repository. These are some of the first things we'll need to change, but none of them are complicated.

Merge Requests: These seem straightforward. We should assume that the merge source may eventually be either a revision or commit (for eventual scenarios in Releeph, or conceivably some kind of "merge this thing into this other thing lolz" button in Diffusion). I believe these should be Buildables to support test-before-merge, but this isn't critical for v0 and is probably easy anyway.

UI Stuff: Our progress bar tech is a little weak but nothing here is very new/interesting and we have similar stuff already in the Bulk Job tool.

WorkingCopy Changes: The WorkingCopy blueprint needs to be able to build merges, but I think this is not too involved.

This does create a second trust problem: an attacker can configure arc to send one change for review and push a different change to staging. We must resolve this before merges can be trusted -- otherwise, what's built on the server may be totally different from what is reviewed.

The ideal solution here is probably that we stop uploading the diff and generate it on the server side from the pushed changes. However, this is quite involved and more or less requires repositories be hosted (we can pull diffs out of imported repositories, but not efficiently since you need a working copy to pull a diff). We could alternatively try to verify that the diffed changes and the pushed changes are the same, or apply the diffed changes instead of the pushed changes, but I'm not excited about these approaches.

There's a third trust problem, too: if you arc diff commits "B + C + D" but they're on top of evil commit "A", we must not bring "A" to master when we merge the revision. If we perform merges naively, we will bring A across. This one we can probably tackle by being careful with merge strategies, but is something we need to be cognizant of, and the default behavior should eventually be to refuse this merge outright. There's some discussion about this in T8090.

I think v0 probably won't try to tackle these problems in much depth. I'm not thrilled about this because I think chain-of-trust is the core value of this feature, but this set of problems seems highly separable from the core set of mechanical build problems.


So that roughly defines a v0 with these limitations:

  • A "Merge" button which works, technically, for Differential revisions if you're using Git. Both hosted and imported repositories will work. Repositories must have a staging area configured.
  • All the policy, merge strategy, etc., stuff is hard-coded to the easiest set of reasonable defaults. Some of these defaults may not be suitable for many installs (e.g., always rebase, always merge to master, require edit capability for imported repositories in order to perform merges). I expect to expand these shortly, I just don't want to focus on making things more flexible until they actually work.
  • Overall process is not trusted yet: the merge process itself will be trustworthy but we will not yet verify that the input to the process is the same as the code which is reviewed.
  • No support for build-before-merge yet, although groundwork will be put in place for it.
  • No support for pre-merging so we can guess whether or not something will merge cleanly. Our working copy acquisition time on Drydock is currently on the order of a couple seconds, so I expect this to still feel very usable: you'll click the button and almost always get a merge outcome a few seconds later, it will just sometimes be a negative result that we could theoretically have warned you about earlier.
  • NOTE: Because Drydock is still in a very rough prototype state, it may be too difficult to configure this stuff until Drydock as a whole is a little more polished and better documented, even once we have it technically working. This usability polish will happen as part of finishing T9252, but may take some time.

Estimates on complexity here:

ComponentEstimateNotes
Blueprint SelectionN/ANot really part of this; this is really remaining work on T9252. Needs additional planning and may take some time to design and implement.
Configuration1 HourAdd minimal required configuration to Diffusion to enable merges per-repository.
Merge Requests4-6 HoursThese are pretty straightforward but need standard infrastructure support components like ApplicationSearch, transactions, CLI tools, workers, etc., that take some time to flesh out. They might need a command queue since I think they'll be race-prone otherwise.
Various UI4-8 HoursMostly just making behaviors and request state clear in Differential. I think there will be a lot of complexity here once this starts getting built out.
WorkingCopy Changes4-8 HoursThis could be cheaper if most of the trust stuff is pushed out, or a little more expensive if some of it makes sense to build now.
qgil removed a subscriber: qgil.Oct 7 2015, 3:21 PM
epriestley moved this task from Preflight to The Queue on the Prioritized board.Oct 10 2015, 1:30 PM

Blueprint selection (which got more details in T9519) is now in a rough-but-functional state so I expect the rest of this to move forward shortly.

General progress update here: this now works, technically. Almost every part of it -- particularly, the UI -- is still total garbage for now, but if you ship a revision against rGITTEST up to this server you'll get a "Land Revision" button on it which has worked twice in the past and might work again. I would recommend extending the list of pasta varieties found in the pasta file if you want to try this (the button should appear and function without requiring the revision to be accepted for now, and everyone has write access to the repo anyway).

Although the UI is a mess, the underlying fundamentals are scalable: because this process leverages Drydock, it could perform two (maybe three!) pulls at the same time if you had enough hardware.

This stuff is done or mostly done or otherwise in good shape:

  • Blueprint selection needs a little more UI work but feels like it's headed in a good direction and generally in a good place already. T9519 has details.
  • Diffusion now has a "Repository Automation" section which allows blueprint selection. This seems fine for v1.
  • Merge requests exist as objects (DrydockRepositoryOperation) and about half the support code is in reasonable shape.
  • Drydock is generally holding up and beginning to feel like an actual piece of useful infrastructure.
  • And the thing does technically work in a reasonable subset of cases today.

This stuff needs significant additional work:

  • DrydockRepositoryOperation is still missing some fundamental interfaces (like a list view), and some major concepts (like making sure patches apply sequentially and probably a command queue) are undeveloped or undeveloped.
  • UI is a huge pile of trash, needs a bunch of work. Particularly, feedback about progress and outcomes is exceptionally poor.
  • The actual merge mechanics are OK, but have three major remaining areas which require work:
    • Mechanics live in DrydockRepositoryOperation, but probably need to live in WorkingCopyBlueprint (or otherwise be accessible from both places). The code could move easily in a purely technical sense, but picking up merge errors from WorkingCopyBlueprint is less easy.
    • We aren't trying to deal with any of the hard cases yet (like revisions not based on the target branch). Worse, we will happily do the wrong thing, which is dangerous.
    • Process is not trusted, and I think getting a chain of custody which covers "what you see is what will land" will be hard in the general case.
  • Lots of hard coding of things like "master" being the target branch.
  • General lack of configuration and workflow/application logic (e.g., no way to only allow accepted revisions to land).

We had a very slow week and haven't made much progress here. There is a new list view and some new, more reasonable UI. We landed our first production change with this mechanism.

The next pieces don't have obvious or unambiguous paths forward yet:

  • We have a hardcoded one-resource-per-blueprint limit we need to lift, but doing this in a way that is too simple may make things harder once the mechanism expands in the future.
  • We need to store results from operations so we can tell the user what happened (or what failed) but don't have a great place to put them right now, and underarchitecting this may also make things harder in the future.

I think these are probably both straightforward once I figure out how to proceed, they just need some thought about what the medium and long terms versions of these features will look like.

I think we should disable the "Land Revision" action item if the revision has already been landed.

The action is always available right now because it makes testing (particularly, testing things like "does it do the right thing when the revision is already landed") easier, but I'm going to require "Accepted" for v0 of this, which will indirectly disable it for landed ("Closed") revisions. We'll probably relax that later but it seems like the right place to start from, at least.

This is in reasonably functional shape for ambitious users. There's some documentation now:

We've landed most of the recent upstream changes with this mechanism. These specific things have been improved:

  • DrydockRepositoryOperation UI is in fairly reasonable shape now and provides critical progress feedback through the use of animation.
  • Merge mechanics are in WorkingCopy now.
  • Merge mechanics are less bad, but still a bit bad.
  • Limits and error handling are in better shape.
  • Only accepted revisions can land, and some other workflow sanity checks now exist.

This stuff remains:

  • Hard-coded target of master.
  • Hard cases (revisions with extra history / need rebase) aren't handled yet and may do the wrong thing.
  • Workflow does not have a chain of custody yet, and what lands can still be different from what was reviewed.
  • Probably plenty of other rough edges that we'll hit over time.

If you try setting this up, I'm interested in feedback about problems you run into or which missing features are the most important for your use case.

BYK added a subscriber: BYK.Oct 28 2015, 1:19 PM

Because this works from the staging repository, it will be possible to trick it right? I mean, I could do this to sneakily commit virus.exe to the repository:

git add virus.exe
git commit -a
# Make a legitimate change
git commit -a
arc diff HEAD~

Yes, that's what this refers to:

Workflow does not have a chain of custody yet, and what lands can still be different from what was reviewed.

Another way to do it is:

  • Comment out the staging push in arc land.
  • arc diff normally.
  • Push arbitrary, wholly different changes to the staging area.

I can't remember which ticket, but I remember a similar discussion regarding arc patch (whether arc patch should patch onto HEAD or pull from the staging area).

Do you have any ideas on how to improve this issue?

I think the discussion is in T8090.

There are two cases here:

  1. The changes to be landed are on top of other changes (as in your example, where virus.exe is snuck in underneath).
  2. The changes to be landed are wholly different from the changes to be reviewed (as in my example, where a malicious user sent a good change for review and pushed virus.exe for merge).

For (1), we already know exactly which commit hashes we expect to land, so the merging work in the LandWorkflow will just get stricter and reject merges which would bring in other commits.

This will create a mild usability issue: if you diff A and then diff B on top of it, and then land A, it would be nice to be able to land B without rebasing it first, but you won't be able to in a mutable repository because the version of A on B will differ from the version of A on master.

For now, you'll just have to rebase. Later, we can refine this by keeping track of prior lands, so when we land commit X (diff A) as commit Q, and then see commit X in the history of B, we can recognize that it's equivalent to Q and just remove both commits from the histories before we check if the merge is exact. This will take some work but we don't need to get it right 100% of the time, since there will always be some cases where we hit conflicts and users legitimately need to rebase anyway.

For (2), I think we need to read the diff out of the staging area instead of having arc submit it. This is going to be pretty messy, but I think it's doable. I'm primarily worried that the client wait may be unreasonably long (I don't see a way to avoid doing a blocking, synchronous read of the pushed commit from arc), but hopefully we can get that down into an acceptable range.

In the long run, with hosted repositories and more flexibility around virtualizing refs (like T5000 and T8092) we can do much better here, and we can do something reasonable today for any hosted repository by generating a diff as a side effect in the commit hook if the push touches a diff tag, but I don't see a super clean way to do this with tracked, external staging areas.

The not-so-clean flow, particularly, is:

  • Push to staging area.
  • Call a new differential.createdifffromstagingarea() Conduit method.
  • This method blocks until the diff is read.

Internally, it has to do something like start a Drydock operation to read the diff, then wait until the operation completes.

One possible alternative is for the import pipeline to start recognizing "magic" refs and doing things as a side effect of import, and to put similar logic on the hook pipeline for hosted repositories. They can theoretically share some code. The workflow as a whole would look the same, except arc would be responsible for polling Conduit to wait until the diff shows up.

All the policy, merge strategy, etc., stuff is hard-coded to the easiest set of reasonable defaults. Some of these defaults may not be suitable for many installs (e.g., always rebase, always merge to master, require edit capability for imported repositories in order to perform merges). I expect to expand these shortly, I just don't want to focus on making things more flexible until they actually work.

Are you planning a fully specifiable merge command so we can run anything we want for merging?

We make heavy use of submodules at my work, such that every change is part of a submodule change. git refuses to merge a super-project if a submodule changed in both branches, even if the changes to the submodule would merge cleanly. We have extended git with a custom command so running git rmerge <branch> from the super project will go through submodules that changed in both branches and merge each of them. If we can fully specify the merge command itself then we can extend git on the phabricator install with our custom rmerge command and invoke it. I realize we could dig into the phabricator source and change anything we want but I'd prefer to stick with the upstream version if possible.

Are you planning a fully specifiable merge command so we can run anything we want for merging?

There will be more flexibility in the future, but I don't anticipate making the merge command configurable, per se (principally, the merge is more complex than a single command, and will become increasingly complex in the future).

You can create a new blueprint which provides WorkingCopy resources and merges in a different way, then use that blueprint type instead of the vanilla WorkingCopy blueprint. However, this will probably be a lot of work to maintain today. Once blueprints stabilize more it may be more realistic.

@epriestley I'll keep an eye on blueprints and the merge configurability. Thank you. Phabricator is awesome/and getting awesomer.

Treri added a subscriber: Treri.Jan 12 2016, 5:57 AM
epriestley moved this task from The Queue to Paused on the Prioritized board.Feb 7 2016, 4:16 AM
eadler added a project: Restricted Project.Feb 23 2016, 7:14 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Feb 23 2016, 7:18 AM
paladox added a subscriber: paladox.Mar 9 2016, 8:43 AM

Add Comment