diff --git a/changelog.d/20251016_101312_ghislain.casier_add_insecure_option.md b/changelog.d/20251016_101312_ghislain.casier_add_insecure_option.md new file mode 100644 index 0000000000..a2efd309e2 --- /dev/null +++ b/changelog.d/20251016_101312_ghislain.casier_add_insecure_option.md @@ -0,0 +1,44 @@ + + +### Added + +- Added `--insecure` CLI option and `insecure` configuration setting as clearer alternatives to `--allow-self-signed` and `allow_self_signed`. The new option explicitly communicates that SSL verification is completely disabled, making the connection vulnerable to man-in-the-middle attacks. +- Added prominent warning messages when SSL verification is disabled (via either `--insecure` or `--allow-self-signed`), explaining the security risks and recommending the secure alternative of using the system certificate trust store (available with Python >= 3.10). + +### Deprecated + +- The `--allow-self-signed` CLI option and `allow_self_signed` configuration setting are now deprecated in favor of `--insecure` and `insecure`. Deprecation warnings are displayed when these options are used, guiding users to the clearer alternative. Both options remain functional for backward compatibility and will be maintained for an extended deprecation period before removal. + +### Security + +- Improved clarity around SSL verification settings. The `--allow-self-signed` option name was misleading as it suggests certificate validation is still performed, when in reality all SSL verification is disabled. The new `--insecure` option makes this behavior explicit. Both options remain functional for backward compatibility. + + + + + diff --git a/ggshield/__main__.py b/ggshield/__main__.py index 250334e211..32a2bdcf6b 100644 --- a/ggshield/__main__.py +++ b/ggshield/__main__.py @@ -82,6 +82,7 @@ def cli( ctx: click.Context, *, allow_self_signed: Optional[bool], + insecure: Optional[bool], config_path: Optional[Path], **kwargs: Any, ) -> None: @@ -97,11 +98,11 @@ def cli( elif user_config.verbose and ui.get_level() < ui.Level.VERBOSE: ensure_level(ui.Level.VERBOSE) - # Update allow_self_signed in the config + # Update SSL verification settings in the config # TODO: this should be reworked: if a command which writes the config is called with - # --allow-self-signed, the config will contain `allow_self_signed: true`. - if allow_self_signed: - user_config.allow_self_signed = allow_self_signed + # --insecure, the config will contain `insecure: true`. + if insecure or allow_self_signed: + user_config.insecure = True ctx_obj.config._dotenv_vars = load_dot_env() diff --git a/ggshield/cmd/auth/login.py b/ggshield/cmd/auth/login.py index 30b4d87879..4796130dbf 100644 --- a/ggshield/cmd/auth/login.py +++ b/ggshield/cmd/auth/login.py @@ -192,7 +192,7 @@ def token_login(config: Config, instance: Optional[str]) -> None: client = create_client( api_key=token, api_url=config.api_url, - allow_self_signed=config.user_config.allow_self_signed, + allow_self_signed=config.user_config.insecure, ) try: response = client.get(endpoint="token") diff --git a/ggshield/cmd/auth/logout.py b/ggshield/cmd/auth/logout.py index 08456d8986..780e14af4c 100644 --- a/ggshield/cmd/auth/logout.py +++ b/ggshield/cmd/auth/logout.py @@ -96,7 +96,7 @@ def revoke_token(config: Config, instance_url: str) -> None: client = create_client( token, dashboard_to_api_url(instance_url), - allow_self_signed=config.user_config.allow_self_signed, + allow_self_signed=config.user_config.insecure, ) try: response = client.post(endpoint="token/revoke") diff --git a/ggshield/cmd/utils/common_options.py b/ggshield/cmd/utils/common_options.py index a2bad58f14..9c00e24d88 100644 --- a/ggshield/cmd/utils/common_options.py +++ b/ggshield/cmd/utils/common_options.py @@ -136,12 +136,30 @@ def log_file_callback( ) +def allow_self_signed_callback( + ctx: click.Context, param: click.Parameter, value: Optional[bool] +) -> Optional[bool]: + if value: + ui.display_warning( + "The --allow-self-signed option is deprecated. Use --insecure instead." + ) + return create_config_callback("insecure")(ctx, param, value) + + _allow_self_signed_option = click.option( "--allow-self-signed", is_flag=True, default=None, - help="Ignore ssl verification.", - callback=create_config_callback("allow_self_signed"), + help="Deprecated: use --insecure.", + callback=allow_self_signed_callback, +) + +_insecure_option = click.option( + "--insecure", + is_flag=True, + default=None, + help="Skip all certificate verification checks. WARNING: this option makes the transfer insecure.", + callback=create_config_callback("insecure"), ) _check_for_updates = click.option( @@ -174,6 +192,7 @@ def decorator(cmd: AnyFunction) -> AnyFunction: _debug_option(cmd) _log_file_option(cmd) _allow_self_signed_option(cmd) + _insecure_option(cmd) _check_for_updates(cmd) return cmd diff --git a/ggshield/core/client.py b/ggshield/core/client.py index e884e8ed85..5cd955c38d 100644 --- a/ggshield/core/client.py +++ b/ggshield/core/client.py @@ -7,6 +7,7 @@ from pygitguardian.models import APITokensResponse, Detail, TokenScope from requests import Session +from . import ui from .config import Config from .constants import DEFAULT_INSTANCE_URL from .errors import ( @@ -50,7 +51,7 @@ def create_client_from_config(config: Config) -> GGClient: return create_client( api_key, api_url, - allow_self_signed=config.user_config.allow_self_signed, + allow_self_signed=config.user_config.insecure, callbacks=callbacks, ) @@ -84,6 +85,16 @@ def create_client( def create_session(allow_self_signed: bool = False) -> Session: session = Session() if allow_self_signed: + ui.display_warning( + "SSL verification is disabled. Your connection to the GitGuardian API is NOT encrypted " + "and is vulnerable to man-in-the-middle attacks. Traffic, including API keys and scan results, " + "can be intercepted and modified." + ) + ui.display_warning( + "To securely use self-signed certificates with Python >= 3.10, disable this option and " + "install your certificate in your system's trust store. " + "See: https://docs.gitguardian.com/ggshield-docs/configuration#support-for-self-signed-certificates" + ) urllib3.disable_warnings() session.verify = False return session diff --git a/ggshield/core/config/user_config.py b/ggshield/core/config/user_config.py index 7d0f925de2..537d4838f6 100644 --- a/ggshield/core/config/user_config.py +++ b/ggshield/core/config/user_config.py @@ -87,7 +87,7 @@ class UserConfig(FilteredConfig): instance: Optional[str] = None exit_zero: bool = False verbose: bool = False - allow_self_signed: bool = False + insecure: bool = False max_commits_for_hook: int = 50 secret: SecretConfig = field(default_factory=SecretConfig) debug: bool = False @@ -198,6 +198,7 @@ def _load_config_dict( if dash_keys: _warn_about_dash_keys(config_path, dash_keys) _fix_ignore_known_secrets(dct) + _fix_allow_self_signed(dct, config_path) elif config_version == 1: deprecation_messages.append( f"{config_path} uses a deprecated configuration file format." @@ -226,6 +227,16 @@ def _fix_ignore_known_secrets(data: Dict[str, Any]) -> None: secret_dct[_IGNORE_KNOWN_SECRETS_KEY] = value +def _fix_allow_self_signed(data: Dict[str, Any], config_path: Path) -> None: + """Convert allow_self_signed to insecure and display a deprecation warning.""" + if insecure := data.pop("allow_self_signed", None): + ui.display_warning( + f"{config_path}: The 'allow_self_signed' option is deprecated. " + "Use 'insecure: true' instead." + ) + data["insecure"] = insecure + + def _warn_about_dash_keys(config_path: Path, dash_keys: Set[str]) -> None: for old_key in sorted(dash_keys): new_key = old_key.replace("-", "_") diff --git a/ggshield/core/config/v1_config.py b/ggshield/core/config/v1_config.py index f957cd2d01..64d96f719a 100644 --- a/ggshield/core/config/v1_config.py +++ b/ggshield/core/config/v1_config.py @@ -60,7 +60,7 @@ def copy_if_set(dct: Dict[str, Any], dst_key: str, src_key: Optional[str] = None copy_if_set(dct, "instance") copy_if_set(dct, "exit_zero") copy_if_set(dct, "verbose") - copy_if_set(dct, "allow_self_signed") + copy_if_set(dct, "insecure", "allow_self_signed") copy_if_set(dct, "max_commits_for_hook") return dct diff --git a/ggshield/verticals/auth/oauth.py b/ggshield/verticals/auth/oauth.py index e8ec3b6f26..73fc4f91e5 100644 --- a/ggshield/verticals/auth/oauth.py +++ b/ggshield/verticals/auth/oauth.py @@ -231,7 +231,7 @@ def _claim_token(self, authorization_code: str) -> None: body=urlparse.urlencode(request_params), ) - session = create_session(self.config.user_config.allow_self_signed) + session = create_session(self.config.user_config.insecure) response = session.post( urljoin(self.api_url, "/v1/oauth/token"), request_body, @@ -265,7 +265,7 @@ def _validate_access_token(self) -> Dict[str, Any]: response = create_client( self._access_token, self.api_url, - allow_self_signed=self.config.user_config.allow_self_signed, + allow_self_signed=self.config.user_config.insecure, ).get(endpoint="token") if not response.ok: raise OAuthError("The created token is invalid.") diff --git a/ggshield/verticals/hmsl/utils.py b/ggshield/verticals/hmsl/utils.py index efe226f249..5fdf171643 100644 --- a/ggshield/verticals/hmsl/utils.py +++ b/ggshield/verticals/hmsl/utils.py @@ -82,7 +82,7 @@ def get_token(config: Config) -> Optional[str]: client = create_client( api_url=config.saas_api_url, api_key=config.saas_api_key, - allow_self_signed=config.user_config.allow_self_signed, + allow_self_signed=config.user_config.insecure, ) audience = config.hmsl_audience except (MissingTokenError, AuthExpiredError): diff --git a/tests/unit/cmd/scan/test_path.py b/tests/unit/cmd/scan/test_path.py index 96ef441fc3..ab6814014c 100644 --- a/tests/unit/cmd/scan/test_path.py +++ b/tests/unit/cmd/scan/test_path.py @@ -236,10 +236,15 @@ def test_instance_option(self, cli_fs_runner): @pytest.mark.parametrize("position", [0, 1, 2, 3, 4]) def test_ssl_verify(self, cli_fs_runner, position): + """ + GIVEN the --insecure flag + WHEN running the path scan command + THEN SSL verification is disabled + """ self.create_files() cmd = ["secret", "scan", "path", "file1"] - cmd.insert(position, "--allow-self-signed") + cmd.insert(position, "--insecure") with patch("ggshield.core.client.GGClient") as client_mock: cli_fs_runner.invoke(cli, cmd) diff --git a/tests/unit/cmd/test_status.py b/tests/unit/cmd/test_status.py index a8ff124ef5..6c8acd6ce6 100644 --- a/tests/unit/cmd/test_status.py +++ b/tests/unit/cmd/test_status.py @@ -122,7 +122,12 @@ def get_api_status(env, instance=None): @pytest.mark.parametrize("verify", [True, False]) def test_ssl_verify(cli_fs_runner, verify): - cmd = ["api-status"] if verify else ["--allow-self-signed", "api-status"] + """ + GIVEN the --insecure flag + WHEN running the api-status command + THEN SSL verification is disabled + """ + cmd = ["api-status"] if verify else ["--insecure", "api-status"] with mock.patch("ggshield.core.client.GGClient") as client_mock: cli_fs_runner.invoke(cli, cmd) @@ -130,6 +135,25 @@ def test_ssl_verify(cli_fs_runner, verify): assert kwargs["session"].verify == verify +@pytest.mark.parametrize( + "cmd", + [ + ["api-status", "--allow-self-signed"], + ["--allow-self-signed", "api-status"], + ], +) +def test_allow_self_signed_backward_compatibility(cli_fs_runner, cmd): + """ + GIVEN the deprecated --allow-self-signed flag + WHEN it's placed before or after the subcommand + THEN SSL verification is disabled in both cases (backward compatibility) + """ + with mock.patch("ggshield.core.client.GGClient") as client_mock: + cli_fs_runner.invoke(cli, cmd) + _, kwargs = client_mock.call_args + assert kwargs["session"].verify is False + + @pytest.mark.parametrize("command", ["api-status", "quota"]) def test_instance_option(cli_fs_runner, command): """ diff --git a/tests/unit/core/config/test_user_config.py b/tests/unit/core/config/test_user_config.py index e829cf8bc3..6fa23fa9e1 100644 --- a/tests/unit/core/config/test_user_config.py +++ b/tests/unit/core/config/test_user_config.py @@ -294,6 +294,26 @@ def test_can_load_ignored_known_secrets_from_secret(self, local_config_path): config, _ = UserConfig.load(local_config_path) assert config.secret.ignore_known_secrets + def test_allow_self_signed_deprecation_warning(self, local_config_path, capsys): + """ + GIVEN a config file containing allow_self_signed: true + WHEN loading the config + THEN it is converted to insecure and a deprecation warning is displayed + """ + write_yaml( + local_config_path, + { + "version": 2, + "allow_self_signed": True, + }, + ) + config, _ = UserConfig.load(local_config_path) + assert config.insecure is True + captured = capsys.readouterr() + assert "allow_self_signed" in captured.err + assert "deprecated" in captured.err + assert "insecure" in captured.err + def test_bad_local_config(self, local_config_path, global_config_path): """ GIVEN a malformed .gitguardian.yaml, with a list of instance