Skip to content

Commit 99c802f

Browse files
ywelschelasticmachine
ywelsch
and
elasticmachine
authored
Fix FLS for frozen tier (#82521) (#82566)
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 Co-authored-by: Elastic Machine <[email protected]>
1 parent 9db70cb commit 99c802f

File tree

2 files changed

+127
-28
lines changed

2 files changed

+127
-28
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/FieldSubsetReader.java

+19-7
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import org.elasticsearch.core.Tuple;
3535
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
3636
import org.elasticsearch.index.mapper.SourceFieldMapper;
37+
import org.elasticsearch.transport.Transports;
3738
import org.elasticsearch.xcontent.XContentBuilder;
3839
import org.elasticsearch.xcontent.XContentType;
3940

@@ -44,6 +45,7 @@
4445
import java.util.Iterator;
4546
import java.util.List;
4647
import java.util.Map;
48+
import java.util.Optional;
4749

4850
/**
4951
* A {@link FilterLeafReader} that exposes only a subset
@@ -119,7 +121,7 @@ public CacheHelper getReaderCacheHelper() {
119121
/** An automaton that only accepts authorized fields. */
120122
private final CharacterRunAutomaton filter;
121123
/** {@link Terms} cache with filtered stats for the {@link FieldNamesFieldMapper} field. */
122-
private final Terms fieldNamesFilterTerms;
124+
private volatile Optional<Terms> fieldNamesFilterTerms;
123125

124126
/**
125127
* Wrap a single segment, exposing a subset of its fields.
@@ -134,8 +136,6 @@ public CacheHelper getReaderCacheHelper() {
134136
}
135137
fieldInfos = new FieldInfos(filteredInfos.toArray(new FieldInfo[filteredInfos.size()]));
136138
this.filter = filter;
137-
final Terms fieldNameTerms = super.terms(FieldNamesFieldMapper.NAME);
138-
this.fieldNamesFilterTerms = fieldNameTerms == null ? null : new FieldNamesTerms(fieldNameTerms);
139139
}
140140

141141
/** returns true if this field is allowed. */
@@ -422,10 +422,22 @@ private Terms wrapTerms(Terms terms, String field) throws IOException {
422422
if (hasField(field) == false) {
423423
return null;
424424
} else if (FieldNamesFieldMapper.NAME.equals(field)) {
425-
// for the _field_names field, fields for the document
426-
// are encoded as postings, where term is the field.
427-
// so we hide terms for fields we filter out.
428-
return fieldNamesFilterTerms;
425+
// For the _field_names field, fields for the document are encoded as postings, where term is the field, so we hide terms for
426+
// fields we filter out.
427+
// Compute this lazily so that the DirectoryReader wrapper works together with RewriteCachingDirectoryReader (used by the
428+
// can match phase in the frozen tier), which does not implement the terms() method.
429+
if (fieldNamesFilterTerms == null) {
430+
synchronized (this) {
431+
if (fieldNamesFilterTerms == null) {
432+
assert Transports.assertNotTransportThread("resolving filter terms");
433+
final Terms fieldNameTerms = super.terms(FieldNamesFieldMapper.NAME);
434+
this.fieldNamesFilterTerms = fieldNameTerms == null
435+
? Optional.empty()
436+
: Optional.of(new FieldNamesTerms(fieldNameTerms));
437+
}
438+
}
439+
}
440+
return fieldNamesFilterTerms.orElse(null);
429441
} else {
430442
return terms;
431443
}

‎x-pack/plugin/searchable-snapshots/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/field_level_security.yml

+108-21
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,11 @@ setup:
2929
{
3030
"names": ["*"],
3131
"privileges": ["read"],
32-
"query": {"match": {"marker": "test_1"}}
32+
"query": {"match": {"marker": "test_1"}},
33+
"field_security" : {
34+
"grant" : [ "*" ],
35+
"except" : [ "forbidden_field" ]
36+
}
3337
}
3438
]
3539
}
@@ -72,14 +76,37 @@ setup:
7276

7377
- do:
7478
indices.create:
75-
index: test_index
79+
index: test_index1
7680
body:
7781
mappings:
7882
properties:
7983
location:
8084
properties:
8185
city:
8286
type: "keyword"
87+
created_at:
88+
type: date # add date field to trigger can-match phase in searches
89+
format: "yyyy-MM-dd"
90+
91+
settings:
92+
index:
93+
number_of_shards: 1
94+
number_of_replicas: 0
95+
96+
- do:
97+
indices.create:
98+
index: test_index2
99+
body:
100+
mappings:
101+
properties:
102+
location:
103+
properties:
104+
city:
105+
type: "keyword"
106+
created_at:
107+
type: date # add date field to trigger can-match phase in searches
108+
format: "yyyy-MM-dd"
109+
83110
settings:
84111
index:
85112
number_of_shards: 1
@@ -89,10 +116,14 @@ setup:
89116
bulk:
90117
refresh: true
91118
body:
92-
- '{"index": {"_index": "test_index"}}'
93-
- '{"marker": "test_1", "location.city": "bos"}'
94-
- '{"index": {"_index": "test_index"}}'
95-
- '{"marker": "test_2", "location.city": "ams"}'
119+
- '{"index": {"_index": "test_index1"}}'
120+
- '{"marker": "test_1", "location.city": "bos", "forbidden_field" : 1, "created_at": "2016-01-01"}'
121+
- '{"index": {"_index": "test_index1"}}'
122+
- '{"marker": "test_2", "location.city": "ams", "forbidden_field" : 2, "created_at": "2016-01-01"}'
123+
- '{"index": {"_index": "test_index2"}}'
124+
- '{"marker": "test_2", "location.city": "bos", "forbidden_field" : 1, "created_at": "2019-01-02"}'
125+
- '{"index": {"_index": "test_index2"}}'
126+
- '{"marker": "test_2", "location.city": "ams", "forbidden_field" : 2, "created_at": "2019-01-02"}'
96127

97128
- do:
98129
snapshot.create:
@@ -102,7 +133,11 @@ setup:
102133

103134
- do:
104135
indices.delete:
105-
index: test_index
136+
index: test_index1
137+
138+
- do:
139+
indices.delete:
140+
index: test_index2
106141

107142
---
108143
teardown:
@@ -136,8 +171,22 @@ teardown:
136171
wait_for_completion: true
137172
storage: full_copy
138173
body:
139-
index: test_index
140-
renamed_index: test_index
174+
index: test_index1
175+
renamed_index: test_index1
176+
177+
- match: { snapshot.snapshot: snapshot }
178+
- match: { snapshot.shards.failed: 0 }
179+
- match: { snapshot.shards.successful: 1 }
180+
181+
- do:
182+
searchable_snapshots.mount:
183+
repository: repository-fs
184+
snapshot: snapshot
185+
wait_for_completion: true
186+
storage: full_copy
187+
body:
188+
index: test_index2
189+
renamed_index: test_index2
141190

142191
- match: { snapshot.snapshot: snapshot }
143192
- match: { snapshot.shards.failed: 0 }
@@ -147,16 +196,22 @@ teardown:
147196
headers: { Authorization: "Basic ZnVsbDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # full - user
148197
search:
149198
rest_total_hits_as_int: true
150-
index: test_index
199+
index: test_index*
151200
size: 0
152201
from: 0
153202
body:
203+
query:
204+
range:
205+
created_at:
206+
"gte": "2016-01-01"
207+
"lt": "2018-02-01"
154208
aggs:
155209
cities:
156210
terms:
157211
field: location.city
158212

159-
- match: { _shards.total: 1 }
213+
- match: { _shards.total: 2 }
214+
- match: { _shards.skipped: 1 }
160215
- match: { hits.total: 2 }
161216
- length: { aggregations.cities.buckets: 2 }
162217
- match: { aggregations.cities.buckets.0.key: "ams" }
@@ -168,24 +223,30 @@ teardown:
168223
headers: { Authorization: "Basic bGltaXRlZDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # limited - user
169224
search:
170225
rest_total_hits_as_int: true
171-
index: test_index
226+
index: test_index*
172227
size: 0
173228
from: 0
174229
body:
230+
query:
231+
range:
232+
created_at:
233+
"gte": "2016-01-01"
234+
"lt": "2018-02-01"
175235
aggs:
176236
cities:
177237
terms:
178238
field: location.city
179239

180-
- match: { _shards.total: 1 }
240+
- match: { _shards.total: 2 }
241+
- match: { _shards.skipped: 1 }
181242
- match: { hits.total: 1 }
182243
- length: { aggregations.cities.buckets: 1 }
183244
- match: { aggregations.cities.buckets.0.key: "bos" }
184245
- match: { aggregations.cities.buckets.0.doc_count: 1 }
185246

186247
- do:
187248
indices.delete:
188-
index: test_index
249+
index: test_index*
189250

190251
---
191252
"Test doc level security with different users on shared_cache index":
@@ -197,8 +258,22 @@ teardown:
197258
wait_for_completion: true
198259
storage: shared_cache
199260
body:
200-
index: test_index
201-
renamed_index: test_index
261+
index: test_index1
262+
renamed_index: test_index1
263+
264+
- match: { snapshot.snapshot: snapshot }
265+
- match: { snapshot.shards.failed: 0 }
266+
- match: { snapshot.shards.successful: 1 }
267+
268+
- do:
269+
searchable_snapshots.mount:
270+
repository: repository-fs
271+
snapshot: snapshot
272+
wait_for_completion: true
273+
storage: shared_cache
274+
body:
275+
index: test_index2
276+
renamed_index: test_index2
202277

203278
- match: { snapshot.snapshot: snapshot }
204279
- match: { snapshot.shards.failed: 0 }
@@ -208,16 +283,22 @@ teardown:
208283
headers: { Authorization: "Basic ZnVsbDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # full - user
209284
search:
210285
rest_total_hits_as_int: true
211-
index: test_index
286+
index: test_index*
212287
size: 0
213288
from: 0
214289
body:
290+
query:
291+
range:
292+
created_at:
293+
"gte": "2016-01-01"
294+
"lt": "2018-02-01"
215295
aggs:
216296
cities:
217297
terms:
218298
field: location.city
219299

220-
- match: { _shards.total: 1 }
300+
- match: { _shards.total: 2 }
301+
- match: { _shards.skipped: 1 }
221302
- match: { hits.total: 2 }
222303
- length: { aggregations.cities.buckets: 2 }
223304
- match: { aggregations.cities.buckets.0.key: "ams" }
@@ -229,21 +310,27 @@ teardown:
229310
headers: { Authorization: "Basic bGltaXRlZDp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # limited - user
230311
search:
231312
rest_total_hits_as_int: true
232-
index: test_index
313+
index: test_index*
233314
size: 0
234315
from: 0
235316
body:
317+
query:
318+
range:
319+
created_at:
320+
"gte": "2016-01-01"
321+
"lt": "2018-02-01"
236322
aggs:
237323
cities:
238324
terms:
239325
field: location.city
240326

241-
- match: { _shards.total: 1 }
327+
- match: { _shards.total: 2 }
328+
- match: { _shards.skipped: 1 }
242329
- match: { hits.total: 1 }
243330
- length: { aggregations.cities.buckets: 1 }
244331
- match: { aggregations.cities.buckets.0.key: "bos" }
245332
- match: { aggregations.cities.buckets.0.doc_count: 1 }
246333

247334
- do:
248335
indices.delete:
249-
index: test_index
336+
index: test_index*

0 commit comments

Comments
 (0)