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:
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:
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:
# 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.
Proposal
Validate the number of includes before validating each object:
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. 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 to prevent unlimited includes.
Update 2023-02-13
Given the potential regression that can result from the proposed fix (See Backend: Validation of max includes passes in C... (#391279 - closed)), we decided to move forward with a first iteration (!111726 (merged)) 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 !111726 (merged) and addressing #391279 (closed), 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 | !108280 (merged) |
backend | Update CI includes counting structure to include duplicates. Feature flag: ci_includes_count_duplicates
|
!111726 (merged) |
backend | Fix FF ci_includes_count_duplicates condition to check variable type (corrective action due to incident) |
!112100 (merged) |
backend | [Feature flag] Roll out ci_includes_count_duplicates
|
Issue #391517 (closed) |
backend | Remove ci_includes_count_duplicates feature flag |
!112951 (merged) |
backend | Fix to prevent unlimited number of CI includes. Feature flag: ci_fix_max_includes
|
!112963 (merged) |
backend | [Feature flag] Roll out ci_fix_max_includes
|
Issue #390909 (closed) |
backend | Remove CI included file count logging | Issue #396776 (closed) |
documentation | Update details about nested includes | !113613 (merged) |