Skip to content

gitaly: Fix access checks with transactions and quarantine environments

What does this MR do?

In order to check whether certain operations are allowed to be executed by a user, Gitaly POSTs to the /internal/allowed endpoint. The request includes information about what change the user wants to perform, but it also contains information about the environment the change is currently performed in.

When a user performs a push, git will first store all pushed objects into a quarantine environment. This is a separate temporary directory containing all new objects such that if the push gets rejected, new objects will not persist in the repository. The crux is that in order to inspect these new objects, git needs to be told that such a quarantine environment exists. This is why Gitaly sends information about this quarantine environment to /internal/allowed, so that we can again relay this information back to Gitaly when we want to inspect newly pushed objects to determine whether they're allowed or not.

While it's a leaky abstraction, it has worked fine until now. But with transactions, that's not true anymore: when multiple Gitaly nodes take part in a push, then they'll all generate a randomly named quarantine environment. But as only the primary node will inject its info into the request, we are not able to acces quarantine environments of secondary nodes. If we now route accessor requests to any of the secondary Gitaly nodes with the quarantine environment of the primary, then the request will fail as git cannot find quarantined objects.

To fix this, Gitaly has recently grown a new GRPC header which allows us to force-route requests to the primary via 1102b0b67 (praefect: Implement force-routing to primary for node-manager router, 2021-02-03) and 4d877d7d5 (praefect: Implement force-routing to primary for per-repo router, 2021-02-03). So let's set that header if we know that we're being executed via a hook, which is the only case where a quarantine environment may exist.

Part of gitaly#3438 (closed)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Patrick Steinhardt

Merge request reports