Skip to content

Commit

Permalink
Merge pull request #6010 from yersan/WFCORE-6825-WFCORE-6833
Browse files Browse the repository at this point in the history
[CVE-2024-4029] [WFCORE-6825] [WFCORE-6833] Options to control resources used for incomming connections and XNIO upgrade
  • Loading branch information
yersan committed May 28, 2024
2 parents 0f720ec + 870e48c commit c8300b3
Show file tree
Hide file tree
Showing 4 changed files with 247 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.xnio.ssl.SslConnection;
import org.xnio.ssl.XnioSsl;

import io.undertow.UndertowOptions;
import io.undertow.protocols.ssl.UndertowXnioSsl;
import io.undertow.security.handlers.AuthenticationCallHandler;
import io.undertow.security.handlers.AuthenticationConstraintHandler;
Expand Down Expand Up @@ -111,6 +112,9 @@ public interface PathRemapper {
private final HttpAuthenticationFactory httpAuthenticationFactory;
private final ExtensionHandlers extensionHandlers;
private final Executor managementExecutor;
private final Integer backlog;
private final Integer connectionHighWater;
private final Integer connectionLowWater;

private ManagementHttpServer(HttpOpenListener openListener, Builder builder, SSLContext sslContext,
SslClientAuthMode sslClientAuthMode, ExtensionHandlers extensionExtensionHandlers) {
Expand All @@ -123,6 +127,9 @@ private ManagementHttpServer(HttpOpenListener openListener, Builder builder, SSL
this.httpAuthenticationFactory = builder.httpAuthenticationFactory;
this.extensionHandlers = extensionExtensionHandlers;
this.managementExecutor = builder.executor;
this.backlog = builder.backlog;
this.connectionHighWater = builder.connectionHighWater;
this.connectionLowWater = builder.connectionLowWater;
}

public void start() {
Expand All @@ -131,6 +138,16 @@ public void start() {
OptionMap.Builder serverOptionsBuilder = OptionMap.builder()
.set(Options.TCP_NODELAY, true)
.set(Options.REUSE_ADDRESSES, true);
if (backlog != null) {
serverOptionsBuilder.set(Options.BACKLOG, backlog);
}
if (connectionHighWater != null) {
serverOptionsBuilder.set(Options.CONNECTION_HIGH_WATER, connectionHighWater);
}
if (connectionLowWater != null) {
serverOptionsBuilder.set(Options.CONNECTION_LOW_WATER, connectionLowWater);
}

ChannelListener acceptListener = ChannelListeners.openListenerAdapter(openListener);
if (httpAddress != null) {
normalServer = worker.createStreamConnectionServer(httpAddress, acceptListener, serverOptionsBuilder.getMap());
Expand Down Expand Up @@ -235,7 +252,14 @@ private static ManagementHttpServer create(Builder builder) {
}
}

HttpOpenListener openListener = new HttpOpenListener(bufferPool);
final OptionMap undertowOptions;
if (builder.noRequestTimeout != null) {
undertowOptions = OptionMap.create(UndertowOptions.NO_REQUEST_TIMEOUT, builder.noRequestTimeout);
} else {
undertowOptions = OptionMap.EMPTY;
}

HttpOpenListener openListener = new HttpOpenListener(bufferPool, undertowOptions);

int secureRedirectPort = builder.secureBindAddress != null ? builder.secureBindAddress.getPort() : -1;
// WFLY-2870 -- redirect not supported if bindAddress and secureBindAddress are using different InetAddress
Expand Down Expand Up @@ -455,6 +479,10 @@ public static class Builder {
private Executor executor;
private Map<String, List<Header>> constantHeaders;
private ConsoleAvailability consoleAvailability;
private Integer backlog;
private Integer connectionHighWater;
private Integer connectionLowWater;
private Integer noRequestTimeout;

private Builder() {
}
Expand Down Expand Up @@ -558,6 +586,50 @@ public Builder setConstantHeaders(Map<String, List<Header>> constantHeaders) {
return this;
}

/**
* Set the TCP backlog for each server socket.
*/
public Builder setBacklog(Integer backlog) {
assertNotBuilt();
this.backlog = backlog;

return this;
}

/**
* Set the high water mark for the number of connections that can be accepted before the server will stop accepting new connections.
*
* This is set on each server socket.
*/
public Builder setConnectionHighWater(Integer connectionHighWater) {
assertNotBuilt();
this.connectionHighWater = connectionHighWater;

return this;
}

/**
* Set the low water mark for the number of connections that can be accepted before the server will start accepting new connections.
*
* This is set on each server socket.
*/
public Builder setConnectionLowWater(Integer connectionLowWater) {
assertNotBuilt();
this.connectionLowWater = connectionLowWater;

return this;
}

/**
* Set the no request timeout for open connections.
*/
public Builder setNoRequestTimeout(Integer noRequestTimeout) {
assertNotBuilt();
this.noRequestTimeout = noRequestTimeout;

return this;
}

public ManagementHttpServer build() {
assertNotBuilt();

Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@
<version.org.jboss.staxmapper>1.5.0.Final</version.org.jboss.staxmapper>
<version.org.jboss.stdio>1.1.0.Final</version.org.jboss.stdio>
<version.org.jboss.threads>2.4.0.Final</version.org.jboss.threads>
<version.org.jboss.xnio>3.8.14.Final</version.org.jboss.xnio>
<version.org.jboss.xnio>3.8.15.Final</version.org.jboss.xnio>
<version.org.jboss.xnio.xnio-api>${version.org.jboss.xnio}</version.org.jboss.xnio.xnio-api>
<version.org.jboss.xnio.xnio-nio>${version.org.jboss.xnio}</version.org.jboss.xnio.xnio-nio>
<version.org.mock-server.mockserver-netty>5.8.1</version.org.mock-server.mockserver-netty>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ public class UndertowHttpManagementService implements Service<HttpManagement> {
public static final String JBOSS_REMOTING = "jboss-remoting";
public static final String MANAGEMENT_ENDPOINT = "management-endpoint";

private static final String PROPERTY_BASE = "org.wildfly.management.";
private static final String BACKLOG_PROPERTY = PROPERTY_BASE + "backlog";
private static final String CONNECTION_HIGH_WATER_PROPERTY = PROPERTY_BASE + "connection-high-water";
private static final String CONNECTION_LOW_WATER_PROPERTY = PROPERTY_BASE + "connection-low-water";
private static final String NO_REQUEST_TIMEOUT_PROPERTY = PROPERTY_BASE + "no-request-timeout";

private final Consumer<HttpManagement> httpManagementConsumer;
private final Supplier<ListenerRegistry> listenerRegistrySupplier;
private final Supplier<ModelController> modelControllerSupplier;
Expand Down Expand Up @@ -339,6 +345,11 @@ public synchronized void start(final StartContext context) throws StartException
}
}

final Integer backlog = Integer.getInteger(BACKLOG_PROPERTY, 50);
final Integer connectionHighWater = Integer.getInteger(CONNECTION_HIGH_WATER_PROPERTY, 100);
final Integer connectionLowWater = Integer.getInteger(CONNECTION_LOW_WATER_PROPERTY, 75);
final Integer noRequestTimeout = Integer.getInteger(NO_REQUEST_TIMEOUT_PROPERTY, 60000);

try {
ManagementHttpServer.Builder serverManagementBuilder = ManagementHttpServer.builder()
.setBindAddress(bindAddress)
Expand All @@ -353,7 +364,12 @@ public synchronized void start(final StartContext context) throws StartException
.setWorker(workerSupplier.get())
.setExecutor(executorSupplier.get())
.setConstantHeaders(constantHeaders)
.setConsoleAvailability(consoleAvailability);
.setConsoleAvailability(consoleAvailability)
.setBacklog(backlog)
.setConnectionHighWater(connectionHighWater)
.setConnectionLowWater(connectionLowWater)
.setNoRequestTimeout(noRequestTimeout)
;

if (virtualSecurityDomainSupplier != null && virtualMechanismFactorySupplier != null) {
// use a virtual http authentication factory instead
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright The WildFly Authors
* SPDX-License-Identifier: Apache-2.0
*/

package org.wildfly.core.test.standalone.mgmt;

import static org.jboss.as.test.shared.TimeoutUtil.adjust;
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
import java.util.logging.Level;
import java.util.logging.Logger;

import jakarta.inject.Inject;

import org.jboss.as.test.integration.management.util.CLIWrapper;
import org.jboss.as.test.shared.TestSuiteEnvironment;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.wildfly.common.function.ExceptionRunnable;
import org.wildfly.core.testrunner.ServerControl;
import org.wildfly.core.testrunner.ServerController;
import org.wildfly.core.testrunner.WildFlyRunner;

/**
* Test case to test resource limits and clean up of management interface connections.
*
* @author <a href="mailto:darran.lofthouse@jboss.com">Darran Lofthouse</a>
*/
@RunWith(WildFlyRunner.class)
@ServerControl(manual = true)
public class ManagementInterfaceResourcesTestCase {
private static final Logger LOG = Logger.getLogger(ManagementInterfaceResourcesTestCase.class.getName());

private static final String BACKLOG_PROPERTY = "org.wildfly.management.backlog";
private static final String CONNECTION_HIGH_WATER_PROPERTY = "org.wildfly.management.connection-high-water";
private static final String CONNECTION_LOW_WATER_PROPERTY = "org.wildfly.management.connection-low-water";
private static final String NO_REQUEST_TIMEOUT_PROPERTY = "org.wildfly.management.no-request-timeout";

@Inject
protected static ServerController controller;

/**
* Test that the management interface will not accept new connections when the number of active connections reaches the
* high water mark. After the number of open connections has been reduced to the low watermark it will test that connections
* are accepted again.
*/
@Test
public void testWatermarks() throws Exception {
runTest(60000, () -> {
String mgmtAddress = TestSuiteEnvironment.getServerAddress();
int mgmtPort = TestSuiteEnvironment.getServerPort();
LOG.info(mgmtAddress + ":" + mgmtPort);
SocketAddress targetAddress = new InetSocketAddress(mgmtAddress, mgmtPort);

int socketsOpened = 0;
boolean oneFailed = false;
Socket[] sockets = new Socket[9];
for (int i = 0 ; i < 9 ; i++) {
LOG.info("Opening socket " + i + " socketsOpened=" + socketsOpened);
try {
sockets[i] = new Socket();
sockets[i].connect(targetAddress, 5000);
socketsOpened++;
} catch (IOException e) {
LOG.log(Level.SEVERE, "Probably an expected exception trying to open a new connection", e);
assertTrue("Less sockets than low watermark opened.", socketsOpened > 3);
oneFailed = true;
}
}
assertTrue("Opening of one socket was expected to fail.", oneFailed);

// Now close the connections and we should be able to connect again.
for (int i = 0 ; i < socketsOpened ; i++) {
sockets[i].close();
}

Socket goodSocket = new Socket();
// This needs a reasonable time to give the server time to respond to the closed connections.
goodSocket.connect(targetAddress, 10000);
goodSocket.close();
});
}

@Test
public void testTimeout() throws Exception {
runTest(10000, () -> {
String mgmtAddress = TestSuiteEnvironment.getServerAddress();
int mgmtPort = TestSuiteEnvironment.getServerPort();
SocketAddress targetAddress = new InetSocketAddress(mgmtAddress, mgmtPort);

int socketsOpened = 0;
boolean oneFailed = false;
Socket[] sockets = new Socket[9];
for (int i = 0 ; i < 9 ; i++) {
LOG.info("Opening socket " + i + " socketsOpened=" + socketsOpened);
try {
sockets[i] = new Socket();
sockets[i].connect(targetAddress, 5000);
socketsOpened++;
} catch (IOException e) {
LOG.log(Level.SEVERE, "Probably an expected exception trying to open a new connection", e);
assertTrue("Less sockets than low watermark opened.", socketsOpened > 3);
oneFailed = true;
}
}
assertTrue("Opening of one socket was expected to fail.", oneFailed);

// Notice that the exception received when we tried to open a new socket could have been a timeout (SocketTimeoutException)
// or a connection refused (IOException). It depends on the OS and the network configuration.
// So, we could also have had 5000ms for each bad socket that triggered a SocketTimeoutException.
Thread.sleep(adjust(12000));

Socket goodSocket = new Socket();
// This needs to be longer than 500ms to give the server time to respond to the closed connections.
goodSocket.connect(targetAddress, 10000);
goodSocket.close();

// Clean up remaining sockets
for (int i = 0 ; i < socketsOpened ; i++) {
sockets[i].close();
}
});
}

private void runTest(int noRequestTimeout, ExceptionRunnable<Exception> test) throws Exception {
controller.startInAdminMode();
try (CLIWrapper cli = new CLIWrapper(true)) {
cli.sendLine(String.format("/system-property=%s:add(value=%d)", BACKLOG_PROPERTY, 2));
cli.sendLine(String.format("/system-property=%s:add(value=%d)", CONNECTION_HIGH_WATER_PROPERTY, 6));
cli.sendLine(String.format("/system-property=%s:add(value=%d)", CONNECTION_LOW_WATER_PROPERTY, 3));
cli.sendLine(String.format("/system-property=%s:add(value=%d)", NO_REQUEST_TIMEOUT_PROPERTY, noRequestTimeout));
}

try {
controller.reload();

test.run();
} finally {
controller.reload();

try (CLIWrapper cli = new CLIWrapper(true)) {
cli.sendLine(String.format("/system-property=%s:remove()", BACKLOG_PROPERTY));
cli.sendLine(String.format("/system-property=%s:remove()", CONNECTION_HIGH_WATER_PROPERTY));
cli.sendLine(String.format("/system-property=%s:remove()", CONNECTION_LOW_WATER_PROPERTY));
cli.sendLine(String.format("/system-property=%s:remove()", NO_REQUEST_TIMEOUT_PROPERTY));
}
controller.stop();
}
}

}

0 comments on commit c8300b3

Please sign in to comment.