Use a private directory for temporary files #27144
Conversation
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
I left some initial thoughts
There was a problem hiding this comment.
I am not sure we should try to clean up stuff in here. We didn't do this for the java.nio.tmpdir so I am not sure we should do this here.
There was a problem hiding this comment.
It feels like this is the wrong place to do this. I think we should do this in Bootstrap and then just pass a Path tmpDir to InternalSettingsPreparer#prepareEnvironment and make sure it's mandatory for all instances of environment that are not the single arg ctor. Btw. it feels like there are some sleeping bugs if this ctor is used in prod code.
There was a problem hiding this comment.
do we really need this or would the tmpFile be enough?
There was a problem hiding this comment.
The only place it's used is in the security manager setup so that the security manager allows access to java.io.tmpdir, not just the new sub-directory below it. The reason I thought this would be a good idea is that there could be a 3rd party Elasticsearch plugin that uses a Java library that assumes it can use java.io.tmpdir. At the moment this would work, but if we changed our security manager rules to disallow access to java.io.tmpdir then any such plugins would stop working.
I can achieve this differently so that java.io.tmpdir is not made available by the Environment class, but the security manager still allows access to it for the benefit of Java libraries used by 3rd party plugins. Or I can just leave the security manager setup as it was so that everything that wants a temp directory has to use Environment.tmpFile(). Do you have a preference?
There was a problem hiding this comment.
can't you just use the system property in the security manager instead?
This change ensures that the contents of the directory where Elasticsearch creates temporary files can only be listed by the user that Elasticsearch is running as. (The temporary files themselves are already read/write for only the user that Elasticsearch is running as. The extra protection this change adds is for the visibility of the file names.)
This reverts commit fdbc53b.
droberts195
force-pushed
the
private_temp_directory
branch
from
4b5b6a7 to
a0009f5
Compare
|
A different approach will be used that achieves the same result with far fewer code changes, hence closing this. |
2 participants
This change ensures that the contents of the directory where Elasticsearch
creates temporary files can only be listed by the user that Elasticsearch
is running as.
(The temporary files themselves are already read/write for only the user
that Elasticsearch is running as. The extra protection this change adds is
for the visibility of the file names.)
This is currently a work in progress as it causes the evil security tests to
fail.