From 179ca3fa3ac25d901d1bd8a989a2146bb7384121 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 20 Dec 2023 17:33:12 +0100 Subject: [PATCH] Sanitize logs in JBossLoggingEventListenerProvider Closes #25078 Signed-off-by: rmartinc --- .../topics/keycloak/changes-23_0_5.adoc | 9 +++ .../upgrading/topics/keycloak/changes.adoc | 4 ++ .../java/org/keycloak/utils/StringUtil.java | 34 +++++++++ .../org/keycloak/utils/StringUtilTest.java | 38 ++++++++++ .../JBossLoggingEventListenerProvider.java | 72 +++++++++++-------- ...ssLoggingEventListenerProviderFactory.java | 23 +++++- 6 files changed, 149 insertions(+), 31 deletions(-) create mode 100644 docs/documentation/upgrading/topics/keycloak/changes-23_0_5.adoc create mode 100644 server-spi/src/test/java/org/keycloak/utils/StringUtilTest.java diff --git a/docs/documentation/upgrading/topics/keycloak/changes-23_0_5.adoc b/docs/documentation/upgrading/topics/keycloak/changes-23_0_5.adoc new file mode 100644 index 0000000000..8f5608c42b --- /dev/null +++ b/docs/documentation/upgrading/topics/keycloak/changes-23_0_5.adoc @@ -0,0 +1,9 @@ += Changes in jboss-logging event messages + +Because of issue https://github.com/keycloak/keycloak/issues/25078[#25078], the `jboss-logging` message values are now quoted (character `"` by default) and sanitized to prevent any line break. There are two new options in the provider (`spi-events-listener-jboss-logging-sanitize` and `spi-events-listener-jboss-logging-quotes`) that allow you to customize the new behavior. For example, to avoid both sanitization and quoting, the server can be started in this manner: + +``` +./kc.sh start --spi-events-listener-jboss-logging-sanitize=false --spi-events-listener-jboss-logging-quotes=none ... +``` + +More information about the options in the https://www.keycloak.org/server/all-provider-config#_jboss_logging[all provider configuration guide]. diff --git a/docs/documentation/upgrading/topics/keycloak/changes.adoc b/docs/documentation/upgrading/topics/keycloak/changes.adoc index 1ae2893e85..1c95495291 100644 --- a/docs/documentation/upgrading/topics/keycloak/changes.adoc +++ b/docs/documentation/upgrading/topics/keycloak/changes.adoc @@ -4,6 +4,10 @@ include::changes-24_0_0.adoc[leveloffset=3] +=== Migrating to 23.0.5 + +include::changes-23_0_5.adoc[leveloffset=3] + === Migrating to 23.0.4 include::changes-23_0_4.adoc[leveloffset=3] diff --git a/server-spi/src/main/java/org/keycloak/utils/StringUtil.java b/server-spi/src/main/java/org/keycloak/utils/StringUtil.java index 5879d425c3..86fda148cd 100644 --- a/server-spi/src/main/java/org/keycloak/utils/StringUtil.java +++ b/server-spi/src/main/java/org/keycloak/utils/StringUtil.java @@ -56,4 +56,38 @@ public class StringUtil { return options.toString(); } + /** + * Utility method that substitutes any isWhitespace char to common space ' ' or character 20. + * The idea is removing any weird space character in the string like \t, \n, \r. + * If quotes character is passed the quotes char is escaped to mark is not the end + * of the value (for example escaped \" if quotes char " is found in the string). + * + * @param str The string to normalize + * @param quotes The quotes to escape (for example " or '). It can be null. + * @return The string without weird whitespaces and quotes escaped + */ + public static String sanitizeSpacesAndQuotes(String str, Character quotes) { + // idea taken from commons-lang StringUtils.normalizeSpace + if (str == null || str.isEmpty()) { + return str; + } + StringBuilder sb = null; + for (int i = 0; i < str.length(); i++) { + final char actualChar = str.charAt(i); + if ((Character.isWhitespace(actualChar) && actualChar != ' ') || actualChar == 160) { + if (sb == null) { + sb = new StringBuilder(str.length() + 10).append(str.substring(0, i)); + } + sb.append(' '); + } else if (quotes != null && actualChar == quotes) { + if (sb == null) { + sb = new StringBuilder(str.length() + 10).append(str.substring(0, i)); + } + sb.append('\\').append(actualChar); + } else if (sb != null) { + sb.append(actualChar); + } + } + return sb == null? str : sb.toString(); + } } diff --git a/server-spi/src/test/java/org/keycloak/utils/StringUtilTest.java b/server-spi/src/test/java/org/keycloak/utils/StringUtilTest.java new file mode 100644 index 0000000000..5fef2c76d8 --- /dev/null +++ b/server-spi/src/test/java/org/keycloak/utils/StringUtilTest.java @@ -0,0 +1,38 @@ +/* + * Copyright 2023 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed 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.keycloak.utils; + +import org.junit.Assert; +import org.junit.Test; + +/** + * + * @author rmartinc + */ +public class StringUtilTest { + + @Test + public void testSanitize() { + Assert.assertEquals("test1 test2 test3", StringUtil.sanitizeSpacesAndQuotes("test1 test2 test3", null)); + Assert.assertEquals("test1 test2 test3", StringUtil.sanitizeSpacesAndQuotes("test1\ntest2\ttest3", null)); + Assert.assertEquals("test1 test2 test3 \"test4\"", StringUtil.sanitizeSpacesAndQuotes("test1\ntest2\ttest3\r\"test4\"", null)); + Assert.assertEquals("teswith\\\"quotes", StringUtil.sanitizeSpacesAndQuotes("teswith\"quotes", '"')); + Assert.assertEquals("test1 test2 test3 \\\"test4\\\"", StringUtil.sanitizeSpacesAndQuotes("test1\ntest2\ttest3\r\"test4\"", '"')); + Assert.assertEquals(" \\\"test", StringUtil.sanitizeSpacesAndQuotes("\n\"test", '"')); + Assert.assertEquals("\\\" test", StringUtil.sanitizeSpacesAndQuotes("\"\rtest", '"')); + } +} diff --git a/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProvider.java b/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProvider.java index b4e3197b5c..f8b7ec8e03 100755 --- a/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProvider.java +++ b/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProvider.java @@ -17,8 +17,8 @@ package org.keycloak.events.log; -import org.keycloak.common.util.StackUtil; import org.jboss.logging.Logger; +import org.keycloak.common.util.StackUtil; import org.keycloak.events.Event; import org.keycloak.events.EventListenerProvider; import org.keycloak.events.EventListenerTransaction; @@ -26,6 +26,7 @@ import org.keycloak.events.admin.AdminEvent; import org.keycloak.models.KeycloakContext; import org.keycloak.models.KeycloakSession; import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.utils.StringUtil; import jakarta.ws.rs.core.Cookie; import jakarta.ws.rs.core.HttpHeaders; @@ -41,14 +42,18 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider private final Logger logger; private final Logger.Level successLevel; private final Logger.Level errorLevel; + private final boolean sanitize; + private final Character quotes; private final EventListenerTransaction tx = new EventListenerTransaction(this::logAdminEvent, this::logEvent); - public JBossLoggingEventListenerProvider(KeycloakSession session, Logger logger, Logger.Level successLevel, Logger.Level errorLevel) { + public JBossLoggingEventListenerProvider(KeycloakSession session, Logger logger, + Logger.Level successLevel, Logger.Level errorLevel, Character quotes, boolean sanitize) { this.session = session; this.logger = logger; this.successLevel = successLevel; this.errorLevel = errorLevel; - + this.sanitize = sanitize; + this.quotes = quotes; this.session.getTransactionManager().enlistAfterCompletion(tx); } @@ -62,6 +67,19 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider tx.addAdminEvent(adminEvent, includeRepresentation); } + private void sanitize(StringBuilder sb, String str) { + if (quotes != null) { + sb.append(quotes); + } + if (sanitize) { + str = StringUtil.sanitizeSpacesAndQuotes(str, quotes); + } + sb.append(str); + if (quotes != null) { + sb.append(quotes); + } + } + private void logEvent(Event event) { Logger.Level level = event.getError() != null ? errorLevel : successLevel; @@ -69,42 +87,36 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider StringBuilder sb = new StringBuilder(); sb.append("type="); - sb.append(event.getType()); + sanitize(sb, event.getType().toString()); sb.append(", realmId="); - sb.append(event.getRealmId()); + sanitize(sb, event.getRealmId()); sb.append(", clientId="); - sb.append(event.getClientId()); + sanitize(sb, event.getClientId()); sb.append(", userId="); - sb.append(event.getUserId()); + sanitize(sb, event.getUserId()); sb.append(", ipAddress="); - sb.append(event.getIpAddress()); + sanitize(sb, event.getIpAddress()); if (event.getError() != null) { sb.append(", error="); - sb.append(event.getError()); + sanitize(sb, event.getError()); } if (event.getDetails() != null) { for (Map.Entry e : event.getDetails().entrySet()) { sb.append(", "); - sb.append(e.getKey()); - if (e.getValue() == null || e.getValue().indexOf(' ') == -1) { - sb.append("="); - sb.append(e.getValue()); - } else { - sb.append("='"); - sb.append(e.getValue()); - sb.append("'"); - } + sb.append(StringUtil.sanitizeSpacesAndQuotes(e.getKey(), null)); + sb.append("="); + sanitize(sb, e.getValue()); } } AuthenticationSessionModel authSession = session.getContext().getAuthenticationSession(); if(authSession!=null) { sb.append(", authSessionParentId="); - sb.append(authSession.getParentSession().getId()); + sanitize(sb, authSession.getParentSession().getId()); sb.append(", authSessionTabId="); - sb.append(authSession.getTabId()); + sanitize(sb, authSession.getTabId()); } if(logger.isTraceEnabled()) { @@ -126,23 +138,23 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider StringBuilder sb = new StringBuilder(); sb.append("operationType="); - sb.append(adminEvent.getOperationType()); + sanitize(sb, adminEvent.getOperationType().toString()); sb.append(", realmId="); - sb.append(adminEvent.getAuthDetails().getRealmId()); + sanitize(sb, adminEvent.getAuthDetails().getRealmId()); sb.append(", clientId="); - sb.append(adminEvent.getAuthDetails().getClientId()); + sanitize(sb, adminEvent.getAuthDetails().getClientId()); sb.append(", userId="); - sb.append(adminEvent.getAuthDetails().getUserId()); + sanitize(sb, adminEvent.getAuthDetails().getUserId()); sb.append(", ipAddress="); - sb.append(adminEvent.getAuthDetails().getIpAddress()); + sanitize(sb, adminEvent.getAuthDetails().getIpAddress()); sb.append(", resourceType="); - sb.append(adminEvent.getResourceTypeAsString()); + sanitize(sb, adminEvent.getResourceTypeAsString()); sb.append(", resourcePath="); - sb.append(adminEvent.getResourcePath()); + sanitize(sb, adminEvent.getResourcePath()); if (adminEvent.getError() != null) { sb.append(", error="); - sb.append(adminEvent.getError()); + sanitize(sb, adminEvent.getError()); } if(logger.isTraceEnabled()) { @@ -163,7 +175,7 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider HttpHeaders headers = context.getRequestHeaders(); if (uriInfo != null) { sb.append(", requestUri="); - sb.append(uriInfo.getRequestUri().toString()); + sanitize(sb, uriInfo.getRequestUri().toString()); } if (headers != null) { @@ -175,7 +187,7 @@ public class JBossLoggingEventListenerProvider implements EventListenerProvider } else { sb.append(", "); } - sb.append(e.getValue().toString()); + sb.append(StringUtil.sanitizeSpacesAndQuotes(e.getValue().toString(), null)); } sb.append("]"); } diff --git a/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProviderFactory.java b/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProviderFactory.java index a57d15672c..78b1ad3df9 100755 --- a/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProviderFactory.java +++ b/services/src/main/java/org/keycloak/events/log/JBossLoggingEventListenerProviderFactory.java @@ -40,16 +40,25 @@ public class JBossLoggingEventListenerProviderFactory implements EventListenerPr private Logger.Level successLevel; private Logger.Level errorLevel; + private boolean sanitize; + private Character quotes; @Override public EventListenerProvider create(KeycloakSession session) { - return new JBossLoggingEventListenerProvider(session, logger, successLevel, errorLevel); + return new JBossLoggingEventListenerProvider(session, logger, successLevel, errorLevel, quotes, sanitize); } @Override public void init(Config.Scope config) { successLevel = Logger.Level.valueOf(config.get("success-level", "debug").toUpperCase()); errorLevel = Logger.Level.valueOf(config.get("error-level", "warn").toUpperCase()); + sanitize = config.getBoolean("sanitize", true); + String quotesString = config.get("quotes", "\""); + if (!quotesString.equals("none") && quotesString.length() > 1) { + logger.warn("Invalid quotes configuration, it should be none or one character to use as quotes. Using default \" quotes"); + quotesString = "\""; + } + quotes = quotesString.equals("none")? null : quotesString.charAt(0); } @Override @@ -88,6 +97,18 @@ public class JBossLoggingEventListenerProviderFactory implements EventListenerPr .options(logLevels) .defaultValue("warn") .add() + .property() + .name("sanitize") + .type("boolean") + .helpText("If true the log messages are sanitized to avoid line breaks. If false messages are not sanitized.") + .defaultValue("true") + .add() + .property() + .name("quotes") + .type("string") + .helpText("The quotes to use for values, it should be one character like \" or '. Use \"none\" if quotes are not needed.") + .defaultValue("\"") + .add() .build(); } }