Fix FLS for frozen tier (#82521) (#82565)

Field level security was interacting in bad ways with the can-match phase on frozen tier shards (interaction between FieldSubsetReader and RewriteCachingDirectoryReader). This made can-match phase fail, which in the normal case would result in extra load on the frozen tier, and in the extreme case (in interaction with #51708) made searches fail. This is a bug that was indirectly introduced by #78988. Closes #82044
elastic · Jan 14, 2022 · 2fbd812 · 2fbd812
1 parent dae07f3
commit 2fbd812
Show file tree

Hide file tree

Showing 2 changed files with 127 additions and 28 deletions.
diff --git a/...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/FieldSubsetReader.java b/...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/FieldSubsetReader.java
@@ -38,6 +38,7 @@
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
 import org.elasticsearch.index.mapper.SourceFieldMapper;
+import org.elasticsearch.transport.Transports;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentType;
 
@@ -48,6 +49,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 /**
  * A {@link FilterLeafReader} that exposes only a subset
@@ -122,7 +124,7 @@ public CacheHelper getReaderCacheHelper() {
     /** An automaton that only accepts authorized fields. */
     private final CharacterRunAutomaton filter;
     /** {@link Terms} cache with filtered stats for the {@link FieldNamesFieldMapper} field. */
-    private final Terms fieldNamesFilterTerms;
+    private volatile Optional<Terms> fieldNamesFilterTerms;
 
     /**
      * Wrap a single segment, exposing a subset of its fields.
@@ -137,8 +139,6 @@ public CacheHelper getReaderCacheHelper() {
         }
         fieldInfos = new FieldInfos(filteredInfos.toArray(new FieldInfo[filteredInfos.size()]));
         this.filter = filter;
-        final Terms fieldNameTerms = super.terms(FieldNamesFieldMapper.NAME);
-        this.fieldNamesFilterTerms = fieldNameTerms == null ? null : new FieldNamesTerms(fieldNameTerms);
     }
 
     /** returns true if this field is allowed. */
@@ -429,10 +429,22 @@ private Terms wrapTerms(Terms terms, String field) throws IOException {
         if (hasField(field) == false) {
             return null;
         } else if (FieldNamesFieldMapper.NAME.equals(field)) {
-            // for the _field_names field, fields for the document
-            // are encoded as postings, where term is the field.
-            // so we hide terms for fields we filter out.
-            return fieldNamesFilterTerms;
+            // For the _field_names field, fields for the document are encoded as postings, where term is the field, so we hide terms for
+            // fields we filter out.
+            // Compute this lazily so that the DirectoryReader wrapper works together with RewriteCachingDirectoryReader (used by the
+            // can match phase in the frozen tier), which does not implement the terms() method.
+            if (fieldNamesFilterTerms == null) {
+                synchronized (this) {
+                    if (fieldNamesFilterTerms == null) {
+                        assert Transports.assertNotTransportThread("resolving filter terms");
+                        final Terms fieldNameTerms = super.terms(FieldNamesFieldMapper.NAME);
+                        this.fieldNamesFilterTerms = fieldNameTerms == null
+                            ? Optional.empty()
+                            : Optional.of(new FieldNamesTerms(fieldNameTerms));
+                    }
+                }
+            }
+            return fieldNamesFilterTerms.orElse(null);
         } else {
             return terms;
         }

diff --git a/...-snapshots/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/field_level_security.yml b/...-snapshots/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/field_level_security.yml
@@ -29,7 +29,11 @@ setup:
                 {
                   "names": ["*"],
                   "privileges": ["read"],
-                  "query": {"match": {"marker": "test_1"}}
+                  "query": {"match": {"marker": "test_1"}},
+                  "field_security" : {
+                    "grant" : [ "*" ],
+                    "except" : [ "forbidden_field" ]
+                  }
                 }
               ]
             }
@@ -72,14 +76,37 @@ setup:
 
   - do:
       indices.create:
-        index: test_index
+        index: test_index1
         body:
           mappings:
             properties:
               location:
                 properties:
                   city:
                     type: "keyword"
+              created_at:
+                type: date # add date field to trigger can-match phase in searches
+                format: "yyyy-MM-dd"
+
+          settings:
+            index:
+              number_of_shards: 1
+              number_of_replicas: 0
+
+  - do:
+      indices.create:
+        index: test_index2
+        body:
+          mappings:
+            properties:
+              location:
+                properties:
+                  city:
+                    type: "keyword"
+              created_at:
+                type: date # add date field to trigger can-match phase in searches
+                format: "yyyy-MM-dd"
+
           settings:
             index:
               number_of_shards: 1
@@ -89,10 +116,14 @@ setup:
       bulk:
         refresh: true
         body:
-          - '{"index": {"_index": "test_index"}}'
-          - '{"marker": "test_1", "location.city": "bos"}'
-          - '{"index": {"_index": "test_index"}}'
-          - '{"marker": "test_2", "location.city": "ams"}'
+          - '{"index": {"_index": "test_index1"}}'
+          - '{"marker": "test_1", "location.city": "bos", "forbidden_field" : 1, "created_at": "2016-01-01"}'
+          - '{"index": {"_index": "test_index1"}}'
+          - '{"marker": "test_2", "location.city": "ams", "forbidden_field" : 2, "created_at": "2016-01-01"}'
+          - '{"index": {"_index": "test_index2"}}'
+          - '{"marker": "test_2", "location.city": "bos", "forbidden_field" : 1, "created_at": "2019-01-02"}'
+          - '{"index": {"_index": "test_index2"}}'
+          - '{"marker": "test_2", "location.city": "ams", "forbidden_field" : 2, "created_at": "2019-01-02"}'
 
   - do:
       snapshot.create:
@@ -102,7 +133,11 @@ setup:
 
   - do:
       indices.delete:
-        index: test_index
+        index: test_index1
+
+  - do:
+      indices.delete:
+        index: test_index2
 
 ---
 teardown:
@@ -136,8 +171,22 @@ teardown:
         wait_for_completion: true
         storage: full_copy
         body:
-          index: test_index
-          renamed_index: test_index
+          index: test_index1
+          renamed_index: test_index1
+
+  - match: { snapshot.snapshot: snapshot }
+  - match: { snapshot.shards.failed: 0 }
+  - match: { snapshot.shards.successful: 1 }
+
+  - do:
+      searchable_snapshots.mount:
+        repository: repository-fs
+        snapshot: snapshot
+        wait_for_completion: true
+        storage: full_copy
+        body:
+          index: test_index2
+          renamed_index: test_index2
 
   - match: { snapshot.snapshot: snapshot }
   - match: { snapshot.shards.failed: 0 }
@@ -147,16 +196,22 @@ teardown:
       headers: { Authorization: "Basic ZnVsbDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # full - user
       search:
         rest_total_hits_as_int: true
-        index: test_index
+        index: test_index*
         size: 0
         from: 0
         body:
+          query:
+            range:
+              created_at:
+                "gte": "2016-01-01"
+                "lt": "2018-02-01"
           aggs:
             cities:
               terms:
                 field: location.city
 
-  - match: { _shards.total: 1 }
+  - match: { _shards.total: 2 }
+  - match: { _shards.skipped: 1 }
   - match: { hits.total: 2 }
   - length: { aggregations.cities.buckets: 2 }
   - match: { aggregations.cities.buckets.0.key: "ams" }
@@ -168,24 +223,30 @@ teardown:
       headers: { Authorization: "Basic bGltaXRlZDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # limited - user
       search:
         rest_total_hits_as_int: true
-        index: test_index
+        index: test_index*
         size: 0
         from: 0
         body:
+          query:
+            range:
+              created_at:
+                "gte": "2016-01-01"
+                "lt": "2018-02-01"
           aggs:
             cities:
               terms:
                 field: location.city
 
-  - match: { _shards.total: 1 }
+  - match: { _shards.total: 2 }
+  - match: { _shards.skipped: 1 }
   - match: { hits.total: 1 }
   - length: { aggregations.cities.buckets: 1 }
   - match: { aggregations.cities.buckets.0.key: "bos" }
   - match: { aggregations.cities.buckets.0.doc_count: 1 }
 
   - do:
       indices.delete:
-        index: test_index
+        index: test_index*
 
 ---
 "Test doc level security with different users on shared_cache index":
@@ -197,8 +258,22 @@ teardown:
         wait_for_completion: true
         storage: shared_cache
         body:
-          index: test_index
-          renamed_index: test_index
+          index: test_index1
+          renamed_index: test_index1
+
+  - match: { snapshot.snapshot: snapshot }
+  - match: { snapshot.shards.failed: 0 }
+  - match: { snapshot.shards.successful: 1 }
+
+  - do:
+      searchable_snapshots.mount:
+        repository: repository-fs
+        snapshot: snapshot
+        wait_for_completion: true
+        storage: shared_cache
+        body:
+          index: test_index2
+          renamed_index: test_index2
 
   - match: { snapshot.snapshot: snapshot }
   - match: { snapshot.shards.failed: 0 }
@@ -208,16 +283,22 @@ teardown:
       headers: { Authorization: "Basic ZnVsbDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # full - user
       search:
         rest_total_hits_as_int: true
-        index: test_index
+        index: test_index*
         size: 0
         from: 0
         body:
+          query:
+            range:
+              created_at:
+                "gte": "2016-01-01"
+                "lt": "2018-02-01"
           aggs:
             cities:
               terms:
                 field: location.city
 
-  - match: { _shards.total: 1 }
+  - match: { _shards.total: 2 }
+  - match: { _shards.skipped: 1 }
   - match: { hits.total: 2 }
   - length: { aggregations.cities.buckets: 2 }
   - match: { aggregations.cities.buckets.0.key: "ams" }
@@ -229,21 +310,27 @@ teardown:
       headers: { Authorization: "Basic bGltaXRlZDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # limited - user
       search:
         rest_total_hits_as_int: true
-        index: test_index
+        index: test_index*
         size: 0
         from: 0
         body:
+          query:
+            range:
+              created_at:
+                "gte": "2016-01-01"
+                "lt": "2018-02-01"
           aggs:
             cities:
               terms:
                 field: location.city
 
-  - match: { _shards.total: 1 }
+  - match: { _shards.total: 2 }
+  - match: { _shards.skipped: 1 }
   - match: { hits.total: 1 }
   - length: { aggregations.cities.buckets: 1 }
   - match: { aggregations.cities.buckets.0.key: "bos" }
   - match: { aggregations.cities.buckets.0.doc_count: 1 }
 
   - do:
       indices.delete:
-        index: test_index
+        index: test_index*