Copilot commented on code in PR #12053:
URL: https://github.com/apache/cloudstack/pull/12053#discussion_r2523669136
##########
ui/src/utils/plugins.js:
##########
@@ -18,14 +18,36 @@
import _ from 'lodash'
import { i18n } from '@/locales'
import { getAPI } from '@/api'
-import { message, notification, Modal } from 'ant-design-vue'
+import { message, notification, Modal, Button } from 'ant-design-vue'
+import { h } from 'vue'
import eventBus from '@/config/eventBus'
import store from '@/store'
import { sourceToken } from '@/utils/request'
import { toLocalDate, toLocaleDate } from '@/utils/date'
export const pollJobPlugin = {
install (app) {
+ function canViewLogs (logIds) {
+ console.log('canViewLogs', store.getters.features.logswebserverenabled,
'createLogsWebSession' in store.getters.apis, logIds, logIds && logIds.length >
0)
Review Comment:
Console.log statement should be removed from production code. This debug
logging can expose sensitive information and impact performance.
##########
ui/src/utils/plugins.js:
##########
@@ -89,10 +113,19 @@ export const pollJobPlugin = {
this.$store.commit('SET_HEADER_NOTICES', jobs)
})
+ const allLogIds = []
+ if (logIds) {
+ allLogIds.push(...logIds)
+ }
+
options.originalPage = options.originalPage ||
this.$router.currentRoute.value.path
getAPI('queryAsyncJobResult', { jobId }).then(json => {
const result = json.queryasyncjobresultresponse
eventBus.emit('update-job-details', { jobId, resourceId })
+ if (result.logids) {
+ allLogIds.push(...result.logids)
+ }
+ console.log('pollJobPlugin', result.logids, allLogIds)
Review Comment:
Console.log statement should be removed from production code. Debug logging
should use proper logging mechanisms instead of console.log.
##########
plugins/logs-web-server/src/main/java/org/apache/cloudstack/logsws/LogsWebSessionTokenCryptoUtil.java:
##########
@@ -0,0 +1,60 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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.apache.cloudstack.logsws;
+
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.util.Base64;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.cloud.serializer.GsonHelper;
+
+public class LogsWebSessionTokenCryptoUtil {
+ private static final String ALGORITHM = "AES";
+ private static final String TRANSFORMATION = "AES";
Review Comment:
Weak encryption: Using AES in ECB mode (default when only "AES" is specified
as transformation) is insecure as it doesn't use an initialization vector.
Consider using "AES/GCM/NoPadding" or "AES/CBC/PKCS5Padding" with a random IV
for better security.
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42300.sql:
##########
@@ -18,3 +18,32 @@
--;
-- Schema upgrade from 4.22.0.0 to 4.23.0.0
--;
+
+-- Add management_server_details table to allow ManagementServer scope configs
+CREATE TABLE IF NOT EXISTS `cloud`.`management_server_details` (
+ `id` bigint unsigned NOT NULL AUTO_INCREMENT COMMENT 'id',
+ `management_server_id` bigint unsigned NOT NULL COMMENT 'management server
the detail is related to',
+ `name` varchar(255) NOT NULL COMMENT 'name of the detail',
+ `value` varchar(255) NOT NULL,
+ `display` tinyint(1) NOT NULL DEFAULT '1' COMMENT 'True if the detail can
be displayed to the end user',
+ PRIMARY KEY (`id`),
+ CONSTRAINT `fk_management_server_details__management_server_id` FOREIGN
KEY
`fk_management_server_details__management_server_id`(`management_server_id`)
REFERENCES `mshost`(`id`) ON DELETE CASCADE,
+ KEY `i_management_server_details__name__value` (`name`(128),`value`(128))
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+
+-- Create table for Logs Web Session
+CREATE TABLE IF NOT EXISTS `cloud`.`logs_web_session` (
+ `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT COMMENT 'id of the
session',
+ `uuid` varchar(40) NOT NULL COMMENT 'UUID generated for the session',
+ `filter` varchar(64) DEFAULT NULL COMMENT 'Filter keyword for the session',
Review Comment:
The field name in the database schema is 'filter' (singular) but the
converter is used for a List. This should be 'filters' (plural) to match the
Java field name and be semantically correct. The SQL definition on line 38
should use `filters` instead of `filter`.
```suggestion
`filters` varchar(64) DEFAULT NULL COMMENT 'Filter keywords for the
session',
```
##########
ui/src/components/view/SettingsTab.vue:
##########
@@ -85,6 +85,7 @@ export default {
}
},
created () {
+ console.log('---------------', this.$route.meta.name)
Review Comment:
Console.log statement should be removed from production code. This appears
to be debug code that was left in.
##########
ui/src/components/view/LogsConsole.vue:
##########
@@ -0,0 +1,515 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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.
+
+<template>
+ <div>
+ <a-drawer
+ class="resizable-drawer"
+ :title="$t('Logs')"
+ placement="bottom"
+ :visible="visible"
+ :height="drawerHeight"
+ :maskClosable="false"
+ :bodyStyle="{ overflow: 'hidden' }"
+ @close="closeAction"
+ >
+ <template #extra>
+ <a-button type="primary" @click="onDownload" v-if="webSocketsValid">{{
$t('label.download') }}</a-button>
+ </template>
+ <!-- Draggable handle at the top of the drawer content -->
+ <div
+ class="drag-handle"
+ @mousedown="startDrag"
+ ></div>
+ <!-- Container that holds both the scrollable content and the fixed
footer -->
+ <div class="drawer-container">
+ <div class="header">
+ <a-row :gutter="[16, 8]" style="width: 100%; margin-bottom: 16px">
+ <a-col :xs="24" :sm="16">
+ <tooltip-label :title="$t('label.source')"
:tooltip="'Sources'" />
+ <a-select
+ v-model:value="selectedWebSockets"
+ mode="multiple"
+ showSearch
+ optionFilterProp="label"
+ :filterOption="(input, option) => {
+ return
option.label.toLowerCase().indexOf(input.toLowerCase()) >= 0
+ }"
+ :loading="webSocketsLoading"
+ :placeholder="'Sources'"
+ @change="handleSourceChange"
+ style="width: 100%">
+ <a-select-option
+ v-for="opt in webSockets"
+ :key="opt.id"
+ :label="opt.name">
+ {{ opt.name }}
+ </a-select-option>
+ </a-select>
+ </a-col>
+
+ <a-col :xs="24" :sm="8">
+ <tooltip-label :title="'Log Type'" :tooltip="'Log severity
level'" />
+ <a-select
+ mode="multiple"
+ v-model:value="selectedLogType"
+ :placeholder="'Log Type'"
+ style="width: 100%">
+ <a-select-option
+ v-for="level in logLevels"
+ :key="level"
+ :value="level">
+ {{ level }}
+ </a-select-option>
+ </a-select>
+ </a-col>
+ </a-row>
+ </div>
+ <div class="content_wrapper">
+ <div
+ v-if="showRawLogs"
+ class="content"
+ v-html="webSocketData">
Review Comment:
Potential XSS vulnerability: HTML content is rendered using `v-html` without
sanitization. The `webSocketData` contains unsanitized log data that could
include malicious scripts. Consider using a library like DOMPurify to sanitize
the HTML before rendering, or avoid using v-html altogether.
##########
framework/websocket-server/src/main/java/org/apache/cloudstack/framework/websocket/server/WebSocketServer.java:
##########
@@ -0,0 +1,207 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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.apache.cloudstack.framework.websocket.server;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.util.concurrent.TimeUnit;
+
+import javax.net.ssl.KeyManagerFactory;
+
+import org.apache.cloudstack.framework.websocket.server.common.WebSocketRouter;
+import org.apache.cloudstack.utils.server.ServerPropertiesUtil;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import io.netty.bootstrap.ServerBootstrap;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelInitializer;
+import io.netty.channel.ChannelOption;
+import io.netty.channel.ChannelPipeline;
+import io.netty.channel.EventLoopGroup;
+import io.netty.channel.nio.NioEventLoopGroup;
+import io.netty.channel.socket.SocketChannel;
+import io.netty.channel.socket.nio.NioServerSocketChannel;
+import io.netty.handler.codec.http.HttpObjectAggregator;
+import io.netty.handler.codec.http.HttpServerCodec;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolConfig;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+
+/**
+ * Netty WebSocket server that delegates routing to WebSocketRouter.
+ * Replaces the previous helper-based WebSocketServer.
+ */
+public final class WebSocketServer {
+ private static final Logger LOG =
LogManager.getLogger(WebSocketServer.class);
+
+ private final String host;
+ private final int port;
+ private final WebSocketRouter router;
+ private final String websocketBasePath;
+ private final boolean sslEnabled;
+
+ private EventLoopGroup bossGroup;
+ private EventLoopGroup workerGroup;
+ private Channel serverChannel;
+ private volatile boolean running;
+
+ public WebSocketServer(int port, WebSocketRouter router, boolean
sslEnabled) {
+ this(null, port, router, null, sslEnabled);
+ }
+
+ public WebSocketServer(String host, int port, WebSocketRouter router,
String websocketBasePath, boolean sslEnabled) {
+ this.host = StringUtils.isBlank(host) ? "0.0.0.0" : host;
+ this.port = port;
+ this.router = router;
+ this.websocketBasePath = StringUtils.isBlank(websocketBasePath) ?
+ WebSocketRouter.WEBSOCKET_PATH_PREFIX : websocketBasePath;
+ this.sslEnabled = sslEnabled;
+ }
+
+ protected KeyManagerFactory buildKeyManagerFactory(Path storePath, char[]
password) throws
+ KeyStoreException, IOException, NoSuchAlgorithmException,
CertificateException, UnrecoverableKeyException {
+ KeyStore ks = KeyStore.getInstance(detectType(storePath));
+ try (InputStream in = Files.newInputStream(storePath)) {
+ ks.load(in, password);
+ }
+ KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ kmf.init(ks, password);
+ return kmf;
+ }
+
+ private static String detectType(Path p) {
+ String name = p.getFileName().toString().toLowerCase();
+ return (name.endsWith(".p12") || name.endsWith(".pfx")) ? "PKCS12" :
"JKS";
+ }
+
+ /**
+ * Creates a Netty SslContext:
+ * uses only a keystore containing the server's private key and
certificate chain.
+ *
+ * @param keystoreFile Path to the keystore file (JKS or PKCS12)
+ * @param keystorePassword Password for both the keystore and key entry
+ * @return configured Netty SslContext
+ */
+ protected SslContext createServerSslContext(String keystoreFile, String
keystorePassword) throws
+ UnrecoverableKeyException, CertificateException,
KeyStoreException, IOException, NoSuchAlgorithmException {
+ KeyManagerFactory kmf = buildKeyManagerFactory(Path.of(keystoreFile),
keystorePassword.toCharArray());
+
+ // Build Netty SSL context (server mode) – same as Jetty's
SSLContextFactory.Server
+ return SslContextBuilder.forServer(kmf)
+ .sslProvider(SslProvider.JDK) // Use JDK
provider (same as Jetty)
+ .protocols("TLSv1.3", "TLSv1.2") // Match
Jetty default supported protocols
+ .build();
+ }
+
+ protected SslContext createServerSslContextIfNeeded() throws
IllegalArgumentException {
+ if (!sslEnabled) {
+ return null;
+ }
+ String keystoreFile =
ServerPropertiesUtil.getProperty(ServerPropertiesUtil.KEY_KEYSTORE_FILE);
+ String keystorePassword =
ServerPropertiesUtil.getProperty(ServerPropertiesUtil.KEY_KEYSTORE_FILE);
+ if (StringUtils.isBlank(keystoreFile) ||
StringUtils.isBlank(keystorePassword)) {
+ throw new IllegalArgumentException("SSL is enabled but keystore
file or password is not configured");
+ }
+ if (Files.exists(Path.of(keystoreFile))) {
Review Comment:
Inverted logic: the condition checks `Files.exists(...)` but the error
message says "does not exist". This should be
`!Files.exists(Path.of(keystoreFile))` to correctly validate that the keystore
file exists.
```suggestion
if (!Files.exists(Path.of(keystoreFile))) {
```
##########
framework/websocket-server/src/main/java/org/apache/cloudstack/framework/websocket/server/WebSocketServer.java:
##########
@@ -0,0 +1,207 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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.apache.cloudstack.framework.websocket.server;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.util.concurrent.TimeUnit;
+
+import javax.net.ssl.KeyManagerFactory;
+
+import org.apache.cloudstack.framework.websocket.server.common.WebSocketRouter;
+import org.apache.cloudstack.utils.server.ServerPropertiesUtil;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import io.netty.bootstrap.ServerBootstrap;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelInitializer;
+import io.netty.channel.ChannelOption;
+import io.netty.channel.ChannelPipeline;
+import io.netty.channel.EventLoopGroup;
+import io.netty.channel.nio.NioEventLoopGroup;
+import io.netty.channel.socket.SocketChannel;
+import io.netty.channel.socket.nio.NioServerSocketChannel;
+import io.netty.handler.codec.http.HttpObjectAggregator;
+import io.netty.handler.codec.http.HttpServerCodec;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolConfig;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+
+/**
+ * Netty WebSocket server that delegates routing to WebSocketRouter.
+ * Replaces the previous helper-based WebSocketServer.
+ */
+public final class WebSocketServer {
+ private static final Logger LOG =
LogManager.getLogger(WebSocketServer.class);
+
+ private final String host;
+ private final int port;
+ private final WebSocketRouter router;
+ private final String websocketBasePath;
+ private final boolean sslEnabled;
+
+ private EventLoopGroup bossGroup;
+ private EventLoopGroup workerGroup;
+ private Channel serverChannel;
+ private volatile boolean running;
+
+ public WebSocketServer(int port, WebSocketRouter router, boolean
sslEnabled) {
+ this(null, port, router, null, sslEnabled);
+ }
+
+ public WebSocketServer(String host, int port, WebSocketRouter router,
String websocketBasePath, boolean sslEnabled) {
+ this.host = StringUtils.isBlank(host) ? "0.0.0.0" : host;
+ this.port = port;
+ this.router = router;
+ this.websocketBasePath = StringUtils.isBlank(websocketBasePath) ?
+ WebSocketRouter.WEBSOCKET_PATH_PREFIX : websocketBasePath;
+ this.sslEnabled = sslEnabled;
+ }
+
+ protected KeyManagerFactory buildKeyManagerFactory(Path storePath, char[]
password) throws
+ KeyStoreException, IOException, NoSuchAlgorithmException,
CertificateException, UnrecoverableKeyException {
+ KeyStore ks = KeyStore.getInstance(detectType(storePath));
+ try (InputStream in = Files.newInputStream(storePath)) {
+ ks.load(in, password);
+ }
+ KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ kmf.init(ks, password);
+ return kmf;
+ }
+
+ private static String detectType(Path p) {
+ String name = p.getFileName().toString().toLowerCase();
+ return (name.endsWith(".p12") || name.endsWith(".pfx")) ? "PKCS12" :
"JKS";
+ }
+
+ /**
+ * Creates a Netty SslContext:
+ * uses only a keystore containing the server's private key and
certificate chain.
+ *
+ * @param keystoreFile Path to the keystore file (JKS or PKCS12)
+ * @param keystorePassword Password for both the keystore and key entry
+ * @return configured Netty SslContext
+ */
+ protected SslContext createServerSslContext(String keystoreFile, String
keystorePassword) throws
+ UnrecoverableKeyException, CertificateException,
KeyStoreException, IOException, NoSuchAlgorithmException {
+ KeyManagerFactory kmf = buildKeyManagerFactory(Path.of(keystoreFile),
keystorePassword.toCharArray());
+
+ // Build Netty SSL context (server mode) – same as Jetty's
SSLContextFactory.Server
+ return SslContextBuilder.forServer(kmf)
+ .sslProvider(SslProvider.JDK) // Use JDK
provider (same as Jetty)
+ .protocols("TLSv1.3", "TLSv1.2") // Match
Jetty default supported protocols
+ .build();
+ }
+
+ protected SslContext createServerSslContextIfNeeded() throws
IllegalArgumentException {
+ if (!sslEnabled) {
+ return null;
+ }
+ String keystoreFile =
ServerPropertiesUtil.getProperty(ServerPropertiesUtil.KEY_KEYSTORE_FILE);
+ String keystorePassword =
ServerPropertiesUtil.getProperty(ServerPropertiesUtil.KEY_KEYSTORE_FILE);
Review Comment:
Wrong property key is being used. Line 127 should retrieve
`KEY_KEYSTORE_PASSWORD` but it's retrieving `KEY_KEYSTORE_FILE` again. This
will cause SSL context creation to fail as it will use the keystore file path
as the password.
```suggestion
String keystorePassword =
ServerPropertiesUtil.getProperty(ServerPropertiesUtil.KEY_KEYSTORE_PASSWORD);
```
--
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]
