8000 HDFS-16945 . RBF: add RouterSecurityAuditLogger for router security manager by Neilxzn · Pull Request #5468 · apache/hadoop · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8000
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.*;
Copy link
Member

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

import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_DEFAULT;

public class RouterSecurityAuditLogger {
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Java8 construct is a bit more readable.

ThreadLocal.withInitial(() -> new StringBuilder());

Comment on lines +38 to +44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be

  private static final ThreadLocal<StringBuilder> STRING_BUILDER =
      ThreadLocal.withInitial(StringBuilder::new);


private int callerContextMaxLen;
private int callerSignatureMaxLen;
Comment on lines +46 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be final?


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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo createAuditLog

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=");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo tokenId

sb.append(tokenId);

sb.append("\t").append("proto=");
sb.append(Server.getProtocol());
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should even check if CallerContext is enabled or not, and log only then
The conf is

hadoop.caller.context.enabled

callerContext != null &&
callerContext.isContextValid()) {
sb.append("\t").append("callerContext=");
Comment on lines +84 to +87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting issue

    if (callerContext != null && callerContext.isContextValid()) {

if (callerContext.getContext().length() > callerContextMaxLen) {
sb.append(callerContext.getContext().substring(0,
callerContextMaxLen));
Comment on lines +89 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can directly do

        sb.append(callerContext.getContext(), 0, callerContextMaxLen);

} 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats what FSNamesystemAuditLogger does, should be inline with that

                .append(escapeJava(new String(callerContext.getSignature(),
                CallerContext.SIGNATURE_ENCODING)));

}
}
return sb.toString();
}

private void logAuditMessage(String message) {
AUDIT_LOG.info(message);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,6 +40,7 @@
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;

/**
Expand All @@ -51,6 +54,8 @@ public class RouterSecurityManager {
private AbstractDelegationTokenSecretManager<DelegationTokenIdentifier>
dtSecretManager = null;

private RouterSecurityAuditLogger auditLogger;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be final?


public RouterSecurityManager(Configuration conf) throws IOException {
AuthenticationMethod authMethodConfigured =
SecurityUtil.getAuthenticationMethod(conf);
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The 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 conf is null don't initialise the auditLogger instead just put a warn log that RouterAuditLogger is ``disabled and something like that

}

public AbstractDelegationTokenSecretManager<DelegationTokenIdentifier>
Expand Down Expand Up @@ -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()) {
Expand All @@ -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) {
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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;
}
Expand All @@ -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()) {
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't user always an empty string here? Should have been user = getRemoteUser().getUserName(); ?

CallerContext.getCurrent(), tokenId);
}
return expiryTime;
}
Expand All @@ -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);
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user is empty here?
There is a line above

 String canceller = getRemoteUser().getUserName();

This guy is your user

}
}

Expand Down Expand Up @@ -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.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No grouping of imports


public class TestRouterSecurityAuditLogger {
Copy link
Member

Choose a reason for hiding this comment

The 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 {
Copy link
Member

Choose a reason for hiding this comment

The 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);
}
}
0