Merge pull request #23 from snegov/feature/secrets-support

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

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

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