ableegoldman commented on a change in pull request #9947:
URL: https://github.com/apache/kafka/pull/9947#discussion_r562317985
##########
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -109,16 +109,27 @@ public StateDirectory(final StreamsConfig config, final
Time time, final boolean
log.warn("Using /tmp directory in the state.dir property can
cause failures with writing the checkpoint file" +
" due to the fact that this directory can be cleared by
the OS");
}
-
// change the dir permission to "rwxr-x---" to avoid world readable
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
+ configurePermissions(Paths.get(baseDir.getPath()));
+ configurePermissions(Paths.get(stateDir.getPath()));
+ }
+ }
+
+ private void configurePermissions(final Path path) {
+ if
(path.getFileSystem().supportedFileAttributeViews().contains("posix")) {
final Set<PosixFilePermission> perms =
PosixFilePermissions.fromString("rwxr-x---");
try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
+ Files.setPosixFilePermissions(path, perms);
} catch (final IOException e) {
- log.error("Error changing permissions for the state or base
directory {} ", stateDir.getPath(), e);
+ log.error("Error changing permissions for the directory {} ",
path, e);
+ }
+ } else {
+ final File file = path.toFile();
+ boolean set = file.setReadable(true, false);
+ set &= file.setWritable(true, true);
Review comment:
If you only have `file.setWritable(true, true)` then the directory will
still be writeable by non-users, I assume? I actually don't know the details
of the `File#setXXX` methods -- but we don't want it to be writeable by just
anyone. Should we instead do something like
```suggestion
set &= file.setWritable(false) && file.setWritable(true, true);
```
##########
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -109,16 +109,27 @@ public StateDirectory(final StreamsConfig config, final
Time time, final boolean
log.warn("Using /tmp directory in the state.dir property can
cause failures with writing the checkpoint file" +
" due to the fact that this directory can be cleared by
the OS");
}
-
// change the dir permission to "rwxr-x---" to avoid world readable
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
+ configurePermissions(Paths.get(baseDir.getPath()));
+ configurePermissions(Paths.get(stateDir.getPath()));
+ }
+ }
+
+ private void configurePermissions(final Path path) {
+ if
(path.getFileSystem().supportedFileAttributeViews().contains("posix")) {
final Set<PosixFilePermission> perms =
PosixFilePermissions.fromString("rwxr-x---");
try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
+ Files.setPosixFilePermissions(path, perms);
} catch (final IOException e) {
- log.error("Error changing permissions for the state or base
directory {} ", stateDir.getPath(), e);
+ log.error("Error changing permissions for the directory {} ",
path, e);
+ }
+ } else {
+ final File file = path.toFile();
+ boolean set = file.setReadable(true, false);
Review comment:
I think we actually want it to be readable _only_ by the user, and
explicitly restrict permissions for all other users. The patch which originally
broke things for Windows users was trying to tighten up the security in exactly
this way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
