Authenticate from mounted secrets and stop passing the password to seaf-cli

The password could only be given through the PASSWORD variable, so Docker
Compose and Swarm secrets were unusable and the password sat in the container
environment.

PASSWORD_FILE and TOKEN_FILE read a secret from a mounted file. Only trailing
newlines are stripped, since secret files are usually written with one and a
password that silently carries it just fails to authenticate; everything else,
including surrounding spaces, belongs to the secret. Setting both a variable
and a file for one secret is rejected rather than resolved by precedence,
because the two are meant to be the same value and quietly picking one hides
the mistake until authentication fails.

The installed seaf-cli supports token authentication and ignores the password
when a token is given, so sync now passes -T instead of -p. The account
password no longer appears in any command line. The token does, because
seaf-cli accepts credentials only as arguments, but a token can be revoked on
the server. This is documented in the README.

Both secrets are masked before a command is logged.

Tests cover newline handling, empty and missing files, conflicting sources,
and that neither secret reaches the logs. Requests are now blocked in tests:
the client retries with backoff, so a test that reached the network appeared
to hang instead of failing.

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:
 

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.

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:

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."""
 

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 secret in secrets:
+        if not secret:
+            continue
         for i, arg in enumerate(cmd):
-        if password in arg:
-            cmd[i] = arg.replace(password, "********")
+            if secret in arg:
+                cmd[i] = arg.replace(secret, "********")
     return cmd

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

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):

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)

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"]
 
 

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""):

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