diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index fb47370..8fca1f9 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -1,3 +1,10 @@ # Unreleased ## Summary + +## Bug Fixes + +* #168: Fixed `get_cli_arg`/`kwargs_to_cli_args` raising `NoSuchOption` for secret + option values starting with `-`/`--` (e.g. SaaS/DB credentials and ids). +* #173: Fixed `secret_callback`'s env-var fallback using a hyphenated name (e.g. + `DB-PASSWORD`) instead of the documented `DB_PASSWORD`. diff --git a/exasol/python_extension_common/cli/std_options.py b/exasol/python_extension_common/cli/std_options.py index 76047e6..6ba70b9 100644 --- a/exasol/python_extension_common/cli/std_options.py +++ b/exasol/python_extension_common/cli/std_options.py @@ -1,5 +1,6 @@ import os import re +import shlex from enum import ( Enum, Flag, @@ -106,6 +107,46 @@ def clear_formatters(self): # This text will be displayed instead of the actual value for a "secret" option. SECRET_DISPLAY = "***" +# A lookalike character used as a reserved delimiter in encode_secret_value's output +# (see there). Click's parser only recognizes the ASCII hyphen-minus (U+002D) as an +# option prefix, so this is invisible to it. +_ESCAPE_BOUNDARY = "‐" + + +def encode_secret_value(value: str) -> str: + """ + Encodes value so that a secret option's value can be put on the command line + without click's parser mistaking it for a new option (see get_cli_arg's + docstring). Use decode_secret_value to reverse this. + + A value that neither starts with "-" (which click's parser would choke on) nor + with _ESCAPE_BOUNDARY (which decode_secret_value would otherwise mistake for its + own encoding) is returned unchanged. Otherwise, the value is encoded as + _ESCAPE_BOUNDARY + + _ESCAPE_BOUNDARY + , e.g. "--secret" -> "‐2‐secret". This length-prefixed form is + an exact inverse for every possible input, including a value that itself starts + with "-" and/or _ESCAPE_BOUNDARY, since decoding only ever needs the first two + occurrences of _ESCAPE_BOUNDARY to recover n and the remainder verbatim. + """ + if not value.startswith("-") and not value.startswith(_ESCAPE_BOUNDARY): + return value + stripped = value.lstrip("-") + n = len(value) - len(stripped) + return f"{_ESCAPE_BOUNDARY}{n}{_ESCAPE_BOUNDARY}{stripped}" + + +def decode_secret_value(value: str) -> str: + """ + Inverse of encode_secret_value. Applied automatically by secret_callback for + options built through this module (see get_cli_arg). Call this explicitly only + if you parse a kwargs_to_cli_args()/get_cli_arg() args string some other way, + e.g. with a different parser or in another program/language. + """ + if not value.startswith(_ESCAPE_BOUNDARY): + return value + _, n, rest = value.split(_ESCAPE_BOUNDARY, 2) + return "-" * int(n) + rest + def secret_callback(ctx: click.Context, param: click.Option, value: Any): """ @@ -115,8 +156,16 @@ def secret_callback(ctx: click.Context, param: click.Option, value: Any): be no way of altering this behaviour. """ if value == SECRET_DISPLAY: - envar_name = param.opts[0][2:].upper() + # Derived from param.opts[0] (the CLI flag itself) rather than param.name, so this + # keeps tracking the flag actually shown to the user (e.g. in --help) even for an + # option declared directly via make_option_secret with a custom internal name. + # Hyphens are converted to underscores because POSIX environment variable names + # can't contain them (#173) - matching the underscored names documented in + # user-guide.md, e.g. "--db-password" -> "DB_PASSWORD". + envar_name = param.opts[0][2:].upper().replace("-", "_") return os.environ.get(envar_name) + if isinstance(value, str): + return decode_secret_value(value) return value @@ -179,7 +228,7 @@ def _get_param_name(std_param: StdParamOrName) -> str: Standard options defined in the form of key-value pairs, where key is the option's StaParam key and the value is a kwargs for creating the click.Options(...). """ -_std_options = { +_std_options: dict[StdParams, dict[str, Any]] = { StdParams.bucketfs_name: {"type": str}, StdParams.bucketfs_host: {"type": str}, StdParams.bucketfs_port: {"type": int}, @@ -249,6 +298,30 @@ def get_bool_opt_name(std_param: StdParamOrName) -> str: return f"--{opt_name}/--no-{opt_name}" +def is_secret_param(std_param: StdParamOrName) -> bool: + """ + True if std_param is a StdParams member defined with hide_input=True in + _std_options. A plain string name is only considered secret if it happens to match + the name of such a StdParams member; any other string name can never be secret, + since it has no entry in _std_options. + + Note: this only reflects the default hide_input in _std_options, not any + hide_input a caller passed directly to create_std_option or via + select_std_options(override=...). get_cli_arg (the only caller) is only ever + given a param name, not the click.Option that was actually constructed, so it + has no way to see such an override. + """ + if isinstance(std_param, StdParams): + member = std_param + elif std_param in StdParams.__members__: + member = StdParams[std_param] + else: + return False + if member not in _std_options: + return False + return bool(_std_options[member].get("hide_input", False)) + + def create_std_option(std_param: StdParamOrName, **kwargs) -> click.Option: """ Creates a Click option. @@ -332,12 +405,32 @@ def get_cli_arg(std_param: StdParamOrName, param_value: Any) -> str: Makes a CLI args string from an option and its value. An option can be given as either an StdParams or its string name. For boolean values the args string takes the form --option-name/--no-option-name. + A non-boolean value is quoted with shlex.quote, so the returned string can be + split back into args with shlex.split (as click.testing.CliRunner.invoke does for + a string args) regardless of what characters the value contains. + + For a "secret" (hide_input) standard parameter, click's parser can't tell an + option value starting with "-"/"--" apart from the option being given with no + value at all, since such an option allows omitting its value (which is how it + lets its value be entered interactively instead) - this holds no matter how the + option and its value are joined in the returned string. To avoid that, such a + value is encoded with encode_secret_value before being put on the command line. + + This is decoded back automatically only if the resulting args string is parsed by + a click.Option built through this module (create_std_option, select_std_options, + make_option_secret), since decoding happens in their shared secret_callback. A + caller who instead parses this string themselves, or hands it to a different + program/language, must call decode_secret_value explicitly to recover the + original value. """ option_name = _get_param_name(std_param).replace("_", "-") if isinstance(param_value, bool): return f"--{option_name}" if param_value else f"--no-{option_name}" - return f'--{option_name} "{param_value}"' + str_value = str(param_value) + if is_secret_param(std_param): + str_value = encode_secret_value(str_value) + return f"--{option_name} {shlex.quote(str_value)}" def kwargs_to_cli_args(**kwargs) -> str: diff --git a/test/integration/cli/test_language_container_deployer_cli.py b/test/integration/cli/test_language_container_deployer_cli.py index 88d9543..62705a3 100644 --- a/test/integration/cli/test_language_container_deployer_cli.py +++ b/test/integration/cli/test_language_container_deployer_cli.py @@ -11,7 +11,6 @@ import pytest from click.testing import CliRunner -from exasol.python_extension_common.cli import std_options from exasol.python_extension_common.cli.language_container_deployer_cli import ( LanguageContainerDeployerCli, ) @@ -30,29 +29,6 @@ CONTAINER_NAME_ARG = "container_name" -@pytest.fixture(autouse=True) -def _patch_get_cli_arg_for_dash_prefixed_values(monkeypatch): - """ - SaaS database ids are randomly generated and may themselves start with "-" - (e.g. "--dI0m90RUKefql382tsWA"). `get_cli_arg` joins an option and its - value with a space, which click's parser can mistake for a new option - when the value itself looks like one. This is patched here, rather than - in `get_cli_arg` itself, to avoid changing that function's behavior for - its other, non-test callers. See - https://github.com/exasol/python-extension-common/issues/168 - """ - original_get_cli_arg = std_options.get_cli_arg - - def patched_get_cli_arg(std_param, param_value): - if isinstance(param_value, bool) or not str(param_value).startswith("-"): - return original_get_cli_arg(std_param, param_value) - option_name = std_param if isinstance(std_param, str) else std_param.name - option_name = option_name.replace("_", "-") - return f'--{option_name}="{param_value}"' - - monkeypatch.setattr(std_options, "get_cli_arg", patched_get_cli_arg) - - @pytest.fixture(scope="session") def onprem_cli_args( backend_aware_onprem_database, exasol_config, bucketfs_config, language_alias diff --git a/test/unit/cli/test_std_options.py b/test/unit/cli/test_std_options.py index babd474..c616a6d 100644 --- a/test/unit/cli/test_std_options.py +++ b/test/unit/cli/test_std_options.py @@ -1,3 +1,5 @@ +import shlex + import click import pytest from click.testing import CliRunner @@ -9,9 +11,12 @@ StdTags, check_params, create_std_option, + decode_secret_value, + encode_secret_value, get_bool_opt_name, get_cli_arg, get_opt_name, + is_secret_param, kwargs_to_cli_args, select_std_options, ) @@ -147,29 +152,44 @@ def test_hidden_opt_with_envar(monkeypatch): """ This test checks the mechanism of providing a value of a confidential parameter via an environment variable. + + Regression test for #173: the env var name must be underscored (DB_PASSWORD), not + the hyphenated form of the CLI flag (DB-PASSWORD), since the latter can't even be + set via `export` in a real shell. """ std_param = StdParams.db_password - envar_name = std_param.name.upper() + envar_name = "DB_PASSWORD" param_value = "my_password" + captured = {} + def func(**kwargs): - assert std_param.name in kwargs - assert kwargs[std_param.name] == param_value + captured.update(kwargs) opt = create_std_option(std_param, type=str, hide_input=True) cmd = click.Command("do_something", params=[opt], callback=func) runner = CliRunner() monkeypatch.setenv(envar_name, param_value) - runner.invoke(cmd) + result = runner.invoke(cmd, catch_exceptions=False, standalone_mode=False) + assert result.exit_code == 0 + assert captured[std_param.name] == param_value + + +_QUOTE_INSIDE_VALUE = 'quote"inside' @pytest.mark.parametrize( ["std_param", "param_value", "expected_result"], [ - (StdParams.db_user, "Me", '--db-user "Me"'), - ("user_rating", 5, '--user-rating "5"'), + (StdParams.db_user, "Me", "--db-user Me"), + ("user_rating", 5, "--user-rating 5"), (StdParams.use_ssl_cert_validation, True, "--use-ssl-cert-validation"), (StdParams.use_ssl_cert_validation, False, "--no-use-ssl-cert-validation"), + ( + StdParams.db_user, + _QUOTE_INSIDE_VALUE, + f"--db-user {shlex.quote(_QUOTE_INSIDE_VALUE)}", + ), ], ) def test_get_cli_arg(std_param, param_value, expected_result): @@ -179,10 +199,176 @@ def test_get_cli_arg(std_param, param_value, expected_result): def test_kwargs_to_cli_args(): arg_string = kwargs_to_cli_args(use_rgb=True, colour="Blue", compress_image=False) arg_set = set(arg_string.split()) - expected_set = {"--use-rgb", "--colour", '"Blue"', "--no-compress-image"} + expected_set = {"--use-rgb", "--colour", "Blue", "--no-compress-image"} assert arg_set == expected_set +def test_get_cli_arg_value_with_double_quote_survives_shlex_round_trip(): + """ + Regression test: get_cli_arg used to wrap the value in unescaped literal double + quotes, so a value containing '"' produced an args string that shlex/click can't + parse (the same class of bug as #168, just triggered by a different character). + """ + value = 'pa"ss' + arg = get_cli_arg(StdParams.db_user, value) + assert shlex.split(arg) == ["--db-user", value] + + +@pytest.mark.parametrize( + ["std_param", "expected"], + [ + (StdParams.saas_database_id, True), + (StdParams.db_password, True), + (StdParams.bucketfs_password, True), + (StdParams.saas_account_id, True), + (StdParams.saas_token, True), + (StdParams.db_user, False), + ("saas_database_id", True), + ("db_user", False), + ("not_a_std_param", False), + ], +) +def test_is_secret_param(std_param, expected): + assert is_secret_param(std_param) is expected + + +@pytest.mark.parametrize( + "value", + [ + "--dI0m90RUKefql382tsWA", + "-dashy", + "---triple-dash", + "-", + "", + # Values that themselves contain the reserved escape-boundary character + # (U+2010), which decode_secret_value used to always treat as its own + # encoding, corrupting a value that legitimately starts with it. + "‐2-", + "‐‐realtoken", + "-‐‐foo", + "‐", + ], +) +def test_encode_decode_secret_value_roundtrip(value): + assert decode_secret_value(encode_secret_value(value)) == value + + +def test_encode_secret_value_leaves_unremarkable_values_unchanged(): + assert encode_secret_value("regular_value") == "regular_value" + + +@pytest.mark.parametrize( + ["value", "expected_encoded"], + [ + # Plain dash-prefixed values: the leading "-" run is replaced by the + # escape-boundary character (U+2010, a lookalike click's parser doesn't + # recognize as an option prefix) followed by its length, so the encoded + # value no longer starts with an ASCII "-". + ("--dI0m90RUKefql382tsWA", "‐2‐dI0m90RUKefql382tsWA"), + ("-dashy", "‐1‐dashy"), + ("---triple-dash", "‐3‐triple-dash"), + ("-", "‐1‐"), + # Values that themselves start with the escape-boundary character (but + # not with an ASCII "-") still get the same two-part prefix, with a + # leading-dash count of 0, so decode_secret_value can still tell them + # apart from a "real" encoding of a dash-prefixed value. + ("‐2-", "‐0‐‐2-"), + ("‐‐realtoken", "‐0‐‐‐realtoken"), + ("-‐‐foo", "‐1‐‐‐foo"), + ("‐", "‐0‐‐"), + ], +) +def test_encode_secret_value_escapes_leading_dashes(value, expected_encoded): + """ + Regression test for the PR #174 review comment: test_encode_decode_secret_value_roundtrip + only proves encode_secret_value and decode_secret_value are inverses of each other, not + that encoding actually strips the leading "-"/_ESCAPE_BOUNDARY that click's parser + chokes on. This pins down the exact encoded form instead. + """ + encoded = encode_secret_value(value) + assert encoded == expected_encoded + assert not encoded.startswith("-") + + +def test_get_cli_arg_secret_param_with_dash_prefixed_value(): + """ + Regression test for #168. A secret option's value that itself starts with "-" + breaks click's parser (see docstring of get_cli_arg for why), regardless of + whether it's joined to the option with a space or "=". get_cli_arg works around + this by encoding the leading dash(es) instead of putting them on the command + line literally. + """ + dashy_value = "--dI0m90RUKefql382tsWA" + arg = get_cli_arg(StdParams.saas_database_id, dashy_value) + assert arg == f"--saas-database-id {shlex.quote(encode_secret_value(dashy_value))}" + + +def test_get_cli_arg_secret_param_end_to_end_via_click(): + """ + End-to-end regression test for #168: builds the real saas_database_id option and + invokes it through click, with a value that used to raise NoSuchOption. + """ + dashy_value = "--dI0m90RUKefql382tsWA" + opt = create_std_option(StdParams.saas_database_id, type=str, hide_input=True) + + captured = {} + + def func(**kwargs): + captured.update(kwargs) + + cmd = click.Command("do_something", params=[opt], callback=func) + arg_string = kwargs_to_cli_args(saas_database_id=dashy_value) + + runner = CliRunner() + result = runner.invoke(cmd, args=arg_string, catch_exceptions=False, standalone_mode=False) + + assert result.exit_code == 0 + assert captured["saas_database_id"] == dashy_value + + +def test_get_cli_arg_secret_params_survive_full_saas_option_set(): + """ + Regression test for #168, exercised through the full option set built the same + way LanguageContainerDeployerCli's SaaS CLI is (select_std_options over DB|SAAS, + BFS|SAAS, SLC tags), to guard against a secret param being silently dropped when + mixed in with many other options. + """ + opts = select_std_options([StdTags.DB | StdTags.SAAS, StdTags.BFS | StdTags.SAAS, StdTags.SLC]) + captured = {} + + def func(**kwargs): + captured.update(kwargs) + + cmd = click.Command("deploy_slc", params=opts, callback=func) + + saas_cli_args = { + StdParams.saas_url.name: "https://cloud.exasol.com", + StdParams.saas_account_id.name: "--saas-acct-dashy", + StdParams.saas_database_id.name: "--dI0m90RUKefql382tsWA", + StdParams.saas_token.name: "--saas-token-dashy", + StdParams.path_in_bucket.name: "container", + StdParams.language_alias.name: "PYTHON3_MY_LANG", + } + slc_cli_args = { + StdParams.alter_system.name: True, + StdParams.allow_override.name: True, + StdParams.wait_for_completion.name: True, + } + extra_cli_args = {StdParams.version.name: "1.2.3"} + + arg_string = kwargs_to_cli_args(**saas_cli_args, **slc_cli_args, **extra_cli_args) + runner = CliRunner() + result = runner.invoke(cmd, args=arg_string, catch_exceptions=False, standalone_mode=False) + + assert result.exit_code == 0 + for name in ( + StdParams.saas_account_id.name, + StdParams.saas_database_id.name, + StdParams.saas_token.name, + ): + assert captured[name] == saas_cli_args[name] + + @pytest.mark.parametrize( ["std_params", "param_kwargs", "expected_result"], [