Scripts: Fix security for deprecation warning (#28485)
If you call `getDates()` on a long or date type field add a deprecation warning to the response and log something to the deprecation logger. This *mostly* worked just fine but if the deprecation logger happens to roll then the roll will be performed with the script's permissions rather than the permissions of the server. And scripts don't have permissions to, say, open files. So the rolling failed. This fixes that by wrapping the call the deprecation logger in `doPriviledged`. This is a strange `doPrivileged` call because it doens't check Elasticsearch's `SpecialPermission`. `SpecialPermission` is a permission that no-script code has and that scripts never have. Usually all `doPrivileged` calls check `SpecialPermission` to make sure that they are not accidentally acting on behalf of a script. But in this case we are *intentionally* acting on behalf of a script. Closes #28408
This commit is contained in:
Nik Everett
GitHub
parent
de6d31ebc2
commit
5003ef18ac
|
|
@ -83,6 +83,9 @@ setup:
|
|||
|
||||
---
|
||||
"date":
|
||||
- skip:
|
||||
features: "warnings"
|
||||
|
||||
- do:
|
||||
search:
|
||||
body:
|
||||
|
|
@ -101,6 +104,28 @@ setup:
|
|||
source: "doc.date.value"
|
||||
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
|
||||
|
||||
- do:
|
||||
warnings:
|
||||
- getDate is no longer necessary on date fields as the value is now a date.
|
||||
search:
|
||||
body:
|
||||
script_fields:
|
||||
field:
|
||||
script:
|
||||
source: "doc['date'].date"
|
||||
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
|
||||
|
||||
- do:
|
||||
warnings:
|
||||
- getDates is no longer necessary on date fields as the values are now dates.
|
||||
search:
|
||||
body:
|
||||
script_fields:
|
||||
field:
|
||||
script:
|
||||
source: "doc['date'].dates.get(0)"
|
||||
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
|
||||
|
||||
---
|
||||
"geo_point":
|
||||
- do:
|
||||
|
|
@ -165,6 +190,9 @@ setup:
|
|||
|
||||
---
|
||||
"long":
|
||||
- skip:
|
||||
features: "warnings"
|
||||
|
||||
- do:
|
||||
search:
|
||||
body:
|
||||
|
|
@ -183,6 +211,28 @@ setup:
|
|||
source: "doc['long'].value"
|
||||
- match: { hits.hits.0.fields.field.0: 12348732141234 }
|
||||
|
||||
- do:
|
||||
warnings:
|
||||
- getDate on numeric fields is deprecated. Use a date field to get dates.
|
||||
search:
|
||||
body:
|
||||
script_fields:
|
||||
field:
|
||||
script:
|
||||
source: "doc['long'].date"
|
||||
- match: { hits.hits.0.fields.field.0: '2361-04-26T03:22:21.234Z' }
|
||||
|
||||
- do:
|
||||
warnings:
|
||||
- getDates on numeric fields is deprecated. Use a date field to get dates.
|
||||
search:
|
||||
body:
|
||||
script_fields:
|
||||
field:
|
||||
script:
|
||||
source: "doc['long'].dates.get(0)"
|
||||
- match: { hits.hits.0.fields.field.0: '2361-04-26T03:22:21.234Z' }
|
||||
|
||||
---
|
||||
"integer":
|
||||
- do:
|
||||
|
|
|
|||
|
|
@ -35,10 +35,13 @@ import org.joda.time.MutableDateTime;
|
|||
import org.joda.time.ReadableDateTime;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.security.AccessController;
|
||||
import java.security.PrivilegedAction;
|
||||
import java.util.AbstractList;
|
||||
import java.util.Arrays;
|
||||
import java.util.Comparator;
|
||||
import java.util.List;
|
||||
import java.util.function.Consumer;
|
||||
import java.util.function.UnaryOperator;
|
||||
|
||||
|
||||
|
|
@ -46,7 +49,7 @@ import java.util.function.UnaryOperator;
|
|||
* Script level doc values, the assumption is that any implementation will
|
||||
* implement a <code>getValue</code> and a <code>getValues</code> that return
|
||||
* the relevant type that then can be used in scripts.
|
||||
*
|
||||
*
|
||||
* Implementations should not internally re-use objects for the values that they
|
||||
* return as a single {@link ScriptDocValues} instance can be reused to return
|
||||
* values form multiple documents.
|
||||
|
|
@ -94,14 +97,30 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
protected static final DeprecationLogger deprecationLogger = new DeprecationLogger(ESLoggerFactory.getLogger(Longs.class));
|
||||
|
||||
private final SortedNumericDocValues in;
|
||||
/**
|
||||
* Callback for deprecated fields. In production this should always point to
|
||||
* {@link #deprecationLogger} but tests will override it so they can test that
|
||||
* we use the required permissions when calling it.
|
||||
*/
|
||||
private final Consumer<String> deprecationCallback;
|
||||
private long[] values = new long[0];
|
||||
private int count;
|
||||
private Dates dates;
|
||||
private int docId = -1;
|
||||
|
||||
/**
|
||||
* Standard constructor.
|
||||
*/
|
||||
public Longs(SortedNumericDocValues in) {
|
||||
this.in = in;
|
||||
this(in, deprecationLogger::deprecated);
|
||||
}
|
||||
|
||||
/**
|
||||
* Constructor for testing the deprecation callback.
|
||||
*/
|
||||
Longs(SortedNumericDocValues in, Consumer<String> deprecationCallback) {
|
||||
this.in = in;
|
||||
this.deprecationCallback = deprecationCallback;
|
||||
}
|
||||
|
||||
@Override
|
||||
|
|
@ -142,7 +161,7 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
|
||||
@Deprecated
|
||||
public ReadableDateTime getDate() throws IOException {
|
||||
deprecationLogger.deprecated("getDate on numeric fields is deprecated. Use a date field to get dates.");
|
||||
deprecated("getDate on numeric fields is deprecated. Use a date field to get dates.");
|
||||
if (dates == null) {
|
||||
dates = new Dates(in);
|
||||
dates.setNextDocId(docId);
|
||||
|
|
@ -152,7 +171,7 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
|
||||
@Deprecated
|
||||
public List<ReadableDateTime> getDates() throws IOException {
|
||||
deprecationLogger.deprecated("getDates on numeric fields is deprecated. Use a date field to get dates.");
|
||||
deprecated("getDates on numeric fields is deprecated. Use a date field to get dates.");
|
||||
if (dates == null) {
|
||||
dates = new Dates(in);
|
||||
dates.setNextDocId(docId);
|
||||
|
|
@ -169,6 +188,22 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
public int size() {
|
||||
return count;
|
||||
}
|
||||
|
||||
/**
|
||||
* Log a deprecation log, with the server's permissions, not the permissions of the
|
||||
* script calling this method. We need to do this to prevent errors when rolling
|
||||
* the log file.
|
||||
*/
|
||||
private void deprecated(String message) {
|
||||
// Intentionally not calling SpecialPermission.check because this is supposed to be called by scripts
|
||||
AccessController.doPrivileged(new PrivilegedAction<Void>() {
|
||||
@Override
|
||||
public Void run() {
|
||||
deprecationCallback.accept(message);
|
||||
return null;
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
public static final class Dates extends ScriptDocValues<ReadableDateTime> {
|
||||
|
|
@ -177,6 +212,12 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
private static final ReadableDateTime EPOCH = new DateTime(0, DateTimeZone.UTC);
|
||||
|
||||
private final SortedNumericDocValues in;
|
||||
/**
|
||||
* Callback for deprecated fields. In production this should always point to
|
||||
* {@link #deprecationLogger} but tests will override it so they can test that
|
||||
* we use the required permissions when calling it.
|
||||
*/
|
||||
private final Consumer<String> deprecationCallback;
|
||||
/**
|
||||
* Values wrapped in {@link MutableDateTime}. Null by default an allocated on first usage so we allocate a reasonably size. We keep
|
||||
* this array so we don't have allocate new {@link MutableDateTime}s on every usage. Instead we reuse them for every document.
|
||||
|
|
@ -184,8 +225,19 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
private MutableDateTime[] dates;
|
||||
private int count;
|
||||
|
||||
/**
|
||||
* Standard constructor.
|
||||
*/
|
||||
public Dates(SortedNumericDocValues in) {
|
||||
this(in, deprecationLogger::deprecated);
|
||||
}
|
||||
|
||||
/**
|
||||
* Constructor for testing deprecation logging.
|
||||
*/
|
||||
Dates(SortedNumericDocValues in, Consumer<String> deprecationCallback) {
|
||||
this.in = in;
|
||||
this.deprecationCallback = deprecationCallback;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -204,7 +256,7 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
*/
|
||||
@Deprecated
|
||||
public ReadableDateTime getDate() {
|
||||
deprecationLogger.deprecated("getDate is no longer necessary on date fields as the value is now a date.");
|
||||
deprecated("getDate is no longer necessary on date fields as the value is now a date.");
|
||||
return getValue();
|
||||
}
|
||||
|
||||
|
|
@ -213,7 +265,7 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
*/
|
||||
@Deprecated
|
||||
public List<ReadableDateTime> getDates() {
|
||||
deprecationLogger.deprecated("getDates is no longer necessary on date fields as the values are now dates.");
|
||||
deprecated("getDates is no longer necessary on date fields as the values are now dates.");
|
||||
return this;
|
||||
}
|
||||
|
||||
|
|
@ -274,6 +326,22 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
|
|||
dates[i] = new MutableDateTime(in.nextValue(), DateTimeZone.UTC);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Log a deprecation log, with the server's permissions, not the permissions of the
|
||||
* script calling this method. We need to do this to prevent errors when rolling
|
||||
* the log file.
|
||||
*/
|
||||
private void deprecated(String message) {
|
||||
// Intentionally not calling SpecialPermission.check because this is supposed to be called by scripts
|
||||
AccessController.doPrivileged(new PrivilegedAction<Void>() {
|
||||
@Override
|
||||
public Void run() {
|
||||
deprecationCallback.accept(message);
|
||||
return null;
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
public static final class Doubles extends ScriptDocValues<Double> {
|
||||
|
|
|
|||
|
|
@ -26,6 +26,17 @@ import org.joda.time.DateTimeZone;
|
|||
import org.joda.time.ReadableDateTime;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.security.AccessControlContext;
|
||||
import java.security.AccessController;
|
||||
import java.security.PermissionCollection;
|
||||
import java.security.Permissions;
|
||||
import java.security.PrivilegedAction;
|
||||
import java.security.ProtectionDomain;
|
||||
import java.util.HashSet;
|
||||
import java.util.Set;
|
||||
import java.util.function.Consumer;
|
||||
|
||||
import static org.hamcrest.Matchers.containsInAnyOrder;
|
||||
|
||||
public class ScriptDocValuesDatesTests extends ESTestCase {
|
||||
public void test() throws IOException {
|
||||
|
|
@ -39,12 +50,19 @@ public class ScriptDocValuesDatesTests extends ESTestCase {
|
|||
values[d][i] = expectedDates[d][i].getMillis();
|
||||
}
|
||||
}
|
||||
Dates dates = wrap(values);
|
||||
Set<String> warnings = new HashSet<>();
|
||||
Dates dates = wrap(values, deprecationMessage -> {
|
||||
warnings.add(deprecationMessage);
|
||||
/* Create a temporary directory to prove we are running with the
|
||||
* server's permissions. */
|
||||
createTempDir();
|
||||
});
|
||||
|
||||
for (int round = 0; round < 10; round++) {
|
||||
int d = between(0, values.length - 1);
|
||||
dates.setNextDocId(d);
|
||||
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getValue());
|
||||
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getDate());
|
||||
|
||||
assertEquals(values[d].length, dates.size());
|
||||
for (int i = 0; i < values[d].length; i++) {
|
||||
|
|
@ -54,9 +72,33 @@ public class ScriptDocValuesDatesTests extends ESTestCase {
|
|||
Exception e = expectThrows(UnsupportedOperationException.class, () -> dates.add(new DateTime()));
|
||||
assertEquals("doc values are unmodifiable", e.getMessage());
|
||||
}
|
||||
|
||||
/*
|
||||
* Invoke getDates without any privileges to verify that
|
||||
* it still works without any. In particularly, this
|
||||
* verifies that the callback that we've configured
|
||||
* above works. That callback creates a temporary
|
||||
* directory which is not possible with "noPermissions".
|
||||
*/
|
||||
PermissionCollection noPermissions = new Permissions();
|
||||
AccessControlContext noPermissionsAcc = new AccessControlContext(
|
||||
new ProtectionDomain[] {
|
||||
new ProtectionDomain(null, noPermissions)
|
||||
}
|
||||
);
|
||||
AccessController.doPrivileged(new PrivilegedAction<Void>() {
|
||||
public Void run() {
|
||||
dates.getDates();
|
||||
return null;
|
||||
}
|
||||
}, noPermissionsAcc);
|
||||
|
||||
assertThat(warnings, containsInAnyOrder(
|
||||
"getDate is no longer necessary on date fields as the value is now a date.",
|
||||
"getDates is no longer necessary on date fields as the values are now dates."));
|
||||
}
|
||||
|
||||
private Dates wrap(long[][] values) {
|
||||
private Dates wrap(long[][] values, Consumer<String> deprecationHandler) {
|
||||
return new Dates(new AbstractSortedNumericDocValues() {
|
||||
long[] current;
|
||||
int i;
|
||||
|
|
@ -75,6 +117,6 @@ public class ScriptDocValuesDatesTests extends ESTestCase {
|
|||
public long nextValue() {
|
||||
return current[i++];
|
||||
}
|
||||
});
|
||||
}, deprecationHandler);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,6 +26,17 @@ import org.joda.time.DateTimeZone;
|
|||
import org.joda.time.ReadableDateTime;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.security.AccessControlContext;
|
||||
import java.security.AccessController;
|
||||
import java.security.PermissionCollection;
|
||||
import java.security.Permissions;
|
||||
import java.security.PrivilegedAction;
|
||||
import java.security.ProtectionDomain;
|
||||
import java.util.HashSet;
|
||||
import java.util.Set;
|
||||
import java.util.function.Consumer;
|
||||
|
||||
import static org.hamcrest.Matchers.containsInAnyOrder;
|
||||
|
||||
public class ScriptDocValuesLongsTests extends ESTestCase {
|
||||
public void testLongs() throws IOException {
|
||||
|
|
@ -36,7 +47,7 @@ public class ScriptDocValuesLongsTests extends ESTestCase {
|
|||
values[d][i] = randomLong();
|
||||
}
|
||||
}
|
||||
Longs longs = wrap(values);
|
||||
Longs longs = wrap(values, deprecationMessage -> {fail("unexpected deprecation: " + deprecationMessage);});
|
||||
|
||||
for (int round = 0; round < 10; round++) {
|
||||
int d = between(0, values.length - 1);
|
||||
|
|
@ -66,7 +77,13 @@ public class ScriptDocValuesLongsTests extends ESTestCase {
|
|||
values[d][i] = dates[d][i].getMillis();
|
||||
}
|
||||
}
|
||||
Longs longs = wrap(values);
|
||||
Set<String> warnings = new HashSet<>();
|
||||
Longs longs = wrap(values, deprecationMessage -> {
|
||||
warnings.add(deprecationMessage);
|
||||
/* Create a temporary directory to prove we are running with the
|
||||
* server's permissions. */
|
||||
createTempDir();
|
||||
});
|
||||
|
||||
for (int round = 0; round < 10; round++) {
|
||||
int d = between(0, values.length - 1);
|
||||
|
|
@ -82,12 +99,36 @@ public class ScriptDocValuesLongsTests extends ESTestCase {
|
|||
assertEquals("doc values are unmodifiable", e.getMessage());
|
||||
}
|
||||
|
||||
assertWarnings(
|
||||
/*
|
||||
* Invoke getDates without any privileges to verify that
|
||||
* it still works without any. In particularly, this
|
||||
* verifies that the callback that we've configured
|
||||
* above works. That callback creates a temporary
|
||||
* directory which is not possible with "noPermissions".
|
||||
*/
|
||||
PermissionCollection noPermissions = new Permissions();
|
||||
AccessControlContext noPermissionsAcc = new AccessControlContext(
|
||||
new ProtectionDomain[] {
|
||||
new ProtectionDomain(null, noPermissions)
|
||||
}
|
||||
);
|
||||
AccessController.doPrivileged(new PrivilegedAction<Void>() {
|
||||
public Void run() {
|
||||
try {
|
||||
longs.getDates();
|
||||
} catch (IOException e) {
|
||||
throw new RuntimeException("unexpected", e);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
}, noPermissionsAcc);
|
||||
|
||||
assertThat(warnings, containsInAnyOrder(
|
||||
"getDate on numeric fields is deprecated. Use a date field to get dates.",
|
||||
"getDates on numeric fields is deprecated. Use a date field to get dates.");
|
||||
"getDates on numeric fields is deprecated. Use a date field to get dates."));
|
||||
}
|
||||
|
||||
private Longs wrap(long[][] values) {
|
||||
private Longs wrap(long[][] values, Consumer<String> deprecationCallback) {
|
||||
return new Longs(new AbstractSortedNumericDocValues() {
|
||||
long[] current;
|
||||
int i;
|
||||
|
|
@ -106,6 +147,6 @@ public class ScriptDocValuesLongsTests extends ESTestCase {
|
|||
public long nextValue() {
|
||||
return current[i++];
|
||||
}
|
||||
});
|
||||
}, deprecationCallback);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue