Copilot commented on code in PR #13683:
URL: https://github.com/apache/cloudstack/pull/13683#discussion_r3658575050
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -83,12 +82,62 @@ public class ConsoleProxy {
static String encryptorPassword = "Dummy";
static final String[] skipProperties = new String[]{"certificate",
"cacertificate", "keystore_password", "privatekey"};
- static Set<String> allowedSessions = new HashSet<>();
+ static Set<String> allowedSessions = ConcurrentHashMap.newKeySet();
+ private static final Object allowedSessionsLock = new Object();
+ private static final Map<String, ReconnectGrant> sessionReconnectGrants =
new ConcurrentHashMap<>();
+ static int sessionReconnectGraceSeconds = 1;
+
+ // Invoked through reflection
public static void addAllowedSession(String sessionUuid) {
allowedSessions.add(sessionUuid);
}
+ /**
+ * Grant a short, single-use window to reconnect with the same session
UUID in case of a disconnection.
+ * The grant is bound to the client IP that was using the session so it
cannot be redeemed by another client.
+ * @param sessionUuid session UUID to grant a reconnect window for
+ * @param clientIp source IP of the client the session was granted to
+ */
+ public static void grantReconnectWindow(String sessionUuid, String
clientIp) {
+ sessionReconnectGrants.put(sessionUuid, new
ReconnectGrant(System.currentTimeMillis() + sessionReconnectGraceSeconds *
1000L, clientIp));
+ }
+
+ private static boolean consumeReconnectGrant(String sessionUuid, String
clientIp) {
+ ReconnectGrant grant = sessionReconnectGrants.remove(sessionUuid);
+ if (grant == null || grant.isExpired(System.currentTimeMillis())) {
+ return false;
+ }
+ if (grant.clientIp != null && !grant.clientIp.equals(clientIp)) {
+ LOGGER.warn("Rejecting reconnect for session " + sessionUuid + "
as it was requested from IP " +
+ clientIp + " but the reconnect window was granted to IP "
+ grant.clientIp);
+ return false;
+ }
+ return true;
+ }
Review Comment:
The reconnect grant is removed from `sessionReconnectGrants` before
validating the requesting IP. This allows a reconnect window to be
consumed/invalidated by any request (e.g., a wrong-IP attempt), effectively
preventing the legitimate client from reconnecting within the grace window. Fix
by only removing the grant after it is validated (and unexpired), e.g., via
`computeIfPresent`, or by doing a get-then-conditional-remove so mismatched-IP
attempts do not consume the grant.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java:
##########
@@ -129,6 +129,28 @@ public void proxyMsgOverWebSocketConnection(ByteBuffer
msg) {
}
}
+ public void close() {
+ if (nioSocketConnection != null) {
+ nioSocketConnection.close();
+ }
+ if (webSocketReverseProxy != null) {
+ webSocketReverseProxy.close();
+ }
+ if (socket != null) {
+ try {
+ if (is != null) {
+ is.close();
+ }
+ if (os != null) {
+ os.close();
+ }
+ socket.close();
+ } catch (IOException e) {
+ logger.debug("Error closing socket: " + e.getMessage(), e);
+ }
+ }
+ }
Review Comment:
All stream/socket closes are inside a single `try` block. If `is.close()`
throws, `os.close()` and `socket.close()` will be skipped, which can leave
resources open and reintroduce the leaked-connection problem this PR is trying
to fix. Close each resource in its own try/catch (or use a close-quietly
helper) so failures in one close do not prevent the others from running.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/network/NioSocketHandler.java:
##########
@@ -41,4 +41,5 @@ public interface NioSocketHandler {
void flushWriteBuffer();
void startTLSConnection(NioSocketSSLEngineManager sslEngineManager);
boolean isTLSConnection();
+ void close();
}
Review Comment:
Since `NioSocketHandler` now defines `close()`, consider extending
`AutoCloseable` (or `java.io.Closeable`) to integrate with try-with-resources
and align with Java resource-lifecycle conventions. This makes correct cleanup
easier to enforce at call sites.
##########
systemvm/agent/conf/consoleproxy.properties:
##########
@@ -22,3 +22,4 @@ consoleproxy.jarDir=./applet/
consoleproxy.viewerLinger=180
consoleproxy.reconnectMaxRetry=5
consoleproxy.defaultBufferSize=65536
+consoleproxy.sessionReconnectGraceSeconds=1
Review Comment:
The new `consoleproxy.sessionReconnectGraceSeconds` property is introduced
without any inline comment explaining its purpose (IP-bound reconnect window
after disconnect) and expected range/behavior. Add a brief comment in the
properties file to make it discoverable for operators and reduce
misconfiguration risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
