-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Handle unavailable MD5 in ES|QL #130158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle unavailable MD5 in ES|QL #130158
Changes from all commits
fd8bfc4
33b6d87
05c8901
249fe6b
b03fa6c
552424f
4169b5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 130158 | ||
summary: Handle unavailable MD5 in ES|QL | ||
area: ES|QL | ||
type: bug | ||
issues: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
package org.elasticsearch.common.util; | ||
|
||
import org.elasticsearch.common.CheckedSupplier; | ||
|
||
import java.util.Optional; | ||
|
||
/** | ||
* A wrapper around either | ||
* <ul> | ||
* <li>a successful result of parameterized type {@code V}</li> | ||
* <li>a failure with exception type {@code E}</li> | ||
* </ul> | ||
*/ | ||
public abstract class Result<V, E extends Exception> implements CheckedSupplier<V, E> { | ||
|
||
public static <V, E extends Exception> Result<V, E> of(V value) { | ||
return new Success<>(value); | ||
} | ||
|
||
public static <V, E extends Exception> Result<V, E> failure(E exception) { | ||
return new Failure<>(exception); | ||
} | ||
|
||
private Result() {} | ||
|
||
public abstract V get() throws E; | ||
|
||
public abstract Optional<E> failure(); | ||
|
||
public abstract boolean isSuccessful(); | ||
|
||
public boolean isFailure() { | ||
return isSuccessful() == false; | ||
}; | ||
|
||
public abstract Optional<V> asOptional(); | ||
|
||
private static class Success<V, E extends Exception> extends Result<V, E> { | ||
private final V value; | ||
|
||
Success(V value) { | ||
this.value = value; | ||
} | ||
|
||
@Override | ||
public V get() throws E { | ||
return value; | ||
} | ||
|
||
@Override | ||
public Optional<E> failure() { | ||
return Optional.empty(); | ||
} | ||
|
||
@Override | ||
public boolean isSuccessful() { | ||
return true; | ||
} | ||
|
||
@Override | ||
public Optional<V> asOptional() { | ||
return Optional.of(value); | ||
} | ||
} | ||
|
||
private static class Failure<V, E extends Exception> extends Result<V, E> { | ||
private final E exception; | ||
|
||
Failure(E exception) { | ||
this.exception = exception; | ||
} | ||
|
||
@Override | ||
public V get() throws E { | ||
throw exception; | ||
} | ||
|
||
@Override | ||
public Optional<E> failure() { | ||
return Optional.of(exception); | ||
} | ||
|
||
@Override | ||
public boolean isSuccessful() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public Optional<V> asOptional() { | ||
return Optional.empty(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
package org.elasticsearch.common.util; | ||
|
||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.ElasticsearchStatusException; | ||
import org.elasticsearch.rest.RestStatus; | ||
import org.elasticsearch.test.ESTestCase; | ||
|
||
import static org.elasticsearch.test.hamcrest.OptionalMatchers.isEmpty; | ||
import static org.elasticsearch.test.hamcrest.OptionalMatchers.isPresentWith; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.sameInstance; | ||
|
||
public class ResultTests extends ESTestCase { | ||
|
||
public void testSuccess() { | ||
final String str = randomAlphaOfLengthBetween(3, 8); | ||
final Result<String, ElasticsearchException> result = Result.of(str); | ||
assertThat(result.isSuccessful(), is(true)); | ||
assertThat(result.isFailure(), is(false)); | ||
assertThat(result.get(), sameInstance(str)); | ||
assertThat(result.failure(), isEmpty()); | ||
assertThat(result.asOptional(), isPresentWith(str)); | ||
} | ||
|
||
public void testFailure() { | ||
final ElasticsearchException exception = new ElasticsearchStatusException( | ||
randomAlphaOfLengthBetween(10, 30), | ||
RestStatus.INTERNAL_SERVER_ERROR | ||
); | ||
final Result<String, ElasticsearchException> result = Result.failure(exception); | ||
assertThat(result.isSuccessful(), is(false)); | ||
assertThat(result.isFailure(), is(true)); | ||
assertThat(expectThrows(Exception.class, result::get), sameInstance(exception)); | ||
assertThat(result.failure(), isPresentWith(sameInstance(exception))); | ||
assertThat(result.asOptional(), isEmpty()); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ | |
|
||
import org.elasticsearch.common.io.stream.NamedWriteableRegistry; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.util.Result; | ||
import org.elasticsearch.xpack.esql.VerificationException; | ||
import org.elasticsearch.xpack.esql.core.expression.Expression; | ||
import org.elasticsearch.xpack.esql.core.tree.NodeInfo; | ||
import org.elasticsearch.xpack.esql.core.tree.Source; | ||
|
@@ -18,17 +20,24 @@ | |
import org.elasticsearch.xpack.esql.expression.function.scalar.string.Hash.HashFunction; | ||
|
||
import java.io.IOException; | ||
import java.security.MessageDigest; | ||
import java.security.NoSuchAlgorithmException; | ||
import java.util.List; | ||
|
||
public class Md5 extends AbstractHashFunction { | ||
|
||
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "MD5", Md5::new); | ||
|
||
private static final HashFunction MD5 = HashFunction.create("MD5"); | ||
/** | ||
* As of Java 14, it is permissible for a JRE to ship without the {@code MD5} {@link MessageDigest}. | ||
* We want the "md5" function in ES|QL to fail at runtime on such platforms (rather than at startup) | ||
* so we wrap the {@link HashFunction} in a {@link Result}. | ||
*/ | ||
private static final Result<HashFunction, NoSuchAlgorithmException> MD5 = HashFunction.tryCreate("MD5"); | ||
|
||
@FunctionInfo( | ||
returnType = "keyword", | ||
description = "Computes the MD5 hash of the input.", | ||
description = "Computes the MD5 hash of the input (if the MD5 hash is available on the JVM).", | ||
examples = { @Example(file = "hash", tag = "md5") } | ||
) | ||
public Md5(Source source, @Param(name = "input", type = { "keyword", "text" }, description = "Input to hash.") Expression input) { | ||
|
@@ -41,7 +50,12 @@ private Md5(StreamInput in) throws IOException { | |
|
||
@Override | ||
protected HashFunction getHashFunction() { | ||
return MD5; | ||
try { | ||
return MD5.get(); | ||
} catch (NoSuchAlgorithmException e) { | ||
// Throw a new exception so that the stack trace reflects this call (rather than the static initializer for the MD5 field) | ||
throw new VerificationException("function 'md5' is not available on this platform: {}", e.getMessage()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth adding e as the cause? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I guess it is okay to skip it for now |
||
} | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,22 +13,30 @@ | |
import org.elasticsearch.common.util.BigArrays; | ||
import org.elasticsearch.common.util.MockBigArrays; | ||
import org.elasticsearch.common.util.PageCacheRecycler; | ||
import org.elasticsearch.common.util.Result; | ||
import org.elasticsearch.compute.data.BlockFactory; | ||
import org.elasticsearch.compute.operator.DriverContext; | ||
import org.elasticsearch.test.ESTestCase; | ||
import org.elasticsearch.test.hamcrest.OptionalMatchers; | ||
import org.elasticsearch.xpack.esql.core.InvalidArgumentException; | ||
import org.elasticsearch.xpack.esql.core.expression.Literal; | ||
import org.elasticsearch.xpack.esql.core.tree.Source; | ||
import org.elasticsearch.xpack.esql.core.type.DataType; | ||
import org.junit.After; | ||
|
||
import java.security.NoSuchAlgorithmException; | ||
import java.security.Provider; | ||
import java.security.Security; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.test.TestMatchers.throwableWithMessage; | ||
import static org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase.evaluator; | ||
import static org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase.field; | ||
import static org.hamcrest.Matchers.containsString; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.startsWith; | ||
|
||
public class HashStaticTests extends ESTestCase { | ||
|
@@ -45,6 +53,27 @@ public void testInvalidAlgorithmLiteral() { | |
assertThat(e.getMessage(), startsWith("invalid algorithm for [hast(\"invalid\", input)]: invalid MessageDigest not available")); | ||
} | ||
|
||
public void testTryCreateUnavailableMd5() throws NoSuchAlgorithmException { | ||
assumeFalse("We run with different security providers in FIPS, and changing them at runtime is more complicated", inFipsJvm()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth having another test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MD5 is usually available on FIPS, and is available in our FIPS testing environment. It's only in very particular configurations that it isn't available (which is why nothing is failing on |
||
final Provider sunProvider = Security.getProvider("SUN"); | ||
try { | ||
Security.removeProvider("SUN"); | ||
final Result<Hash.HashFunction, NoSuchAlgorithmException> result = Hash.HashFunction.tryCreate("MD5"); | ||
assertThat(result.isSuccessful(), is(false)); | ||
assertThat(result.failure(), OptionalMatchers.isPresentWith(throwableWithMessage(containsString("MD5")))); | ||
expectThrows(NoSuchAlgorithmException.class, result::get); | ||
} finally { | ||
Security.addProvider(sunProvider); | ||
} | ||
|
||
{ | ||
final Result<Hash.HashFunction, NoSuchAlgorithmException> result = Hash.HashFunction.tryCreate("MD5"); | ||
assertThat(result.isSuccessful(), is(true)); | ||
assertThat(result.failure(), OptionalMatchers.isEmpty()); | ||
assertThat(result.get().algorithm(), is("MD5")); | ||
} | ||
} | ||
|
||
/** | ||
* The following fields and methods were borrowed from AbstractScalarFunctionTestCase | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider an option to emit a warning and supply null instead of md5 hash?
Or is it proffered to fail the entire query instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to me like a poor place to be lenient - I would definitely argue against it. The behaviour here is roughly the same as what
hash("md5", field)
orhash("does-not-exist", field)
would provide, which seems right to me.