Repository navigation
feat(sync)!: write lockfile v2 and replay locked archives - #569
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Core logic looks solid: lockfile v2 validation, replay, reuse, downloading and checking every archive before writing the lockfile, project-lock accounting (clean under -race), redirect header stripping, and path-traversal checks on registry-path. The tests are thorough. Four things I'd like fixed before merge:
1. config/install_package_reuse.go:101-109: a no-op sync now fails on installs the user can't write. CheckInstalledPackage always takes the driver and namespace locks, and fslock opens the lock files with O_CREATE|O_RDWR. Suppose an admin set up a conda or venv environment, or a system-level install, and a user without write access runs dbc sync on an unchanged lockfile. On #567 that exits 0. Here it fails with failed to check installed driver …: acquire driver install lock: fslock: open …: permission denied. Suggested fix: if a lock can't be opened with fs.ErrPermission/EROFS, either run the checks without the lock or take a shared lock on a read-only descriptor. Generations are published by rename, so a concurrent writer can only make the result "not reusable". Please add a test for a read-only install.
2. docs/guides/driver_list.md:162 and the PR description: "Older dbc versions cannot read v2" is only true for 0.3.1. v0.3.1 rejects the file with unsupported lock file version 2. v0.3.0 and earlier don't check the version at all. They read name/version, install those versions without any hash check, and then rewrite the file as version = 1 (s.locked.Version = lockFileVersion, v0.3.0 cmd/dbc/sync.go:353). That silently strips the v2 data from a committed dbc.lock. The docs should say so and tell teams to upgrade everyone who shares the lockfile first.
3. cmd/dbc/lockfile_v2.go:141, :425-430: a lockfile URL can send the registry token over plain HTTP. validateLockedURL accepts http, URL-type artifacts are fetched directly, and makereq picks credentials by host only (auth/credentials.go:272). So http://<credentialed-host>/… in dbc.lock gets Authorization: Bearer <token> in cleartext before the hash is checked. That is the downgrade checkRedirect refuses. dbc.toml already allows http registries, so this is defence in depth. Still, dbc.lock is a generated file that reviewers skim. Please reject http URL locations when the matched registry is https.
4. cmd/dbc/sync_plan.go:417, :484-489: the short-secret redaction fallback hides unrelated errors. If any query or userinfo value in a configured registry is shorter than 4 characters (for example ?v=1 or ?tenant=eu), nearly every prepare error matches it and becomes "registry request failed; details are hidden…". A hash mismatch and a TOML syntax error in dbc.lock both get replaced this way. Please redact only registry and download errors. URL stripping already removes userinfo and queries.
Nits:
cmd/dbc/sync_plan.go:296-310: if one of another platform's packages can't be recorded (malformed hash, a URL with?sig=, a duplicate target), fresh resolution fails on every platform. That contradicts "just the affected package is rejected" indrivers.go:328-330. Consider skipping those packages for non-current platforms.config/package_receipt.go:60-62: the TODO about needing source evidence before strict locked-artifact reuse is stale now that this PR implements that reuse.cmd/dbc/sync.go:341: warnings are written withfmt.Fprintln(os.Stderr, …)while the TUI is drawing, which may garble the output (not checked visually).tea.Printfwould avoid that.- Docs: a second concurrent sync waits only 10s for the project lock and then fails. "Serializes" suggests it waits until the first finishes. Also, two registries that differ only in query or userinfo now make sync fail as ambiguous, and that isn't documented.
CI: the ubuntu-latest failure is the sudo -E issue that #568 fixes.
b151f4b to
f020c13
Compare
f020c13 to
caafbd8
Compare
|
All items are addressed. The branch also merges #567's follow-up (7fe261b) and main.
Nits:
|
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
All four points are addressed, thanks. The read-only fallback re-checks the right things after the verifier (directories, receipt, library digest, signature bytes, registration), and contention and cancellation are still errors. The downgrade docs are accurate now, HTTP artifact URLs are rejected at validation, recording and replay, and hash and lockfile errors are no longer hidden. I re-ran go test ./... and -race on cmd/dbc and config, and the read-only tests run and pass as non-root.
One non-blocking nit inline, fine to do in a follow-up.
| pkg, err = drv.GetPackage(nil, p.target, spec.Prerelease == "allow") | ||
| } | ||
| if err != nil { | ||
| return plannedDriver{}, redactRegistryError(err, p.registries, sourceURL) |
There was a problem hiding this comment.
Nit (non-blocking): GetPackage/GetWithConstraint errors (here and at :209) and newLockedArtifact/lockArtifacts errors (:213, :272) are built locally, so URL stripping is enough; value-based redaction still hides them. With a registry like https://r.example/?v=1, both driver mysql: no package for version 1.4.0 on linux_amd64 and driver mysql: cannot record artifact location: invalid hash "sha256:01ab" become "registry request failed; details are hidden…". Keeping redactRegistryError for the index fetch (:71, :201) and downloads only would avoid that.
CheckInstalledPackage reports whether a runtime ID's current primary registration is a dbc-owned generation installed from the expected archive. Under the driver and registration namespace locks it re-reads the registration and requires the receipt, registration fingerprint, installed library bytes, archive digest, version and, when requested, signature evidence to agree. Any other state reports no match so the caller can reinstall; the check never creates or changes anything.
…identity Lockfile v2 records each driver's version, its registry source and one archive per target with a sha256 hash. The registry is identified by a canonical URL without userinfo, query or fragment, and artifacts are stored as registry-relative paths (or credential-free absolute URLs), so secrets in configured registry URLs never reach the lockfile. On replay, the identity is bound back to exactly one configured registry to rebuild the request URL. The reader decodes v2 strictly and fails closed on unknown fields, duplicate drivers or targets, malformed hashes and paths that could escape the registry; v1 files keep using the existing reader. The writer sorts entries and replaces the file atomically. Sync does not use v2 yet.
…ests InspectPackageArchive extracts an archive into a private temporary directory with the same safety checks as InstallPackage and reports the archive and driver library digests, so callers can verify an archive before changing any installation. CheckInstalledPackage can now also require the installed library to have a given digest, compared within the same locked check.
`dbc sync` now plans every driver before it changes anything: drivers locked in a v2 lockfile are replayed from their recorded archive URL and hash without contacting the registry index, a missing target is completed from the same registry at the same version, and new drivers record every target whose hash the registry publishes (the current target is measured when no hash is published). A v1 lockfile is migrated: its version stays pinned while it satisfies the constraint, and its library checksum is verified on the same platform or dropped with a warning otherwise. All needed archives are downloaded and verified first, then the new lock is written atomically, and only then are drivers installed. An installed driver is reused without downloading only when its receipt proves it came from the locked archive; a driver installed with --no-verify is reinstalled with signature verification by a normal sync. Archive and v1 checksum mismatches are reported as checksum_failed, and registry credentials are kept out of the lockfile, output and errors. Download file names no longer include the URL query. BREAKING CHANGE: `dbc sync` writes lockfile version 2, which older dbc versions reject.
Credentials are chosen per URL host, including the port, but Go's default redirect policy forwards the Authorization header to the same hostname on a different port. The default HTTP clients now remove the header whenever a redirect leaves the original host:port, so a registry credential is not sent to another server, for example when replaying an archive URL from a lockfile.
…esult The install worker removed its archive in a deferred call after sending the result, so when the last driver finished the program could exit first and leave the temporary directory behind. Remove it as soon as InstallPackage returns.
Skip tests that expect a reuse match where CheckInstalledPackage always reports none (js), compare registration locations by the files they resolve to (the check reads them from the symlink-resolved directory), and close archives after InspectPackageArchive so Windows can remove temp dirs.
Use structured targets and typed artifact locations, retain optional archive sizes, and accept evidence and host requirement metadata for future resolvers. Keep registry identities and credential rebinding unchanged. BREAKING CHANGE: Unreleased flat v2 lockfiles must be regenerated. Version 1 migration remains supported.
Check optional archive sizes only when downloading, and classify size mismatches as checksum_failed in JSON output.
CheckInstalledPackage reads the registration from its symlink-resolved location, so on macOS (/var -> /private/var) the read-only fallback test saw a different FilePath than GetDriver returned. Share the resolved-path comparison already used by the strict reuse test.
02075f0 to
5ce1789
Compare
Closes #400
Builds on the archive hash verification from #567. This PR adds lockfile v2 and archive-based reuse to
dbc sync.config.CheckInstalledPackageAPI checks installed registration, receipt, library, and archive digest before reuse, and re-verifies signatures unless--no-verifyis set. If a lock file cannot be opened because of permissions or a read-only filesystem, it checks an optimistic read-only snapshot; observed changes prevent reuse. Theconfig.InspectPackageArchiveAPI checks v1 library proofs. Otherwise sync downloads and checks required archives before saving the lock and installing. Installation failures retain the lock and prior successful installs so a later sync retries with the same archives. Concurrent syncs wait up to 10 seconds for the project lock, which remains held through installation.Normal sync reinstalls drivers previously installed with
--no-verifyusing signature verification; directdbc installretains its already-installed behavior. Default HTTP clients strip Authorization and Referer across origins and reject HTTPS-to-HTTP redirects. Download filenames omit URL queries.Network cancellation and local path sources remain deferred; quitting is immediate, and an in-flight download may leave a temporary file.