diff --git a/weed/s3api/s3api_directory_marker.go b/weed/s3api/s3api_directory_marker.go index 08c5f55603..ec075da53c 100644 --- a/weed/s3api/s3api_directory_marker.go +++ b/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 diff --git a/weed/s3api/s3api_object_anonymous_read.go b/weed/s3api/s3api_object_anonymous_read.go index 1e16c73d71..ec55a65fd9 100644 --- a/weed/s3api/s3api_object_anonymous_read.go +++ b/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" { diff --git a/weed/s3api/s3api_object_anonymous_read_test.go b/weed/s3api/s3api_object_anonymous_read_test.go index 0efac46b93..c40f772a94 100644 --- a/weed/s3api/s3api_object_anonymous_read_test.go +++ b/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")) diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 18f7dc56b5..5580c344b1 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/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 { diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index abd5403bf3..8d120ed4d2 100644 --- a/weed/s3api/s3api_server.go +++ b/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)