Add support for containers scan in cloud by default (AST-00000) #1024
Conversation
cx-alex-cohen
requested review from
cx-ben-alvo,
cx-daniel-greenspan and
cx-miryam-foifer
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Great job, no security vulnerabilities found in this Pull Request |
| clearFlags() | ||
| clearFlags() | ||
| mock.Flag = wrappers.FeatureFlagResponseModel{Name: wrappers.ContainerEngineCLIEnabled, Status: true} | ||
| execCmdNilAssertion(t, "results", "show", "--scan-id", "MOCK", "--report-format", "json") |
There was a problem hiding this comment.
Consider validating the --scan-id flag to ensure that it is not empty and has a proper format before executing the command.
| clearFlags() | ||
| mock.Flag = wrappers.FeatureFlagResponseModel{Name: wrappers.ContainerEngineCLIEnabled, Status: true} | ||
| execCmdNilAssertion(t, "results", "show", "--scan-id", "MOCK", "--report-format", "json") | ||
| assertTypePresentJSON(t, params.ContainersType, 1) |
There was a problem hiding this comment.
It's unclear what assertTypePresentJSON does. Ensure that the function checks for the presence of container scan results in the output as expected by the PR title.
| execCmdNilAssertion(t, "results", "show", "--scan-id", "MOCK", "--report-format", "json") | ||
| assertTypePresentJSON(t, params.ContainersType, 1) | ||
| // Remove generated json file | ||
| removeFileBySuffix(t, printer.FormatJSON) |
There was a problem hiding this comment.
The removal of the generated file should be done in a defer statement immediately after the file creation to ensure it's cleaned up even if the test fails or panics.
| fmt.Sprintf("Parameters to use in SCA resolver (requires --%s).", commonParams.ScaResolverFlag), | ||
| ) | ||
| createScanCmd.PersistentFlags().String(commonParams.ContainerImagesFlag, "", "List of container images to scan, ex: manuelbcd/vulnapp:latest,debian:10. (Not supported yet)") | ||
| createScanCmd.PersistentFlags().String(commonParams.ContainerResolveLocallyFlag, "", "Execute container resolver locally.") |
There was a problem hiding this comment.
The flag commonParams.ContainerResolveLocallyFlag should have a more descriptive help message explaining what it means to execute the container resolver locally and the implications of using this flag.
| ) | ||
| createScanCmd.PersistentFlags().String(commonParams.ContainerImagesFlag, "", "List of container images to scan, ex: manuelbcd/vulnapp:latest,debian:10. (Not supported yet)") | ||
| createScanCmd.PersistentFlags().String(commonParams.ContainerResolveLocallyFlag, "", "Execute container resolver locally.") | ||
| createScanCmd.PersistentFlags().String(commonParams.ContainerImagesFlag, "", "List of container images to scan, ex: manuelbcd/vulnapp:latest,debian:10.") |
There was a problem hiding this comment.
The help message for commonParams.ContainerImagesFlag should mention the new default behavior of scanning containers in the cloud if not specified otherwise.
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The removal of the containerEngineCLIEnabled check might cause unintended behavior if the feature flag is still relevant for other parts of the code. Ensure that the removal of this feature flag check does not affect other functionalities that depend on it.
| configArr = append(configArr, apiSecConfig) | ||
| } | ||
| var containersConfig = addContainersScan(containerEngineCLIEnabled.Status) | ||
| var containersConfig = addContainersScan() |
There was a problem hiding this comment.
The removal of the containerEngineCLIEnabled.Status parameter from addContainersScan() call might lead to unintended behavior if the function relies on this status check to determine whether to perform a containers scan. Ensure that the addContainersScan function is updated to handle the default behavior correctly without requiring this parameter.
|
|
||
| func addContainersScan(containerEngineCLIEnabled bool) map[string]interface{} { | ||
| if !scanTypeEnabled(commonParams.ContainersType) || !containerEngineCLIEnabled { | ||
| func addContainersScan() map[string]interface{} { |
There was a problem hiding this comment.
The removal of the containerEngineCLIEnabled parameter could lead to unintended behavior if the function relies on this flag to determine whether to add container scanning. Ensure that the removal of this parameter aligns with the intended logic and that any necessary checks are still performed elsewhere.
|
|
||
| if isSingleContainerScanTriggered() && containerResolveLocally { | ||
| logger.PrintIfVerbose("Single container scan triggered: compressing only the container resolution file") | ||
| containerResolutionFilePath := filepath.Join(directoryPath, containerResolutionFileName) |
There was a problem hiding this comment.
The variable containerResolutionFileName is used but not defined in the diff. Ensure that it is properly declared and initialized before use.
| if isSingleContainerScanTriggered() && containerResolveLocally { | ||
| logger.PrintIfVerbose("Single container scan triggered: compressing only the container resolution file") | ||
| containerResolutionFilePath := filepath.Join(directoryPath, containerResolutionFileName) | ||
| zipFilePath, dirPathErr = util.CompressFile(containerResolutionFilePath, containerResolutionFileName, directoryCreationPrefix) |
There was a problem hiding this comment.
The error handling for util.CompressFile is missing. Ensure that any errors returned by this function are properly handled.
| } | ||
|
|
||
| func runContainerResolver(cmd *cobra.Command, directoryPath string) error { | ||
| func runContainerResolver(cmd *cobra.Command, directoryPath string, containerResolveLocally bool) error { |
There was a problem hiding this comment.
The new parameter containerResolveLocally is added to the function signature but not used within the function body. Ensure that the parameter is utilized as intended or remove it if it's unnecessary.
| containerResolverError := runContainerResolver(cmd, directoryPath) | ||
| if containerResolverError != nil { | ||
| return containerResolverError | ||
| if containerResolveLocally || len(containerImagesList) > 0 { |
There was a problem hiding this comment.
The variable containerResolveLocally is not defined in the provided diff. Ensure that it is properly declared and initialized before use.
| } | ||
|
|
||
| imageParts := strings.Split(containerImage, ":") | ||
| if len(imageParts) != 2 || imageParts[0] == "" || imageParts[1] == "" { |
There was a problem hiding this comment.
The validation logic for the container image format only checks for the presence of a colon and non-empty parts before and after the colon. However, it does not validate the actual format of the image name and tag. Consider adding more robust validation to ensure the image name and tag conform to the expected patterns.
1 participant
By submitting a PR to this repository, you agree to the terms within the Checkmarx Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
References
Testing
Checklist