| Age | Commit message (Collapse) | Author |
|---|
|
git: Upgrade to Git security release v2.38.4.gl1 and v2.39.2
See merge request https://gitlab.com/gitlab-org/security/gitaly/-/merge_requests/79
Merged-by: Reuben Pereira <2967854-rpereira2@users.noreply.gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Fixes https://gitlab.com/gitlab-org/gitaly/-/issues/4645 by avoiding situations where the working dir may be
a symlink, but SOURCE_DIR is the resolved path.
|
|
Upgrade our Git version to v2.38.4.gl1 and v2.39.2, which pulls in the
security releases Git v2.38.4 and v2.39.2 that addresses the following
CVEs:
- CVE-2023-22490:
Using a specially-crafted repository, Git can be tricked into using
its local clone optimization even when using a non-local transport.
Though Git will abort local clones whose source $GIT_DIR/objects
directory contains symbolic links (c.f., CVE-2022-39253), the objects
directory itself may still be a symbolic link. These two may be
combined to include arbitrary files based on known paths on the
victim's filesystem within the malicious repository's working copy,
allowing for data exfiltration in a similar manner as CVE-2022-39253.
- CVE-2023-23946:
By feeding a crafted input to "git apply", a path outside the working
tree can be overwritten as the user who is running "git apply".
Changelog: security
|
|
Build and install a new bundled Git version for the v2.39 release track.
|
|
|
|
The libgit2 project has published a security release for CVE-2023-22742.
This vulnerability allows for man-in-the-middle attacks because by
default, libgit2 would not verify SSH keys of the remote server when
connecting to it.
Gitaly is not impacted by this CVE given that we never use libgit2 to
connect to a remote server. But let's upgrade anyway in case anybody
wonders.
|
|
The `build-proto-gem` target has its own Gemfile to require the
`grpc-tools` gem, but does not run `bundle install` in `tools/protogem`
directory first.
Update the Makefile to install protogem's dependency before building.
Changelog: fixed
|
|
At GitLab we have recently received a report where a repository was left
with a corrupted `packed-refs` file after the node hard-crashed even
though `core.fsync=reference` was set. This is something that in theory
should not happen if we correctly did the atomic-rename dance. So even
if we crash in the middle of writing the `packed-refs` file we should
only ever see either the old or the new state of the file.
And while we do the dance when writing the `packed-refs` file, there is
indeed one gotcha: we use a `FILE *` stream to write the temporary file,
but don't flush it before synchronizing it to disk. As a consequence any
data that is still buffered will not get synchronized and a crash of the
machine may cause corruption.
We have upstreamed a fix into Git v2.39.0 and backported that fix to Git
v2.38.3.gl2. So let's upgrade our Git version to fix this bug.
Changelog: fixed
|
|
Typically, we have up to three different Git versions in Gitaly:
- Two bundled Git versions that can be toggled with a feature flag.
- The distributed Git version.
The distributed Git version will always be matching one of the bundled
Git versions, namely the one that is the current default. So let's
deduplicate these versions and just reuse the bundled Git's version so
that we don't need to remember updating the version in multiple places
on minor version bumps.
|
|
The Git project has published security releases for two different CVEs:
* CVE-2022-41903:
git log has the ability to display commits using an arbitrary
format with its --format specifiers. This functionality is also
exposed to git archive via the export-subst gitattribute.
When processing the padding operators (e.g., %<(, %<|(, %>(,
%>>(, or %><( ), an integer overflow can occur in
pretty.c::format_and_pad_commit() where a size_t is improperly
stored as an int, and then added as an offset to a subsequent
memcpy() call.
This overflow can be triggered directly by a user running a
command which invokes the commit formatting machinery (e.g., git
log --format=...). It may also be triggered indirectly through
git archive via the export-subst mechanism, which expands format
specifiers inside of files within the repository during a git
archive.
This integer overflow can result in arbitrary heap writes, which
may result in remote code execution.
* CVE-2022-23521:
gitattributes are a mechanism to allow defining attributes for
paths. These attributes can be defined by adding a `.gitattributes`
file to the repository, which contains a set of file patterns and
the attributes that should be set for paths matching this pattern.
When parsing gitattributes, multiple integer overflows can occur
when there is a huge number of path patterns, a huge number of
attributes for a single pattern, or when the declared attribute
names are huge.
These overflows can be triggered via a crafted `.gitattributes` file
that may be part of the commit history. Git silently splits lines
longer than 2KB when parsing gitattributes from a file, but not when
parsing them from the index. Consequentially, the failure mode
depends on whether the file exists in the working tree, the index or
both.
This integer overflow can result in arbitrary heap reads and writes,
which may result in remote code execution.
Upgrade Git to v2.38.3 to address these CVEs.
Changelog: security
|
|
The script to build the Gitaly ruby gem compares the VERSION file with
the current tag (via `git describe --tags`), this is a safety check so
that we don't build an publish non-tagged releases.
But this also blocks us from building the gem for local testing. So add
a new option `---skip-verify-tag` to the build script which can skip
this verification.
Since we're adding OptionParser also move the previously used argument
to an option.
|
|
git: Bump minimum required Git version to v2.38.0
Closes #4672
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5210
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
We have removed the infrastructure to use bundled Git v2.37 in v15.7,
but retained the binaries for backwards compatibility. Remove them.
|
|
Before converting to the new `WithGoBuildInformation()` monitoring
option in the preceding commit we always exposed the build time. We have
stopped doing that now and instead only expose information that Go puts
into the binary itself.
As a result, we can now drop the infrastructure to embed the build time
into the binary. This has the advantage that it significantly improves
incremental build times: previously we'd basically have to rebuild all
binaries every single time, as the embedded build time changes every
second. As we stop doing that we can in many cases skip this step. On my
machine this leads to a 3.5x build speedup.
Benchmark 1: make
Time (mean ± σ): 12.163 s ± 0.148 s [User: 16.677 s, System: 3.496 s]
Range (min … max): 12.044 s … 12.328 s 3 runs
Benchmark 2: make GO_LDFLAGS=
Time (mean ± σ): 3.469 s ± 0.060 s [User: 5.593 s, System: 1.880 s]
Range (min … max): 3.433 s … 3.539 s 3 runs
Summary
'make GO_LDFLAGS=' ran
3.51 ± 0.07 times faster than 'make'
This has been benchmarked with the preceding commit which still embedded
the build time into the binary. By stubbing out the `GO_LDFLAGS`, which
is how we put the build time into the resulting binary, we are able to
emulate the new behaviour with this commit.
But besides the quality-of-life improvements for developers another
significant advantage is that we now have reproducible builds.
Changelog: fixed
|
|
Remove infrastructure to clone the "gitlab-test-mirror.git" and
"gitlab-git-test.git" seed repositories. They are not used anymore.
|
|
Git has recently created the bugfix release v2.38.2. This commit
upgrades the Git version used by Gitaly to pull in Git v2.38.2.gl1,
which contains all bugfixes of Git v2.38.2. Furthermore, we have
backported a bunch of patches to fix performance issues when pushing
into repostiories with loads of references. These backported changes
have been released as part of Git v2.39.0 already.
While at it, also bump the Git v2.37 release series to use v2.37.4.gl1.
This includes bugfixes as well as fixes for CVEs that don't apply in the
context of Gitaly. Note that the naming of the `GIT_VERSION_2_37_1`
variable is now kind of awkward as it refers to Git v2.37.4.gl1. We're
phasing out this Git version anyway though, so it doesn't feel worth it
to adjust naming of this variable now.
Changelog: fixed
|
|
In the previous commit we started to use git 2.38 all the time when
using bundled git.
This change bumps the non-bundled git version to also default to 2.38,
unless the user specifies otherwise.
|
|
Makefile: Fix broken Git version in nightly CI jobs
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5047
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
In our nightly CI jobs we test with Git's `main` respectivey `next`
branch. Starting with 8bd7becbb (Makefile: Use Gitaly's tagged Git
versions instead of ad-hoc patching, 2022-11-11), we unconditionally
write the `GIT_VERSION` into Git's source tree to override the version
that e.g. `git --version` would report. But when `GIT_VERSION=master`,
then we fail to parse the Git version in Gitaly and thus fail all code
paths that depend on this.
Fix this by introducing a new variable that, if set, skips overriding
the Git version. This variable is set in our nightly jobs now.
|
|
Now that we've moved our test-related Protobuf definitions into `proto/`
our Protobuf linter automatically picks them up to be linted. This does
not make a whole lot of sense though given that the definitions are not
typically used outside of Gitaly, so we don't care whether they're well
commented or any somesuch thing.
Exclude these Protobuf files from our linter.
|
|
The `no-proto-changes` target first regenerates the Protobuf files and
then checks whether there are any changes compared to the Protobuf
definitions. To regenerate the files, we first delete the target
directory that contains the generated code so that we start from a clean
slate. This is causing problems though when this target is executed via
`make verify`, which besides other targets also executes the `lint`
target. Now if the code is getting deleted while the linter runs in
parallel then it may happen that linting fails due to the missing code.
Now that we have the ability to generate Protobuf code in a different
directory thoulgh this is easy to fix. Instead of modifying our source
directory, we can just generate Protobuf files in our build directory
and compare this directory with our sources. Like this, actual source
files files will never get deleted and thus the linter will be happy.
|
|
Now that all generated Protobuf code lives in `proto/go` it is simple
enough for us to change the location of generated code to a different
destination. Introduce a variable that allows the caller to override
this location. This will be used by the subsequent commit, which changes
the logic for how we verify that generated code matches our definitions.
|
|
In the past, there were Go code files other than the generated ones in
the `proto/` directory. That's not the case nowadays anymore as we have
since moved code for Protobuf-related tooling into `tools/`.
Simplify the condition to find our Go sources to just exclude all Go
files located in `proto/`. While at it, convert the excludes for the
top-level `_build` and `ruby` directory to also use `-path` so that we
don't accidentally skip any directories in Go-related subdirectories
that happen to be called like this. And finally, drop the exclude for
the `vendor` folder that we don't use anymore.
|
|
Generating the Protobuf code is not a long-running tasks, and thus it's
not really necessary to print this command by default. Silence it so
that the output is less cluttered by default.
|
|
While we were able to get rid of all custom Protobuf definitions used
for tests, this is definitely not possible for our tests that verify the
behaviour of `protoc-gen-gitaly-lint`. Its whole purpose is to lint
invalid Protobuf definitions, and we of course ain't got any in our
normal gRPC services.
We can still make the generation of this test-specific Protobuf code
easier by moving the definitions into a new `proto/testproto` directory
so that they live in the same filesystem hierarchy as the other Protobuf
definitions. This both allows us to unify the call to protoc and will
allow us to easily change the directory where the code is generated in
in a subsequent commit.
|
|
The tests for our `cache` middleware use a custom gRPC service
definition to assert its behaviour. This is not necessary though as
we can just pick gRPC calls from different Gitaly services.
Refactor the test to use this package and get rid of our own Protobuf
definitions. Note that we indeed have to drop one testcase for streaming
maintenance-style RPCs as there is no such RPC definition.
|
|
Remove the mock service that has previously been used by our Praefect
tests.
|
|
The tests for our `chunk` helper use a custom gRPC service definition to
assert its behaviour. This is not necessary though as `grpc-go` already
provides a `grpc_testing` package that contains a gRPC service that is
designed for testing gRPC-specific behaviour.
Refactor the test to use this package and get rid of our own Protobuf
definitions.
|
|
The tests for our `limithandler` middleware use a custom gRPC service
definition to assert its behaviour. This is not necessary though as
`grpc-go` already provides a `grpc_testing` package that contains a gRPC
service that is designed for testing gRPC-specific behaviour.
Refactor the test to use this package and get rid of our own Protobuf
definitions.
|
|
Right now, the way we apply Git patches is by adding them to the Gitaly
project and using git-apply(1) to apply them ad-hoc. This is starting to
show its limits though:
- It is hard for us to provide a simple pointer to the Git sources
that we distribute to the customer.
- It is hard to execute tests for the patched Git version in an
automated fashion.
- It is hard to work on top of the already-patched Git distribution
to for example apply more patches.
These limitations are getting more noticeable now that we have split up
the Gitaly team into two teams, where we potentially want to backport
patches more aggressively.
With the split we have now made the repository at [1] the canonical Git
repository for all our efforts. This indeed opens up a much better way
to use custom Git versions: instead of hosting the patches in the Gitaly
repository, we start to tag Gitaly-specific releases in that repository.
These releases then carry all the additional patches on top.
This makes it trivial to use normal workflows:
- We can point customers to the Gitaly-specific tags which hold our
patches on top.
- We can automatically trigger CI pipelines on top of patched
Gitaly-specific releases by just pushing branches or tags.
- You can just clone the repository and checkout out the specific
tags.
Drop the infrastructure to patch releases in-place in favor of this new
architecture.
[1]: https://gitlab.com/gitlab-org/git.git
|
|
Expose logs as artifacts when running tests on CI
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5002
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Gitaly test suite on CI doesn't expose any logs. Sometimes, it's not
easy to re-produce the tests on local environment. This commit exposes
the logs generated while running tests. Those logs are then persisted
as the artifacts of CI jobs for a reasonable amount of time.
The folder storing logs are configured from TEST_LOG_DIR env variable.
The dir is created and injected into testcfg builder and testserver as
the dependency of started Gitaly server.
We can also produce the logs on the local environment with
TEST_LOG_DIR=/tmp/gitaly-logs make test-go
Changelog: other
|
|
The default TEST_FORMAT is 'short', which doesn't output any expect for
success or failure. This is pretty useless in the case of benchmarks,
because you want to see the numbers of the benchmark.
Therefore override the TEST_FORMAT to 'standard-verbose' for the 'bench'
target.
|
|
The 'bench' target relied on Make handling the prerequisites in order,
never the less, this is not guaranteed.
To fix this, specify the recipe explicitly so all prerequisites are met
before the benchmark is started.
|
|
Now that we have converted our CI to both build and test Gitaly as the
same unprivileged user there is no need for the `UNPRIVILEGED_CI_SKIP`
variable anymore that skips building binaries. Remove it.
|
|
The `.ruby-bundle` file is used to control when we need to run `bundle
install`: when older than either `Gemfile` or `Gemfile.lock` then we
know we need to re-run the command. This mechanism is also used by CNG
and Omnibus to control execution of this command. They both touch(1) the
file so that it is newer than its dependencies as a workaround so that
`make build` won't re-install any Ruby Gems that have already been
installed anyway.
This is definitely an awful workaround and should ideally be changed so
that we instead provide a variable that lets callers disable installing
Ruby Gems instead of having to reach into some internal details of our
build system. But fixing this now would be quite pointless as we are in
the process of retiring the Ruby sidecar.
The current way this works is about to cause problems though: we need to
adapt the way our unprivileged CI testing works to both build and run
tests as the unprivileged user, who cannot modify the source directory
at all. But as the `.ruby-bundle` file is located directly in the source
directory this refactor would break installing the Ruby Gem.
Let's work around this issue by making the location of `.ruby-bundle`
configurable via a Makefile variable. While ugly, we'll get rid of this
in a few releases anyway.
|
|
Our CI job that builds Gitaly's binaries will build and install a Git
distribution when not instructed to use bundled Git binaries. While we
indeed want to build the Git distribution so that we can cache it,
installing it into `/usr/local` is entirely unnecessary. Furthermore,
we're about to convert the build to run as an unprivileged user who
cannot install into `/usr/local`, and thus the build would break.
Expose a new build target `build-git` that allows us to easily build the
Git distribution without installing it and convert the CI job to use it
instead of the old `git` target.
|
|
Add git 2.38 as a build target in Makefile
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4943
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
|
|
The `generate-proto-ruby` script is not only generating the Ruby sources
anymore, but also builds the `gitaly` Gem. It wasn't renamed in the
preceding commit though as Git doesn't detect the rename by default,
which would have made the diff hard to read.
Rename the file now to the more fitting `build-proto-gem`.
|
|
The Ruby sidecar is slowly nearing its end: right now, it only
implements a single RPC that is in the process of being migrated to Go.
This means we'll start to soon remove all the Ruby infrastructure in the
Gitaly project.
Part of the infrastructure is the Ruby code generated from our Protobuf
definitions. And while we won't need that code ourselves anymore, it is
important for clients written in Ruby that want to interface with us. As
such, the Gitaly project still has to release the `gitaly` Gem. In the
past, we built the Gem by simply soaking up the code in `ruby/proto`.
But as it does not make any sense for us to keep the Ruby code around
anymore this strategy will fail once we have removed it.
Merge the `generate-proto-ruby` any `publish-gem` scripts into a single
script that:
1. Generates all required Ruby sources into a temporary directory.
2. Writes the `gitaly.gemspec` file into the same directory.
3. Builds the Gitaly Gem from those temporary files.
Like this, all code is generated ad-hoc and doesn't need to be committed
to the repository anymore.
Change the Makefile's `proto` target to not generate Ruby code anymore.
Instead, there are two new Makefile targets `build-proto-gem` and
`publish-proto-gem` that are responsible for building and publishing the
Gem, respecfively.
The generated Ruby code is thus stale now and will be removed in a
subsequent commit.
|
|
We've got multiple scripts that are required to generate Ruby code from
our Protobuf definitions in the `_support` directory. This has multiple
smells:
- It's out-of-line with all the other tools, which nowadays are
located in the `tools` directory.
- It's hard to discover and find out which parts logically form a
unit.
- We are reusing the Gemfile of the Ruby sidecar to pin the
`grpc-tools` dependency to a specific version.
Move the tooling into its own `tools/protogem` directory that's got its
own Gemfile to fix these points. This also allows us to auto-update
dependencies via the Renovate bot like we do for our other tools.
|
|
The PHONY variable `debug-go-tests` doesn't have a target. Seems like it
was added by mistake instead of adding `debug-test-go` which has a
target. This often causes confusion on the term as autocomplete provides
both options but the former doesn't have any effect.
|
|
Also expand reach of Markdown linting to more files
|
|
We're compiling the Go module version of the Gitaly package into the
code. This was used in the past to locate the correct set of auxiliary
binaries like `gitaly-git2go-v15`. But since we have started to package
those binaries into the main `gitaly` executable we don't need to tell
apart different module versions anymore. We have thus removed the last
user of `version.GetModuleVersion()` in 1ba0ce586 (Hardcode module
version in git2go binary name variable, 2022-06-28).
Drop the infrastructure to expose the module version.
|
|
git: Bump minimum version to v2.37.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4876
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
Makefile: Build git with gitlab-org/git
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4939
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
|
|
In the commit (cc04215eb) we removed the flag to enable git v2.37.0.
Making it now the default git version. Now we can remove the older git
version v2.35.0. In this commit remove it from the Makefile. This means
it will no longer be bundled with Gitaly.
Also remove the patches added for git v2.35.0, which are no longer
required.
|
|
The `check-proto` target gets executed as part of the `verify`
target by our CI to assert that code generated from our Protobuf
definitions matches the committed code and that it passes our linting
rules. But the former part to check for changes is racy: `check-proto`
depends on both `proto` and `no-proto-changes` in order to first
regenerate files and then check for changes. But if make(1) is executed
with multiple jobs these can run in parallel as `no-proto-changes` does
not depend on `proto`. Because executing git-diff(1) is faster than
generating the code the end result is that we don't ever notice
out-of-date code.
Fix this error by fixing the dependency chain so that `no-proto-changes`
depends on `proto`.
|
|
Upgrade the protoc compiler to the latest version at the time of writing
this commit, which is v21.7. There are no relevant changes, except for
improved formatting of Go comments according to recent changes in gofmt.
Regenerate the Protobuf code.
|
