Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improved startup time #250

Merged
merged 6 commits into from
Nov 12, 2024
Merged

Improved startup time #250

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f8c006
ZAP: disable verify, deprecated
Nov 6, 2024
fe1f2fb
ZAP: disable auto update by default + typo fix
Nov 6, 2024
10ee287
ZAP: disable addon check during the scan
Nov 6, 2024
075a2b4
ZAP: fix linting
Nov 7, 2024
51e272b
zap/podman: handle_plugins() no longer returns the entire command
Nov 12, 2024
0fd7f62
fix black error
Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,9 +449,9 @@ Example: `podman pod create --userns=keep-id:uid=1000,gid=1000 myApp_Pod`

This is useful for debugging. Set `scanners.zap.miscOptions.enableUI: True` (default: False). Then, the ZAP desktop will run with GUI on your host and show the progress of scanning.

+ Disable add-on updates:
+ Enable add-on updates:

Set `scanners.zap.miscOptions.updateAddons: False` (default: True). Then, ZAP will update its addons first and run the scan.
Set `scanners.zap.miscOptions.updateAddons: True` (default: False). ZAP will first update its addons and then run the scan.

+ Install additional addons:

Expand Down
2 changes: 1 addition & 1 deletion config/config-template-zap-long.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ scanners:
# EnableUI (default: false), requires a compatible runtime (e.g.: `type: none`)
enableUI: False

# Defaults to True, set False to prevent auto update of ZAP plugins
# Defaults to False, set True to force auto update of ZAP plugins
updateAddons: True

# List (comma-separated string or list) of additional addons to install
Expand Down
2 changes: 1 addition & 1 deletion e2e-tests/manifests/rapidast-vapi-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ data:
miscOptions:
# enableUI (default: false), requires a compatible runtime (e.g.: flatpak or no containment)
#enableUI: True
# Defaults to True, set False to prevent auto update of ZAP plugins
# Defaults to False, set True to force auto update of ZAP plugins
updateAddons: False
# additionalAddons: ascanrulesBeta
# If set to True and authentication is oauth2_rtoken and api.apiUrl is set, download the API outside of ZAP
Expand Down
5 changes: 4 additions & 1 deletion scanners/zap/zap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def get_update_command(self):
*self._get_standard_options(),
"-cmd",
]
if self.my_conf("miscOptions.updateAddons", default=True):
if self.my_conf("miscOptions.updateAddons"):
Copy link
Collaborator

@ccronca ccronca Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure if I understand correctly, so please bear with me. If updateAddons is not "truthy", get_update_command method will return an empty list. In the _handle_plugins function of zap_podman module, it seems that this would result in the command being ["sh", "-c"] plus the zap_cli command. In this case, would it be better to skip the handle_plugin execution altogether similar to how it's handled in zap_none ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in zap_podman, the situation is a little dirty because, afaik, podman can run only a single command, and since we need to run the update separately from the scan, the code needs to cheat a little with a "sh -c '; '".
When there's no need to handle plugins, it's would probably be preferable to avoid this sh -c all together, especially if updateAddons is off by default.

I will improve on that, such that and skip the handle_plugin when possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, I tried to improve this a bit, but: _handle_plugins() can't actually do the same as its zap_none counterpart, because it can't actually run a podman command.

I updated in this way:

  • _handle_plugins() now simply returns the command to handle the plugins as a list (i.e.: empty list if there's nothing to do)
  • the caller (run()) will then either run the regular ZAP command if there are not plugin to be handled (now default), or bundle both the plugin command and the scan command in a single sh wrapper.

Let me know if that's a better solution, or if we should approach it differently.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach looks like a good improvement.

Just to understand better, why are both commands run in a single podman execution? Could they run separately in different podman executions while sharing storage? I’m not suggesting a change, just curious about how it works

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be doable, yes

command.append("-addonupdate")

addons = self.my_conf("miscOptions.additionalAddons", default=[])
Expand Down Expand Up @@ -191,6 +191,9 @@ def _setup_zap_cli(self):
"""
self.zap_cli.extend(self._get_standard_options())

# Addon update has already been done, if enabled. Prevent a new check for update
self.zap_cli.append("-silent")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if this option will place nice with the -addonupdate option, ie if a user has updateAddons=true in config, will the -silent option prevent this from working, or maybe cause the command to fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It indeed does not play nice with -addonupdate, but this is why I added the option in _setup_zap_cli() and not in _get_standard_options() : unless I missed something in the call tree, the -silent should be added only on the main ZAP command, not the command that deals with installing and updating plugins.
Unless I missed something, it should be fine (it looked fine in my tests)


# Create a session, to store them as evidence
self.zap_cli.extend(["-newsession", f"{self.container_work_dir}/session_data/session"])

Expand Down
54 changes: 1 addition & 53 deletions scanners/zap/zap_none.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,12 @@
import os
import platform
import pprint
import shutil
import subprocess
import sys
from shutil import disk_usage

from .zap import MODULE_DIR
from .zap import Zap
from scanners import State
from scanners.downloaders import anonymous_download
from scanners.path_translators import make_mapping_for_scanner

CLASSNAME = "ZapNone"
Expand Down Expand Up @@ -106,8 +103,6 @@ def run(self):
if not self.state == State.READY:
raise RuntimeError("[ZAP SCANNER]: ERROR, not ready to run")

self._check_plugin_status()
ccronca marked this conversation as resolved.
Show resolved Hide resolved

# temporary workaround: cleanup addon state
# see https://github.com/zaproxy/zaproxy/issues/7590#issuecomment-1308909500
statefile = f"{self.host_home_dir}/add-ons-state.xml"
Expand Down Expand Up @@ -260,7 +255,7 @@ def _handle_plugins(self):

command = self.get_update_command()
if not command:
logging.debug("Skpping addon handling: no install, no update")
logging.debug("Skipping addon handling: no install, no update")
return
# manually specify directory
command.extend(["-dir", self.container_home_dir])
Expand All @@ -273,53 +268,6 @@ def _handle_plugins(self):
f"ZAP did not handle the addon requirements correctly, and exited with code {result.returncode}"
)

def _check_plugin_status(self):
"""MacOS workaround for "The mandatory add-on was not found" error
See https://github.com/zaproxy/zaproxy/issues/7703
"""
logging.info("Zap: verifying the viability of ZAP")

cmd = self.my_conf("container.parameters.executable")
if shutil.which(cmd) is None:
logging.error(f"{cmd} not found in PATH, is ZAP installed?")
sys.exit(1)

command = [cmd]
command.extend(self._get_standard_options())
command.extend(["-dir", self.container_home_dir])
command.append("-cmd")

logging.debug(f"ZAP create home command: {command}")
result = subprocess.run(command, check=False, capture_output=True)
if result.returncode == 0:
logging.debug("ZAP appears to be in a correct state")
elif result.stderr.find(bytes("The mandatory add-on was not found:", "ascii")) > 0:
logging.info("Missing mandatory plugins. Fixing")
url_root = "https://github.com/zaproxy/zap-extensions/releases/download"
anonymous_download(
url=f"{url_root}/callhome-v0.6.0/callhome-release-0.6.0.zap",
dest=f"{self.host_home_dir}/plugin/callhome-release-0.6.0.zap",
proxy=self.my_conf("proxy", default=None),
)
anonymous_download(
url=f"{url_root}/network-v0.9.0/network-beta-0.9.0.zap",
dest=f"{self.host_home_dir}/plugin/network-beta-0.9.0.zap",
proxy=self.my_conf("proxy", default=None),
)
logging.info("Workaround: installing all addons")

command = [self.my_conf("container.parameters.executable")]
command.extend(self._get_standard_options())
command.extend(["-dir", self.container_home_dir])
command.append("-cmd")
command.append("-addoninstallall")

logging.debug(f"ZAP: installing all addons: {command}")
result = subprocess.run(command, check=False)

else:
logging.warning(f"ZAP appears to be in a incorrect state. Error: {result.stderr}")

def _create_home_if_needed(self):
"""Some tools (most notably: ZAP's Ajax Spider with Firefox) require a writable home directory.
When RapiDAST is run in Openshift, the user's home is /, which is not writable.
Expand Down
40 changes: 27 additions & 13 deletions scanners/zap/zap_podman.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,26 @@ def run(self):
if not self.state == State.READY:
raise RuntimeError("[ZAP SCANNER]: ERROR, not ready to run")

cli = self._handle_plugins()
# zap_podman's _handle_plugins simply returns the CLI to run as a list
# [empty if no update], to be assembled with the scan command in a
# single `sh` wrapper
plugins_cmd = self._handle_plugins()
if plugins_cmd:
# We need to merge the update and the scan in a single `sh` wrapped
# command, split by `;`
# 1) protect & turn the update command in a string
full_cmd_as_string = self._zap_cli_list_to_str_for_sh(plugins_cmd)
# 2) Add a separator
full_cmd_as_string += "; "
# 3) protect & turn the scan command in a string
full_cmd_as_string += self._zap_cli_list_to_str_for_sh(self.zap_cli)

cli = ["sh", "-c", full_cmd_as_string]

else:
# No update: we can run a single scan command
cli = self.zap_cli

cli = self.podman.get_complete_cli(cli)

# DO STUFF
Expand Down Expand Up @@ -199,19 +218,14 @@ def _handle_plugins(self):
By running a separate instance of ZAP prior to the real scan.
This is required because some addons require a restart of ZAP.

In "podman" mode, we have to run both the plugin command and
the scan command in the same run. So we inject that in shell.
In "podman" mode, we have to run both the plugin command and the scan
command in the same run, so the _handle_plugins function itself can't
run the update.
So we simply return the `get_update_command()` [command to execute to
make the update, or empty list], and the caller will figure how to make
use of that]
"""

shell = ["sh", "-c"]
update_cmd = self._zap_cli_list_to_str_for_sh(self.get_update_command())
if update_cmd:
update_cmd += "; "
update_cmd += self._zap_cli_list_to_str_for_sh(self.zap_cli)
shell.append(update_cmd)

logging.debug(f"Update command: {shell}")
return shell
return self.get_update_command()

def _setup_podman_cli(self):
"""Prepare the podman command.
Expand Down
9 changes: 2 additions & 7 deletions tests/scanners/zap/test_setup_podman.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,5 @@ def test_podman_handling_plugins(test_config):
assert "pluginB" in test_zap.get_update_command()

shell = test_zap._handle_plugins()
assert len(shell) == 3
assert shell[0] == "sh"
assert shell[1] == "-c"
assert re.search(
"^zap.sh .* -cmd -addonupdate -addoninstall pluginA -addoninstall pluginB; .*",
shell[2],
)
assert_shell = ["zap.sh", "-config", "network.connection.httpProxy.enabled=false", "-config", "network.localServers.mainProxy.port=47691", "-cmd", "-addonupdate", "-addoninstall", "pluginA", "-addoninstall", "pluginB"]
assert shell == assert_shell
Loading