Skip to content

Update the logging properties to opt-out of the prefix events #844 #845

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

Merged
merged 10 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions configuration/esapi/ESAPI.properties
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,10 @@ Logger.UserInfo=true
# Determines whether ESAPI should log the session id and client IP.
Logger.ClientInfo=true

# Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
# If all above Logger entries are set to false and LogIgnorePrefix is true, then the output would be the same like if no ESAPI was used
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm missing something here. Are you saying that for this new property to have the effect of omitting the event type, that ALL of these other Logger.* property names from lines 400 through 408 inclusive MUST be false? That seems overly complicated to me.

Logger.LogIgnorePrefix=true
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jeremiahjstacey about the non-intuitive property name. But also we can't have this new property be set in a manner that is going to change the default log output as it has previously existed since ESAPI 2.0. As project co-lead, I'm flat out not going to allow that. So whatever the default is set to, the logs have to be formatted as they were before. The only times we break backward compatibility is to:

  • If doing so is the only way to fix a security issue.
  • Something behavior of a dependency forces us to do so.
  • Without first deprecating something (generally for a 2 year period) before changing the behavior.
  • When changing major # in semantic versioning (e.g., going from ESAPI 1.x to 2.x or 2.x to 3.x).

We've already learned that people do not read the release notes. Instead, they post GitHub issues, send the team private emails, post on Stack Overflow, etc. and we are left trying to provide answers. So, no, the default logging behavior is not going to change on my watch. If people like yourself want it changed, you will have to tweak the value in your application's private copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, VERY important note: We can't assume that people will update their ESAPI.properties file. They in fact often don't until something goes wrong. So be sure to test it to make sure that it behaves as it does in 2.5.4.0 and earlier even if the completely omit that new property file. We can't be changing behavior and more importantly, we don't want to throw a ConfigurationException if that property is missing.


#===========================================================================
# ESAPI Intrusion Detection
#
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/esapi/PropNames.java
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public final class PropNames {
public static final String LOG_ENCODING_REQUIRED = "Logger.LogEncodingRequired";
public static final String LOG_APPLICATION_NAME = "Logger.LogApplicationName";
public static final String LOG_SERVER_IP = "Logger.LogServerIP";
public static final String LOG_IGNORE_PREFIX = "Logger.LogIgnorePrefix";
Copy link
Contributor

Choose a reason for hiding this comment

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

If the property name changes as per @jeremiahjstacey's suggestion, then please rename this variable in a corresponding fashion as well.


public static final String VALIDATION_PROPERTIES = "Validator.ConfigurationFile";
public static final String VALIDATION_PROPERTIES_MULTIVALUED = "Validator.ConfigurationFile.MultiValued";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,27 @@ public class EventTypeLogSupplier // implements Supplier<String>
{
/** EventType reference to supply log representation of. */
private final EventType eventType;
/** Whether to log or not the event type */
private boolean ignoreLogEventType = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you take @jeremiahjstacey's suggestion, maybe instead it makes more sense to call this keepLogEventType (or even just logEventType it that's not used and you don't think it would be confusing) and set it to true. Of course, you'd have to invert all your logic, but long term, it probably would result in more readability, especially if the property is renamed as @jeremiahjstacey recommends.


/**
* Ctr
*
* @param evtyp EventType reference to supply log representation for
* @param eventType EventType reference to supply log representation for
*/
public EventTypeLogSupplier(EventType evtyp) {
this.eventType = evtyp == null ? Logger.EVENT_UNSPECIFIED : evtyp;
public EventTypeLogSupplier(EventType eventType) {
this.eventType = eventType == null ? Logger.EVENT_UNSPECIFIED : eventType;
}

// @Override -- Uncomment when we switch to Java 8 as minimal baseline.
public String get() {
if (this.ignoreLogEventType) {
return "";
}
return eventType.toString();
}

public void setIgnoreLogEventType(boolean ignoreLogEventType) {
this.ignoreLogEventType = ignoreLogEventType;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public class LogPrefixAppender implements LogAppender {
private final boolean logApplicationName;
/** Application Name to record. */
private final String appName;
/** Whether or not to print the prefix. */
private final boolean ignoreLogPrefix;

/**
* Ctr.
Expand All @@ -51,11 +53,32 @@ public LogPrefixAppender(boolean logUserInfo, boolean logClientInfo, boolean log
this.logServerIp = logServerIp;
this.logApplicationName = logApplicationName;
this.appName = appName;
this.ignoreLogPrefix = false;
}

/**
* Ctr.
*
* @param logUserInfo Whether or not to record user information
* @param logClientInfo Whether or not to record client information
* @param logServerIp Whether or not to record server ip information
* @param logApplicationName Whether or not to record application name
* @param appName Application Name to record.
* @param ignoreLogPrefix Whether or not to print the prefix
*/
public LogPrefixAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName, boolean ignoreLogPrefix) {
this.logUserInfo = logUserInfo;
this.logClientInfo = logClientInfo;
this.logServerIp = logServerIp;
this.logApplicationName = logApplicationName;
this.appName = appName;
this.ignoreLogPrefix = ignoreLogPrefix;
}

@Override
public String appendTo(String logName, EventType eventType, String message) {
EventTypeLogSupplier eventTypeSupplier = new EventTypeLogSupplier(eventType);
eventTypeSupplier.setIgnoreLogEventType(this.ignoreLogPrefix);

UserInfoSupplier userInfoSupplier = new UserInfoSupplier();
userInfoSupplier.setLogUserInfo(logUserInfo);
Expand All @@ -66,6 +89,7 @@ public String appendTo(String logName, EventType eventType, String message) {
ServerInfoSupplier serverInfoSupplier = new ServerInfoSupplier(logName);
serverInfoSupplier.setLogServerIp(logServerIp);
serverInfoSupplier.setLogApplicationName(logApplicationName, appName);
serverInfoSupplier.setIgnoreLogName(ignoreLogPrefix);

String eventTypeMsg = eventTypeSupplier.get().trim();
String userInfoMsg = userInfoSupplier.get().trim();
Expand All @@ -81,8 +105,10 @@ public String appendTo(String logName, EventType eventType, String message) {
String[] optionalPrefixContent = new String[] {userInfoMsg + clientInfoMsg, serverInfoMsg};

StringBuilder logPrefix = new StringBuilder();
//EventType is always appended
logPrefix.append(eventTypeMsg);
//EventType is always appended (unless we specifically asked not to Log Prefix)
if (!this.ignoreLogPrefix) {
logPrefix.append(eventTypeMsg);
}

for (String element : optionalPrefixContent) {
if (!element.isEmpty()) {
Expand All @@ -91,6 +117,9 @@ public String appendTo(String logName, EventType eventType, String message) {
}
}

if (logPrefix.toString().isEmpty()) {
return message;
}
return String.format(RESULT_FORMAT, logPrefix.toString(), message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public class ServerInfoSupplier // implements Supplier<String>
private boolean logAppName = true;
/** The application name to log. */
private String applicationName = "";

/** Whether to log the Name */
private boolean ignoreLogName = false;
/** Reference to the associated logname/module name. */
private final String logName;

Expand All @@ -57,10 +58,19 @@ public String get() {
appInfo.append(request.getLocalAddr()).append(":").append(request.getLocalPort());
}
}
if (logAppName) {
appInfo.append("/").append(applicationName);

if (this.logAppName) {
if (this.applicationName != null && !this.applicationName.isEmpty()) {
appInfo.append("/").append(this.applicationName);
}
else if (this.applicationName == null) {
appInfo.append("/").append(this.applicationName);
}
}

if (!this.ignoreLogName) {
appInfo.append("/").append(logName);
}
appInfo.append("/").append(logName);

return appInfo.toString();
}
Expand All @@ -74,6 +84,15 @@ public void setLogServerIp(boolean log) {
this.logServerIP = log;
}

/**
* Specify whether the instance should record the prefix.
*
* @param ignoreLogName {@code true} to record
*/
public void setIgnoreLogName(boolean ignoreLogName) {
this.ignoreLogName = ignoreLogName;
}

/**
* Specify whether the instance should record the application name
*
Expand Down
18 changes: 17 additions & 1 deletion src/main/java/org/owasp/esapi/logging/java/JavaLogFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.owasp.esapi.PropNames.LOG_ENCODING_REQUIRED;
import static org.owasp.esapi.PropNames.LOG_SERVER_IP;
import static org.owasp.esapi.PropNames.LOG_USER_INFO;
import static org.owasp.esapi.PropNames.LOG_IGNORE_PREFIX;

import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -79,7 +80,8 @@ public class JavaLogFactory implements LogFactory {
boolean logApplicationName = ESAPI.securityConfiguration().getBooleanProp(LOG_APPLICATION_NAME);
String appName = ESAPI.securityConfiguration().getStringProp(APPLICATION_NAME);
boolean logServerIp = ESAPI.securityConfiguration().getBooleanProp(LOG_SERVER_IP);
JAVA_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
boolean logIgnorePrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_IGNORE_PREFIX);
JAVA_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName, logIgnorePrefix);

Map<Integer, JavaLogLevelHandler> levelLookup = new HashMap<>();
levelLookup.put(Logger.ALL, JavaLogLevelHandlers.ALWAYS);
Expand Down Expand Up @@ -144,6 +146,20 @@ public class JavaLogFactory implements LogFactory {
return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
}

/**
* Populates the default log appender for use in factory-created loggers.
* @param appName
* @param logApplicationName
* @param logServerIp
* @param logClientInfo
* @param logIgnorePrefix
*
* @return LogAppender instance.
*/
/*package*/ static LogAppender createLogAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName, boolean logIgnorePrefix) {
return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName, logIgnorePrefix);
}


@Override
public Logger getLogger(String moduleName) {
Expand Down
17 changes: 16 additions & 1 deletion src/main/java/org/owasp/esapi/logging/slf4j/Slf4JLogFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import static org.owasp.esapi.PropNames.LOG_APPLICATION_NAME;
import static org.owasp.esapi.PropNames.APPLICATION_NAME;
import static org.owasp.esapi.PropNames.LOG_SERVER_IP;
import static org.owasp.esapi.PropNames.LOG_IGNORE_PREFIX;
import org.slf4j.LoggerFactory;
/**
* LogFactory implementation which creates SLF4J supporting Loggers.
Expand Down Expand Up @@ -69,7 +70,8 @@ public class Slf4JLogFactory implements LogFactory {
boolean logApplicationName = ESAPI.securityConfiguration().getBooleanProp(LOG_APPLICATION_NAME);
String appName = ESAPI.securityConfiguration().getStringProp(APPLICATION_NAME);
boolean logServerIp = ESAPI.securityConfiguration().getBooleanProp(LOG_SERVER_IP);
SLF4J_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
boolean logIgnorePrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_IGNORE_PREFIX);
SLF4J_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName, logIgnorePrefix);

Map<Integer, Slf4JLogLevelHandler> levelLookup = new HashMap<>();
levelLookup.put(Logger.ALL, Slf4JLogLevelHandlers.TRACE);
Expand Down Expand Up @@ -114,6 +116,19 @@ public class Slf4JLogFactory implements LogFactory {
return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
}

/**
* Populates the default log appender for use in factory-created loggers.
* @param appName
* @param logApplicationName
* @param logServerIp
* @param logClientInfo
* @param logIgnorePrefix
*
* @return LogAppender instance.
*/
/*package*/ static LogAppender createLogAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName, boolean logIgnorePrefix) {
return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName, logIgnorePrefix);
}

@Override
public Logger getLogger(String moduleName) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.owasp.esapi.logging.appender;

import static org.junit.Assert.assertEquals;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.owasp.esapi.Logger;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;

@RunWith(Parameterized.class)
public class EventTypeLogSupplierIgnoreEventTypeTest {

@Parameterized.Parameters (name="{0} -> {1}")
public static Collection<Object[]> assembleTests() {
List<Object[]> paramSets = new ArrayList<>();
paramSets.add(new Object[] {Logger.EVENT_FAILURE,""});
paramSets.add(new Object[] {Logger.EVENT_SUCCESS,""});
paramSets.add(new Object[] {Logger.EVENT_UNSPECIFIED,""});
paramSets.add(new Object[] {Logger.SECURITY_AUDIT,""});
paramSets.add(new Object[] {Logger.SECURITY_FAILURE,""});
paramSets.add(new Object[] {Logger.SECURITY_SUCCESS,""});
paramSets.add(new Object[] {null, ""});

return paramSets;
}

private final Logger.EventType eventType;
private final String expectedResult;

public EventTypeLogSupplierIgnoreEventTypeTest(Logger.EventType eventType, String result) {
this.eventType = eventType;
this.expectedResult = result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add blank line here for improved readability.

@Test
public void testEventTypeLogIgnoreEventType() {
EventTypeLogSupplier supplier = new EventTypeLogSupplier(eventType);
supplier.setIgnoreLogEventType(true);
assertEquals(expectedResult, supplier.get());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public class LogPrefixAppenderTest {
private String testLogMessage = testName.getMethodName() + "-MESSAGE";
private String testApplicationName = testName.getMethodName() + "-APPLICATION_NAME";
private EventType testEventType = Logger.EVENT_UNSPECIFIED;
private boolean testIgnorePrefix = true;

private EventTypeLogSupplier etlsSpy;
private ClientInfoSupplier cisSpy;
Expand Down Expand Up @@ -145,7 +146,6 @@ public void testLogContentWhenUserInfoEmptyAndClientInfoEmptyAndServerInfoEmpty(
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, "[EVENT_TYPE]");
}


private void runTest(String typeResult, String userResult, String clientResult, String serverResult, String exResult) throws Exception{
when(etlsSpy.get()).thenReturn(typeResult);
when(uisSpy.get()).thenReturn(userResult);
Expand All @@ -163,4 +163,57 @@ private void runTest(String typeResult, String userResult, String clientResult,

assertEquals(exResult + " " + testName.getMethodName() + "-MESSAGE", result);
}

@Test
public void testLogContentWhenServerInfoEmptyAndIgnoreLogPrefix() throws Exception {
runTestWithLogPrefixIgnore(ETL_RESULT, UIS_RESULT, CIS_RESULT, EMPTY_RESULT, true, "[ USER_INFO:CLIENT_INFO]");
}

@Test
public void testLogContentWhenUserInfoEmptyAndServerInfoEmptyAndIgnoreLogPrefix() throws Exception {
runTestWithLogPrefixIgnore(ETL_RESULT, EMPTY_RESULT, CIS_RESULT, EMPTY_RESULT, true, "[ CLIENT_INFO]");
}

@Test
public void testLogContentWhenUserInfoEmptyAndClientInfoEmptyAndIgnoreLogPrefix() throws Exception {
runTestWithLogPrefixIgnore(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, SIS_RESULT, true, "[ -> SERVER_INFO]");
}

@Test
public void testLogContentWhenClientInfoEmptyAndServerInfoEmptyAndIgnoreLogPrefix() throws Exception {
runTestWithLogPrefixIgnore(ETL_RESULT, UIS_RESULT, EMPTY_RESULT, EMPTY_RESULT, true, "[ USER_INFO]");
}

@Test
public void testLogContentWhenUserInfoEmptyAndClientInfoEmptyAndServerInfoEmptyAndIgnoreLogPrefix() throws Exception {
runTestWithLogPrefixIgnore(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, true, "");
}

private void runTestWithLogPrefixIgnore(String typeResult, String userResult, String clientResult, String serverResult, boolean ignoreLogPrefix, String exResult) throws Exception{
etlsSpy.setIgnoreLogEventType(ignoreLogPrefix);
when(etlsSpy.get()).thenReturn(typeResult);

when(uisSpy.get()).thenReturn(userResult);
when(cisSpy.get()).thenReturn(clientResult);

sisSpy.setIgnoreLogName(ignoreLogPrefix);
when(sisSpy.get()).thenReturn(serverResult);

whenNew(EventTypeLogSupplier.class).withArguments(testEventType).thenReturn(etlsSpy);
whenNew(UserInfoSupplier.class).withNoArguments().thenReturn(uisSpy);
whenNew(ClientInfoSupplier.class).withNoArguments().thenReturn(cisSpy);
whenNew(ServerInfoSupplier.class).withArguments(testLoggerName).thenReturn(sisSpy);

//Since everything is mocked these booleans don't much matter aside from the later verifies
LogPrefixAppender lpa = new LogPrefixAppender(false, false, false, false, null, true);
String result = lpa.appendTo(testLoggerName, testEventType, testLogMessage);

if (exResult.isEmpty()) {
assertEquals( testName.getMethodName() + "-MESSAGE", result);
}
else {
assertEquals(exResult + " " + testName.getMethodName() + "-MESSAGE", result);
}
}

}
Loading