Compare commits

..
Author SHA1 Message Date
sneak 061ecc69dc Refuse credentials in a storage URL's endpoint or rclone remote name (closes #322)
check / check (push) Waiting to run
storage_url refused only the outer URL's userinfo and unknown query
parameters. Userinfo inside the s3 endpoint= value, and rclone's
remote,param=value parameters in the remote name, were accepted and
then printed by info, remote info and snapshot create. Both are now
refused without echoing the value: a host name never contains "@", and
an rclone remote name never contains a comma. The comma is checked
before url.Parse, because url.Parse refuses a name such as
":s3,secret_access_key=..." or "x,secret=a:b", and its error prints
the name from the colon on. The rclone case has its own error pointing
at rclone config, since the existing credentials error names the s3.*
keys.

Model: opus-5-5
2026-10-13 13:41:26 +00:00
6 changed files with 117 additions and 317 deletions
+31 -5
View File
@@ -30,6 +30,9 @@ var (
"storage URL must not carry credentials; " +
"set s3.access_key_id and s3.secret_access_key in the config instead")
ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
ErrRcloneRemoteParams = errors.New(
"rclone URL must not carry parameters in the remote name; " +
"set them on the remote with rclone config instead")
)
// URL represents a parsed storage URL.
@@ -90,8 +93,9 @@ func parseFileURL(rawPath string) (*URL, error) {
}
// parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
// the userinfo and any query parameter other than endpoint, region and
// ssl, so a credential-bearing URL is never stored or echoed.
// the userinfo or in the endpoint parameter's userinfo, and any query
// parameter other than endpoint, region and ssl, so a credential-bearing
// URL is never stored or echoed.
func parseS3URL(rawURL string) (*URL, error) {
u, err := url.Parse(rawURL)
if err != nil {
@@ -114,6 +118,14 @@ func parseS3URL(rawURL string) (*URL, error) {
return nil, err
}
// A host name never contains "@", so one in an endpoint, with or
// without a scheme, can only introduce userinfo.
for _, endpoint := range query["endpoint"] {
if strings.Contains(endpoint, "@") {
return nil, ErrURLCredentials
}
}
return &URL{
Scheme: schemeS3,
Bucket: bucket,
@@ -125,9 +137,23 @@ func parseS3URL(rawURL string) (*URL, error) {
}
// parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
// query parameters, so credentials in the userinfo and any parameter at
// all are rejected rather than silently ignored.
// query parameters, so credentials in the userinfo, parameters in the
// remote name and any query parameter at all are rejected rather than
// silently ignored.
func parseRcloneURL(rawURL string) (*URL, error) {
// The remote name runs up to the first "/", "?" or "#". It never
// contains a comma; one starts rclone's remote,param=value form,
// whose values can be credentials. This is checked before url.Parse,
// whose error for such a name can print part of it.
remote := strings.TrimPrefix(rawURL, "rclone://")
if end := strings.IndexAny(remote, "/?#"); end >= 0 {
remote = remote[:end]
}
if strings.Contains(remote, ",") {
return nil, ErrRcloneRemoteParams
}
u, err := url.Parse(rawURL)
if err != nil {
return nil, wrapParseError(err)
@@ -137,7 +163,7 @@ func parseRcloneURL(rawURL string) (*URL, error) {
return nil, ErrURLCredentials
}
remote := u.Host
remote = u.Host
if remote == "" {
return nil, ErrMissingRemote
}
+61
View File
@@ -150,6 +150,19 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
wantErr: storage.ErrURLCredentials,
secrets: []string{key, secret, "mybucket"},
},
{
name: "s3 userinfo in endpoint",
raw: "s3://mybucket?endpoint=https://" +
key + ":" + secret + "@minio.example.com",
wantErr: storage.ErrURLCredentials,
secrets: []string{key, secret},
},
{
name: "s3 userinfo in endpoint without scheme",
raw: "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
wantErr: storage.ErrURLCredentials,
secrets: []string{key, secret},
},
{
name: "s3 unknown query param",
raw: "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -203,6 +216,54 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
}
}
// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
// remote name carrying parameters (rclone's "remote,param=value" form,
// which can hold a credential) is rejected without echoing the value,
// including the URLs that url.Parse itself refuses.
func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
t.Parallel()
cases := []struct {
name string
raw string
secrets []string // must not appear in the error message
}{
{
name: "configured remote",
raw: "rclone://gdrive,client_secret=topsecret/backups",
secrets: []string{"topsecret"},
},
{
name: "remote defined inline",
raw: "rclone://:s3,access_key_id=KEY,secret_access_key=SECRET/bucket",
secrets: []string{"KEY", "SECRET"},
},
{
name: "colon in a parameter value",
raw: "rclone://gdrive,client_secret=ab:cd/backups",
secrets: []string{"cd"},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
_, err := storage.ParseStorageURL(tc.raw)
if !errors.Is(err, storage.ErrRcloneRemoteParams) {
t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
tc.raw, err, storage.ErrRcloneRemoteParams)
}
for _, secret := range tc.secrets {
if strings.Contains(err.Error(), secret) {
t.Errorf("error message leaked %q: %v", secret, err.Error())
}
}
})
}
}
// TestParseStorageURLParseFailureHidesURL checks that when url.Parse
// itself fails, the wrapped error carries only the inner cause, not the
// *url.Error whose text embeds the raw (possibly credential-bearing) URL.
@@ -161,14 +161,13 @@ func TestVerifyManifestBlobsRejectsShortHash(t *testing.T) {
Blobs: []snapshot.BlobInfo{{Hash: "abc", CompressedSize: 1}},
}
verified, missing, mismatched, missingSize, unchecked, err :=
verified, missing, mismatched, missingSize, err :=
v.verifyManifestBlobs(manifest, &VerifyOptions{JSON: true})
require.ErrorIs(t, err, errInvalidBlobHash)
require.Zero(t, verified)
require.Zero(t, missing)
require.Zero(t, mismatched)
require.Zero(t, missingSize)
require.Empty(t, unchecked)
}
// TestVerifyBlobChunksRejectsNegativeLength proves a blob_chunks row with a
-223
View File
@@ -4,20 +4,15 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"os"
"path"
"path/filepath"
"slices"
"testing"
"github.com/spf13/afero"
"github.com/stretchr/testify/require"
"sneak.berlin/go/vaultik/internal/log"
"sneak.berlin/go/vaultik/internal/snapshot"
"sneak.berlin/go/vaultik/internal/storage"
"sneak.berlin/go/vaultik/internal/ui"
"sneak.berlin/go/vaultik/internal/vaultik"
)
@@ -156,224 +151,6 @@ func TestShallowVerifyDetectsMissingDatabase(t *testing.T) {
"shallow verify must fail when db.zst.age is absent")
}
// errStatDenied stands in for any storage error other than not-found: a
// permission error, an S3 403, a dropped connection.
var errStatDenied = errors.New("injected stat failure")
// statFailingStorer fails Stat with errStatDenied for the keys in
// failKeys and passes every other call to the wrapped Storer.
type statFailingStorer struct {
storage.Storer
failKeys []string
}
func (s statFailingStorer) Stat(
ctx context.Context, key string,
) (*storage.ObjectInfo, error) {
if slices.Contains(s.failKeys, key) {
return nil, fmt.Errorf("stat %q: %w", key, errStatDenied)
}
return s.Storer.Stat(ctx, key)
}
// TestShallowVerifyReportsStatErrorsApartFromMissing backs up a real
// snapshot, deletes one blob, and makes storage fail Stat with an error
// other than not-found for a second blob and for the encrypted database.
// Only the deleted blob may be counted as missing. The second blob and
// the database must be reported as could not be checked, with the error
// storage returned, and verify must fail, in the text output and under
// --json.
func TestShallowVerifyReportsStatErrorsApartFromMissing(t *testing.T) {
log.Initialize(log.Config{})
t.Parallel()
fs := afero.NewOsFs()
tempDir := t.TempDir()
dataDir := filepath.Join(tempDir, "source")
storeDir := filepath.Join(tempDir, "remote")
dbPath := filepath.Join(tempDir, "index.sqlite")
chunkSize := int64(32 * 1024)
maxBlobSize := int64(128 * 1024)
// Random content, so the snapshot spans several blobs.
require.NoError(t, fs.MkdirAll(dataDir, 0o755))
appendRandomBytes(t, fs, map[string][]byte{},
filepath.Join(dataDir, "data.bin"), maxBlobSize*4)
ctx := context.Background()
cfg, storer, snapshotID := runFileStorageBackup(
ctx, t, fs, dataDir, storeDir, dbPath, chunkSize, maxBlobSize)
blobKeys := storedBlobKeys(t, storeDir)
require.GreaterOrEqual(t, len(blobKeys), 2, "expected at least two blobs")
missingKey, uncheckedKey := blobKeys[0], blobKeys[1]
missingHash, uncheckedHash := path.Base(missingKey), path.Base(uncheckedKey)
require.NoError(t, os.Remove(filepath.Join(storeDir, missingKey)))
dbKey := "metadata/" + snapshot.RemoteSnapshotKey(snapshotID) + "/db.zst.age"
failing := statFailingStorer{
Storer: storer,
failKeys: []string{uncheckedKey, dbKey},
}
newVerifier := func(out io.Writer) *vaultik.Vaultik {
v := &vaultik.Vaultik{
Config: cfg,
Storage: failing,
Fs: fs,
Stdout: out,
Stderr: io.Discard,
UI: ui.NewWithColor(io.Discard, false),
}
v.SetContext(ctx)
return v
}
var out bytes.Buffer
err := newVerifier(&out).VerifySnapshotWithOptions(
snapshotID, &vaultik.VerifyOptions{JSON: true})
require.Error(t, err)
result := decodeVerifyResult(t, out.Bytes())
require.Equal(t, "failed", result.Status)
require.Equal(t, 1, result.Missing, "only the deleted blob is missing")
require.Equal(t, result.BlobCount-2, result.Verified)
require.Len(t, result.Unchecked, 1)
require.Equal(t, uncheckedHash, result.Unchecked[0].Hash)
require.Contains(t, result.Unchecked[0].Error, errStatDenied.Error())
require.False(t, result.DatabaseMissing,
"a database that could not be checked is not missing")
require.Contains(t, result.DatabaseCheckError, errStatDenied.Error())
out.Reset()
err = newVerifier(&out).VerifySnapshotWithOptions(
snapshotID, &vaultik.VerifyOptions{})
require.Error(t, err)
text := out.String()
require.Contains(t, text, "Missing: "+missingHash)
require.NotContains(t, text, "Missing: "+uncheckedHash)
require.Contains(t, text, "Could not check: "+uncheckedHash)
require.Contains(t, text, "Encrypted database: could not be checked")
require.Contains(t, text, errStatDenied.Error())
require.Contains(t, text, "FAILED")
}
// TestShallowVerifyQuietFailsOnStatErrorAlone backs up a healthy snapshot,
// then makes storage fail Stat, with an error other than not-found, for
// one blob alone and then for the encrypted database alone. Each run must
// fail verify, and under --quiet, which drops the report, the error
// storage returned must still reach stderr.
//
//nolint:paralleltest // captureProcessStderr replaces os.Stderr
func TestShallowVerifyQuietFailsOnStatErrorAlone(t *testing.T) {
log.Initialize(log.Config{})
fs := afero.NewOsFs()
tempDir := t.TempDir()
dataDir := filepath.Join(tempDir, "source")
storeDir := filepath.Join(tempDir, "remote")
dbPath := filepath.Join(tempDir, "index.sqlite")
chunkSize := int64(32 * 1024)
maxBlobSize := int64(128 * 1024)
require.NoError(t, fs.MkdirAll(dataDir, 0o755))
require.NoError(t, afero.WriteFile(fs,
filepath.Join(dataDir, "data.bin"),
bytesPattern("shallow-quiet-", int(maxBlobSize*2)), 0o644))
ctx := context.Background()
cfg, storer, snapshotID := runFileStorageBackup(
ctx, t, fs, dataDir, storeDir, dbPath, chunkSize, maxBlobSize)
newQuietVerifier := func(s storage.Storer) *vaultik.Vaultik {
v := &vaultik.Vaultik{
Config: cfg,
Storage: s,
Fs: fs,
Stdout: io.Discard,
Stderr: io.Discard,
UI: ui.NewWithColor(io.Discard, false),
}
v.SetContext(ctx)
v.UI.SetQuiet(true)
return v
}
require.NoError(t,
newQuietVerifier(storer).VerifySnapshotWithOptions(
snapshotID, &vaultik.VerifyOptions{}),
"shallow verify should pass on a healthy snapshot")
blobKeys := storedBlobKeys(t, storeDir)
require.NotEmpty(t, blobKeys)
dbKey := "metadata/" + snapshot.RemoteSnapshotKey(snapshotID) + "/db.zst.age"
for _, failKey := range []string{blobKeys[0], dbKey} {
v := newQuietVerifier(statFailingStorer{
Storer: storer,
failKeys: []string{failKey},
})
var err error
stderr := captureProcessStderr(t, func() {
err = v.VerifySnapshotWithOptions(
snapshotID, &vaultik.VerifyOptions{})
})
require.Error(t, err,
"verify must fail when %s could not be checked", failKey)
require.Contains(t, stderr, errStatDenied.Error(),
"under --quiet the error for %s must still reach stderr", failKey)
}
}
// storedBlobKeys returns the storage key of every blob file under
// storeDir, sorted.
func storedBlobKeys(t *testing.T, storeDir string) []string {
t.Helper()
var keys []string
err := filepath.Walk(filepath.Join(storeDir, "blobs"),
func(p string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
rel, err := filepath.Rel(storeDir, p)
if err != nil {
return err
}
keys = append(keys, filepath.ToSlash(rel))
return nil
})
require.NoError(t, err)
return keys
}
// growOneBlob appends bytes to the first blob file found under blobsDir,
// changing its on-disk size so it no longer matches the manifest.
func growOneBlob(t *testing.T, fs afero.Fs, blobsDir string) {
+21 -70
View File
@@ -14,7 +14,6 @@ import (
"sneak.berlin/go/vaultik/internal/log"
"sneak.berlin/go/vaultik/internal/snapshot"
"sneak.berlin/go/vaultik/internal/storage"
"sneak.berlin/go/vaultik/internal/types"
)
@@ -775,10 +774,18 @@ func (v *Vaultik) VerifySnapshotWithOptions(
v.stdoutf("Checking blob presence and sizes...\n")
}
v.checkSnapshotDatabase(remoteKey, result)
// A snapshot is only restorable if its encrypted database is present
// alongside the blobs. Shallow verify checks that the object exists; it
// does not decrypt it (that is deep verify's job).
dbPath := fmt.Sprintf("metadata/%s/db.zst.age", remoteKey)
result.Verified, result.Missing, result.Mismatched, result.MissingSize,
result.Unchecked, err = v.verifyManifestBlobs(manifest, opts)
_, dbErr := v.Storage.Stat(v.ctx, dbPath)
if dbErr != nil {
result.DatabaseMissing = true
}
result.Verified, result.Missing, result.Mismatched, result.MissingSize, err =
v.verifyManifestBlobs(manifest, opts)
if err != nil {
if opts.JSON {
result.Status = verifyStatusFailed
@@ -815,46 +822,25 @@ func (v *Vaultik) printVerifyHeader(snapshotID string, opts *VerifyOptions) {
}
}
// checkSnapshotDatabase records in result whether the snapshot's
// encrypted database is missing or could not be checked. A snapshot is
// only restorable if its database is present alongside the blobs.
// Shallow verify checks that the object exists; it does not decrypt it
// (that is deep verify's job).
func (v *Vaultik) checkSnapshotDatabase(remoteKey string, result *VerifyResult) {
dbPath := fmt.Sprintf("metadata/%s/db.zst.age", remoteKey)
_, err := v.Storage.Stat(v.ctx, dbPath)
switch {
case errors.Is(err, storage.ErrNotFound):
result.DatabaseMissing = true
case err != nil:
result.DatabaseCheckError = err.Error()
}
}
// verifyManifestBlobs checks that each blob in the manifest is present in
// storage with the size the manifest records, returning the counts of
// blobs that were present with the right size, absent, and present but the
// wrong size, the total bytes of the absent blobs, and the blobs it could
// not check because storage returned an error other than not-found. It
// does not read blob contents; deep verification (RunDeepVerify) does
// that. The size comparison matches the deep path (see
// verifyBlobExistenceFromDB).
// wrong size, plus the total bytes of the absent blobs. It does not read
// blob contents; deep verification (RunDeepVerify) does that. The size
// comparison matches the deep path (see verifyBlobExistenceFromDB).
func (v *Vaultik) verifyManifestBlobs(
manifest *snapshot.Manifest, opts *VerifyOptions,
) (int, int, int, int64, []UncheckedBlob, error) {
) (int, int, int, int64, error) {
var (
verified, missing, mismatched int
missingSize int64
unchecked []UncheckedBlob
)
for _, blob := range manifest.Blobs {
// The manifest is unauthenticated, so its blob hashes are checked
// before being spliced into a storage path.
if !isBlobHash(blob.Hash) {
return 0, 0, 0, 0, nil, fmt.Errorf("%w: %s",
return 0, 0, 0, 0, fmt.Errorf("%w: %s",
errInvalidBlobHash, shortHash(blob.Hash))
}
@@ -863,7 +849,7 @@ func (v *Vaultik) verifyManifestBlobs(
stat, err := v.Storage.Stat(v.ctx, blobPath)
switch {
case errors.Is(err, storage.ErrNotFound):
case err != nil:
if !opts.JSON && !v.UI.Quiet() {
v.stdoutf(" Missing: %s (%s)\n",
blob.Hash, ubytes(blob.CompressedSize))
@@ -871,13 +857,6 @@ func (v *Vaultik) verifyManifestBlobs(
missing++
missingSize += blob.CompressedSize
case err != nil:
if !opts.JSON && !v.UI.Quiet() {
v.stdoutf(" Could not check: %s (%v)\n", blob.Hash, err)
}
unchecked = append(unchecked,
UncheckedBlob{Hash: blob.Hash, Error: err.Error()})
case stat.Size != blob.CompressedSize:
if !opts.JSON && !v.UI.Quiet() {
v.stdoutf(" Wrong size: %s (store has %s, manifest lists %s)\n",
@@ -890,7 +869,7 @@ func (v *Vaultik) verifyManifestBlobs(
}
}
return verified, missing, mismatched, missingSize, unchecked, nil
return verified, missing, mismatched, missingSize, nil
}
// formatVerifyResult outputs the final verification results as JSON or
@@ -911,19 +890,9 @@ func (v *Vaultik) formatVerifyResult(
return v.outputVerifyJSON(result)
}
// --quiet drops the report but not the errors storage returned, which
// are logged on stderr. A failure is still returned either way, and
// the cli layer prints it on stderr.
if v.UI.Quiet() {
for _, blob := range result.Unchecked {
log.Warn("Could not check blob", "blob", blob.Hash, "error", blob.Error)
}
if result.DatabaseCheckError != "" {
log.Warn("Could not check the encrypted database",
"snapshot", result.SnapshotID, "error", result.DatabaseCheckError)
}
} else {
// Under --quiet a failure is still returned, and the cli layer
// prints it on stderr.
if !v.UI.Quiet() {
v.printVerifySummary(result, failure)
}
@@ -949,19 +918,10 @@ func (v *Vaultik) printVerifySummary(result *VerifyResult, failure string) {
v.stdoutf(" Wrong size: %d blobs\n", result.Mismatched)
}
if len(result.Unchecked) > 0 {
v.stdoutf(" Could not check: %d blobs\n", len(result.Unchecked))
}
if result.DatabaseMissing {
v.stdoutf(" Encrypted database: missing\n")
}
if result.DatabaseCheckError != "" {
v.stdoutf(" Encrypted database: could not be checked (%s)\n",
result.DatabaseCheckError)
}
v.stdoutf(" Status: ")
if failure != "" {
@@ -989,19 +949,10 @@ func shallowVerifyFailure(result *VerifyResult) string {
fmt.Sprintf("%d blobs have the wrong size", result.Mismatched))
}
if len(result.Unchecked) > 0 {
parts = append(parts, fmt.Sprintf("%d blobs could not be checked",
len(result.Unchecked)))
}
if result.DatabaseMissing {
parts = append(parts, "the encrypted database is missing")
}
if result.DatabaseCheckError != "" {
parts = append(parts, "the encrypted database could not be checked")
}
return strings.Join(parts, "; ")
}
-14
View File
@@ -57,27 +57,13 @@ type VerifyResult struct {
Missing int `json:"missing"`
MissingSize int64 `json:"missing_size,omitempty"`
Mismatched int `json:"mismatched,omitempty"`
// Unchecked lists the blobs shallow verify could not check, because
// storage answered with an error other than not-found, so whether
// they are present is unknown.
Unchecked []UncheckedBlob `json:"unchecked,omitempty"`
// DatabaseMissing is set by shallow verify when the snapshot's
// encrypted database (metadata/<key>/db.zst.age) is absent, which
// makes the snapshot unrestorable regardless of the blobs.
DatabaseMissing bool `json:"database_missing,omitempty"`
// DatabaseCheckError is set by shallow verify to the error storage
// returned when the encrypted database could not be checked.
DatabaseCheckError string `json:"database_check_error,omitempty"`
ErrorMessage string `json:"error,omitempty"`
}
// UncheckedBlob is a blob shallow verify could not check, with the error
// storage returned for it.
type UncheckedBlob struct {
Hash string `json:"hash"`
Error string `json:"error"`
}
// deepVerifyFailure records a failure in the result and returns it appropriately
func (v *Vaultik) deepVerifyFailure(
result *VerifyResult, opts *VerifyOptions, msg string, err error,