From c6eaf0d5c7c5827a552e2fad3c3a5876c519148d Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 8 Nov 2022 16:48:54 +0100 Subject: [PATCH 01/15] Add `id_tokens` CI keyword The new `id_tokens` keyword can be used to define JWTs for a job that can be accessed using the specified variable name. Changelog: added --- app/models/ci/build.rb | 2 +- .../build_metadata_id_tokens.json | 23 ++++++++------- .../ee/ci/build_runner_presenter.rb | 8 +++++- ee/lib/gitlab/ci/config/entry/secret.rb | 9 +++++- lib/gitlab/ci/config/entry/id_token.rb | 28 +++++++++++++++++++ lib/gitlab/ci/config/entry/job.rb | 10 +++++-- lib/gitlab/ci/yaml_processor/result.rb | 1 + 7 files changed, 66 insertions(+), 15 deletions(-) create mode 100644 lib/gitlab/ci/config/entry/id_token.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index f44ba124fe206d..266998c2a02622 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1229,7 +1229,7 @@ def id_tokens_variables Gitlab::Ci::Variables::Collection.new.tap do |variables| id_tokens.each do |var_name, token_data| - token = Gitlab::Ci::JwtV2.for_build(self, aud: token_data['id_token']['aud']) + token = Gitlab::Ci::JwtV2.for_build(self, aud: token_data['aud']) variables.append(key: var_name, value: token, public: false, masked: true) end diff --git a/app/validators/json_schemas/build_metadata_id_tokens.json b/app/validators/json_schemas/build_metadata_id_tokens.json index 7f39c7274f3942..6c87b2bc12823e 100644 --- a/app/validators/json_schemas/build_metadata_id_tokens.json +++ b/app/validators/json_schemas/build_metadata_id_tokens.json @@ -5,18 +5,21 @@ "patternProperties": { ".*": { "type": "object", - "patternProperties": { - "^id_token$": { - "type": "object", - "required": ["aud"], - "properties": { - "aud": { "type": "string" }, - "field": { "type": "string" } - }, - "additionalProperties": false + "required": [ + "aud" + ], + "properties": { + "aud": { + "type": [ + "array", + "string" + ], + "items": { + "type": "string" + } } }, "additionalProperties": false } } -} +} \ No newline at end of file diff --git a/ee/app/presenters/ee/ci/build_runner_presenter.rb b/ee/app/presenters/ee/ci/build_runner_presenter.rb index 254913e97be021..ec09c4df580a06 100644 --- a/ee/app/presenters/ee/ci/build_runner_presenter.rb +++ b/ee/app/presenters/ee/ci/build_runner_presenter.rb @@ -40,10 +40,16 @@ def vault_jwt(secret) def id_token_var(secret) return unless id_tokens? - token = secret['token'] || id_tokens.each_key.first + token = specified_token(secret) || id_tokens.each_key.first "${#{token}}" end + + def specified_token(secret) + return unless secret['token'].present? + + secret['token'][1..] + end end end end diff --git a/ee/lib/gitlab/ci/config/entry/secret.rb b/ee/lib/gitlab/ci/config/entry/secret.rb index d0f595f2a1d632..3fe514af4e176c 100644 --- a/ee/lib/gitlab/ci/config/entry/secret.rb +++ b/ee/lib/gitlab/ci/config/entry/secret.rb @@ -12,7 +12,7 @@ class Secret < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Attributable REQUIRED_KEYS = %i[vault].freeze - ALLOWED_KEYS = (REQUIRED_KEYS + %i[file]).freeze + ALLOWED_KEYS = (REQUIRED_KEYS + %i[file token]).freeze attributes ALLOWED_KEYS @@ -21,6 +21,13 @@ class Secret < ::Gitlab::Config::Entry::Node validations do validates :config, allowed_keys: ALLOWED_KEYS, required_keys: REQUIRED_KEYS + validates :token, type: String, allow_nil: true + end + + def value + return super unless token.present? + + super.merge(token: token) end end end diff --git a/lib/gitlab/ci/config/entry/id_token.rb b/lib/gitlab/ci/config/entry/id_token.rb new file mode 100644 index 00000000000000..2d277773e83115 --- /dev/null +++ b/lib/gitlab/ci/config/entry/id_token.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + class Config + module Entry + ## + # Entry that represents a JWT definition. + # + class IdToken < ::Gitlab::Config::Entry::Node + include ::Gitlab::Config::Entry::Attributable + include ::Gitlab::Config::Entry::Validatable + + attributes %i[aud] + + validations do + validates :config, required_keys: %i[aud], allowed_keys: %i[aud] + validates :aud, presence: true, array_of_strings_or_string: true + end + + def value + { aud: aud } + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 8e7f6ba4326fdf..ab17e1e3870b3f 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -14,7 +14,7 @@ class Job < ::Gitlab::Config::Entry::Node ALLOWED_KEYS = %i[tags script image services start_in artifacts cache dependencies before_script after_script environment coverage retry parallel interruptible timeout - release].freeze + release id_tokens].freeze validations do validates :config, allowed_keys: Gitlab::Ci::Config::Entry::Job.allowed_keys + PROCESSABLE_ALLOWED_KEYS @@ -116,6 +116,11 @@ class Job < ::Gitlab::Config::Entry::Node description: 'Indicates whether this job is allowed to fail or not.', inherit: false + entry :id_tokens, ::Gitlab::Config::Entry::ComposableHash, + description: 'Configured JWTs for this job', + inherit: false, + metadata: { composable_class: ::Gitlab::Ci::Config::Entry::IdToken } + attributes :script, :tags, :when, :dependencies, :needs, :retry, :parallel, :start_in, :interruptible, :timeout, @@ -158,7 +163,8 @@ def value ignore: ignored?, allow_failure_criteria: allow_failure_criteria, needs: needs_defined? ? needs_value : nil, - scheduling_type: needs_defined? ? :dag : :stage + scheduling_type: needs_defined? ? :dag : :stage, + id_tokens: id_tokens_value ).compact end diff --git a/lib/gitlab/ci/yaml_processor/result.rb b/lib/gitlab/ci/yaml_processor/result.rb index ff255543d3b2f2..31a4e10a53511d 100644 --- a/lib/gitlab/ci/yaml_processor/result.rb +++ b/lib/gitlab/ci/yaml_processor/result.rb @@ -107,6 +107,7 @@ def build_attributes(name) cache: job[:cache], resource_group_key: job[:resource_group], scheduling_type: job[:scheduling_type], + id_tokens: job[:id_tokens], options: { image: job[:image], services: job[:services], -- GitLab From eab9c3790d7709f276a1c2a761a49295a7e1051f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 8 Nov 2022 18:08:54 +0100 Subject: [PATCH 02/15] Update `secrets` CI schema with `token` Also, adds `file` and improves the schema --- app/assets/javascripts/editor/schema/ci.json | 101 ++++++++++--------- 1 file changed, 56 insertions(+), 45 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 45f063a2048cdf..d66f9c6faaa3a4 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -588,53 +588,64 @@ "secrets": { "type": "object", "markdownDescription": "Defines secrets to be injected as environment variables. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#secrets).", - "additionalProperties": { - "type": "object", - "description": "Environment variable name", - "properties": { - "vault": { - "oneOf": [ - { - "type": "string", - "description": "The secret to be fetched from Vault (e.g. 'production/db/password@ops' translates to secret 'ops/data/production/db', field `password`)" - }, - { - "type": "object", - "properties": { - "engine": { - "type": "object", - "properties": { - "name": { - "type": "string" + "patternProperties": { + ".*": { + "type": "object", + "properties": { + "vault": { + "oneOf": [ + { + "type": "string", + "description": "The secret to be fetched from Vault (e.g. 'production/db/password@ops' translates to secret 'ops/data/production/db', field `password`)" + }, + { + "type": "object", + "properties": { + "engine": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "path": { + "type": "string" + } }, - "path": { - "type": "string" - } + "required": [ + "name", + "path" + ] }, - "required": [ - "name", - "path" - ] - }, - "path": { - "type": "string" + "path": { + "type": "string" + }, + "field": { + "type": "string" + } }, - "field": { - "type": "string" - } - }, - "required": [ - "engine", - "path", - "field" - ] - } - ] - } - }, - "required": [ - "vault" - ] + "required": [ + "engine", + "path", + "field" + ] + } + ] + }, + "file": { + "type": "boolean", + "default": true, + "description": "Configures the secret to be stored as either a file or variable type CI/CD variable." + }, + "token": { + "type": "string", + "description": "Specifies the JWT variable that should be used to authenticate with Hashicorp Vault." + } + }, + "required": [ + "vault" + ], + "additionalProperties": false + } } }, "before_script": { @@ -1863,4 +1874,4 @@ } } } -} \ No newline at end of file +} -- GitLab From 921a8cab1094acd99cfc361fc7cccc7f47736518 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 8 Nov 2022 18:26:03 +0100 Subject: [PATCH 03/15] Add `id_tokens` to CI JSON schema With `aud` --- app/assets/javascripts/editor/schema/ci.json | 32 +++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index d66f9c6faaa3a4..def7a8721cbcc7 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -585,6 +585,36 @@ ] } }, + "id_tokens": { + "type": "object", + "markdownDescription": "Defines JWTs to be injected as environment variables.", + "patternProperties": { + ".*": { + "type": "object", + "properties": { + "aud": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "array", + "items": { + "type": "string" + }, + "minItems": 1, + "uniqueItems": true + } + ] + } + }, + "required": [ + "aud" + ], + "additionalProperties": false + } + } + }, "secrets": { "type": "object", "markdownDescription": "Defines secrets to be injected as environment variables. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#secrets).", @@ -1874,4 +1904,4 @@ } } } -} +} \ No newline at end of file -- GitLab From ab8e01b30fa1d4fff5bf23227d7deb9c49107882 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 8 Nov 2022 18:33:31 +0100 Subject: [PATCH 04/15] Add `CreatePipelineService` spec This spec checks that ID token configuration is persisted as expected on pipeline creation --- .../ci/create_pipeline_service_spec.rb | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 67c13649c6f560..afa9ab81851bc6 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -710,6 +710,29 @@ def previous_commit_sha_from_ref(ref) end end + context 'when the configuration includes ID tokens' do + it 'creates variables for the ID tokens' do + config = YAML.dump({ + job_with_id_tokens: { + script: 'ls', + id_tokens: { + 'TEST_ID_TOKEN' => { + aud: 'https://gitlab.com' + } + } + } + }) + stub_ci_pipeline_yaml_file(config) + + result = execute_service.payload + + expect(result).to be_persisted + expect(result.builds.first.id_tokens).to eq({ + 'TEST_ID_TOKEN' => { 'aud' => 'https://gitlab.com' } + }) + end + end + context 'with manual actions' do before do config = YAML.dump({ deploy: { script: 'ls', when: 'manual' } }) -- GitLab From 81374cca5e41edd78a67f6bd4900c3a6b1fd260a Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Nov 2022 15:54:01 +0100 Subject: [PATCH 05/15] Fix broken Ci::Build specs The ID tokens need to use the new format --- spec/models/ci/build_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 813b4b3faa6506..9269b341627081 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3539,8 +3539,8 @@ rsa_key = OpenSSL::PKey::RSA.generate(3072).to_s stub_application_setting(ci_jwt_signing_key: rsa_key) build.metadata.update!(id_tokens: { - 'ID_TOKEN_1' => { id_token: { aud: 'developers' } }, - 'ID_TOKEN_2' => { id_token: { aud: 'maintainers' } } + 'ID_TOKEN_1' => { aud: 'developers' }, + 'ID_TOKEN_2' => { aud: 'maintainers' } }) end -- GitLab From 923a9537219b83e2c2c50a15c4050755f406cd83 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Nov 2022 16:09:17 +0100 Subject: [PATCH 06/15] Use token value as written We're not going to require that users preface the token name with `$` --- ee/app/presenters/ee/ci/build_runner_presenter.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/ee/app/presenters/ee/ci/build_runner_presenter.rb b/ee/app/presenters/ee/ci/build_runner_presenter.rb index ec09c4df580a06..254913e97be021 100644 --- a/ee/app/presenters/ee/ci/build_runner_presenter.rb +++ b/ee/app/presenters/ee/ci/build_runner_presenter.rb @@ -40,16 +40,10 @@ def vault_jwt(secret) def id_token_var(secret) return unless id_tokens? - token = specified_token(secret) || id_tokens.each_key.first + token = secret['token'] || id_tokens.each_key.first "${#{token}}" end - - def specified_token(secret) - return unless secret['token'].present? - - secret['token'][1..] - end end end end -- GitLab From bb65e27b3b1ab96cfe780e2f6639fcbd54982a8e Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Nov 2022 16:46:13 +0100 Subject: [PATCH 07/15] Fix `Ci::BuildMetadata` spec The `id_token` format needed to be updated --- spec/models/ci/build_metadata_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb index e728ce0f474025..3028f49a49c860 100644 --- a/spec/models/ci/build_metadata_spec.rb +++ b/spec/models/ci/build_metadata_spec.rb @@ -107,9 +107,7 @@ } metadata.id_tokens = { TEST_JWT_TOKEN: { - id_token: { - aud: 'https://gitlab.test' - } + aud: 'https://gitlab.test' } } -- GitLab From 34e9ffd4a2d247c5d27ff769c6c2939fcd01f6c7 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Nov 2022 17:37:15 +0100 Subject: [PATCH 08/15] Add specs for `Entry::IdToken` The new keyword is tested --- .../gitlab/ci/config/entry/id_token_spec.rb | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 spec/lib/gitlab/ci/config/entry/id_token_spec.rb diff --git a/spec/lib/gitlab/ci/config/entry/id_token_spec.rb b/spec/lib/gitlab/ci/config/entry/id_token_spec.rb new file mode 100644 index 00000000000000..0a4bc057d598d6 --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/id_token_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::Config::Entry::IdToken do + context 'when given `aud` as a string' do + it 'is valid' do + config = { aud: 'https://gitlab.com' } + id_token = described_class.new(config) + + id_token.compose! + + expect(id_token).to be_valid + expect(id_token.value).to eq(aud: 'https://gitlab.com') + end + end + + context 'when given `aud` as an array' do + it 'is valid and concatenates the values' do + config = { aud: ['https://gitlab.com', 'https://aws.com'] } + id_token = described_class.new(config) + + id_token.compose! + + expect(id_token).to be_valid + expect(id_token.value).to eq(aud: ['https://gitlab.com', 'https://aws.com']) + end + end + + context 'when not given an `aud`' do + it 'is invalid' do + config = {} + id_token = described_class.new(config) + + id_token.compose! + + expect(id_token).not_to be_valid + expect(id_token.errors).to match_array([ + 'id token config missing required keys: aud', + "id token aud can't be blank", + 'id token aud should be an array of strings or a string' + ]) + end + end + + context 'when given an unknown keyword' do + it 'is invalid' do + config = { aud: 'https://gitlab.com', unknown: 'test' } + id_token = described_class.new(config) + + id_token.compose! + + expect(id_token).not_to be_valid + expect(id_token.errors).to match_array([ + 'id token config contains unknown keys: unknown' + ]) + end + end +end -- GitLab From 8de4302678f99ef5dd3663416f6a988c421f2e9a Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Nov 2022 17:46:03 +0100 Subject: [PATCH 09/15] Test new `token` sub-keyword Adds a test to the `Entry::Secret` specs --- ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb b/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb index a7c63059b7fa70..c112f0ebba5286 100644 --- a/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb +++ b/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb @@ -53,6 +53,21 @@ it_behaves_like 'configures secrets' end + + context 'when `token` is defined' do + let(:config) do + { + vault: { + engine: { name: 'kv-v2', path: 'kv-v2' }, + path: 'production/db', + field: 'password' + }, + token: 'TEST_ID_TOKEN' + } + end + + it_behaves_like 'configures secrets' + end end end -- GitLab From c034356fac5db44b6709653430239b0fe67309d3 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 11 Nov 2022 15:14:13 +0100 Subject: [PATCH 10/15] Improve private method name `predefined` is more relevant than `legacy` --- app/models/ci/build.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 266998c2a02622..6441928835c6ce 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1208,11 +1208,11 @@ def job_jwt_variables if project.ci_cd_settings.opt_in_jwt? id_tokens_variables else - legacy_jwt_variables.concat(id_tokens_variables) + predefined_jwt_variables.concat(id_tokens_variables) end end - def legacy_jwt_variables + def predefined_jwt_variables Gitlab::Ci::Variables::Collection.new.tap do |variables| jwt = Gitlab::Ci::Jwt.for_build(self) jwt_v2 = Gitlab::Ci::JwtV2.for_build(self) -- GitLab From 83962a7127e6ffd33557f8ff4fad5f7a8053eb2f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 11 Nov 2022 15:25:45 +0100 Subject: [PATCH 11/15] Add `id_tokens` specs Tests the `id_tokens` attribute for `YamlProcessor::Result` and `Entry::Job` --- spec/lib/gitlab/ci/config/entry/job_spec.rb | 6 ++++-- spec/lib/gitlab/ci/yaml_processor/result_spec.rb | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb index acf60a6cddab6b..22cb1f480c56da 100644 --- a/spec/lib/gitlab/ci/config/entry/job_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb @@ -716,7 +716,8 @@ let(:config) do { before_script: %w[ls pwd], script: 'rspec', - after_script: %w[cleanup] } + after_script: %w[cleanup], + id_tokens: { TEST_ID_TOKEN: { aud: 'https://gitlab.com' } } } end it 'returns correct value' do @@ -730,7 +731,8 @@ only: { refs: %w[branches tags] }, job_variables: {}, root_variables_inheritance: true, - scheduling_type: :stage) + scheduling_type: :stage, + id_tokens: { TEST_ID_TOKEN: { aud: 'https://gitlab.com' } }) end end end diff --git a/spec/lib/gitlab/ci/yaml_processor/result_spec.rb b/spec/lib/gitlab/ci/yaml_processor/result_spec.rb index 7f2031687061bb..e2741cc9ef2ffb 100644 --- a/spec/lib/gitlab/ci/yaml_processor/result_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor/result_spec.rb @@ -12,6 +12,18 @@ class YamlProcessor let(:ci_config) { Gitlab::Ci::Config.new(config_content, user: user) } let(:result) { described_class.new(ci_config: ci_config, warnings: ci_config&.warnings) } + describe '#builds' do + let(:config_content) do + YAML.dump( + test: { stage: 'test', script: 'echo', id_tokens: { TEST_ID_TOKEN: { aud: 'https://gitlab.com' } } } + ) + end + + it 'includes `id_tokens`' do + expect(result.builds.first[:id_tokens]).to eq({ TEST_ID_TOKEN: { aud: 'https://gitlab.com' } }) + end + end + describe '#config_metadata' do subject(:config_metadata) { result.config_metadata } -- GitLab From be01e07954344fa8682ec3a02d7b8a6e54f8f523 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 11 Nov 2022 17:47:35 +0100 Subject: [PATCH 12/15] Remove `token` sub-keyword To make reviews easier, `token` will be added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/103801 --- app/assets/javascripts/editor/schema/ci.json | 99 +++++++++---------- ee/lib/gitlab/ci/config/entry/secret.rb | 9 +- .../lib/gitlab/ci/config/entry/secret_spec.rb | 15 --- 3 files changed, 45 insertions(+), 78 deletions(-) diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index def7a8721cbcc7..74df711174449b 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -618,64 +618,53 @@ "secrets": { "type": "object", "markdownDescription": "Defines secrets to be injected as environment variables. [Learn More](https://docs.gitlab.com/ee/ci/yaml/#secrets).", - "patternProperties": { - ".*": { - "type": "object", - "properties": { - "vault": { - "oneOf": [ - { - "type": "string", - "description": "The secret to be fetched from Vault (e.g. 'production/db/password@ops' translates to secret 'ops/data/production/db', field `password`)" - }, - { - "type": "object", - "properties": { - "engine": { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "path": { - "type": "string" - } + "additionalProperties": { + "type": "object", + "description": "Environment variable name", + "properties": { + "vault": { + "oneOf": [ + { + "type": "string", + "description": "The secret to be fetched from Vault (e.g. 'production/db/password@ops' translates to secret 'ops/data/production/db', field `password`)" + }, + { + "type": "object", + "properties": { + "engine": { + "type": "object", + "properties": { + "name": { + "type": "string" }, - "required": [ - "name", - "path" - ] - }, - "path": { - "type": "string" + "path": { + "type": "string" + } }, - "field": { - "type": "string" - } + "required": [ + "name", + "path" + ] }, - "required": [ - "engine", - "path", - "field" - ] - } - ] - }, - "file": { - "type": "boolean", - "default": true, - "description": "Configures the secret to be stored as either a file or variable type CI/CD variable." - }, - "token": { - "type": "string", - "description": "Specifies the JWT variable that should be used to authenticate with Hashicorp Vault." - } - }, - "required": [ - "vault" - ], - "additionalProperties": false - } + "path": { + "type": "string" + }, + "field": { + "type": "string" + } + }, + "required": [ + "engine", + "path", + "field" + ] + } + ] + } + }, + "required": [ + "vault" + ] } }, "before_script": { diff --git a/ee/lib/gitlab/ci/config/entry/secret.rb b/ee/lib/gitlab/ci/config/entry/secret.rb index 3fe514af4e176c..d0f595f2a1d632 100644 --- a/ee/lib/gitlab/ci/config/entry/secret.rb +++ b/ee/lib/gitlab/ci/config/entry/secret.rb @@ -12,7 +12,7 @@ class Secret < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Attributable REQUIRED_KEYS = %i[vault].freeze - ALLOWED_KEYS = (REQUIRED_KEYS + %i[file token]).freeze + ALLOWED_KEYS = (REQUIRED_KEYS + %i[file]).freeze attributes ALLOWED_KEYS @@ -21,13 +21,6 @@ class Secret < ::Gitlab::Config::Entry::Node validations do validates :config, allowed_keys: ALLOWED_KEYS, required_keys: REQUIRED_KEYS - validates :token, type: String, allow_nil: true - end - - def value - return super unless token.present? - - super.merge(token: token) end end end diff --git a/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb b/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb index c112f0ebba5286..a7c63059b7fa70 100644 --- a/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb +++ b/ee/spec/lib/gitlab/ci/config/entry/secret_spec.rb @@ -53,21 +53,6 @@ it_behaves_like 'configures secrets' end - - context 'when `token` is defined' do - let(:config) do - { - vault: { - engine: { name: 'kv-v2', path: 'kv-v2' }, - path: 'production/db', - field: 'password' - }, - token: 'TEST_ID_TOKEN' - } - end - - it_behaves_like 'configures secrets' - end end end -- GitLab From 207ce92537141775fe84cdf538fc84c7cdefcb86 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 14 Nov 2022 16:59:26 +0100 Subject: [PATCH 13/15] Make fixes from MR review Fixes the ID token validation schema and makes other improvements --- .../build_metadata_id_tokens.json | 20 ++++++++++++------- lib/gitlab/ci/config/entry/id_token.rb | 2 +- .../gitlab/ci/yaml_processor/result_spec.rb | 18 +++++++++-------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/app/validators/json_schemas/build_metadata_id_tokens.json b/app/validators/json_schemas/build_metadata_id_tokens.json index 6c87b2bc12823e..d97b2241ca3472 100644 --- a/app/validators/json_schemas/build_metadata_id_tokens.json +++ b/app/validators/json_schemas/build_metadata_id_tokens.json @@ -10,13 +10,19 @@ ], "properties": { "aud": { - "type": [ - "array", - "string" - ], - "items": { - "type": "string" - } + "oneOf": [ + { + "type": "string" + }, + { + "type": "array", + "items": { + "type": "string" + }, + "minItems": 1, + "uniqueItems": true + } + ] } }, "additionalProperties": false diff --git a/lib/gitlab/ci/config/entry/id_token.rb b/lib/gitlab/ci/config/entry/id_token.rb index 2d277773e83115..12e0975d1b175a 100644 --- a/lib/gitlab/ci/config/entry/id_token.rb +++ b/lib/gitlab/ci/config/entry/id_token.rb @@ -15,7 +15,7 @@ class IdToken < ::Gitlab::Config::Entry::Node validations do validates :config, required_keys: %i[aud], allowed_keys: %i[aud] - validates :aud, presence: true, array_of_strings_or_string: true + validates :aud, array_of_strings_or_string: true end def value diff --git a/spec/lib/gitlab/ci/yaml_processor/result_spec.rb b/spec/lib/gitlab/ci/yaml_processor/result_spec.rb index e2741cc9ef2ffb..5c9f156e054743 100644 --- a/spec/lib/gitlab/ci/yaml_processor/result_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor/result_spec.rb @@ -13,14 +13,16 @@ class YamlProcessor let(:result) { described_class.new(ci_config: ci_config, warnings: ci_config&.warnings) } describe '#builds' do - let(:config_content) do - YAML.dump( - test: { stage: 'test', script: 'echo', id_tokens: { TEST_ID_TOKEN: { aud: 'https://gitlab.com' } } } - ) - end - - it 'includes `id_tokens`' do - expect(result.builds.first[:id_tokens]).to eq({ TEST_ID_TOKEN: { aud: 'https://gitlab.com' } }) + context 'when a job has ID tokens' do + let(:config_content) do + YAML.dump( + test: { stage: 'test', script: 'echo', id_tokens: { TEST_ID_TOKEN: { aud: 'https://gitlab.com' } } } + ) + end + + it 'includes `id_tokens`' do + expect(result.builds.first[:id_tokens]).to eq({ TEST_ID_TOKEN: { aud: 'https://gitlab.com' } }) + end end end -- GitLab From 5e3ffcb1e766a50a390a803a6c31d4232af5d009 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 14 Nov 2022 17:47:53 +0100 Subject: [PATCH 14/15] Fix a broken spec The spec needed to be updated after removing `presence: true` --- spec/lib/gitlab/ci/config/entry/id_token_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/lib/gitlab/ci/config/entry/id_token_spec.rb b/spec/lib/gitlab/ci/config/entry/id_token_spec.rb index 0a4bc057d598d6..12585d662ec515 100644 --- a/spec/lib/gitlab/ci/config/entry/id_token_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/id_token_spec.rb @@ -37,7 +37,6 @@ expect(id_token).not_to be_valid expect(id_token.errors).to match_array([ 'id token config missing required keys: aud', - "id token aud can't be blank", 'id token aud should be an array of strings or a string' ]) end -- GitLab From 121df0ef5bb76cb193009dc2486cd3eabf4268a4 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 22 Nov 2022 18:53:26 +0100 Subject: [PATCH 15/15] Add tests for CI schema Also - appropriately reference the ID token section in the schema --- app/assets/javascripts/editor/schema/ci.json | 3 +++ spec/frontend/editor/schema/ci/ci_schema_spec.js | 4 ++++ .../schema/ci/yaml_tests/negative_tests/id_tokens.yml | 11 +++++++++++ .../schema/ci/yaml_tests/positive_tests/id_tokens.yml | 11 +++++++++++ 4 files changed, 29 insertions(+) create mode 100644 spec/frontend/editor/schema/ci/yaml_tests/negative_tests/id_tokens.yml create mode 100644 spec/frontend/editor/schema/ci/yaml_tests/positive_tests/id_tokens.yml diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index 74df711174449b..4c4ad81fed7350 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -1239,6 +1239,9 @@ "cache": { "$ref": "#/definitions/cache" }, + "id_tokens": { + "$ref": "#/definitions/id_tokens" + }, "secrets": { "$ref": "#/definitions/secrets" }, diff --git a/spec/frontend/editor/schema/ci/ci_schema_spec.js b/spec/frontend/editor/schema/ci/ci_schema_spec.js index 32126a5fd9a9de..9c622d49db98a0 100644 --- a/spec/frontend/editor/schema/ci/ci_schema_spec.js +++ b/spec/frontend/editor/schema/ci/ci_schema_spec.js @@ -30,6 +30,7 @@ import RulesYaml from './yaml_tests/positive_tests/rules.yml'; import ProjectPathYaml from './yaml_tests/positive_tests/project_path.yml'; import VariablesYaml from './yaml_tests/positive_tests/variables.yml'; import JobWhenYaml from './yaml_tests/positive_tests/job_when.yml'; +import IdTokensYaml from './yaml_tests/positive_tests/id_tokens.yml'; // YAML NEGATIVE TEST import ArtifactsNegativeYaml from './yaml_tests/negative_tests/artifacts.yml'; @@ -45,6 +46,7 @@ import RulesNegativeYaml from './yaml_tests/negative_tests/rules.yml'; import TriggerNegative from './yaml_tests/negative_tests/trigger.yml'; import VariablesInvalidSyntaxDescYaml from './yaml_tests/negative_tests/variables/invalid_syntax_desc.yml'; import VariablesWrongSyntaxUsageExpand from './yaml_tests/negative_tests/variables/wrong_syntax_usage_expand.yml'; +import IdTokensNegativeYaml from './yaml_tests/negative_tests/id_tokens.yml'; const ajv = new Ajv({ strictTypes: false, @@ -80,6 +82,7 @@ describe('positive tests', () => { RulesYaml, VariablesYaml, ProjectPathYaml, + IdTokensYaml, }), )('schema validates %s', (_, input) => { // We construct a new "JSON" from each main key that is inside a @@ -103,6 +106,7 @@ describe('negative tests', () => { // YAML ArtifactsNegativeYaml, CacheKeyNeative, + IdTokensNegativeYaml, IncludeNegativeYaml, JobWhenNegativeYaml, RulesNegativeYaml, diff --git a/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/id_tokens.yml b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/id_tokens.yml new file mode 100644 index 00000000000000..aff2611f16c625 --- /dev/null +++ b/spec/frontend/editor/schema/ci/yaml_tests/negative_tests/id_tokens.yml @@ -0,0 +1,11 @@ +id_token_with_wrong_aud_type: + id_tokens: + INVALID_ID_TOKEN: + aud: + invalid_prop: invalid + +id_token_with_extra_properties: + id_tokens: + INVALID_ID_TOKEN: + aud: 'https://gitlab.com' + sub: 'not a valid property' diff --git a/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/id_tokens.yml b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/id_tokens.yml new file mode 100644 index 00000000000000..169b09ee56f365 --- /dev/null +++ b/spec/frontend/editor/schema/ci/yaml_tests/positive_tests/id_tokens.yml @@ -0,0 +1,11 @@ +valid_id_tokens: + script: + - echo $ID_TOKEN_1 + - echo $ID_TOKEN_2 + id_tokens: + ID_TOKEN_1: + aud: 'https://gitlab.com' + ID_TOKEN_2: + aud: + - 'https://aws.com' + - 'https://google.com' -- GitLab