Fix/files list panics - #86
Open
ogarcia wants to merge 15 commits into
Open
Conversation
Refresh Cargo.lock to the latest semver-compatible releases and raise the pinned minimums for chrono, uuid, rustls and once_cell. The htpasswd snapshot is updated because toml 0.8.23 no longer emits a blank line between serialized tables.
http-range, inquire, pin-project, dircmp and once_cell are not referenced anywhere in the crate.
…jors - rand 0.8 -> 0.10: `distributions` is now `distr`, `thread_rng()` is `rng()` and the convenience methods moved from `Rng` to `RngExt` - strum 0.26 -> 0.28 - toml 0.8 -> 1.1 - base64 0.22 -> 0.23, rstest 0.23 -> 0.26, serial_test 3 -> 4
abscissa_core and abscissa_tokio 0.8 -> 0.9, no source changes required.
- axum 0.7 -> 0.8, axum-extra 0.9 -> 0.12, axum-macros 0.4 -> 0.5,
axum-auth 0.7 -> 0.8, axum-server 0.7 -> 0.8, axum-range 0.4 -> 1
- path captures use the new `{param}` syntax instead of `:param`
- `FromRequestParts` no longer needs `#[async_trait]`
axum-server 0.8 also drops the unmaintained rustls-pemfile dependency.
`bad_style` is a lint group, so it needs an explicit lower priority than the individual lints in the table; without it clippy refuses to run on current toolchains. Also resolve the warnings this uncovers: drop an unused import, remove a needless `return`, use `io::Error::other`, iterate over map values and inline format arguments.
htpasswd-verify has not been released since 2020 and pulls in rust-crypto,
rustc-serialize, time 0.1 and gcc, which accounted for all three RUSTSEC
vulnerabilities reported by `cargo audit`. htauth covers the same job with
maintained RustCrypto primitives and constant-time comparisons.
Behaviour changes:
- verification accepts bcrypt, SHA-256/SHA-512 crypt and APR1-MD5; the
broken `{SHA}` and `crypt(3)` formats are no longer accepted
- an unsupported or malformed hash is logged and rejected instead of
panicking, which the old bcrypt path could do
- `Credential::new` is fallible now, since hashing can fail
- htauth generates its own salt, so the rand dependency is gone
`cargo audit` now reports no vulnerabilities.
The updated dependencies require it: htauth needs 1.88 and abscissa 0.9, axum 0.8, clap 4.6 and uuid 1.24 all need 1.85. Verified with a 1.88.0 toolchain. Note that `cargo test` needs a newer toolchain than that, because serial_test 4 declares 1.93.1; building the crate itself does not.
The rsa crate is no longer in the dependency tree, so the ignore has no effect other than hiding a future regression.
cargo-deny checks the license of the crate under inspection too, so `check licenses` rejected rustic_server itself for being AGPL-3.0-or-later. No dependency license was involved.
Compiles clean on rustc 1.97.1 with no edition compatibility warnings left in our own code. Two changes were needed: - `check_auth_and_acl` and `check_name` returned `impl IntoResponse` while taking a reference argument. In edition 2024 the opaque type captures every lifetime in scope, so both now take a named type parameter and declare `use<T>` to capture nothing else. Neither actually borrows: they return `StatusCode` and `()`. - Three sites triggered the tail expression drop order lint from inside a `tracing::debug!` expansion. The temporaries only own memory, so the order was harmless, but binding them first removes the warning and drops a duplicated `repo()`/`join()` call in each case. The edition also implies resolver 3 and rustfmt style edition 2024.
Mechanical reordering of use statements: the 2024 style edition sorts uppercase-first and splits nested groups differently. No code changes.
`print_request_response` ran `body.collect()` on both the request and the
response, so every upload and download was held in memory in full before
being forwarded. Worse, `web.rs` installed the middleware at INFO, which is
the default level, while the middleware itself only emits `debug!` records:
by default the server buffered every body and then logged nothing.
Bodies here are pack, index and key files, so the buffer was also scanned
by `str::from_utf8` on every request just to discover it is not text.
Measured with restic pushing 6 cycles of 120 MB through one server process,
resident memory after each cycle:
before 101 -> 184 -> 203 -> 190 -> 152 -> 196 MB, 222 MB when idle
after 11 -> 13 -> 14 -> 13 -> 14 -> 13 MB, 14 MB when idle
The middleware now logs method, URI, status and headers and passes the body
through untouched, and it is only installed at DEBUG or TRACE. Uploads also
stream to disk end to end now, which halved the backup wall time in the
same test.
Also drop a `Box::leak` of the ACL file contents. `RepoAcl` owns its data,
so the `'static` borrow was never needed; it leaked the file on every
startup.
…ng response
`list_files` copied the request's `Authorization` header into its own
response. That served no purpose and sent the client's Basic credentials
back out, where any proxy or cache on the path could record them.
Reading the header also assumed it was always present. It is not: with
`--no-auth` the extractor accepts a request that carries no credentials, so
`headers.get(AUTHORIZATION).unwrap()` panicked. Because the release profile
sets `panic = "abort"`, that killed the whole server rather than failing the
one request, making a single unauthenticated GET enough to stop the service:
GET /{repo}/{tpe}/ without an Authorization header
-> thread panicked at src/handlers/files_list.rs:139
-> Aborted (core dumped)
`list_files` unwrapped both the file name and the metadata of every entry it walked, so either one failing killed the process, again because the release profile sets `panic = "abort"`. Both fail in practice: - `entry.metadata()` returns `NotFound` when the file is removed between the directory walk and the size lookup. The listing is serialized lazily, so the window stays open for the whole response. A `forget --prune` running while any client lists files is enough, with no malice involved. Verified by deleting 20000 files under 40 concurrent listings, which aborted the server before this change and does not now. - `file_name().to_str()` returns `None` for a name that is not valid UTF-8. Such a name cannot be written through the API, but it can reach the directory by other means, and it made every listing of that type fail. Both cases now drop the entry from the listing, the vanished one at debug level and the undecodable one at warn level. Neither can be a valid repository object anyway: they are named after a hex digest.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #85, this change resolves a series of errors that the server may throw due to fairly simple operations, such as making a GET request to
/{repo}/{tpe}/without the authorization header.