KAFKA-4943: Make /config/users with SCRAM credentials not world-readable#2733
KAFKA-4943: Make /config/users with SCRAM credentials not world-readable#2733rajinisivaram wants to merge 3 commits intoapache:trunkfrom
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
cc @junrao |
rajinisivaram
force-pushed
the
KAFKA-4943
branch
from
8ab3c5a to
d78da53
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Should it not be 0.10.2.1 since you set the target version of the JIRA to 0.10.2.1?
There was a problem hiding this comment.
Methods should start with lowercase. Yes, DefaultAcls was not following that convention. Maybe we can leave the deprecated one like that, but we should have the right capitalisation for the newly introduced ones.
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@rajinisivaram : Thanks for the patch. LGTM. Just a minor comment.
Not sure if this is critical enough for 0.10.2 since there is a workaround to change the permission in ZK manually.
There was a problem hiding this comment.
Is there a reason that null should be a sensitive path?
There was a problem hiding this comment.
No, null check was mainly to prevent NPE. I have changed to return false for null.
|
@junrao When it comes to security issues like this one, the default position should be to backport unless there's a reason not to. It's true that there is a workaround, but people may not realise that they have to do it until it's too late. |
|
I'd like to see it in 0.10.2 unless there is special complication in cherry-picking. |
rajinisivaram
force-pushed
the
KAFKA-4943
branch
from
d78da53 to
dbd2f9d
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| @deprecated("This is deprecated, use DefaultAcls(isSecure, path) which doesn't make sensitive data world readable", since = "0.10.2.1") | ||
| def DefaultAcls(isSecure: Boolean): java.util.List[ACL] = DefaultAcls(isSecure, "") | ||
|
|
||
| def DefaultAcls(isSecure: Boolean, path: String): java.util.List[ACL] = { |
There was a problem hiding this comment.
Shall we make this lowercase as well?
There was a problem hiding this comment.
@ijuma Thank you for the review. Have changed to lower case.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
LGTM, merging to trunk and 0.10.2.
Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ismael Juma, Jun Rao Closes #2733 from rajinisivaram/KAFKA-4943 (cherry picked from commit 67fc2a9) Signed-off-by: Gwen Shapira <cshapi@gmail.com>
|
@rajinisivaram @gwenshap Can you please check #2829 |
5 participants
No description provided.