Speed: Process work items in parallel (WebRunnerMachine)

Problem

The WebRunnerCheckStrategy generates IWebRunnerWorkItems which are currently processed in series. To improve speed and allow taking advantage of a larger testing machine with multiple CPUs, the work items should be processed in parallel up to some reasonable max parallelism based on CPU/Core counts.

Proposal

Use the .NET Dataflow objects to process work items generated by WebRunnerCheckStrategy in parallel. The existing 344864-threaded-runner branch will be used as a reference only. The refactoring of WebRunnerCheckStrategy should provide a much better foundation than existed in the prior working branch for threading.

NOTE: Please keep the 344864-threaded-runner until @mikeeddington returns. Some hotpath methods were optimized in this branch.

One issue found in the existing branch was the minimum workers count on ThreadPool should not match the vCPU, it should be higher by at least 1. On single vCPU runners, such as the GitLab shared runners, increasing the worker thread count improved performance.

To determine the correct levels for ThreadPool MinThreads WorkerThreads and also MaxDegreeOfParallelism for the TransformBlock. It's recommended to build a 4 vCPU runner in GPC to use with a custom label, and also use the shared runners with a single vCPU. Do not expose any new variables for configuring parallelism at this time.

• At start of job run each operation record to generate work items
  • #353293 (closed)
• Work items added to TransformBlock for processing
• Automatically check CPU/core count and update max parallelization and task limits accordingly
  • Override task minimums to prevent major slow down on 1 vCPU runners
• Work Items
  • Active checks
  • Mutational checks
    • Parameter + Check
• Documentation update for how to increase testing speed using a larger instance

Implementation

1. Remove benchmark stage from gitlab ci file
2. Verify no use of custom runners left, move everything to gitlab-org,high-cpu
3. If the CPU count is 1, and the job time is > 10min, call out our parallel documentation on the console
4. Getting occasional failures on ShouldReturnRequestCountAndSumResponseSize test
   • https://gitlab.com/gitlab-org/security-products/analyzers/api-fuzzing-src/-/jobs/2810807453
   • Does this test really work? What about when we test path parameters?
   • #371086 (closed)
5. tests_int_config_fuzzing-headers-quick
   • failing due to two missing response analysis with "Internal server error" response (500)
   • Need to make these two vulnerabilities optional in nature, so changes to the checkreportfaultcounts.sh needed
6. Rename tuning variable API_SECURITY_WORKER_MAX_THREADS to DAST_API_TUNE_MAX_CONCURRENCY
7. Using a Dataflow block from the Task Parallel Library, parallelize the work items produced by the strategy.
8. Automatically check CPU/core count and update max parallelization and task limits accordingly
   1. Good enough is - Shared runner (1 vCPU) is not allot slower. And multi-cpu gets allot faster.
   2. Allow override via configuration as a potential workaround
9. Fix any concurrency issues caused by parallelization of the work items
   1. Wait for record to complete before starting other work items
      • Prevent issues with Resources, EndPoints in terms of adding PathTrie's and recorded operations
   2. WebRunnerCheckStrategy.PerformMutationCheckWork -- Re-check configurations, I think we are safe, but..
      1. Create a new instance of check for each work item to avoid concurrency issues
      2. Fix broken tests that mock Repository.LoadCheck causing them to never return
      • To be safe we could request a new instance of a check in the strategy work item. This would also make checks safer to implement. Maybe this is follow on work?
      • Goes away with this FrameworkDebugModeCheck -- private FrameworkChecks
      • Goes away with this FuzzingCheck -- LOTS OF STUFF HERE!! All sorts of private storage that changes between uses
      • Any issues with Assertion/Check initialization goes away
   3. PathTrie -- Used by EndPoint, has Dictionary
      • Mikes branch commit: 8c1d027836fc57a537e2ee993732fe41f4371961
   4. WebApiService -- Has several Dictionary types that will be used cross-thread
      • Mikes branch commit: 8c1d027836fc57a537e2ee993732fe41f4371961
   5. Resource -- List -> ConcurrentBag
      • Mikes branch commit: 8c1d027836fc57a537e2ee993732fe41f4371961
   6. FaultCollector -- Add a new FaultDetectedThreadSafe method with locking
      • Mikes branch commit: 8c1d027836fc57a537e2ee993732fe41f4371961
   7. BlindInjectionAssertion -- associated checks are also still stateful.
      1. OsCommandInjectionCheck
      2. ShellShockCheck
      3. SqlInjectionCheck
   8. ResponseAnalysisAssertion
      1. Cache needs locking
      2. _entries needs a solution, changes based on config
   9. Repository.FindOrCreateFinding -- Needs lock to avoid duplicate findings
   10. XmlExternalEntityCheck -- Cached XML document on local private variable
       1. Put a fix in place
       2. Have herb review fix for perf
   11. CustomBodyContainsAssertionConfig
       1. IsMatch needs locking on setting _searchStringRegex?
       2. Can we cache the regex and still work w/concurrency
   12. CustomParameterContainsAssertionConfig
       1. IsMatch needs locking on setting _matchRegex?
       2. Can we cache the regex and still work w/concurrency
   13. LogAnalysisAssertionConfigEntry
       1. Do we need locking in Regex property? Mike: No we do not.
   14. ResponseAnalysisAssertionConfigEntry
       1. Do we need locking in Regex property? Mike: No we do not.
   15. SessionCookieCheckConfigEntry
       1. Do we need locking in Regex property? Mike: No we do not.
   16. InsecureHttpMethodsCheck
       1. Has configuration dependent internal state
   17. TlsConfigurationCheck -- Caches configuration, if we test two hosts at once, could collide
   18. XmlExternalEntityCheck -- Looks like it has some internal state
   19. RegexCache -- Can we move to a concurrent dictionary and remove locking? Lots of calls to this class.
   20. Issues exposed through test failures
10. Perform concurrency audit
    1. RegexCache
    2. Assertion initialization Not needed if we use a new check instance in WebRunnerCheckStrategy
    3. Check initialization Not needed if we use a new check instance in WebRunnerCheckStrategy
       1. Make sure Initialize is only called once
    4. Assertions - Check assertion and also configurations, look for private variables in configs
    5. Checks - Check main check and also configurations, look for private variables in configs
       1. Review check configurations
       2. Checks include base classes
    6. Fault reporting flow (just lock all of it)
    7. Send request networking flow
    8. Review Mike's old work branch to identify other concurrency issues
    9. If check/assert configurations can change by route, that's a concurrency problem because each work item could be a different route operating in parallel.
       • Checks and Assertions are InstancePerDependency for autofac. Always new instances.
       • Check instances created by Repository always have a unique instance
       • It looks like we are safe here, but...
       • To be safe we could request a new instance of a check in the strategy work item
11. Update tests and add any new tests needed.
    1. Set max concurrent requests in integration tests and confirm that multiple requests are made to the target concurrently
    2. Set max concurrent requests in worker-entry tests and confirm that scanner receives the setting
12. HttpConnectionSettings - This can change with the route (external proxy)! Will need a unique instance per-operation to use.
    • #369501
13. Optimizations
    1. Benchmark if making _faultCollector.FaultDetectedThreadSafe call a background task improves perf - This is a can of worms. The variables we pass in change if tasked off.
    2. Modify Repository.LoadCheck to not get new instances of all checks, just the one check #371088
    3. Http11Connection -- Could remove locking if we used thread safe bool pattern already in use #371089
       • https://stackoverflow.com/questions/29411961/c-sharp-and-thread-safety-of-a-bool
    4. Request.WriteHeaders -- Use stringbuffers append over string interpolation
14. Documentation #371085 (closed)

David's transition notes as of 7/17:

1. Configuration via environment variable is implemented and tested
   1. During testing it was found that setting the value to 0 causes worker-entry to not serialize the value in the create session request (presumably because it is a default value, but I was not able to track down specifically where this determination was made). This effectively makes 0 equal to not setting it at all, which causes the DOP setting to be calculated based on CPUs. That seemed like a reasonable meaning for 0 - particularly because actually setting MaxDop to 0 in the Dataflow blocks disables them - so it is documented and tested that way. Although it cannot be set from worker-entry, if a 0 does show up in the runner options (because .NET integration tests do not strip the 0 value for example), it is explicitly recognized as being equivalent to null. See this code comment.
   2. Setting the value to -1 means "Unbounded" to Dataflow. While initially it seemed like it might be a good idea to support this value, integration testing with unbounded DOP showed that it can easily overwhelm the target. Since it seemed likely to cause more problems than it solves, I decided not to support it. See this code comment and this one.
   3. Initially I named this MAX_CONCURRENT_REQUESTS, trying to tie it to something the user would understand. However, while I think it is accurate for now (though hard to prove), I realized that might be a stronger commitment than we are willing to make. So I changed it to MAX_CONCURRENCY as recommended in the task list.
2. Testing
   1. A few tests are not passing:
      1. tests_int_config_fuzzing-headers-quick and tests_int_config_fuzzing-quick need to have expectations updated. StatusCode-based vulnerabilities coming from our Flask target are now unreliable, since they depend on the order of the incoming requests. The solution (for now) is to ignore status code vulnerabilities in our integration tests.
      2. ShouldFindVulnerabilityRemoteFileInclusionInJsonBodyWithEmbeddedXml integration test is failing intermittently because XmlExternalEntityCheck is stateful and not safe for concurrent execution¹.
      3. There is a Debug.Assert in BlindInjectionAssertion that is causing multiple integration tests to fail because BlindInjectionAssertion is stateful and not safe for concurrent execution¹. The Debug.Assert is currently commented out to prevent it from masking other issues.
   2. New integration tests specifically for concurrent execution have been added
      1. These tests use a RunnerOptions property which is not intended to be exposed to consumers which waits to run checks until after all recording is done. We could make this the default behavior instead, which would simplify the code slightly; but since we previously talked about keeping the current behavior, I made it an option for now. See this code comment.
      2. These tests are sending more requests than necessary, and could be faster if the requests could be reduced. See this code comment.
   3. Worker-entry tests for configuration of concurrent execution have been added
      1. Testing that parallel execution is actually happening at this level would be quite difficult, as well as redundant with the integration tests. So these tests do as little as possible and focus on confirming that the setting is passed correctly from worker-entry to the scanner.

1. The spike branch created a MutationContext which could hold the state from these checks in order to make them safe for concurrent execution. However that solution caused large changes throughout the entire architecture. Instead we discussed creating concurrency-safe caches inside the checks, keyed to each work item, to store the needed state. ↩ ↩²