HDFS-16945 . RBF: add RouterSecurityAuditLogger for router security manager #5468
+184
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
simbadzina
reviewed
Apr 4, 2023
Comment on lines
+38
to
+44
| private static final ThreadLocal<StringBuilder> STRING_BUILDER = | ||
| new ThreadLocal<StringBuilder>() { | ||
| @Override | ||
| protected StringBuilder initialValue() { | ||
| return new StringBuilder(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The Java8 construct is a bit more readable.
ThreadLocal.withInitial(() -> new StringBuilder());
| sb.append("ip=").append(addr).append("\t"); | ||
| sb.append("cmd=").append(cmd).append("\t"); | ||
|
|
||
| sb.append("\t").append("toeknId="); |
| } | ||
|
|
||
| @VisibleForTesting | ||
| public String creatAuditLog(boolean succeeded, String userName, |
| @@ -152,7 +160,8 @@ public Token<DelegationTokenIdentifier> getDelegationToken(Text renewer) | |||
| tokenId = dtId.toStringStable(); | |||
| success = true; | |||
| } finally { | |||
| logAuditEvent(success, operationName, tokenId); | |||
| logAuditEvent(success, user, Server.getRemoteIp(), operationName, | |||
There was a problem hiding this comment.
The remote address should be part of the CallerContext as well after HDFS-13248.
ayushtkn
requested changes
Apr 20, 2023
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.*; | ||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_DEFAULT; | ||
|
|
||
| public class RouterSecurityAuditLogger { |
There was a problem hiding this comment.
I think this extend any child class of AuditLogger?
|
|
||
| import java.net.InetAddress; | ||
|
|
||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.*; |
There was a problem hiding this comment.
Don't use *, expand the imports
Comment on lines
+38
to
+44
| private static final ThreadLocal<StringBuilder> STRING_BUILDER = | ||
| new ThreadLocal<StringBuilder>() { | ||
| @Override | ||
| protected StringBuilder initialValue() { | ||
| return new StringBuilder(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
May be
private static final ThreadLocal<StringBuilder> STRING_BUILDER =
ThreadLocal.withInitial(StringBuilder::new);
Comment on lines
+46
to
+47
| private int callerContextMaxLen; | ||
| private int callerSignatureMaxLen; |
Comment on lines
+84
to
+87
| if ( | ||
| callerContext != null && | ||
| callerContext.isContextValid()) { | ||
| sb.append("\t").append("callerContext="); |
There was a problem hiding this comment.
Formatting issue
if (callerContext != null && callerContext.isContextValid()) {
| @@ -186,7 +196,8 @@ public long renewDelegationToken(Token<DelegationTokenIdentifier> token) | |||
| tokenId = id.toStringStable(); | |||
| throw ace; | |||
| } finally { | |||
| logAuditEvent(success, operationName, tokenId); | |||
| logAuditEvent(success, user, Server.getRemoteIp(), operationName, | |||
There was a problem hiding this comment.
isn't user always an empty string here? Should have been user = getRemoteUser().getUserName(); ?
Comment on lines
+228
to
+229
| logAuditEvent(success, user, Server.getRemoteIp(), operationName, | ||
| CallerContext.getCurrent(), tokenId); |
There was a problem hiding this comment.
user is empty here?
There is a line above
String canceller = getRemoteUser().getUserName();
This guy is your user
|
|
||
| import java.io.IOException; | ||
|
|
||
| import static org.junit.Assert.*; |
|
|
||
| import static org.junit.Assert.*; | ||
|
|
||
| public class TestRouterSecurityAuditLogger { |
There was a problem hiding this comment.
Add a javadoc for the test class that It has tests related to RouterSecurityAuditLogs
| public class TestRouterSecurityAuditLogger { | ||
|
|
||
| @Test | ||
| public void testRouterSecurityAuditLog() throws IOException { |
There was a problem hiding this comment.
This is very basic and invoking directly the audit logger not invoking from the actual API. any scope to add tests which invokes audit logging via API invocation and you can grep the logs?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
HDFS-16945
we should add audit log for router security manager for token APIs. For examples,
How was this patch tested?
add testRouterSecurityAuditLog
For code changes:
add RouterSecurityAuditLogger