You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/11/10 16:27:10 UTC

[GitHub] [kafka] lct45 opened a new pull request #9583: MINOR: Make state stores not readable by others

lct45 opened a new pull request #9583:
URL: https://github.com/apache/kafka/pull/9583


   Change permissions on the folders for the state store so they're no readable or writable by "others", but still accessible by owner and group members.
   
   ### Committer Checklist (excluded from commit message)
   - [x] Verify design and implementation 
   - [x] Verify test coverage and CI build status
   - [x] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] mimaison commented on pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-727189048


   @ableegoldman I see you merged this in 2.6. How critical is this fix? Do I need to spin a new RC?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520723720



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,13 @@ 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");
         }
+        final Path path = Paths.get(baseDir.getPath());

Review comment:
       It should be, the `stateDir` is inside of the `baseDir` so the permissions should apply there as well. I guess if we wanted to be _more_ selective we could apply permissions to `stateDir` and not `baseDir` but from what I see it doesn't make a big difference. In the test though, there is an `appDir` that had broader permissions even with the changes to `baseDir` so maybe we _do_ need to change permissions to both `baseDir` and `stateDir`




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r523130136



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       >  I ran the streams test without having execute for group and I got an exception in StreamTableJoinTopologyOptimizationIntegrationTest test
   
   I've noticed that test is actually pretty flaky at the moment, IIRC it's been due to a `Directory not empty` exception). So the failure you saw might not be related to this PR. I'm pretty sure it's write permissions, not execute, that let you delete files within a directory. Execute just lets you `cd` in and traverse the directory. So I guess read+execute does make sense here, and probably we wouldn't want anyone other than the owner to be able to clear the directory anyway




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] wcarlson5 removed a comment on pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
wcarlson5 removed a comment on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-724976610


   retest this please


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-757068919


   Cherrypicked to 2.7


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r523055221



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       I agree with what Bruno said for the reading. I think allowing execute for group members isn't a security risk, since it's still somewhat contained. I ran the streams test without having execute for group and I got an exception in `StreamTableJoinTopologyOptimizationIntegrationTest` test `shouldDoStreamTableJoinWithDifferentNumberOfPartitions`. It looks like the integration test utils weren't able to clear the directory because groups didn't have execute authorization. Since it doesn't seem like allowing groups to execute would constitute a security risk, it seems like this is a good thing to keep for testing capabilities




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520719992



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -30,11 +30,16 @@
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
 import java.nio.channels.OverlappingFileLockException;
-import java.nio.file.NoSuchFileException;

Review comment:
       We did not




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] cadonna commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r522770608



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       I thought it would be handy to be able to log into a node to debug state store issues, e.g., with some RocksDB command line tools, with a user that is able to read not able to write, i.e., is in the group but it is not the owner.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] wcarlson5 commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
wcarlson5 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520710986



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,13 @@ 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");
         }
+        final Path path = Paths.get(baseDir.getPath());

Review comment:
       Is changing the `baseDir` sufficient?

##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -30,11 +30,16 @@
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
 import java.nio.channels.OverlappingFileLockException;
-import java.nio.file.NoSuchFileException;

Review comment:
       Did we need to move this?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] wcarlson5 commented on pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
wcarlson5 commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-724976610


   retest this please


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-724813681


   @wcarlson5 @cadonna @ableegoldman for review


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] cadonna commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520860033



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -106,6 +110,27 @@ public void shouldCreateBaseDirectory() {
         assertTrue(appDir.isDirectory());
     }
 
+    @Test
+    public void shouldHaveSecurePermissions() {
+        final Set<PosixFilePermission> expectedPermissions = EnumSet.of(
+            PosixFilePermission.OWNER_EXECUTE,
+            PosixFilePermission.GROUP_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.GROUP_EXECUTE,
+            PosixFilePermission.OWNER_READ);
+
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Path basePath = Paths.get(appDir.getPath());
+        try {
+            final Set<PosixFilePermission> baseFilePermissions = Files.getPosixFilePermissions(statePath);
+            final Set<PosixFilePermission> appFilePermissions = Files.getPosixFilePermissions(basePath);
+            assertEquals(expectedPermissions, baseFilePermissions);
+            assertEquals(expectedPermissions, appFilePermissions);

Review comment:
       Recently, we prefer to use `assertThat()` instead of `assertEquals()`. 

##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -106,6 +110,27 @@ public void shouldCreateBaseDirectory() {
         assertTrue(appDir.isDirectory());
     }
 
+    @Test
+    public void shouldHaveSecurePermissions() {
+        final Set<PosixFilePermission> expectedPermissions = EnumSet.of(
+            PosixFilePermission.OWNER_EXECUTE,
+            PosixFilePermission.GROUP_READ,
+            PosixFilePermission.OWNER_WRITE,
+            PosixFilePermission.GROUP_EXECUTE,
+            PosixFilePermission.OWNER_READ);
+
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Path basePath = Paths.get(appDir.getPath());
+        try {
+            final Set<PosixFilePermission> baseFilePermissions = Files.getPosixFilePermissions(statePath);
+            final Set<PosixFilePermission> appFilePermissions = Files.getPosixFilePermissions(basePath);
+            assertEquals(expectedPermissions, baseFilePermissions);
+            assertEquals(expectedPermissions, appFilePermissions);
+        } catch (final IOException e) {
+            // okay

Review comment:
       If we swallow the exception here, and the test always throws an IO exception, we will never notice. I guess it would be better to use `fail()` with a message. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman merged pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman merged pull request #9583:
URL: https://github.com/apache/kafka/pull/9583


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-728523131


   I don't think so. It would be nice to have if you happen to end up cutting a new RC, but I wouldn't delay the ongoing release over this


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520723720



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,13 @@ 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");
         }
+        final Path path = Paths.get(baseDir.getPath());

Review comment:
       It should be, the `stateDir` is inside of the `baseDir` so the permissions should apply there as well. I guess if we wanted to be _more_ selective we could apply permissions to `stateDir` and not `baseDir` but from what I see it doesn't make a big difference




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-757068919


   Cherrypicked to 2.7


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r522514992



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       Just wondering, why "read" and "execute" permissions for the group?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r523142600



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       No I think this looks ok, just wanted to ask




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] lct45 commented on a change in pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
lct45 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r523137877



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,15 @@ 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");
         }
+        final Path basePath = Paths.get(baseDir.getPath());
+        final Path statePath = Paths.get(stateDir.getPath());
+        final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");

Review comment:
       Hmm yeah that was the exception so it probably wasn't related to this PR. I would err on the side of leaving those permissions but can take them out if you think that's better




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on pull request #9583: [KAFKA-10705]: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#issuecomment-726978481


   Cherrypicked to 2.6, will wait for 2.7.0 release to finish up before merging to 2.7


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] wcarlson5 commented on a change in pull request #9583: MINOR: Make state stores not readable by others

Posted by GitBox <gi...@apache.org>.
wcarlson5 commented on a change in pull request #9583:
URL: https://github.com/apache/kafka/pull/9583#discussion_r520733454



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -102,6 +107,13 @@ 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");
         }
+        final Path path = Paths.get(baseDir.getPath());

Review comment:
       I'm not sure off the top of my head I just wanted to verify, but its probably best to be overly specific. LGTM




----------------------------------------------------------------
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:
users@infra.apache.org