Copilot commented on code in PR #13058:
URL: https://github.com/apache/cloudstack/pull/13058#discussion_r3153177256
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -233,20 +234,21 @@ public static ConsoleProxyAuthenticationResult
authenticateConsoleAccess(Console
Object result;
try {
result =
- authMethod.invoke(ConsoleProxy.context,
param.getClientHostAddress(), String.valueOf(param.getClientHostPort()),
param.getClientTag(),
- param.getClientHostPassword(),
param.getTicket(), reauthentication, param.getSessionUuid());
+ authMethod.invoke(ConsoleProxy.context,
param.getClientHostAddress(), String.valueOf(param.getClientHostPort()),
+ param.getClientTag(),
param.getClientHostPassword(), param.getTicket(), reauthentication,
+ param.getSessionUuid(), param.getClientIp());
} catch (IllegalAccessException e) {
Review Comment:
authenticateConsoleAccess(...) now invokes authMethod with an extra
param.getClientIp() argument. On 4.20,
ConsoleProxyResource.authenticateConsoleAccess(...) only accepts 7 arguments,
so this call will fail if/when authMethod is resolved. Keep the invocation
arguments consistent with the reflected method signature (or implement a
dual-signature invocation path).
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -308,7 +311,7 @@ public static void startWithContext(Properties conf, Object
context, byte[] ksBi
final ClassLoader loader =
Thread.currentThread().getContextClassLoader();
Class<?> contextClazz =
loader.loadClass("com.cloud.agent.resource.consoleproxy.ConsoleProxyResource");
authMethod =
contextClazz.getDeclaredMethod("authenticateConsoleAccess", String.class,
String.class,
- String.class, String.class, String.class, Boolean.class,
String.class);
+ String.class, String.class, String.class, Boolean.class,
String.class, String.class);
Review Comment:
The reflective lookup of ConsoleProxyResource.authenticateConsoleAccess(...)
now expects an extra String parameter, but in this branch ConsoleProxyResource
defines authenticateConsoleAccess(host, port, vmId, sid, ticket,
isReauthentication, sessionToken) with 7 params. This will trigger
NoSuchMethodException at startup, leaving authMethod unset and causing
authenticateConsoleAccess(...) to fall back to "offline mode" (success=true)
which effectively bypasses management-server authentication. Align the
getDeclaredMethod(...) signature (and corresponding invoke call) with the
actual ConsoleProxyResource method in 4.20, or add a backwards-compatible
lookup that tries both signatures.
```suggestion
String.class, String.class, String.class, Boolean.class,
String.class);
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -596,50 +611,55 @@ public void execute(Runnable r) {
}
public static ConsoleProxyNoVncClient
getNoVncViewer(ConsoleProxyClientParam param, String ajaxSession,
- Session session) throws AuthenticationException {
+ Session session)
throws AuthenticationException {
boolean reportLoadChange = false;
String clientKey = param.getClientMapKey();
- synchronized (connectionMap) {
- ConsoleProxyClient viewer = connectionMap.get(clientKey);
- if (viewer == null || viewer.getClass() !=
ConsoleProxyNoVncClient.class) {
- authenticationExternally(param);
- viewer = new ConsoleProxyNoVncClient(session);
- viewer.initClient(param);
+ LOGGER.debug("Getting NoVNC viewer for {}. Client tag: {}. session
UUID: {}",
+ clientKey, param.getClientTag(), param.getSessionUuid());
+synchronized (connectionMap) {
+ ConsoleProxyClient viewer = connectionMap.get(clientKey);
+ if (viewer == null || viewer.getClass() != ConsoleProxyNoVncClient.class) {
+ authenticationExternally(param);
+ viewer = new ConsoleProxyNoVncClient(session);
+ viewer.initClient(param);
+
+ connectionMap.put(clientKey, viewer);
+ reportLoadChange = true;
+ } else {
+ if (param.getClientHostPassword() == null ||
param.getClientHostPassword().isEmpty()
+ ||
!param.getClientHostPassword().equals(viewer.getClientHostPassword())) {
+ throw new AuthenticationException("Cannot use the existing viewer
" + viewer + ": bad sid");
+ }
- connectionMap.put(clientKey, viewer);
- reportLoadChange = true;
- } else {
- if (param.getClientHostPassword() == null ||
param.getClientHostPassword().isEmpty() ||
-
!param.getClientHostPassword().equals(viewer.getClientHostPassword()))
- throw new AuthenticationException("Cannot use the existing
viewer " + viewer + ": bad sid");
+ try {
+ authenticationExternally(param);
+ } catch (Exception e) {
+ LOGGER.error("Authentication failed for param: " + param);
Review Comment:
In getNoVncViewer(...), the AuthenticationException path logs
"Authentication failed" without including the caught exception, which loses the
stack trace/context needed to debug auth failures. Log the exception (and
preferably include key identifiers like sessionUuid/clientKey) when catching
the failure.
```suggestion
LOGGER.error("Authentication failed for sessionUuid={},
clientKey={}, param={}",
param.getSessionUuid(), clientKey, param, e);
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -93,15 +94,16 @@ public void onConnect(final Session session) throws
IOException, InterruptedExce
String websocketUrl = queryMap.get("websocketUrl");
String sessionUuid = queryMap.get("sessionUuid");
String clientIp =
session.getRemoteAddress().getAddress().getHostAddress();
+ boolean sessionRequiresNewViewer =
Boolean.parseBoolean(queryMap.get("sessionRequiresNewViewer"));
Review Comment:
sessionRequiresNewViewer is parsed from the query string but never used. If
this branch intentionally does not support that behavior (per the PR
description), remove this local variable (and possibly the query param
handling) to avoid confusion and dead code.
```suggestion
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:
##########
@@ -32,9 +32,13 @@
* management software
*/
public class ConsoleProxyGCThread extends Thread {
- protected Logger logger = LogManager.getLogger(ConsoleProxyGCThread.class);
+ private static final Logger logger =
LogManager.getLogger(ConsoleProxyGCThread.class);
- private final static int MAX_SESSION_IDLE_SECONDS = 180;
+ /**
+ * Maximum time (in seconds) a console session is allowed to be idle
before it is closed.
+ * This value should be kept in sync with
ConsoleProxy.VIEWER_LINGER_SECONDS.
+ */
+ private static final int MAX_SESSION_IDLE_SECONDS = 180;
Review Comment:
MAX_SESSION_IDLE_SECONDS is still hard-coded to 180s, which does not match
the documented/expected behavior of honoring the global
consoleproxy.session.timeout (default 300000ms) and is also duplicating
ConsoleProxy.VIEWER_LINGER_SECONDS. To honor the setting and avoid drift,
derive the idle timeout from the configured consoleproxy.session.timeout and
use a single shared value for both GC and viewer liveness checks.
```suggestion
* Reuse the shared viewer linger timeout so GC honors the configured
* consoleproxy.session.timeout and does not drift from viewer liveness
checks.
*/
private static final int MAX_SESSION_IDLE_SECONDS =
ConsoleProxy.VIEWER_LINGER_SECONDS;
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -183,12 +186,20 @@ public void onClose(Session session, int statusCode,
String reason) throws IOExc
@OnWebSocketFrame
public void onFrame(Frame f) throws IOException {
+ if (viewer == null) {
+ logger.debug("Ignoring WebSocket frame because viewer is not
initialized yet.");
+ return;
+ }
Review Comment:
NoVNC session idle tracking relies on getClientLastFrontEndActivityTime(),
but this handler does not update the viewer's front-end activity timestamp when
frames arrive from the browser. As a result, GC/linger logic may not reflect
actual browser activity and can prevent consoleproxy.session.timeout from being
enforced. Update the viewer's front-end activity time on each received
WebSocket frame (and ensure the timestamp isn't continuously refreshed by
backend reads alone).
```suggestion
}
viewer.setClientLastFrontEndActivityTime(System.currentTimeMillis());
```
--
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]
