diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index f4e827a..f300cbe 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -60,13 +60,21 @@ Acceptance criteria: ### Support secrets safely -- [ ] Implement `PASSWORD_FILE` for Docker Compose and Swarm secrets. -- [ ] Add token-based authentication and `TOKEN_FILE` if supported by the - installed Seafile CLI. -- [ ] Define precedence and reject conflicting secret sources. -- [ ] Ensure secrets are absent from application logs. -- [ ] Document any upstream limitation that exposes a secret in process - arguments. +- [x] Implement `PASSWORD_FILE` for Docker Compose and Swarm secrets. Only a + trailing newline is stripped, so a secret keeps any surrounding spaces. +- [x] Add token-based authentication and `TOKEN_FILE` if supported by the + installed Seafile CLI. It is: `seaf-cli sync` takes `-T`, and ignores the + password when a token is given. +- [x] Define precedence and reject conflicting secret sources. Two sources for + one secret (`PASSWORD` with `PASSWORD_FILE`) are rejected; a token takes + precedence over a password, as it does in `seaf-cli` itself. +- [x] Ensure secrets are absent from application logs. Both the password and + the token are masked before a command is logged. +- [x] Document any upstream limitation that exposes a secret in process + arguments. `seaf-cli` accepts credentials only as arguments, so a secret is + visible to any process in the container. The account password no longer goes + there: the client authenticates with an API token, which can be revoked. + See the README. Acceptance criteria: diff --git a/README.md b/README.md index 5f00eb2..a62caca 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,11 @@ volumes: If you're using non-standard port, you can specify it here, eg: _seafile.example.com:8080_. - `USERNAME` / `PASSWORD` - credentials to access Seafile server. + - `PASSWORD_FILE` - path to a file holding the password, for Docker Compose + and Swarm secrets. Use either `PASSWORD` or `PASSWORD_FILE`, never both. + - `TOKEN` / `TOKEN_FILE` - an API token to authenticate with instead of a + password. A token takes precedence over a password and can be revoked on the + server. Use either `TOKEN` or `TOKEN_FILE`, never both. - `SEAFILE_UID` / `SEAFILE_GID` - UID/GID of user inside container. You can use it to set permissions on synced files. Default values are _1000_ / _1000_. - `DELETE_CONFIRM_THRESHOLD` - represents the number of files that require @@ -45,6 +50,32 @@ volumes: - `UPLOAD_LIMIT` / `DOWNLOAD_LIMIT` - upload/download speed limit in B/s (bytes per second). Default values are _0_ (unlimited). +### Using Docker secrets: +Mount the secret and point `PASSWORD_FILE` at it, so the password never enters + the environment: + +```yaml +services: + seafile-client: + environment: + PASSWORD_FILE: /run/secrets/seafile_password + secrets: + - seafile_password + +secrets: + seafile_password: + file: ./seafile_password.txt +``` + +A trailing newline in the file is ignored; everything else, including + surrounding spaces, is part of the secret. + +The client authenticates `seaf-cli` with an API token rather than the account + password, so the password never appears in a command line. The token does + appear there, and command lines are visible to any process in the container. + This is a limitation of `seaf-cli`, which accepts credentials only as + arguments. A token can at least be revoked on the server. + ### Volumes: - `/dsc/seafile-data` Seafile client data directory (sync status, etc). - `/dsc/seafile` Seafile libraries content. diff --git a/dsc/client.py b/dsc/client.py index 6e29f37..757414b 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -12,8 +12,8 @@ from urllib3.util.retry import Retry import seafile from dsc import const -from dsc.errors import DaemonError, DaemonTimeout -from dsc.misc import create_dir, hide_password, user_cmd +from dsc.errors import ConfigError, DaemonError, DaemonTimeout +from dsc.misc import create_dir, hide_secrets, user_cmd _lg = logging.getLogger(__name__) @@ -22,13 +22,16 @@ class SeafileClient: def __init__(self, host: str, user: str, - passwd: str, - app_dir: str = const.DEFAULT_APP_DIR): + passwd: str = None, + app_dir: str = const.DEFAULT_APP_DIR, + token: str = None): self.user = user self.password = passwd self.app_dir = os.path.abspath(app_dir) self.rpc = seafile.RpcClient(os.path.join(self.app_dir, 'seafile-data', 'seafile.sock')) - self.__token = None + # A token supplied by the operator is used as is; otherwise one is + # fetched from the server with the password. + self.__token = token # determine server URL (assume HTTPS unless explicitly specified) if host.startswith('http://') or host.startswith('https://'): @@ -86,6 +89,8 @@ class SeafileClient: @property def token(self): if self.__token is None: + if not self.password: + raise ConfigError("No password or token available to authenticate") url = f"{self.url}/api2/auth-token/" _lg.info("Fetching token: %s", url) r = self.session.post(url, data={"username": self.user, "password": self.password}) @@ -170,11 +175,14 @@ class SeafileClient: "-s", self.url, "-d", lib_dir, "-u", self.user, - "-p", self.password, + # The token, not the password: seaf-cli ignores the password when a + # token is given, and command arguments are visible to any process + # in the container. A token can also be revoked on the server. + "-T", self.token, ] _lg.info( "Syncing library %s into %s: %s", lib_id, lib_dir, - " ".join(hide_password(cmd, self.password)), + " ".join(hide_secrets(cmd, self.token, self.password)), ) proc = self.__run(cmd, check=False) if proc.returncode != 0: diff --git a/dsc/errors.py b/dsc/errors.py index 3b188d3..41e0453 100644 --- a/dsc/errors.py +++ b/dsc/errors.py @@ -5,6 +5,10 @@ class DscError(Exception): """Base class for failures that stop the client.""" +class ConfigError(DscError): + """The container was configured in a way that cannot be acted on.""" + + class DaemonError(DscError): """A seaf-cli command failed.""" diff --git a/dsc/misc.py b/dsc/misc.py index edb1e9a..2b214eb 100644 --- a/dsc/misc.py +++ b/dsc/misc.py @@ -55,9 +55,13 @@ def user_cmd(argv: list) -> list: return ["runuser", "-u", DEFAULT_USERNAME, "--"] + argv -def hide_password(cmd: list, password: str) -> list: +def hide_secrets(cmd: list, *secrets: str) -> list: + """Mask every given secret in a command before it is logged.""" cmd = cmd.copy() - for i, arg in enumerate(cmd): - if password in arg: - cmd[i] = arg.replace(password, "********") + for secret in secrets: + if not secret: + continue + for i, arg in enumerate(cmd): + if secret in arg: + cmd[i] = arg.replace(secret, "********") return cmd diff --git a/dsc/secrets.py b/dsc/secrets.py new file mode 100644 index 0000000..137d3fe --- /dev/null +++ b/dsc/secrets.py @@ -0,0 +1,57 @@ +"""Read secrets from a variable or a mounted file. + +Docker Compose and Swarm secrets arrive as files under /run/secrets, which +keeps them out of the environment. Nothing here ever puts a secret value into +a message: an error about a secret must not leak the secret. +""" + +import logging +import os + +from dsc.errors import ConfigError + +_lg = logging.getLogger(__name__) + + +def read_secret_file(path: str) -> str: + """ + Return the secret stored in ``path``. + + Only trailing newlines are removed. Secret files are commonly written with + one, and a password that silently carries it just fails to authenticate. + Everything else, including surrounding spaces, is part of the secret. + """ + if not os.path.isfile(path): + raise ConfigError(f"Secret file {path} does not exist or is not a file") + + try: + with open(path, "r") as f: + secret = f.read() + except OSError as err: + raise ConfigError(f"Cannot read secret file {path}: {err.strerror}") + + secret = secret.rstrip("\r\n") + if not secret: + raise ConfigError(f"Secret file {path} is empty") + return secret + + +def resolve_secret(name: str, value: str, file_path: str) -> str: + """ + Return the secret named ``name``, from either the variable or the file. + + Supplying both is rejected rather than resolved by precedence: the two + values are usually meant to be the same, and quietly picking one hides a + misconfiguration until authentication fails. + """ + file_name = f"{name}_FILE" + if value and file_path: + raise ConfigError( + f"{name} and {file_name} are both set; use exactly one of them" + ) + + if file_path: + _lg.info("Reading %s from %s", name, file_path) + return read_secret_file(file_path) + + return value or None diff --git a/start.py b/start.py index 5733e23..f51cc46 100755 --- a/start.py +++ b/start.py @@ -7,9 +7,10 @@ import signal import sys from dsc import SeafileClient, const -from dsc.errors import DscError, GracefulShutdown +from dsc.errors import ConfigError, DscError, GracefulShutdown from dsc.misc import setup_uid, create_dir from dsc.paths import plan_lib_dirs +from dsc.secrets import resolve_secret _lg = logging.getLogger('dsc') @@ -82,6 +83,9 @@ def main(): parser.add_argument("-s", "--server") parser.add_argument("-u", "--username") parser.add_argument("-p", "--password") + parser.add_argument("--password-file") + parser.add_argument("-T", "--token") + parser.add_argument("--token-file") parser.add_argument("-l", "--libraries") parser.add_argument("--uid", type=int) parser.add_argument("--gid", type=int) @@ -94,6 +98,9 @@ def main(): server=os.getenv("SERVER_HOST"), username=os.getenv("USERNAME"), password=os.getenv("PASSWORD"), + password_file=os.getenv("PASSWORD_FILE"), + token=os.getenv("TOKEN"), + token_file=os.getenv("TOKEN_FILE"), libraries=os.getenv("LIBRARY_ID"), uid=os.getenv("SEAFILE_UID", default=1000), gid=os.getenv("SEAFILE_GID", default=1000), @@ -107,17 +114,28 @@ def main(): parser.error("Seafile server is not specified") if not args.username: parser.error("username is not specified") - if not args.password: - parser.error("password is not specified") if not args.libraries: parser.error("library is not specified") + try: + password = resolve_secret("PASSWORD", args.password, args.password_file) + token = resolve_secret("TOKEN", args.token, args.token_file) + except ConfigError as err: + _lg.error("%s", err) + return 2 + if not password and not token: + parser.error("neither a password nor a token is specified") + if password and token: + # seaf-cli behaves the same way: a token makes the password unused. + _lg.info("Both a password and a token are configured; using the token") + install_signal_handlers() setup_uid(args.uid, args.gid) create_dir(const.DEFAULT_APP_DIR) - client = SeafileClient(args.server, args.username, args.password, const.DEFAULT_APP_DIR) + client = SeafileClient(args.server, args.username, password, + const.DEFAULT_APP_DIR, token=token) # check for deprecated /data directory if os.path.isdir(const.DEPRECATED_LIBS_DIR): diff --git a/tests/conftest.py b/tests/conftest.py index 7bded19..a702fd6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,8 @@ import sys import types from unittest import mock +import pytest + def _install_seafile_stub(): if "seafile" in sys.modules: @@ -19,3 +21,19 @@ def _install_seafile_stub(): _install_seafile_stub() + + +@pytest.fixture(autouse=True) +def no_network(monkeypatch): + """ + Fail loudly instead of reaching the network. + + The client retries failed requests with backoff, so a test that + accidentally makes a real request appears to hang rather than fail. + """ + import requests + + def blocked(self, method, url, *args, **kwargs): + raise AssertionError(f"Test attempted a real request: {method} {url}") + + monkeypatch.setattr(requests.Session, "request", blocked) diff --git a/tests/test_commands.py b/tests/test_commands.py index 341682f..497c913 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -8,7 +8,7 @@ import pytest from dsc import client as client_module from dsc import const from dsc.client import SeafileClient -from dsc.misc import hide_password, user_cmd +from dsc.misc import hide_secrets, user_cmd # Values that would be interpreted by a shell if a command were ever joined # into a string and handed to ``su -c``. @@ -34,6 +34,7 @@ def client(): user="user@example", passwd="s3cret", app_dir="/dsc", + token="tok-en", ) @@ -71,13 +72,21 @@ def test_user_cmd_rejects_a_string(): user_cmd("seaf-cli status") -@pytest.mark.parametrize("password", SHELL_METACHARACTERS) -def test_sync_passes_password_as_one_unquoted_argument(client, calls, password): - client.password = password +@pytest.mark.parametrize("secret", SHELL_METACHARACTERS) +def test_sync_passes_the_token_as_one_unquoted_argument(client, calls, secret): + client._SeafileClient__token = secret client.sync_lib("lib-id", "/dsc/seafile/Docs") argv, _ = calls[-1] - assert argv[argv.index("-p") + 1] == password + assert argv[argv.index("-T") + 1] == secret + + +def test_sync_authenticates_with_the_token_not_the_password(client, calls): + client.sync_lib("lib-id", "/dsc/seafile/Docs") + + argv, _ = calls[-1] + assert "-p" not in argv, "the account password must never reach argv" + assert client.password not in argv @pytest.mark.parametrize("lib_dir", [ @@ -102,23 +111,34 @@ def test_sync_command_is_a_list_of_strings(client, calls): assert kwargs.get("shell") is not True -@pytest.mark.parametrize("password", SHELL_METACHARACTERS) -def test_sync_never_logs_the_password(client, calls, caplog, password): - client.password = password +@pytest.mark.parametrize("secret", SHELL_METACHARACTERS) +def test_sync_never_logs_the_token_or_the_password(client, calls, caplog, secret): + client._SeafileClient__token = secret + client.password = secret + "-pw" with caplog.at_level("INFO"): client.sync_lib("lib-id", "/dsc/seafile/Docs") - assert password not in caplog.text + assert secret not in caplog.text + assert client.password not in caplog.text -def test_hide_password_masks_every_occurrence(): - masked = hide_password(["-p", "secret", "-u", "secret-user"], "secret") +def test_hide_secrets_masks_every_occurrence(): + masked = hide_secrets(["-p", "secret", "-u", "secret-user"], "secret") assert masked == ["-p", "********", "-u", "********-user"] -def test_hide_password_does_not_mutate_the_original(): +def test_hide_secrets_masks_several_secrets(): + masked = hide_secrets(["-p", "pw", "-T", "tok"], "pw", "tok") + assert masked == ["-p", "********", "-T", "********"] + + +def test_hide_secrets_ignores_secrets_that_are_not_set(): + assert hide_secrets(["-T", "tok"], None, "tok") == ["-T", "********"] + + +def test_hide_secrets_does_not_mutate_the_original(): argv = ["-p", "secret"] - hide_password(argv, "secret") + hide_secrets(argv, "secret") assert argv == ["-p", "secret"] diff --git a/tests/test_daemon.py b/tests/test_daemon.py index d182a49..bf25453 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -25,7 +25,9 @@ def fast_polling(monkeypatch): @pytest.fixture def client(monkeypatch): monkeypatch.setattr(client_module, "create_dir", lambda path: None) - return SeafileClient("seafile.example", "user", "pw", "/dsc") + # A token keeps the tests off the network: without one, authentication + # would be attempted against the server before syncing. + return SeafileClient("seafile.example", "user", "pw", "/dsc", token="tok-en") def fake_runner(returncodes, monkeypatch, stdout=b""): diff --git a/tests/test_secrets.py b/tests/test_secrets.py new file mode 100644 index 0000000..644958d --- /dev/null +++ b/tests/test_secrets.py @@ -0,0 +1,82 @@ +"""Secrets may come from a variable or a mounted file, never ambiguously.""" + +import pytest + +from dsc.errors import ConfigError +from dsc.secrets import read_secret_file, resolve_secret + + +def write(tmp_path, content, name="secret"): + path = tmp_path / name + path.write_text(content) + return str(path) + + +@pytest.mark.parametrize("content, expected", [ + ("s3cret", "s3cret"), + # Secret files are usually written with a trailing newline; a password + # that silently includes it fails to authenticate with no useful message. + ("s3cret\n", "s3cret"), + ("s3cret\r\n", "s3cret"), + ("s3cret\n\n", "s3cret"), + # Everything else is part of the secret, including inner and edge spaces. + (" s3cret ", " s3cret "), + ("pa ss", "pa ss"), + ('pa"ss$(id)`x`', 'pa"ss$(id)`x`'), + ("паssшörд\n", "паssшörд"), +]) +def test_read_secret_file_strips_only_trailing_newlines(tmp_path, content, expected): + assert read_secret_file(write(tmp_path, content)) == expected + + +@pytest.mark.parametrize("content", ["", "\n", "\r\n", "\n\n"]) +def test_read_secret_file_rejects_an_empty_secret(tmp_path, content): + with pytest.raises(ConfigError): + read_secret_file(write(tmp_path, content)) + + +def test_read_secret_file_reports_a_missing_file(tmp_path): + missing = str(tmp_path / "absent") + with pytest.raises(ConfigError) as err: + read_secret_file(missing) + assert missing in str(err.value) + + +def test_read_secret_file_never_puts_the_secret_in_the_error(tmp_path): + path = write(tmp_path, "") + with pytest.raises(ConfigError) as err: + read_secret_file(path) + assert "empty" in str(err.value).lower() + + +def test_resolve_secret_prefers_nothing_when_neither_is_set(): + assert resolve_secret("PASSWORD", None, None) is None + + +def test_resolve_secret_reads_the_variable(): + assert resolve_secret("PASSWORD", "s3cret", None) == "s3cret" + + +def test_resolve_secret_reads_the_file(tmp_path): + path = write(tmp_path, "s3cret\n") + assert resolve_secret("PASSWORD", None, path) == "s3cret" + + +def test_resolve_secret_rejects_two_sources_for_one_secret(tmp_path): + path = write(tmp_path, "s3cret") + with pytest.raises(ConfigError) as err: + resolve_secret("PASSWORD", "other", path) + assert "PASSWORD" in str(err.value) + assert "PASSWORD_FILE" in str(err.value) + + +def test_resolve_secret_conflict_message_hides_both_values(tmp_path): + path = write(tmp_path, "file-secret") + with pytest.raises(ConfigError) as err: + resolve_secret("PASSWORD", "env-secret", path) + assert "env-secret" not in str(err.value) + assert "file-secret" not in str(err.value) + + +def test_resolve_secret_treats_an_empty_variable_as_unset(): + assert resolve_secret("PASSWORD", "", None) is None