From 293c518a62f9e056ff83dddfdf42ce20478b6e76 Mon Sep 17 00:00:00 2001 From: dappelt Date: Wed, 22 Jan 2025 11:43:16 +0100 Subject: [PATCH 01/10] Add integrity verification for remote CI includes Adds support for SHA256 integrity verification of remote CI includes. This helps ensure that remote includes haven't been tampered with and allows users to pin specific versions of remote includes. Example usage: include: - remote: 'https://host.com/foo-ci.yml' integrity: 'sha256-BozLjE+uzwZJEC5jBWZoJ+ZfvcTiBlj80d0CsVi+ZsI=' Changelog: added --- lib/gitlab/ci/config/entry/include.rb | 21 ++++++- lib/gitlab/ci/config/external/file/remote.rb | 15 ++++- .../gitlab/ci/config/entry/include_spec.rb | 59 +++++++++++++++++++ .../ci/config/external/file/remote_spec.rb | 38 ++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/config/entry/include.rb b/lib/gitlab/ci/config/entry/include.rb index 1896aad4817fdf..26f6d7cc270f29 100644 --- a/lib/gitlab/ci/config/entry/include.rb +++ b/lib/gitlab/ci/config/entry/include.rb @@ -12,7 +12,7 @@ class Include < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Configurable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[local file remote template component artifact inputs job project ref rules].freeze + ALLOWED_KEYS = %i[local file remote template component artifact inputs job project ref rules integrity].freeze validations do validates :config, hash_or_string: true @@ -28,6 +28,25 @@ class Include < ::Gitlab::Config::Entry::Node if config[:project] && config[:file].blank? errors.add(:config, "must specify the file where to fetch the config from") end + + if config[:integrity] && config[:remote].blank? + errors.add(:config, "integrity can only be specified for remote includes") + end + + if config[:integrity] + unless config[:integrity].is_a?(String) && config[:integrity].start_with?('sha256-') + errors.add(:config, "integrity hash must start with 'sha256-'") + end + + if config[:integrity].is_a?(String) + hash = config[:integrity].delete_prefix('sha256-') + begin + Base64.strict_decode64(hash) + rescue ArgumentError + errors.add(:config, "integrity hash must be base64 encoded") + end + end + end end with_options allow_nil: true do diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb index a7c7c0c2d05c89..33d4dda5567634 100644 --- a/lib/gitlab/ci/config/external/file/remote.rb +++ b/lib/gitlab/ci/config/external/file/remote.rb @@ -20,7 +20,9 @@ def preload_content def content fetch_with_error_handling do - fetch_async_content.value + content = fetch_async_content.value + verify_integrity(content) if params[:integrity] + content end end strong_memoize_attr :content @@ -76,6 +78,17 @@ def fetch_with_error_handling response.body if errors.none? end + + def verify_integrity(content) + expected_hash = params[:integrity].delete_prefix('sha256-') + actual_hash = Base64.strict_encode64( + Digest::SHA256.digest(content) + ) + + unless Rack::Utils.secure_compare(actual_hash, expected_hash) + errors.push("Remote file `#{masked_location}` failed integrity check!") + end + end end end end diff --git a/spec/lib/gitlab/ci/config/entry/include_spec.rb b/spec/lib/gitlab/ci/config/entry/include_spec.rb index c3b826954874f5..68864f5c8f319c 100644 --- a/spec/lib/gitlab/ci/config/entry/include_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/include_spec.rb @@ -114,6 +114,65 @@ end end end + + context 'when using "remote" with integrity' do + let(:config) do + { + remote: 'https://example.com/file.yml', + integrity: 'sha256-abc123def456' + } + end + + it { is_expected.to be_valid } + + context 'when integrity has invalid format' do + let(:config) do + { + remote: 'https://example.com/file.yml', + integrity: 'invalid-hash' + } + end + + it { is_expected.not_to be_valid } + + it 'has specific error' do + expect(include_entry.errors) + .to include('include config integrity hash must start with \'sha256-\'') + end + end + + context 'when integrity is not base64 encoded' do + let(:config) do + { + remote: 'https://example.com/file.yml', + integrity: 'sha256-not!valid@base64' + } + end + + it { is_expected.not_to be_valid } + + it 'has specific error' do + expect(include_entry.errors) + .to include('include config integrity hash must be base64 encoded') + end + end + + context 'when integrity is used without remote' do + let(:config) do + { + local: 'test.yml', + integrity: 'sha256-abc123def456' + } + end + + it { is_expected.not_to be_valid } + + it 'has specific error' do + expect(include_entry.errors) + .to include('include config integrity can only be specified for remote includes') + end + end + end end context 'when value is something else' do diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index 60cd338b49fc67..d505453b06e7f6 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -94,6 +94,30 @@ it { is_expected.to be_falsy } end + + context 'when integrity is specified' do + let(:params) { { remote: location, integrity: integrity_hash } } + + context 'with matching integrity hash' do + let(:integrity_hash) { "sha256-#{Base64.strict_encode64(Digest::SHA256.digest(remote_file_content))}" } + + before do + stub_full_request(location).to_return(body: remote_file_content) + end + + it { is_expected.to be_truthy } + end + + context 'with non-matching integrity hash' do + let(:integrity_hash) { "sha256-#{Base64.strict_encode64(Digest::SHA256.digest('different content'))}" } + + before do + stub_full_request(location).to_return(body: remote_file_content) + end + + it { is_expected.to be_falsy } + end + end end describe "#content" do @@ -244,6 +268,20 @@ expect(subject).to eq "Remote file could not be fetched because Connection refused!" end end + + context 'when integrity check fails' do + let(:params) { { remote: location, integrity: "sha256-#{Base64.strict_encode64(Digest::SHA256.digest('different content'))}" } } + + before do + stub_full_request(location).to_return(body: remote_file_content) + end + + it 'returns error message about integrity check failure' do + expect(error_message).to eq( + 'Remote file `https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.[MASKED]xxx.yml` failed integrity check!' + ) + end + end end describe '#expand_context' do -- GitLab From 980f736c7cb9ea1e05192d20757efaa7722b7ac4 Mon Sep 17 00:00:00 2001 From: dappelt Date: Wed, 22 Jan 2025 12:01:58 +0100 Subject: [PATCH 02/10] Add integrity property to schema --- app/assets/javascripts/editor/schema/ci.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 4d7dc7f26a0e1e..481f5221287e55 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -521,6 +521,11 @@ "format": "uri-reference", "pattern": "^https?://.+\\.ya?ml$" }, + "integrity": { + "description": "SHA256 integrity hash of the remote file content.", + "type": "string", + "pattern": "^sha256-[A-Za-z0-9+/]{43}=$" + }, "rules": { "$ref": "#/definitions/includeRules" }, -- GitLab From 29d509d795c41ca20fa66298dbc41a69ca22b29e Mon Sep 17 00:00:00 2001 From: Dennis Appelt Date: Thu, 23 Jan 2025 09:44:56 +0000 Subject: [PATCH 03/10] Apply 5 suggestion(s) to 4 file(s) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martin Čavoj --- lib/gitlab/ci/config/entry/include.rb | 18 +++++------- lib/gitlab/ci/config/external/file/remote.rb | 6 ++-- .../gitlab/ci/config/entry/include_spec.rb | 28 +++++++++++-------- .../ci/config/external/file/remote_spec.rb | 11 +++----- 4 files changed, 30 insertions(+), 33 deletions(-) diff --git a/lib/gitlab/ci/config/entry/include.rb b/lib/gitlab/ci/config/entry/include.rb index 26f6d7cc270f29..a52fa9c9bb95a4 100644 --- a/lib/gitlab/ci/config/entry/include.rb +++ b/lib/gitlab/ci/config/entry/include.rb @@ -29,22 +29,18 @@ class Include < ::Gitlab::Config::Entry::Node errors.add(:config, "must specify the file where to fetch the config from") end - if config[:integrity] && config[:remote].blank? - errors.add(:config, "integrity can only be specified for remote includes") - end - if config[:integrity] + errors.add(:config, "integrity can only be specified for remote includes") if config[:remote].blank? unless config[:integrity].is_a?(String) && config[:integrity].start_with?('sha256-') errors.add(:config, "integrity hash must start with 'sha256-'") + next end - if config[:integrity].is_a?(String) - hash = config[:integrity].delete_prefix('sha256-') - begin - Base64.strict_decode64(hash) - rescue ArgumentError - errors.add(:config, "integrity hash must be base64 encoded") - end + hash = config[:integrity].delete_prefix('sha256-') + begin + Base64.strict_decode64(hash) + rescue ArgumentError + errors.add(:config, "integrity hash must be base64 encoded") end end end diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb index 33d4dda5567634..3afba902206004 100644 --- a/lib/gitlab/ci/config/external/file/remote.rb +++ b/lib/gitlab/ci/config/external/file/remote.rb @@ -20,9 +20,9 @@ def preload_content def content fetch_with_error_handling do - content = fetch_async_content.value - verify_integrity(content) if params[:integrity] - content + fetch_async_content.value.tap do |content| + verify_integrity(content) if params[:integrity] + end end end strong_memoize_attr :content diff --git a/spec/lib/gitlab/ci/config/entry/include_spec.rb b/spec/lib/gitlab/ci/config/entry/include_spec.rb index 68864f5c8f319c..cf0c94fab6c5e5 100644 --- a/spec/lib/gitlab/ci/config/entry/include_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/include_spec.rb @@ -126,18 +126,22 @@ it { is_expected.to be_valid } context 'when integrity has invalid format' do - let(:config) do - { - remote: 'https://example.com/file.yml', - integrity: 'invalid-hash' - } - end - - it { is_expected.not_to be_valid } - - it 'has specific error' do - expect(include_entry.errors) - .to include('include config integrity hash must start with \'sha256-\'') + ['invalid-hash', 123].each do |invalid_integrity| + context "when integrity is #{invalid_integrity}" do + let(:config) do + { + remote: 'https://example.com/file.yml', + integrity: invalid_integrity + } + end + + it { is_expected.not_to be_valid } + + it 'has specific error' do + expect(include_entry.errors) + .to include('include config integrity hash must start with \'sha256-\'') + end + end end end diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index d505453b06e7f6..5bfb00167a400b 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -98,22 +98,19 @@ context 'when integrity is specified' do let(:params) { { remote: location, integrity: integrity_hash } } + before do + stub_full_request(location).to_return(body: remote_file_content) + end + context 'with matching integrity hash' do let(:integrity_hash) { "sha256-#{Base64.strict_encode64(Digest::SHA256.digest(remote_file_content))}" } - before do - stub_full_request(location).to_return(body: remote_file_content) - end - it { is_expected.to be_truthy } end context 'with non-matching integrity hash' do let(:integrity_hash) { "sha256-#{Base64.strict_encode64(Digest::SHA256.digest('different content'))}" } - before do - stub_full_request(location).to_return(body: remote_file_content) - end it { is_expected.to be_falsy } end -- GitLab From 450b28700a58d4de48a6eee9a3476de65c49a5d1 Mon Sep 17 00:00:00 2001 From: dappelt Date: Thu, 23 Jan 2025 11:30:38 +0100 Subject: [PATCH 04/10] Add linebreak around conditional blocks --- lib/gitlab/ci/config/entry/include.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/ci/config/entry/include.rb b/lib/gitlab/ci/config/entry/include.rb index a52fa9c9bb95a4..13aa8fef929277 100644 --- a/lib/gitlab/ci/config/entry/include.rb +++ b/lib/gitlab/ci/config/entry/include.rb @@ -31,6 +31,7 @@ class Include < ::Gitlab::Config::Entry::Node if config[:integrity] errors.add(:config, "integrity can only be specified for remote includes") if config[:remote].blank? + unless config[:integrity].is_a?(String) && config[:integrity].start_with?('sha256-') errors.add(:config, "integrity hash must start with 'sha256-'") next -- GitLab From 5689b58e4e5646a22ed93d289ab876c48698aab7 Mon Sep 17 00:00:00 2001 From: dappelt Date: Thu, 23 Jan 2025 11:31:43 +0100 Subject: [PATCH 05/10] Remove extra blank line --- spec/lib/gitlab/ci/config/external/file/remote_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index 5bfb00167a400b..b0173de0f8a16f 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -111,7 +111,6 @@ context 'with non-matching integrity hash' do let(:integrity_hash) { "sha256-#{Base64.strict_encode64(Digest::SHA256.digest('different content'))}" } - it { is_expected.to be_falsy } end end -- GitLab From 5820c0d3e5f9b15e942f741d2d1ed9c96ca65b33 Mon Sep 17 00:00:00 2001 From: dappelt Date: Wed, 12 Feb 2025 14:31:09 +0100 Subject: [PATCH 06/10] Add docs for include:remote integrity hash --- doc/ci/yaml/_index.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/doc/ci/yaml/_index.md b/doc/ci/yaml/_index.md index 5386cc61296aab..34fa8e63131764 100644 --- a/doc/ci/yaml/_index.md +++ b/doc/ci/yaml/_index.md @@ -177,6 +177,7 @@ And optionally: - [`include:inputs`](#includeinputs) - [`include:rules`](#includerules) +- [`include:integrity`](#includeintegrity) **Additional details**: @@ -349,7 +350,8 @@ include: so you can only include public projects or templates. No variables are available in the `include` section of nested includes. - Be careful when including another project's CI/CD configuration file. No pipelines or notifications trigger when the other project's files change. From a security perspective, this is similar to - pulling a third-party dependency. If you link to another GitLab project you own, consider the use of both + pulling a third-party dependency. To verify the integrity of the included file, consider specifying using the [`integrity`](#includesintegrity) keyword. + If you link to another GitLab project you own, consider the use of both [protected branches](../../user/project/repository/branches/protected.md) and [protected tags](../../user/project/protected_tags.md#prevent-tag-creation-with-the-same-name-as-branches) to enforce change management rules. @@ -465,6 +467,22 @@ In this example, if the `INCLUDE_BUILDS` variable is: - [`rules:changes`](includes.md#include-with-ruleschanges). - [`rules:exists`](includes.md#include-with-rulesexists). +#### `includes:integrity` + +**Keyword type**: Global keyword. + +**Supported values**: Base64-encoded string of the included file's SHA256 hash. + +You can use `integrity` with `include:remote` to specifiy a SHA256 hash of the included remote file. If `integrity` does not match the actual content, the remote file will not be executed and the pipeline fails. + +**Example of `include:integrity`**: + +```yaml +include: + - remote: 'https://gitlab.com/example-project/-/raw/main/.gitlab-ci.yml' + integrity: 'sha256-L3/GAoKaw0Arw6hDCKeKQlV1QPEgHYxGBHsH4zG1IY8=' +``` + ### `stages` > - Support for nested array of strings [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/439451) in GitLab 16.9. -- GitLab From 4d8ffd1e08a73375855703ad4f0e3dd0a9167518 Mon Sep 17 00:00:00 2001 From: dappelt Date: Wed, 12 Feb 2025 14:32:41 +0100 Subject: [PATCH 07/10] Fix doc --- doc/ci/yaml/_index.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ci/yaml/_index.md b/doc/ci/yaml/_index.md index 34fa8e63131764..92d03450a3873f 100644 --- a/doc/ci/yaml/_index.md +++ b/doc/ci/yaml/_index.md @@ -350,7 +350,7 @@ include: so you can only include public projects or templates. No variables are available in the `include` section of nested includes. - Be careful when including another project's CI/CD configuration file. No pipelines or notifications trigger when the other project's files change. From a security perspective, this is similar to - pulling a third-party dependency. To verify the integrity of the included file, consider specifying using the [`integrity`](#includesintegrity) keyword. + pulling a third-party dependency. To verify the integrity of the included file, consider using the [`integrity`](#includesintegrity) keyword. If you link to another GitLab project you own, consider the use of both [protected branches](../../user/project/repository/branches/protected.md) and [protected tags](../../user/project/protected_tags.md#prevent-tag-creation-with-the-same-name-as-branches) to enforce change management rules. @@ -471,7 +471,7 @@ In this example, if the `INCLUDE_BUILDS` variable is: **Keyword type**: Global keyword. -**Supported values**: Base64-encoded string of the included file's SHA256 hash. +**Supported values**: Base64-encoded SHA256 hash of the included content. You can use `integrity` with `include:remote` to specifiy a SHA256 hash of the included remote file. If `integrity` does not match the actual content, the remote file will not be executed and the pipeline fails. -- GitLab From b941ad4d00e6cf88d9236cbe351361eddafb3811 Mon Sep 17 00:00:00 2001 From: dappelt Date: Wed, 12 Feb 2025 15:19:03 +0100 Subject: [PATCH 08/10] Fix link --- doc/ci/yaml/_index.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ci/yaml/_index.md b/doc/ci/yaml/_index.md index 92d03450a3873f..28e665b66ad34e 100644 --- a/doc/ci/yaml/_index.md +++ b/doc/ci/yaml/_index.md @@ -350,7 +350,7 @@ include: so you can only include public projects or templates. No variables are available in the `include` section of nested includes. - Be careful when including another project's CI/CD configuration file. No pipelines or notifications trigger when the other project's files change. From a security perspective, this is similar to - pulling a third-party dependency. To verify the integrity of the included file, consider using the [`integrity`](#includesintegrity) keyword. + pulling a third-party dependency. To verify the integrity of the included file, consider using the [`integrity`](#includeintegrity) keyword. If you link to another GitLab project you own, consider the use of both [protected branches](../../user/project/repository/branches/protected.md) and [protected tags](../../user/project/protected_tags.md#prevent-tag-creation-with-the-same-name-as-branches) to enforce change management rules. @@ -467,7 +467,7 @@ In this example, if the `INCLUDE_BUILDS` variable is: - [`rules:changes`](includes.md#include-with-ruleschanges). - [`rules:exists`](includes.md#include-with-rulesexists). -#### `includes:integrity` +#### `include:integrity` **Keyword type**: Global keyword. -- GitLab From 911924250aa150d7a3b9e614140ac3d014889cad Mon Sep 17 00:00:00 2001 From: dappelt Date: Mon, 17 Feb 2025 13:57:48 +0100 Subject: [PATCH 09/10] Address review feedback --- doc/ci/yaml/_index.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/ci/yaml/_index.md b/doc/ci/yaml/_index.md index 28e665b66ad34e..63becccdd25194 100644 --- a/doc/ci/yaml/_index.md +++ b/doc/ci/yaml/_index.md @@ -469,12 +469,14 @@ In this example, if the `INCLUDE_BUILDS` variable is: #### `include:integrity` +Use `integrity` with `include:remote` to specifiy a SHA256 hash of the included remote file. +If `integrity` does not match the actual content, the remote file is not processed +and the pipeline fails. + **Keyword type**: Global keyword. **Supported values**: Base64-encoded SHA256 hash of the included content. -You can use `integrity` with `include:remote` to specifiy a SHA256 hash of the included remote file. If `integrity` does not match the actual content, the remote file will not be executed and the pipeline fails. - **Example of `include:integrity`**: ```yaml -- GitLab From bf0e06ebaf5346065cd186db014bf191e163381d Mon Sep 17 00:00:00 2001 From: Dennis Appelt Date: Mon, 17 Feb 2025 12:59:23 +0000 Subject: [PATCH 10/10] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Marcel Amirault --- doc/ci/yaml/_index.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/ci/yaml/_index.md b/doc/ci/yaml/_index.md index 63becccdd25194..95b5587922909b 100644 --- a/doc/ci/yaml/_index.md +++ b/doc/ci/yaml/_index.md @@ -469,6 +469,8 @@ In this example, if the `INCLUDE_BUILDS` variable is: #### `include:integrity` +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178593) in GitLab 17.9. + Use `integrity` with `include:remote` to specifiy a SHA256 hash of the included remote file. If `integrity` does not match the actual content, the remote file is not processed and the pipeline fails. -- GitLab