Maxim Shulga
c15763d9f8
Commands were joined into a string and passed to "su -c", so a login shell parsed every interpolated value. A library name is chosen on the server, and "sync -d /dsc/seafile/<name>" put it straight into that string: a library named 'Docs; touch /tmp/pwned; #' ran as an arbitrary command. Confirmed against the built image before and after this change. Every command is now an argument list executed by "runuser --", which execs directly with no shell. runuser -u sets the same HOME and a PATH that still finds the seaf-cli wrapper, so the daemon environment is unchanged. The password no longer needs its surrounding quotes, which also fixes passwords containing quotes, dollar signs, and backticks. Library names are untrusted, so dsc/paths.py reduces each one to a single path component and proves the result stays under the libraries directory with symlinks resolved. Empty, ".", "..", and names containing a separator are skipped with a warning; names that would collide are suffixed with their library ID rather than sharing a directory. Tests cover quotes, command substitutions, separators, newlines, absolute paths, parent traversal, Unicode, and colliding names. They run in a separate CI job so application failures stay distinguishable from packaging failures.
228 lines
10 KiB
Markdown
228 lines
10 KiB
Markdown
# Improvement Plan
|
|
|
|
This plan prioritizes user safety and a working release pipeline before public
|
|
portfolio polish. The project already has useful adoption evidence: more than
|
|
5,800 Docker Hub pulls, external issues, and external contributions.
|
|
|
|
Security-sensitive findings should be fixed and released before detailed
|
|
exploit scenarios are discussed in public issues.
|
|
|
|
## P0 - Release blockers
|
|
|
|
### Restore reproducible image builds
|
|
|
|
- [x] Replace the unavailable Seafile APT repository used in `Dockerfile`.
|
|
Upstream discontinued it; the CLI now ships as an x86_64-only AppImage.
|
|
- [x] Install a supported Seafile CLI version from an official source.
|
|
- [x] Pin the version and verify its checksum during the build. The checksum is
|
|
trust-on-first-use: upstream publishes no digest or signature alongside the
|
|
AppImage, so this is weaker than the signed APT repository it replaces.
|
|
- [x] Remove `apt-key`, which is deprecated and trusts keys globally.
|
|
- [x] Add a smoke check that `seaf-cli` runs to the image build. Note that
|
|
`seaf-cli` has no `--version` flag; use `seaf-cli --help` and assert the
|
|
expected subcommands, and record the installed version from the package
|
|
manager instead.
|
|
- [ ] Scan the resulting image and review every high or critical finding.
|
|
- [ ] Establish a stronger trust anchor for the AppImage than a self-derived
|
|
checksum: an upstream digest or signature, or a verified artifact mirrored
|
|
somewhere we control.
|
|
|
|
Acceptance criteria:
|
|
|
|
- A clean `docker build --no-cache .` succeeds in CI.
|
|
- The installed Seafile version is explicit and current.
|
|
- An invalid artifact checksum fails the build. Upstream publishes no signature
|
|
for the AppImage, so signature verification is not achievable from this
|
|
source.
|
|
|
|
### Remove shell and path injection risks
|
|
|
|
- [x] Pass subprocess arguments as arrays instead of joining values into shell
|
|
command strings in `dsc/client.py` and `dsc/misc.py`. The password is no
|
|
longer wrapped in quotes to survive a shell, which also fixes passwords
|
|
containing quotes, `$`, and backticks.
|
|
- [x] Replace `su -c` with a mechanism such as `runuser --` that does not need a
|
|
shell. `runuser -u` sets the same `HOME` and a `PATH` that still finds the
|
|
`seaf-cli` wrapper, so the daemon environment is unchanged.
|
|
- [x] Treat server-provided library names as untrusted input.
|
|
- [x] Resolve every target directory and prove that it remains under
|
|
`/dsc/seafile` before creating or writing it. Symlinks are resolved first,
|
|
so an existing symlink in the libraries directory cannot redirect a write.
|
|
- [x] Define how duplicate, absolute, empty, and traversal-style library names
|
|
are handled. See `dsc/paths.py` and the "Some notes" section of the README.
|
|
|
|
Acceptance criteria:
|
|
|
|
- Tests cover quotes, command substitutions, separators, newlines, absolute
|
|
paths, parent traversal, Unicode, and colliding library names.
|
|
- No configuration or server value can execute a command or escape the data
|
|
directory.
|
|
|
|
### 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.
|
|
|
|
Acceptance criteria:
|
|
|
|
- A container can authenticate with a mounted file under `/run/secrets`.
|
|
- Tests reject conflicting inputs and verify newline handling.
|
|
- Logs never contain the password or token.
|
|
|
|
### Correct environment parsing and defaults
|
|
|
|
- [ ] Replace direct `os.getenv()` assignments in `start.py` with typed parsers.
|
|
- [ ] Preserve the documented defaults for upload limit, download limit, and
|
|
delete-confirm threshold when variables are absent.
|
|
- [ ] Validate UID, GID, limits, threshold, and booleans before daemon startup.
|
|
- [ ] Reject negative and malformed numeric values with useful messages.
|
|
- [ ] Do not pass `None` as a Seafile configuration value.
|
|
|
|
Acceptance criteria:
|
|
|
|
- Table-driven tests cover unset, zero, valid, negative, and malformed values.
|
|
- A default container receives exactly the defaults documented in the README.
|
|
|
|
### Make startup and shutdown deterministic
|
|
|
|
- [ ] Check every important subprocess return code.
|
|
- [ ] Add timeouts to daemon startup, shutdown, and readiness polling.
|
|
Confirmed: `start_daemon` and `stop_daemon` poll `while True` with no bound,
|
|
so a daemon that never changes state hangs the container silently.
|
|
- [ ] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
|
|
- [ ] Return a nonzero container exit code on configuration or daemon failure.
|
|
- [ ] Produce actionable errors instead of waiting forever.
|
|
|
|
Acceptance criteria:
|
|
|
|
- A daemon that never becomes ready causes a bounded failure.
|
|
- `docker stop` performs a graceful shutdown within Docker's timeout.
|
|
- Failure-injection tests cover command errors and interrupted startup.
|
|
|
|
### Add real tests
|
|
|
|
- [ ] Add unit tests for configuration parsing, command construction, library
|
|
resolution, path validation, retries, and status parsing. Command
|
|
construction and path validation are covered in `tests/`; configuration
|
|
parsing, retries, and status parsing are not.
|
|
- [ ] Add regression tests for externally reported password and deletion
|
|
threshold issues.
|
|
- [ ] Add a container smoke test for imports, installed CLI version, startup,
|
|
and shutdown using controlled fakes.
|
|
- [x] Keep image construction as a separate CI job so packaging failures are
|
|
distinguishable from application failures. The `unit-test` job does not
|
|
depend on the image build.
|
|
|
|
Acceptance criteria:
|
|
|
|
- CI fails on a deliberate parsing, quoting, or path-safety regression.
|
|
- Application tests run without requiring a live production server.
|
|
|
|
### Repair and validate the quick start
|
|
|
|
- [x] Change the Compose example to mapping syntax so quote characters are not
|
|
passed as part of `LIBRARY_ID`.
|
|
- [x] Pin the example to a versioned image instead of an implicit latest tag.
|
|
- [x] Add a checked-in `compose.example.yaml`.
|
|
- [x] Validate it with `docker compose config` in CI.
|
|
|
|
## P1 - Reliability and supply chain
|
|
|
|
### Networking and protocol behavior
|
|
|
|
- [ ] Add explicit connect and read timeouts to all HTTP requests.
|
|
- [ ] Define a bounded retry budget with backoff and jitter.
|
|
- [ ] Handle authentication, rate limits, malformed responses, and retryable
|
|
server errors separately.
|
|
- [ ] Make TLS configuration consistent between Python requests and Seafile.
|
|
- [ ] Support a custom CA bundle; keep certificate disabling as a last resort.
|
|
- [ ] Prefer structured JSON output from `seaf-cli` over parsing display text.
|
|
- [ ] Reject ambiguous library names and an empty requested library set.
|
|
|
|
### Runtime hardening
|
|
|
|
- [ ] Run the long-lived process as the configured non-root user after setup.
|
|
- [ ] Reject UID or GID zero unless a documented use case requires it.
|
|
- [ ] Add a health check for the daemon and RPC endpoint.
|
|
- [ ] Handle temporary RPC failures and daemon restarts without corrupting
|
|
state.
|
|
- [ ] Document supported CPU architectures instead of implying multi-platform
|
|
support.
|
|
|
|
### CI and release pipeline
|
|
|
|
- [x] Replace `actions/checkout@v2` and `docker/build-push-action@v1`.
|
|
- [x] Set workflow permissions explicitly to `contents: read` by default.
|
|
- [x] Use a scoped Docker Hub access token. The workflow now reads
|
|
`secrets.DOCKER_TOKEN`; the repository secret still has to be created from a
|
|
scoped Docker Hub token before the next tag is pushed.
|
|
- [x] Build a release image once and attach all version and `latest` tags to
|
|
the same manifest digest.
|
|
- [x] Add OCI labels via `docker/metadata-action`.
|
|
- [ ] Add an SBOM, provenance, and image signing.
|
|
- [x] Add dependency updates for Python, Docker, and GitHub Actions. A weekly
|
|
Dependabot configuration covers `requirements.txt`, the Dockerfile base
|
|
image, and the workflow actions. The Seafile AppImage is fetched by URL and
|
|
checksum, so it still has to be bumped by hand.
|
|
- [ ] Run dependency and image scans on pull requests and on a schedule.
|
|
|
|
Acceptance criteria:
|
|
|
|
- Every tag for one release resolves to the same manifest digest.
|
|
- Pull requests cannot access publication credentials.
|
|
- The current dependency update pull request passes all checks.
|
|
|
|
## P2 - Maintainability and presentation
|
|
|
|
### Simplify the implementation
|
|
|
|
- [ ] Separate configuration, subprocess execution, library resolution, and
|
|
status monitoring so each can be tested independently.
|
|
- [x] Remove unused imports and unnecessary dependencies. Both pinned
|
|
requirements are used; `urllib3` is imported directly but only pinned
|
|
transitively through `requests`.
|
|
- [ ] Add type annotations and static checks at external-input boundaries.
|
|
- [ ] Make logging configurable and avoid forced dependency debug logs.
|
|
- [ ] Add `.dockerignore` and reduce unnecessary image packages. `.dockerignore`
|
|
is in place; `binutils` and `squashfs-tools` are still shipped in the final
|
|
image because the AppImage is unpacked in the same stage.
|
|
|
|
### Establish a visible maintenance process
|
|
|
|
- [ ] Publish GitHub Releases for future tags with compatibility and rollback
|
|
notes.
|
|
- [ ] Add `CHANGELOG.md`, `SECURITY.md`, and concise contribution guidance.
|
|
- [ ] Add issue templates that request image, client, and server versions plus
|
|
sanitized logs.
|
|
- [ ] Triage stale issues and document the status of Docker secret support.
|
|
- [ ] Enable branch protection and require passing CI before merge.
|
|
|
|
### Improve the public project page
|
|
|
|
- [x] Link directly to the Docker Hub image near the top of `README.md`.
|
|
- [ ] Add CI, release, Docker pulls, and license badges after the checks are
|
|
trustworthy.
|
|
- [ ] Add a short architecture and process-lifecycle section.
|
|
- [ ] Add a tested compatibility matrix.
|
|
- [ ] Explain deletion behavior, volume ownership, state loss, backup
|
|
expectations, and recovery.
|
|
- [ ] Fix example inconsistencies and typographical errors.
|
|
|
|
## Release gate
|
|
|
|
Do not publish the next image until all of the following are true:
|
|
|
|
- [ ] A clean, reproducible build uses a supported Seafile client.
|
|
- [ ] External values cannot reach a shell or escape the data directory.
|
|
- [ ] Secret-file or token authentication is covered by tests.
|
|
- [ ] Startup, retries, and shutdown are bounded and observable.
|
|
- [ ] Unit and container smoke tests pass.
|
|
- [ ] The steady-state process is non-root and has a useful health check.
|
|
- [ ] Release tags point to one scanned and signed image digest.
|
|
- [ ] The README quick start succeeds from a clean environment.
|