fix(analyzer): improvement to dockerfile scanning

[cx-andre-pereira]

Reason for Proposed Changes
The current implementation for dockerfile scanning is overly restrictive. The GetExtension function can identify "dockerfile" type files in two ways as of now:

• Using the Contains function on files that have no extension set, allowing detection of files that contain "Dockerfile" in a case-sensitive way, values such as:

   - "Dockerfile"
   - "123Dockerfile"
   - "Dockerfile123" 

• Checking that a file is:

  • a valid text file and then, through the "readPossibleDockerFile" function:
    • Return true immediately if it is a file with suffix "gitignore"
    • Return true if it contains a "FROM" (case-sensitive) command after skipping any commented lines "# ...";
  • Given all these conditions, the result for "GetExtension" will be "possibleDockerfile". As a final check for the file to be included in a given scan, the analyzer's worker calls the "isDockerfile" function meant to ensure that only files containing the "FROM" and "RUN"(both case-sensitive) commands are scanned, files that fail this check are promptly set as "unwanted".

• The way dockerfile identification is meant to work is as follows:

  • Case-insensitive support for all "dockerfile", "dockerfile.any_extension" and "any_name.dockerfile" files.
  • Check all files inside folders with the names : "docker/", "dockerfile/" or "dockerfiles/", also in a case-insensitive manner.

• Additionally ".ubi8" and ".debian" files should be included in the scan if they are valid docker configurations.

• The current implementation has a plethora of issues:

  • It is case-sensitive both for "Dockerfile" identification as well as command scanning which is case-sensitive by convention only.
  • Restricts dockerfile scanning to those that contain the "FROM" and "RUN" commands when the "FROM" would suffice.
  • Does not properly skip empty lines and "ARG" type arguments when searching for the "FROM" statement in the "readPossibleDockerFile" function.
  • Has a major redundancy by using both the "readPossibleDockerFile" and the "isDockerfile" functions. Both serve identical purposes and since the latter is only called when "readPossibleDockerFile" returns a positive result, this check should be simplified into a single function.

Proposed Changes

• Reworked the "GetExtension" function so that it first :

  • Calls the (new) "isDockerfileExtension" function, here all files that are named "dockerfile.something" or "something.dockerfile" in a case-insensitive manner, or files inside the target folders mentioned before that are deemed valid dockerfile files (through "readPossibleDockerFile") will be immediately determined as ".dockerfile" files.

• The "readPossibleDockerFile" function was changed to support both "ARG" commands as well as empty lines before the "FROM" command. Additionally the order of these checks was changed to enhance performance so that it is first checked if a line is not a "FROM" command; previously, since every line was compared to the "FROM" statement, a file with a large number of comments would take significantly longer to skim through. (tests on a dockerfile with 260 thousand comments were two times faster with this change)

• In the analyzer the worker was simplified, given the changes on the utils/get_extension's "GetExtension" function, we only need to worry about 2 results "gitignore" and ".dockerfile" , additionally the "isDockerfile" function is now gone; it was overly restrictive and served the same purpose as the "readPossibleDockerFile" function does now.

• Unified all "dockerfile" type resource references to ".dockerfile", previously there was use of "dockerfile", "Dockerfile" and "possibleDockerfile", I deemed these unnecessary since they all represent the same thing, except for "possibleDockerfile" but files labeled as such could and are now determined as valid dockerfiles before being attributed a value for their "extension".

• After initial reviews the case handling for "gitignore" values was simplified so that any ".gitignore" or any extensionless file with "gitignore" as a suffix will have identical flows, previously unit-tests for the analyzer forced the latter to be added to the "unwanted" path list, the test failed if we simple discarded the file in the analyze function before calling the worker (the way any invalid extension outside target folders is treated).

• Tests

  Many new files were added to test/fixtures/dockerfile as well as a new folder test/fixtures/negative_dockerfile.

• test/fixtures/dockerfile/

  ├── Dockerfile-example                (pre-existing test)                               
  ├── corrupted_dockerfile              (pre-existing test)
  ├── any_name/                                                                                                                                                                                                                    
  │   ├── DOCKERfile.txt
  │   ├── Dockerfile.something                                                                                                                                                                                                     
  │   ├── any_name.debian                                                                                                                         
  │   ├── any_name.ubi8
  │   ├── dockerFILE
  │   ├── file.Dockerfile
  │   ├── file_2.DOCKERfile
  │   └── random_name
  ├── case_insensitive_tests/
  │   ├── DOCKERfile.txt
  │   ├── Dockerfile.something
  │   ├── any_name.debian
  │   ├── any_name.ubi8
  │   ├── dockerFILE
  │   ├── file.Dockerfile
  │   ├── file_2.DOCKERfile
  │   └── random_name
  ├── should_generate_payload/
  │   ├── base_image_reference
  │   ├── big_host_name
  │   ├── big_indent_from
  │   ├── full_sha_digest
  │   ├── host_ip_address
  │   ├── multiline_from_statement
  │   ├── platform_flag
  │   ├── platform_flag_and_alias
  │   ├── platform_flag_scratch
  │   ├── platform_with_arg_reference
  │   ├── use_of_host
  │   └── with_excluded_comments
  ├── test_folder_names/
  │   ├── docker/
  │   │   └── any_file.txt
  │   ├── dockerfile/
  │   │   └── any_file.txt
  │   └── dockerfiles/
  │       └── any_file.txt
  └── test_folder_names_case/
      ├── Docker/
      │   └── any_file.txt
      ├── Dockerfile/
      │   └── any_file.txt
      └── Dockerfiles/
          └── any_file.txt

The files themselves include configurations with empty lines, ARG commands and comments before the target FROM command. Additionally all naming conventions are tested. Note that files inside the target folders would not be identified if they were not inside said folders (because isDockerfileExtension is called before checking literal extension).

The case_insensitive_tests folder includes identical tests where all docker commands have been written in lowercase — note that most queries logic / the engine itself will not properly allow final results pointing to target lines when a query is triggered, but they still trigger similarly to comparable conventional dockerfiles (fixed in #8006).

Lastly the "should_generate_payload" folder has 12 different valid variations of a docker "FROM" command to ensure the regex used to identify dockerfiles do not exclude any of the samples. As the name of the folder implies all 12 samples should generate a payload ensuring the expected behavior.

• test/fixtures/negative_dockerfile/

  ├── CW671X02_EBM_EVENT_RULE              (pre-existing test)
  ├── not_dockerfile.debian                                                                                                                                                                                                        
  ├── not_dockerfile.txt
  ├── not_dockerfile.ubi8                                                                                                                                                                                                          
  └── should_not_generate_payload/                                                                                                           
      ├── invalid_syntax                                                                                                                                                                                                           
      ├── negative_email                                                                                                                       
      ├── negative_email_2                                                                                                                                                                                                         
      ├── negative_python
      ├── negative_python_2
      ├── negative_sparql
      └── negative_sql

The .debian and .ubi8 files here should not be flagged as valid dockerfiles since they do not correspond to valid docker configurations. not_dockerfile.txt should not be flagged since, although it contains a valid docker configuration, it has a .txt extension and is not named "dockerfile" (case-insensitive) explicitly — it only contains the word "dockerfile".

The should_not_generate_payload samples test extensionless files containing FROM statements in non-Docker contexts (email header, Python imports, SPARQL queries, SQL) to ensure they are not misidentified as dockerfiles.

These files are used for the util/get_extension unit tests and the new E2E test 106.

As a final note for regarding the tests I should point out that the regex used to identify/exclude samples could never prevent false positives on Files with basic "FROM <any_string>" commands, often related to SQL syntax, in case the files with this text are either:

• Files without extension
• Files stored inside the target ("docker/", "dockerfile/" or "dockerfiles/") folders

Moreover a new function, "TestParser_Parse_CaseInsensitive", was added to the parser/docker/parser_test.go unit tests; as the name implies it is meant to ensure docker samples are parsed identically regardless of casing in the dockerfile commands syntax.

I submit this contribution under the Apache-2.0 license.

[cx-artur-ribeiro]

Nothing to point out besides that.

[cx-rui-araujo]

Hi @cx-andre-pereira, pls check me comments

[cx-artur-ribeiro]

LGTM, nice job André.

[cx-rui-araujo]

@cx-andre-pereira check my comments pls

[cx-rui-araujo]

LGTM, nice work @cx-andre-pereira 💪

[janderssonse]

This would be great, any hope of seeing it moving along be rebased and move into main?