s3api: deny signed reads of a version file through its physical path (#12574)

* s3api: deny signed reads that name a version file by its physical path

A stored version lives at <key>.versions/v_<id>, and that path is also a
valid object key. Anonymous reads already refuse it; signed GET, HEAD,
copy sources and object subresources did not, so a caller without
s3:GetObjectVersion, or under a Deny on the logical key, could read any
version through it. Apply the same check in the shared entry recheck.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3api: recheck a version file key before answering a conditional read

A conditional GET or HEAD answered 304 or 412 with the ETag before the
handler's entry recheck, so the physical version path still confirmed a
version's ETag to signed callers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3api: look up version file keys in the lookup-based policy recheck

Retention, legal hold, tagging writes and deletes only fetched the entry
for the recheck when the bucket had a policy, so without one they still
acted on a version file named by its physical path. Fetch it whenever
the key has the version file shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3api: refuse a directory-marker delete that names a version file

A trailing slash kept "<key>.versions/v_<id>/" out of the version file
shape check, and the lookup recheck misses the entry for that form, so a
DELETE reached deleteDirectoryMarker and removed a chunkless version or
delete marker without s3:DeleteObjectVersion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

weed/s3api/s3api_directory_marker.go

@@ -45,6 +45,8 @@ func (s3a *S3ApiServer) deleteDirectoryMarker(r *http.Request, bucket, object st
 		// A promoted file, not a marker: "dir/" does not name its data.
 		glog.V(2).Infof("deleteDirectoryMarker: %s/%s holds uploaded data, leaving it alone", bucket, object)
 		return s3err.ErrNone
+	case s3a.isVersionFileKey(trimmedObject, entry.Extended):
+		return s3err.ErrAccessDenied
 	}
 
 	// The key is deleted the unversioned way, but Object Lock still covers it: the

weed/s3api/s3api_object_anonymous_read.go

@@ -52,16 +52,25 @@ func (s3a *S3ApiServer) deferAnonymousObjectRead(r *http.Request, action Action,
 	return r.WithContext(context.WithValue(ctx, anonymousObjectReadContextKey{}, true)), true
 }
 
+// isVersionFileKey reports whether object names a stored version's physical
+// filer path rather than a logical key: accepting it would bypass
+// GetObjectVersion policies and key-scoped denies on the logical key. Ordinary
+// user keys containing ".versions" without version metadata are unaffected.
+func (s3a *S3ApiServer) isVersionFileKey(object string, extended map[string][]byte) bool {
+	version := string(extended[s3_constants.ExtVersionIdKey])
+	return version != "" && s3a.hasVersionFileShape(object) && path.Base(s3_constants.NormalizeObjectKey(object)) == s3a.getVersionFileName(version)
+}
+
+func (s3a *S3ApiServer) hasVersionFileShape(object string) bool {
+	key := strings.TrimSuffix(s3_constants.NormalizeObjectKey(object), "/")
+	return strings.HasSuffix(path.Dir(key), s3_constants.VersionsFolder) && strings.HasPrefix(path.Base(key), s3a.getVersionFileName(""))
+}
+
 // authorizeAnonymousObjectRead evaluates policies and ACLs against the same
 // entry used for the response, including conditional and versioned reads.
 // Explicit policy denies take precedence over every public-access grant.
 func (s3a *S3ApiServer) authorizeAnonymousObjectRead(r *http.Request, bucket, object string, extended map[string][]byte) s3err.ErrorCode {
-	// A version's physical filer path is not another S3 key: accepting it would
-	// bypass GetObjectVersion policies on the logical key. Ordinary user keys
-	// containing ".versions" without internal version metadata are unaffected.
-	key := s3_constants.NormalizeObjectKey(object)
-	version := string(extended[s3_constants.ExtVersionIdKey])
-	if version != "" && strings.HasSuffix(path.Dir(key), s3_constants.VersionsFolder) && path.Base(key) == s3a.getVersionFileName(version) {
+	if s3a.isVersionFileKey(object, extended) {
 		return s3err.ErrAccessDenied
 	}
 	if string(extended[s3_constants.ExtDeleteMarkerKey]) == "true" {

weed/s3api/s3api_object_anonymous_read_test.go

@@ -223,6 +223,51 @@ func TestAnonymousObjectACLVersionAndDirectory(t *testing.T) {
 	require.Equal(t, 403, rr.Code)
 }
 
+func TestSignedReadOfVersionFileKey(t *testing.T) {
+	s3a, f := newAnonymousReadTestServer(t)
+	config, _ := s3a.getBucketConfig("b")
+	config.Versioning = "Enabled"
+	version := anonymousReadEntry(nil)
+	version.Name = "v_v1"
+	version.Extended[s3_constants.ExtVersionIdKey] = []byte("v1")
+	f.entries["/buckets/b/o.versions/v_v1"] = version
+	f.entries["/buckets/b/u.versions/v_literal"] = anonymousReadEntry(nil)
+	identity := &Identity{Name: "reader", Actions: []Action{s3_constants.ACTION_READ}}
+	serve := func(method, object, query string, headers map[string]string) int {
+		r := httptest.NewRequest(method, "/b/"+object+"?"+query, nil)
+		r = mux.SetURLVars(r, map[string]string{"bucket": "b", "object": object})
+		r = r.WithContext(s3_constants.SetIdentityInContext(r.Context(), identity))
+		for name, value := range headers {
+			r.Header.Set(name, value)
+		}
+		handler := s3a.GetObjectHandler
+		if method == http.MethodHead {
+			handler = s3a.HeadObjectHandler
+		}
+		rr := httptest.NewRecorder()
+		handler(rr, r)
+		return rr.Code
+	}
+	for _, method := range []string{http.MethodGet, http.MethodHead} {
+		require.Equal(t, 200, serve(method, "o", "versionId=v1", nil))
+		require.Equal(t, 403, serve(method, "o.versions/v_v1", "", nil))
+		require.NotEqual(t, 200, serve(method, "o.versions/v_v1/", "", nil))
+		require.Equal(t, 403, serve(method, "o.versions/v_v1", "", map[string]string{"If-None-Match": "*"}))
+		require.Equal(t, 200, serve(method, "u.versions/v_literal", "", nil))
+	}
+	r := httptest.NewRequest(http.MethodPut, "/b/dst", nil)
+	r = r.WithContext(s3_constants.SetIdentityInContext(r.Context(), identity))
+	require.Equal(t, s3err.ErrAccessDenied, s3a.recheckCopySourcePolicy(r, "b", "o.versions/v_v1", "", version.Extended, "CopyObjectHandler"))
+	require.Equal(t, s3err.ErrNone, s3a.recheckCopySourcePolicy(r, "b", "o", "v1", version.Extended, "CopyObjectHandler"))
+	for _, action := range []string{s3_constants.ACTION_GET_OBJECT_RETENTION, s3_constants.ACTION_PUT_OBJECT_LEGAL_HOLD, string(s3_constants.ACTION_WRITE)} {
+		require.Equal(t, s3err.ErrAccessDenied, s3a.recheckTagConditionedPolicyWithLookup(r, "b", "o.versions/v_v1", "", action, "t"))
+		require.Equal(t, s3err.ErrNone, s3a.recheckTagConditionedPolicyWithLookup(r, "b", "o", "v1", action, "t"))
+		require.Equal(t, s3err.ErrNone, s3a.recheckTagConditionedPolicyWithLookup(r, "b", "u.versions/v_literal", "", action, "t"))
+	}
+	require.Equal(t, s3err.ErrAccessDenied, s3a.deleteDirectoryMarker(r, "b", "o.versions/v_v1/"))
+	require.Contains(t, f.entries, "/buckets/b/o.versions/v_v1")
+}
+
 func TestAnonymousObjectACLDoesNotReplaceAuthentication(t *testing.T) {
 	s3a, f := newAnonymousReadTestServer(t)
 	f.entries["/buckets/b/o"] = anonymousReadEntry(anonymousReadGrant("READ"))

weed/s3api/s3api_object_handlers.go

@@ -789,7 +789,7 @@ func (s3a *S3ApiServer) processConditionalHeaders(w http.ResponseWriter, r *http
 	}
 
 	result := s3a.checkConditionalHeadersForReads(r, bucket, object)
-	if isAnonymousObjectRead(r) && result.Entry != nil {
+	if result.Entry != nil && (isAnonymousObjectRead(r) || s3a.isVersionFileKey(object, result.Entry.Extended)) {
 		// 304/412 are responses too: authorize before exposing the ETag or the
 		// outcome of a condition on an entry the caller cannot read.
 		if code := s3a.recheckPolicyWithObjectEntry(r, bucket, object, string(s3_constants.ACTION_READ), result.Entry.Extended, handlerName); code != s3err.ErrNone {

weed/s3api/s3api_server.go

@@ -688,6 +688,9 @@ func (s3a *S3ApiServer) recheckPolicyWithObjectEntry(r *http.Request, bucket, ob
 		identity, _ := GetIdentityFromContext(r).(*Identity)
 		return refineDenialForBucketName(r, identity, s3a.authorizeAnonymousObjectRead(r, bucket, object, objectEntry))
 	}
+	if s3a.isVersionFileKey(object, objectEntry) {
+		return s3err.ErrAccessDenied
+	}
 	identityRaw := GetIdentityFromContext(r)
 	var identity *Identity
 	if identityRaw != nil {
@@ -731,10 +734,10 @@ func (s3a *S3ApiServer) recheckCopySourcePolicy(r *http.Request, bucket, object,
 // recheckTagConditionedPolicyWithLookup repeats policy evaluation with the
 // object's entry in hand so s3:ExistingObjectTag conditions apply to
 // operations that did not otherwise fetch the entry. It only does the extra
-// lookup when the bucket has a policy; entry misses return ErrNone because
-// the handler reports its own NoSuchKey.
+// lookup when the bucket has a policy or the key may name a version file;
+// entry misses return ErrNone because the handler reports its own NoSuchKey.
 func (s3a *S3ApiServer) recheckTagConditionedPolicyWithLookup(r *http.Request, bucket, object, versionId, action, handlerName string) s3err.ErrorCode {
-	if s3a.policyEngine == nil || !s3a.policyEngine.HasPolicyForBucket(bucket) {
+	if (s3a.policyEngine == nil || !s3a.policyEngine.HasPolicyForBucket(bucket)) && !s3a.hasVersionFileShape(object) {
 		return s3err.ErrNone
 	}
 	entry, err := s3a.resolveObjectEntry(bucket, object, versionId)