Move exec syscall filtering to NativeAccess #108970
Conversation
This commit moves the system call filtering initialization into NativeAccess. The code is essentially unmodified from its existing state, now existing within the *NativeAccess implementations. relates elastic#104876
elasticsearchmachine
added
v8.15.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
ldematte
left a comment
ldematte
left a comment
There was a problem hiding this comment.
I see we are dropping support for Solaris, FreeBSD, OpenBSD, but I suppose this is OK, as this is in line with all the rest of native functions
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Installs a system call filter to block process execution. |
There was a problem hiding this comment.
Nit: I would split up this comment and put it above the new code in *NativeAccess.java, above each tryInstallExecSandbox implementation. I see there are some, but not all the details (also, it needs some update, e.g. supporting aarch64 on Linux).
|
|
||
| // TODO: move this UOE to when binding methods in jna/jdk impls | ||
| // we couldn't link methods, could be some really ancient kernel (e.g. < 2.1.57) or some bug | ||
| /*if (linux_libc == null) { |
There was a problem hiding this comment.
Is this something we can do now, or is it for a follow-up?
| * | ||
| * https://msdn.microsoft.com/en-us/library/windows/desktop/ms684147%28v=vs.85%29.aspx | ||
| */ | ||
| public static class JnaJobObjectBasicLimitInformation extends Structure implements ByReference, JobObjectBasicLimitInformation { |
There was a problem hiding this comment.
I think the corresponding classes/functions in org/elasticsearch/bootstrap/JNAKernel32Library.java can be cleaned up too (e.g. class JOBOBJECT_BASIC_LIMIT_INFORMATION)
There was a problem hiding this comment.
Oh good catch, that entire class can be removed.
| public void setUp() throws Exception { | ||
| super.setUp(); | ||
| assumeTrue("requires system call filter installation", Natives.isSystemCallFilterInstalled()); | ||
| assumeTrue( |
There was a problem hiding this comment.
I'm a bit concerned about testing. At the very least, I would run them locally on a Mac, on CI with test-windows and test-arm, with JDK 17 and 21 (to check both JNA and JDK implementations), and check they are actually run (not skipped by this assumeTrue). It's a lot of work :/
There was a problem hiding this comment.
I've added several more labels that should trigger complete testing on all platforms.
There was a problem hiding this comment.
with JDK 17 and 21
Actually, these labels won't trigger either of these versions precisely. I'll double check packaging tests, if we turn on bootstrap checks it should cover all these test scenarios.
We never officially supported these, so I don't see this as dropping support, just cleaning up unused code. There are numerous other things that will break/fail on those platforms. |
rjernst
added
test-arm
|
Pinging @elastic/es-delivery (Team:Delivery) |
ldematte
left a comment
ldematte
left a comment
There was a problem hiding this comment.
As long as the additional testing/validation we discussed shows no problems (thanks for the effort!), LGTM
rjernst
added
the
auto-merge-without-approval
rjernst
removed
the
auto-merge-without-approval
| versions = [bwcVersion.toString(), project.version] | ||
| setting 'cluster.remote.node.attr', 'gateway' | ||
| setting 'xpack.security.enabled', 'false' | ||
| setting 'logger.org.elasticsearch.env.NodeEnvironment', 'TRACE' |
There was a problem hiding this comment.
Do we want to keep this in here?
There was a problem hiding this comment.
No, i've just been adding these for debugging the windows bwc test failure.
This commit moves the system call filtering initialization into NativeAccess. The code is essentially unmodified from its existing state, now existing within the *NativeAccess implementations. relates #104876
This commit moves the system call filtering initialization into NativeAccess. The code is essentially unmodified from its existing state, now existing within the *NativeAccess implementations. relates #104876
4 participants
This commit moves the system call filtering initialization into NativeAccess. The code is essentially unmodified from its existing state, now existing within the *NativeAccess implementations.
relates #104876