Backend: Prevent unlimited number of includes from being allowed
It is possible to bypass the 100 includes limit by using recursive includes because we [validate the limit after we check each include entry](https://gitlab.com/gitlab-org/gitlab/-/blob/8f13abaed45c1aec0a7d443e480a721f7ae21bff/lib/gitlab/ci/config/external/mapper.rb#L128-132): ```ruby def verify!(location_object) verify_max_includes! location_object.validate! expandset.add(location_object) end ``` `location_object.validate!` expands all its nested includes before adding itself to the the structure that we use for counting the includes `expandset.add(location_object)`. ## Setup Create an API that returns a referencing YAML configuration: ```ruby require 'rack' require 'yaml' def build_yaml(env) id = env['REQUEST_PATH'].match(/\/(\d+)\//)[1].to_i yaml = { :"job #{id}" => { script: 'exit 0' } } if id > 0 yaml[:include] = [ { remote: env['REQUEST_URI'].gsub(/\/#{id}\//, "/#{id.pred}/") } ] end yaml.to_yaml end app = Proc.new do |env| [ 200, { "Content-Type" => "text/plain" }, [build_yaml(env)] ] end Rack::Server.start(app: app, Port: 9292) ``` Use this in the project to start a pipeline or in the lint page: ```yaml # includes the file 400 times :include: - :remote: http://localhost:9292/400/ci.yaml ``` I've tried different values, but it looks like it's timing out after fetching around 435 includes. ![image](/uploads/2c29245a6fe295da5e900d620976c70f/image.png) ## Proposal Validate the number of includes before validating each object: ```diff diff --git a/lib/gitlab/ci/config/external/mapper.rb b/lib/gitlab/ci/config/external/mapper.rb index 2a1060a6059..2983184072c 100644 --- a/lib/gitlab/ci/config/external/mapper.rb +++ b/lib/gitlab/ci/config/external/mapper.rb @@ -127,8 +127,8 @@ def select_first_matching_without_instrumentation(location) def verify!(location_object) verify_max_includes! - location_object.validate! expandset.add(location_object) + location_object.validate! end def verify_max_includes! ``` ### Update 2023-01-05 Prior to fixing the issue, log the included file count of pipelines that exceed the limit, as discussed [here](https://gitlab.com/gitlab-org/gitlab/-/issues/367150#note_1219875901). This is to gauge how many customers would be affected by fixing the includes limit. The logging is to be removed after a sufficient monitoring period. ### Update 2023-02-08 As a result of the incident described in https://gitlab.com/gitlab-org/gitlab/-/issues/390545, we are moving forward with the [suggested changes](https://gitlab.com/gitlab-org/gitlab/-/issues/390545#note_1268297430) to prevent unlimited includes. ### Update 2023-02-13 Given the potential regression that can result from the [proposed fix](https://gitlab.com/gitlab-org/gitlab/-/issues/390545#note_1268297430) (See https://gitlab.com/gitlab-org/gitlab/-/issues/391279+), we decided to [move forward](https://gitlab.com/gitlab-org/gitlab/-/issues/390545#note_1273818085) with a first iteration (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111726) that just updates the `expandset` object from `Set` to `Array`, which enables counting of duplicate includes. This change is also a precursor to fix https://gitlab.com/gitlab-org/gitlab/-/issues/390545. After rolling out https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111726 and addressing https://gitlab.com/gitlab-org/gitlab/-/issues/391279, we should be able to move forward with the MR to prevent unlimited includes. ## Implementation | | Description | MR / Issue | | ------ | ------ | ------ | | ~backend | Log the included file count of CI pipelines that exceed the limit | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108280 | | ~backend | Update CI includes counting structure to include duplicates. **Feature flag:** `ci_includes_count_duplicates` | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111726 | | ~backend | Fix FF `ci_includes_count_duplicates` condition to check variable type ([corrective action](https://gitlab.com/gitlab-com/gl-infra/reliability/-/issues/17420) due to [incident](https://gitlab.com/gitlab-com/gl-infra/production/-/issues/8405)) | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112100 | | ~backend | [Feature flag] Roll out `ci_includes_count_duplicates` | Issue https://gitlab.com/gitlab-org/gitlab/-/issues/391517 | | ~backend | Remove `ci_includes_count_duplicates` feature flag | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112951 | | ~backend | Fix to prevent unlimited number of CI includes. **Feature flag:** `ci_fix_max_includes` | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112963 | | ~backend | [Feature flag] Roll out `ci_fix_max_includes` | Issue https://gitlab.com/gitlab-org/gitlab/-/issues/390909 | | ~backend | Remove CI included file count logging | Issue https://gitlab.com/gitlab-org/gitlab/-/issues/396776 | | ~documentation | Update details about nested includes | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113613 |
issue