Repository navigation
fix(analyzer): improvement to dockerfile scanning - #8114
Conversation
…s named 'dockerfile'
…es and all files with prefix 'dockerfile.' as well as all files with the '.dockerfile' extension type in a case insensitive matter (improvement on first commit)
…sion, added support for all ubi8/debian files in case of valid dockerfile structure, added support for lower case dockerfile commands - most queries will have issues with this but relevant text files are properly detected as a 'dockerfile' as intended
…ormats for consistency
…ility to lower cyclomatic complexity
…rors, fixed 'gitignore' files exclusion, docker parser will handle said case like before but with explicit 'gitignore' extension rather than 'possibleDockerfile' like before
…sion so that it 1- gets detected regardless of syntax inside 2- gets detected withouth checking syntax inside through the code optimizing detection speed for said files
…wice and minor simplificaton of query arguments
…test 105, improved uni tests to include new case insensitive samples
… dockerfiles as '.dockerfile'
…ailing whitespace
… unnecessary 'gitignore' case in analyzer's workers
…have to be explicitly set as unwanted to allign with '.gitignore' behaviour
✅ No secrets detectedTruffleHog found no secrets in the current commits of this PR. |
…40477--Improvement-to-dockerfile-scanning
cx-artur-ribeiro
left a comment
There was a problem hiding this comment.
Hi @cx-andre-pereira,
Just a small quality of life change. Other than that, I don't see any issues!
Let me know what you think!
…40477--Improvement-to-dockerfile-scanning
…ed for windows OS as requested
…r 1921 expectation (github action result)
cx-rui-araujo
left a comment
There was a problem hiding this comment.
@cx-andre-pereira please check my review
… simple note for people testing locally
…40477--Improvement-to-dockerfile-scanning
cx-artur-ribeiro
left a comment
There was a problem hiding this comment.
Nice job André, LGTM.
|
Thank you very much for your efforts! 👍 Unfortunately, however, the policy has now become stricter instead of being relaxed: None of our Since we can’t simply rename our Dockerfiles to adapt them to the changed matching logic, I’m really hoping for a very short-term hotfix at your upstream end, as even working around the problem by creating custom jobs to pin the old image involves an enormous amount of work. Regardless, I'd like to express my gratitude for your ongoing maintenance and expansion of KICS—it's an indispensable tool in our daily work on GitLab. |
kics 2.2.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed * docs(release): update queries catalog, index and dockerfile for upcoming release by @cx-artur-ribeiro in Checkmarx/kics#8094 * fix(query): added missing case to "Last User is Root" Dockerfile query. by @cx-andre-pereira in Checkmarx/kics#8095 * feat(queries): new queries to check if synapse workspace managed virtual network is enabled by @cx-ricardo-jesus in Checkmarx/kics#8097 * refactor(descriptions): remove pkg/descriptions and related CLI flag by @cx-ricardo-jesus in Checkmarx/kics#8092 * CISO-1264 - Update GitHub Actions runner labels by @cx-jonathan-hartman in Checkmarx/kics#8093 * fix(query): Fix for Missing Backslash Support on Copy_With_More_Than_Two_Arguments_Not_Ending_With_Slash query by @cx-andre-pereira in Checkmarx/kics#8099 * fix(actions): refactor gh actions to fix CI by @cx-artur-ribeiro in Checkmarx/kics#8098 * fix(query): change to keyExpectedValue on ARM 'default_azure_storage_account_network_access_is_too_permissive' query by @cx-andre-pereira in Checkmarx/kics#8101 * update(version): rename VERSION build-arg to ENGINE_VERSION by @cx-artur-ribeiro in Checkmarx/kics#8102 * fix(query): correct keyExpectedValue and keyActualValue in redshift_not_encrypted by @cx-ricardo-jesus in Checkmarx/kics#8105 * fix(analyzer): bound file analysis workers by @omribz156 in Checkmarx/kics#8058 * fix(actions): remove outdated actions and update documentation accordingly by @cx-artur-ribeiro in Checkmarx/kics#8110 * fix(action): remove concurrent group from run projects github action by @cx-artur-ribeiro in Checkmarx/kics#8111 * fix(test): normalize timestamp comparison in TestInitCycloneDxReport by @cx-artur-ribeiro in Checkmarx/kics#8113 * fix(actions): fix security vulnerabilities and update ci with new enforced rules by @cx-artur-ribeiro in Checkmarx/kics#8118 * fix(release): gate kics release workflows behind release environment by @cx-lior-poterman in Checkmarx/kics#8108 * fix(validator): update queries validator for cwe and risk score fields by @cx-artur-ribeiro in Checkmarx/kics#8028 * fix(version): new available version with additional v prefix by @cx-artur-ribeiro in Checkmarx/kics#8119 * fix(analyzer): improvement to dockerfile scanning by @cx-andre-pereira in Checkmarx/kics#8114 * chore(release): removed unused goreleaser configuration files by @cx-ricardo-jesus in Checkmarx/kics#8122 * fix(filesystem): skip cache files by extension by @cx-laura-rodrigues in Checkmarx/kics#8112 * fix(query): changed all dockerfile queries for case insensitive support of dockerfile commands by @cx-andre-pereira in Checkmarx/kics#8115 * docs(release): update queries catalog, index and dockerfile for 2.2.0 by @cx-artur-ribeiro in Checkmarx/kics#8126 ## New Contributors * @cx-jonathan-hartman made their first contribution in Checkmarx/kics#8093 * @omribz156 made their first contribution in Checkmarx/kics#8058 * @cx-lior-poterman made their first contribution in Checkmarx/kics#8108 **Full Changelog**: https://github.com/Checkmarx/kics/compare/v2.1.21...v2.2.0</pre> <p>View the full release notes at <a href="https://github.com/Checkmarx/kics/releases/tag/v2.2.0">https://github.com/Checkmarx/kics/releases/tag/v2.2.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!20473
|
Apologies — only after further investigation did we notice that GitLab had not even upgraded to 2.2.0 yet: Contrary to what I had previously assumed, the issue we are experiencing is very likely fixed by the changes of this PR. Unfortunately, we could not test against 2.2.0, as no image for it has been released yet. Further information on the behavior of KICS for Dockerfiles with different naming and content can be found in the corresponding GitLab issue and test repository: https://gitlab.com/gitlab-org/gitlab/-/work_items/630002 https://gitlab.com/aidigy.com/issue-630002 Of those, test-1 and test-5 represent the failure scenarios. To summarize:
|
|
As for a multi-stage See: https://gitlab.com/gitlab-org/gitlab/-/work_items/630002#note_3879546221 |
* Changed identification of docker files to be case insensitive on files named 'dockerfile' * removed legacy redundant function 'isDockerfile' from analyzer * Improved dockerfile identification to account for relevant folder names and all files with prefix 'dockerfile.' as well as all files with the '.dockerfile' extension type in a case insensitive matter (improvement on first commit) * Fixed 'dockerfile' keyword not being recognized as a valid file extension, added support for all ubi8/debian files in case of valid dockerfile structure, added support for lower case dockerfile commands - most queries will have issues with this but relevant text files are properly detected as a 'dockerfile' as intended * Minor optimization * Initial test files/cases plus minor changes to supported dockerfile formats for consistency * Added new helper function 'isDockerfileExtension' to get_extension utility to lower cyclomatic complexity * reverted accidental query change, fixed linting errors, fixed test errors, fixed 'gitignore' files exclusion, docker parser will handle said case like before but with explicit 'gitignore' extension rather than 'possibleDockerfile' like before * linting fix and optimized case of file named dockerfile without extension so that it 1- gets detected regardless of syntax inside 2- gets detected withouth checking syntax inside through the code optimizing detection speed for said files * More changes to fix go lint, d variable so 'dockerfile' is not used twice and minor simplificaton of query arguments * Added samples for case insensitive testing on dockerfiles, added E2E test 105, improved uni tests to include new case insensitive samples * fix for E2E * Changed relevant functions to always treat/set the extension of valid dockerfiles as '.dockerfile' * Removed last mention of 'dockerfile' without dot notation * Changed 'gitignore' check for better check order in 'GetExtension' function * Slightly more restrictive check to FROM command to ensure it has a trailing whitespace * Updates to functions, removed unnecessary if statement on scan.go and unnecessary 'gitignore' case in analyzer's workers * fix previous commit * fix analyzer uni tests * simplified new if condition * lint fix * fixed analyze unit tests, with names ending in 'gitignore' no longer have to be explicitly set as unwanted to allign with '.gitignore' behaviour * Case-insensitive unit tests for dockerfile samples * Slight changes to new test * Slight simplification of new docker/parser unit test * Mini fix on insensitive_sample * Changed E2E to 106 to fix merge conflict * fix E2E tests * Final E2E fix * Update to 'Docker' related documentation * Requested change - made extDockerfile a constant * Fixed E2E 106 fixture 'RESULT' file name * Refactor to 'get_extension' and 'analyzer' to reduce redudancy * Lint error fix * Newline removed (lint) * Renamed variable to prevent confusing shadowing * Requested E2E change * Removed .ubi8 and .debian extensions checks * Fallback on debian and ubi removal from docker/parser to test E2E * E2E test 2 * New 'python' samples to test for edge case 'from' statements on files without extension or within docker folder, adjusted readPossibleDockerfile to an improved regex based logic, adjusted file.Dockerfile to test for whitespaces before a from statement * New samples and improved, tailored regex for dockerfile FROM statement identification - E2E test should break * Removed duplicated sample(negative) that was in positive test fixture dockerfile folder, updated E2E results * Final fix for E2E plus the test file removal that should have been in previous commit * Made UrlRegex a constant * New samples, changed fockerfile syntax identification to aproach to exclusion based regex * Linter fix * The actual linter fix * fix E2E payload files changed during rebase * updated go and git base images * fix test re using fs cuasing unexpected result during assert * go mod and sum * go mod and sum update plus fix for filesystem test that is only valid on unix based systems so that test files in pkg/ can be tested locally * Actions re-trigger * Extracted file getter for path(s) to helper funcion (ci-lint fix - Analyzer function too long) * More linter fixes * Added alternative assertion in TestFileSystemSourceProvider_AddExcluded for windows OS as requested * Improved test_too_many_levels_of_symbolic_links test for Windows error 1921 expectation (github action result) * Fix for the TestFileSystemSourceProvider_AddExcluded test, now with a simple note for people testing locally * Updated dockerfile git/go images * Revert to correct go image
* Changed identification of docker files to be case insensitive on files named 'dockerfile' * removed legacy redundant function 'isDockerfile' from analyzer * Improved dockerfile identification to account for relevant folder names and all files with prefix 'dockerfile.' as well as all files with the '.dockerfile' extension type in a case insensitive matter (improvement on first commit) * Fixed 'dockerfile' keyword not being recognized as a valid file extension, added support for all ubi8/debian files in case of valid dockerfile structure, added support for lower case dockerfile commands - most queries will have issues with this but relevant text files are properly detected as a 'dockerfile' as intended * Minor optimization * Initial test files/cases plus minor changes to supported dockerfile formats for consistency * Added new helper function 'isDockerfileExtension' to get_extension utility to lower cyclomatic complexity * reverted accidental query change, fixed linting errors, fixed test errors, fixed 'gitignore' files exclusion, docker parser will handle said case like before but with explicit 'gitignore' extension rather than 'possibleDockerfile' like before * linting fix and optimized case of file named dockerfile without extension so that it 1- gets detected regardless of syntax inside 2- gets detected withouth checking syntax inside through the code optimizing detection speed for said files * More changes to fix go lint, d variable so 'dockerfile' is not used twice and minor simplificaton of query arguments * Added samples for case insensitive testing on dockerfiles, added E2E test 105, improved uni tests to include new case insensitive samples * fix for E2E * Changed relevant functions to always treat/set the extension of valid dockerfiles as '.dockerfile' * Removed last mention of 'dockerfile' without dot notation * Changed 'gitignore' check for better check order in 'GetExtension' function * Slightly more restrictive check to FROM command to ensure it has a trailing whitespace * Updates to functions, removed unnecessary if statement on scan.go and unnecessary 'gitignore' case in analyzer's workers * fix previous commit * fix analyzer uni tests * simplified new if condition * lint fix * fixed analyze unit tests, with names ending in 'gitignore' no longer have to be explicitly set as unwanted to allign with '.gitignore' behaviour * Case-insensitive unit tests for dockerfile samples * Slight changes to new test * Slight simplification of new docker/parser unit test * Mini fix on insensitive_sample * Changed E2E to 106 to fix merge conflict * fix E2E tests * Final E2E fix * Update to 'Docker' related documentation * Requested change - made extDockerfile a constant * Fixed E2E 106 fixture 'RESULT' file name * Refactor to 'get_extension' and 'analyzer' to reduce redudancy * Lint error fix * Newline removed (lint) * Renamed variable to prevent confusing shadowing * Requested E2E change * Removed .ubi8 and .debian extensions checks * Fallback on debian and ubi removal from docker/parser to test E2E * E2E test 2 * New 'python' samples to test for edge case 'from' statements on files without extension or within docker folder, adjusted readPossibleDockerfile to an improved regex based logic, adjusted file.Dockerfile to test for whitespaces before a from statement * New samples and improved, tailored regex for dockerfile FROM statement identification - E2E test should break * Removed duplicated sample(negative) that was in positive test fixture dockerfile folder, updated E2E results * Final fix for E2E plus the test file removal that should have been in previous commit * Made UrlRegex a constant * New samples, changed fockerfile syntax identification to aproach to exclusion based regex * Linter fix * The actual linter fix * fix E2E payload files changed during rebase * updated go and git base images * fix test re using fs cuasing unexpected result during assert * go mod and sum * go mod and sum update plus fix for filesystem test that is only valid on unix based systems so that test files in pkg/ can be tested locally * Actions re-trigger * Extracted file getter for path(s) to helper funcion (ci-lint fix - Analyzer function too long) * More linter fixes * Added alternative assertion in TestFileSystemSourceProvider_AddExcluded for windows OS as requested * Improved test_too_many_levels_of_symbolic_links test for Windows error 1921 expectation (github action result) * Fix for the TestFileSystemSourceProvider_AddExcluded test, now with a simple note for people testing locally * Updated dockerfile git/go images * Revert to correct go image
Note
A lot of links are to Original PR, but diff is equivalent.
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:
Checking that a file is:
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:
dockerfile", "dockerfile.any_extension" and "any_name.dockerfile" files.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:
Proposed Changes
Reworked the "GetExtension" function so that it first :
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/dockerfileas well as a new foldertest/fixtures/negative_dockerfile.test/fixtures/dockerfile/
The files themselves include configurations with empty lines,
ARGcommands and comments before the targetFROMcommand. Additionally all naming conventions are tested. Note that files inside the target folders would not be identified if they were not inside said folders (becauseisDockerfileExtensionis called before checking literal extension).The
case_insensitive_testsfolder 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.
The
.debianand.ubi8files here should not be flagged as valid dockerfiles since they do not correspond to valid docker configurations.not_dockerfile.txtshould not be flagged since, although it contains a valid docker configuration, it has a.txtextension and is not named "dockerfile" (case-insensitive) explicitly — it only contains the word "dockerfile".The
should_not_generate_payloadsamples test extensionless files containingFROMstatements 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_extensionunit 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:docker/", "dockerfile/" or "dockerfiles/") foldersMoreover 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.