-
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
base: main
Are you sure you want to change the base?
Handle unavailable MD5 in ES|QL #130158
Conversation
In Java 14 the `MessageDigest` specification was changed so that the "MD5" hash function is no longer required. It is permissible for a JRE to ship without support for MD5 hashes. This commit modifies the ES|QL MD5 hash function implementation so that the `MessageDigest` object is no longer loaded on startup, and instead is lazily instantiated when needed. If an ES|QL query makes use of md5, and it is unavailable, then the query will fail with an ES|QL verification exception Resolves: elastic#129689
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @tvernum, I've created a changelog YAML for you. |
It's quite tricky to set up a JVM that has sufficient security providers to make ES run, but doesn't have MD5. To save others the trouble of setting that up, this is the behaviour you get (with this PR)
Without this PR Elasticsearch doesn't start
I picked I've marked this as |
|
||
import static org.hamcrest.Matchers.notNullValue; | ||
|
||
public class Md5UnavailableTests extends ESTestCase { |
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.
NIT: we usually put tests that do not require parametrization in *StaticTests, in this case it would be Md5StaticTests
.
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.
Thanks, will do that.
/** | ||
* 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 build the {@link HashFunction} lazily. |
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.
Is the same applicable for SHA functions?
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.
It is probably worth mentioning that this might not be available in certain JVMs at
Line 31 in fd1be8c
description = "Computes the MD5 hash of the input.", |
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.
No, as at Java 24, both SHA-1 and SHA-256 are still required
I could change those functions to work similarly, but I opted for the smallest change that solves the known problem. I actually started with a change in HashFunction
that would be lenient for any algorithm, but switched to something MD5 specific.
throw new VerificationException("function 'md5' is not available on this platform: {}", e.getMessage()); | ||
} | ||
} | ||
return function; |
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.
I would prefer if we do not try to initialize it for every query even if it is not available. May be something like:
private static final HashFunction MD5 = HashFunction.tryCreate("MD5");
and in Hash.java:
public static HashFunction tryCreate(String algorithm) {
try {
return new HashFunction(algorithm, MessageDigest.getInstance(algorithm));
} catch (NoSuchAlgorithmException e) {
logger.warn("[{}] hash fuinction is not available", algorithm);
return null;
}
}
This would also mean we need to modify org.elasticsearch.xpack.esql.expression.function.scalar.string.AbstractHashFunction#toEvaluator to use different (warning/error only) hash evaluator when getHashFunction
returns null
.
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.
I would prefer if we do not try to initialize it for every query even if it is not available.
So would I.
We're lacking an appropriate Result
(Maybe
) type, that has either a value or an error.
That is what I really wanted to use here, but opted for a lazy reference instead.
I can handle it as you describe, the issue is that we lose the original exception (its only in the logs) and have to treat null
as a marker for "not available".
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.
Possibly it worth introducing wrapper that captures exception and re-throws it when executed in new or existing constant evaluator.
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.
Since this is JVM specific, I am wondering if there are scenarios (like from test | eval hash=hash("md5", message) | stats count(hash) by foo
where the stats
is a pipeline breaker which sends the plan fragment to each data node) the hash is computed at data node level and if data nodes have different JVMs than that of the coordinator, a lazy approach is actually the correct approach (like what @tvernum is suggesting).
Sorry if I am mentioning something obvious, but thought on voicing it loud rather than just assuming this is already considered.
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.
Simple from index | eval hash = md5(field)
would spread md5 evaluation to each node too.
Since md5 is not going to become available after then node has started there is no need to try to initialize it more than once after the start. In case of nodes running distinct JVMs in this approach we might have nulls and actual hashes from different nodes.
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.
Simple from index | eval hash = md5(field) would spread md5 evaluation to each node too
It doesn't. The hash is computed on the coordinator after each node replies back with 1000 (the default) rows. The coordinator will pick the final 1000 rows and then compute the hash
on them.
This is how a physical plan would look like on the coordinator:
EvalExec[[HASH(md5[KEYWORD],field{f}#119) AS hash#115]]
\_LimitExec[1000[INTEGER],null]
\_ExchangeExec[[field{f}#119],false]
\_FragmentExec[fragment=[<>
Project[[field{f}#119]]
\_Limit[1000[INTEGER],false]
\_EsRelation[index][field{f}#119]<>]]
In Java 14 the
MessageDigest
specification was changed so that the "MD5" hash function is no longer required. It is permissible for a JRE to ship without support for MD5 hashes.This commit modifies the ES|QL MD5 hash function implementation so that the
MessageDigest
object is no longer loaded on startup, and instead is lazily instantiated when needed.If an ES|QL query makes use of md5, and it is unavailable, then the query will fail with an ES|QL verification exception
Resolves: #129689