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
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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