[KAFKA-10705]: Make state stores not readable by others#9583
[KAFKA-10705]: Make state stores not readable by others#9583ableegoldman merged 2 commits intoapache:trunkfrom
Conversation
|
@wcarlson5 @cadonna @ableegoldman for review |
There was a problem hiding this comment.
Is changing the baseDir sufficient?
There was a problem hiding this 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
There was a problem hiding this 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
lct45
force-pushed
the
rocksdb_security
branch
from
7688749 to
79f387c
Compare
lct45
changed the title
| } | ||
| final Path basePath = Paths.get(baseDir.getPath()); | ||
| final Path statePath = Paths.get(stateDir.getPath()); | ||
| final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---"); |
There was a problem hiding this comment.
Just wondering, why "read" and "execute" permissions for the group?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this comment.
No I think this looks ok, just wanted to ask
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. Reviewers: Bruno Cadonna <bruno@confluent.io>, Walker Carlson <wcarlson@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
|
Cherrypicked to 2.6, will wait for 2.7.0 release to finish up before merging to 2.7 |
|
@ableegoldman I see you merged this in 2.6. How critical is this fix? Do I need to spin a new RC? |
|
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 |
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. Reviewers: Bruno Cadonna <bruno@confluent.io>, Walker Carlson <wcarlson@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
|
Cherrypicked to 2.7 |
5 participants
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)