-
Notifications
You must be signed in to change notification settings - Fork 9k
HDFS-16945 . RBF: add RouterSecurityAuditLogger for router security manager #5468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.hadoop.hdfs.server.federation.router.security; | ||
|
||
import org.apache.hadoop.classification.VisibleForTesting; | ||
import org.apache.hadoop.conf.Configuration; | ||
import org.apache.hadoop.ipc.CallerContext; | ||
import org.apache.hadoop.ipc.Server; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import java.net.InetAddress; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this extend any child class of AuditLogger? |
||
|
||
public static final Logger AUDIT_LOG = LoggerFactory.getLogger( | ||
RouterSecurityManager.class.getName() + ".audit"); | ||
|
||
private static final ThreadLocal<StringBuilder> STRING_BUILDER = | ||
new ThreadLocal<StringBuilder>() { | ||
@Override | ||
protected StringBuilder initialValue() { | ||
return new StringBuilder(); | ||
} | ||
}; | ||
Comment on lines
+38
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Java8 construct is a bit more readable.
Comment on lines
+38
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be
|
||
|
||
private int callerContextMaxLen; | ||
private int callerSignatureMaxLen; | ||
Comment on lines
+46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be |
||
|
||
public RouterSecurityAuditLogger(Configuration conf) { | ||
callerContextMaxLen = conf.getInt( | ||
HADOOP_CALLER_CONTEXT_MAX_SIZE_KEY, | ||
HADOOP_CALLER_CONTEXT_MAX_SIZE_DEFAULT); | ||
callerSignatureMaxLen = conf.getInt( | ||
HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_KEY, | ||
HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_DEFAULT); | ||
} | ||
|
||
public void logAuditEvent(boolean succeeded, String userName, | ||
InetAddress addr, String cmd, | ||
CallerContext callerContext, String tokenId) { | ||
if (AUDIT_LOG.isDebugEnabled() || AUDIT_LOG.isInfoEnabled()) { | ||
logAuditMessage( | ||
creatAuditLog(succeeded, userName, addr, cmd, callerContext, | ||
tokenId)); | ||
} | ||
} | ||
|
||
@VisibleForTesting | ||
public String creatAuditLog(boolean succeeded, String userName, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo |
||
InetAddress addr, String cmd, | ||
CallerContext callerContext, String tokenId) { | ||
final StringBuilder sb = STRING_BUILDER.get(); | ||
sb.setLength(0); | ||
sb.append("allowed=").append(succeeded).append("\t"); | ||
sb.append("ugi=").append(userName).append("\t"); | ||
sb.append("ip=").append(addr).append("\t"); | ||
sb.append("cmd=").append(cmd).append("\t"); | ||
|
||
sb.append("\t").append("toeknId="); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo |
||
sb.append(tokenId); | ||
|
||
sb.append("\t").append("proto="); | ||
sb.append(Server.getProtocol()); | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should even check if
|
||
callerContext != null && | 8000||
callerContext.isContextValid()) { | ||
sb.append("\t").append("callerContext="); | ||
Comment on lines
+84
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formatting issue
|
||
if (callerContext.getContext().length() > callerContextMaxLen) { | ||
sb.append(callerContext.getContext().substring(0, | ||
callerContextMaxLen)); | ||
Comment on lines
+89
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can directly do
|
||
} else { | ||
sb.append(callerContext.getContext()); | ||
} | ||
if (callerContext.getSignature() != null && | ||
callerContext.getSignature().length > 0 && | ||
callerContext.getSignature().length <= callerSignatureMaxLen) { | ||
sb.append(":"); | ||
sb.append(new String(callerContext.getSignature(), | ||
CallerContext.SIGNATURE_ENCODING)); | ||
Comment on lines
+98
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats what
|
||
} | ||
} | ||
return sb.toString(); | ||
} | ||
|
||
private void logAuditMessage(String message) { | ||
AUDIT_LOG.info(message); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ | |
import org.apache.hadoop.hdfs.server.federation.router.RouterRpcServer; | ||
import org.apache.hadoop.hdfs.server.federation.router.Router; | ||
import org.apache.hadoop.io.Text; | ||
import org.apache.hadoop.ipc.CallerContext; | ||
import org.apache.hadoop.ipc.Server; | ||
import org.apache.hadoop.security.AccessControlException; | ||
import org.apache.hadoop.security.Credentials; | ||
import org.apache.hadoop.security.SecurityUtil; | ||
|
@@ -38,6 +40,7 @@ | |
import org.slf4j.LoggerFactory; | ||
|
||
import java.io.IOException; | ||
import java.net.InetAddress; | ||
import java.net.InetSocketAddress; | ||
|
||
/** | ||
|
@@ -51,6 +54,8 @@ public class RouterSecurityManager { | |
private AbstractDelegationTokenSecretManager<DelegationTokenIdentifier> | ||
dtSecretManager = null; | ||
|
||
private RouterSecurityAuditLogger auditLogger; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be |
||
|
||
public RouterSecurityManager(Configuration conf) throws IOException { | ||
AuthenticationMethod authMethodConfigured = | ||
SecurityUtil.getAuthenticationMethod(conf); | ||
|
@@ -62,12 +67,14 @@ public RouterSecurityManager(Configuration conf) throws IOException { | |
throw new IOException("Failed to create SecretManager"); | ||
} | ||
} | ||
auditLogger = new RouterSecurityAuditLogger(conf); | ||
} | ||
|
||
@VisibleForTesting | ||
public RouterSecurityManager(AbstractDelegationTokenSecretManager | ||
<DelegationTokenIdentifier> dtSecretManager) { | ||
this.dtSecretManager = dtSecretManager; | ||
auditLogger = new RouterSecurityAuditLogger(new Configuration()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doing new Configuration() is usually a bad idea, This method is used only in test, explore deprecating this and add a new method which takes in the conf and pass it from the test. In the new method if |
||
} | ||
|
||
public AbstractDelegationTokenSecretManager<DelegationTokenIdentifier> | ||
|
@@ -127,6 +134,7 @@ public Token<DelegationTokenIdentifier> getDelegationToken(Text renewer) | |
final String operationName = "getDelegationToken"; | ||
boolean success = false; | ||
String tokenId = ""; | ||
String user = ""; | ||
Token<DelegationTokenIdentifier> token; | ||
try { | ||
if (!isAllowedDelegationTokenOp()) { | ||
|
@@ -139,7 +147,7 @@ public Token<DelegationTokenIdentifier> getDelegationToken(Text renewer) | |
return null; | ||
} | ||
UserGroupInformation ugi = getRemoteUser(); | ||
String user = ugi.getUserName(); | ||
user = ugi.getUserName(); | ||
Text owner = new Text(user); | ||
Text realUser = null; | ||
if (ugi.getRealUser() != null) { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The remote address should be part of the CallerContext as well after HDFS-13248. |
||
CallerContext.getCurrent(), tokenId); | ||
} | ||
return token; | ||
} | ||
|
@@ -169,6 +178,7 @@ public long renewDelegationToken(Token<DelegationTokenIdentifier> token) | |
final String operationName = "renewDelegationToken"; | ||
boolean success = false; | ||
String tokenId = ""; | ||
String user = ""; | ||
long expiryTime; | ||
try { | ||
if (!isAllowedDelegationTokenOp()) { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't user always an empty string here? Should have been |
||
CallerContext.getCurrent(), tokenId); | ||
} | ||
return expiryTime; | ||
} | ||
|
@@ -201,6 +212,7 @@ public void cancelDelegationToken(Token<DelegationTokenIdentifier> token) | |
final String operationName = "cancelDelegationToken"; | ||
boolean success = false; | ||
String tokenId = ""; | ||
String user = ""; | ||
try { | ||
String canceller = getRemoteUser().getUserName(); | ||
LOG.info("Cancel request by " + canceller); | ||
|
@@ -213,7 +225,8 @@ public void cancelDelegationToken(Token<DelegationTokenIdentifier> token) | |
tokenId = id.toStringStable(); | ||
throw ace; | ||
} finally { | ||
logAuditEvent(success, operationName, tokenId); | ||
logAuditEvent(success, user, Server.getRemoteIp(), operationName, | ||
CallerContext.getCurrent(), tokenId); | ||
Comment on lines
+228
to
+229
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. user is empty here?
This guy is your user |
||
} | ||
} | ||
|
||
|
@@ -249,11 +262,10 @@ public void verifyToken(DelegationTokenIdentifier identifier, | |
* Log status of delegation token related operation. | ||
* Extend in future to use audit logger instead of local logging. | ||
*/ | ||
void logAuditEvent(boolean succeeded, String cmd, String tokenId) | ||
throws IOException { | ||
LOG.debug( | ||
"Operation:" + cmd + | ||
" Status:" + succeeded + | ||
" TokenId:" + tokenId); | ||
void logAuditEvent(boolean succeeded, String userName, | ||
InetAddress addr, String cmd, | ||
CallerContext callerContext, String tokenId) { | ||
auditLogger.logAuditEvent(succeeded, userName, addr, cmd, | ||
callerContext, tokenId); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.hadoop.hdfs.server.federation.security; | ||
|
||
import org.apache.hadoop.conf.Configuration; | ||
import org.apache.hadoop.hdfs.server.federation.router.security.RouterSecurityAuditLogger; | ||
import org.apache.hadoop.ipc.CallerContext; | ||
import org.apache.hadoop.ipc.Server; | ||
import org.junit.Test; | ||
|
||
import java.io.IOException; | ||
|
||
import static org.junit.Assert.*; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No grouping of imports |
||
|
||
public class TestRouterSecurityAuditLogger { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a javadoc for the test class that It has tests related to RouterSecurityAuditLogs |
||
|
||
@Test | ||
public void testRouterSecurityAuditLog() throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
RouterSecurityAuditLogger auditLogger = | ||
new RouterSecurityAuditLogger(new Configuration()); | ||
String fLog1 = auditLogger.creatAuditLog(false, "foo_user", | ||
Server.getRemoteIp(), "getDelegationToken", CallerContext.getCurrent(), | ||
"tokenId-123"); | ||
String expLog1 = | ||
"allowed=false\tugi=foo_user\tip=null\tcmd=getDelegationToken\t" + | ||
"\ttoeknId=tokenId-123\tproto=null"; | ||
assertEquals(expLog1, fLog1); | ||
|
||
String fLog2 = auditLogger.creatAuditLog(true, "foo2_user", | ||
Server.getRemoteIp(), "renewDelegationToken", CallerContext.getCurrent(), | ||
"tokenId-456"); | ||
String expLog2 = | ||
"allowed=true\tugi=foo2_user\tip=null\tcmd=renewDelegationToken\t" + | ||
"\ttoeknId=tokenId-456\tproto=null"; | ||
assertEquals(expLog2, fLog2); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use *, expand the imports