This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch karaf-4.4.x
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/karaf-4.4.x by this push:
new 4576eaa694 [4.4.x] Reject shell metacharacters in instance javaOpts
and add missing instance ACL (#2879)
4576eaa694 is described below
commit 4576eaa6945c175954652177ac7e698844c890b3
Author: JB Onofré <[email protected]>
AuthorDate: Sat Sep 12 05:58:24 2026 +0200
[4.4.x] Reject shell metacharacters in instance javaOpts and add missing
instance ACL (#2879)
* Reject shell metacharacters in instance javaOpts and add missing instance
ACL
InstanceServiceImpl builds the child-instance launch command by
concatenating
the caller-supplied javaOpts unquoted into a string that is ultimately
handed
to /bin/sh (via ScriptUtils/ProcessImpl), so a javaOpts value containing
shell
metacharacters was executed as an OS command rather than passed to the JVM.
This was reachable through instance:create, instance:start, instance:restart
and instance:change-opts (and the corresponding JMX operations).
Reject any javaOpts value that isn't valid JVM-option syntax before it is
used to build the launch command, in both doStart() and the self-restart
branch of restartInstance().
Also ship org.apache.karaf.command.acl.instance.cfg, since the instance
subshell had no ACL config at all and therefore failed open: any
authenticated shell user could invoke instance:* commands regardless of
role. The new ACL restricts state-changing operations to admin and
read-only ones (list/status) to viewer, matching the pattern used by the
other command ACLs.
* Fix InstanceTest to authenticate with admin/viewer roles for new instance
ACL
The new org.apache.karaf.command.acl.instance.cfg restricts instance:*
commands to the admin (and list/status to viewer) role. InstanceTest was
invoking these commands without any role principal, so the secured shell
rejected them as CommandNotFoundException, failing CI.
---
.../features/standard/src/main/feature/feature.xml | 46 ++++++++++++++++++++++
.../core/internal/InstanceServiceImpl.java | 15 +++++++
.../etc/org.apache.karaf.command.acl.instance.cfg | 41 +++++++++++++++++++
.../java/org/apache/karaf/itests/InstanceTest.java | 46 ++++++++++++----------
4 files changed, 127 insertions(+), 21 deletions(-)
diff --git a/assemblies/features/standard/src/main/feature/feature.xml
b/assemblies/features/standard/src/main/feature/feature.xml
index 3d9d474aca..0dab1e9c9e 100644
--- a/assemblies/features/standard/src/main/feature/feature.xml
+++ b/assemblies/features/standard/src/main/feature/feature.xml
@@ -649,6 +649,52 @@ update = manager
<feature name="instance" description="Provide Instance support"
version="${project.version}">
<bundle
start-level="30">mvn:org.apache.karaf.instance/org.apache.karaf.instance.core/${project.version}</bundle>
+ <conditional>
+ <condition>shell</condition>
+ <config name="org.apache.karaf.command.acl.instance">
+################################################################################
+#
+# 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.
+#
+################################################################################
+
+#
+# This configuration file defines the ACLs for commands in the instance
subshell
+#
+# For an explanation of the syntax of this file, see the file:
+# org.apache.karaf.command.acl.system.cfg
+#
+list = viewer
+status = viewer
+clone = admin
+connect = admin
+create = admin
+destroy = admin
+opts-change = admin
+package = admin
+rename = admin
+restart = admin
+rmi-registry-port-change = admin
+rmi-server-port-change = admin
+ssh-host-change = admin
+ssh-port-change = admin
+start = admin
+stop = admin
+ </config>
+ </conditional>
</feature>
<feature name="jaas-deployer" description="Allow support of blueprint JAAS
configuration in the deploy folder" version="${project.version}">
diff --git
a/instance/src/main/java/org/apache/karaf/instance/core/internal/InstanceServiceImpl.java
b/instance/src/main/java/org/apache/karaf/instance/core/internal/InstanceServiceImpl.java
index 2c56c3c15f..5c375f62ba 100644
---
a/instance/src/main/java/org/apache/karaf/instance/core/internal/InstanceServiceImpl.java
+++
b/instance/src/main/java/org/apache/karaf/instance/core/internal/InstanceServiceImpl.java
@@ -40,6 +40,7 @@ import java.util.Map;
import java.util.Scanner;
import java.util.Set;
import java.util.TreeMap;
+import java.util.regex.Pattern;
import org.apache.felix.utils.properties.InterpolationHelper;
import org.apache.felix.utils.properties.Properties;
@@ -88,6 +89,10 @@ public class InstanceServiceImpl implements InstanceService {
public static final String DEFAULT_JAVA_OPTS = "-Xmx512m
-XX:+UnlockDiagnosticVMOptions";
+ // javaOpts is concatenated unquoted into a shell command line (see
doStart/restartInstance), so it must
+ // never contain shell metacharacters; only characters that can legally
appear in JVM option syntax are allowed.
+ private static final Pattern SAFE_JAVA_OPTS = Pattern.compile("[A-Za-z0-9_
\\t\\-+.,:=/@*]*");
+
private LinkedHashMap<String, InstanceImpl> proxies = new
LinkedHashMap<>();
private File storageLocation;
@@ -351,6 +356,7 @@ public class InstanceServiceImpl implements InstanceService
{
"etc/org.apache.karaf.command.acl.bundle.cfg",
"etc/org.apache.karaf.command.acl.config.cfg",
"etc/org.apache.karaf.command.acl.feature.cfg",
+ "etc/org.apache.karaf.command.acl.instance.cfg",
"etc/org.apache.karaf.command.acl.jaas.cfg",
"etc/org.apache.karaf.command.acl.kar.cfg",
"etc/org.apache.karaf.command.acl.scope_bundle.cfg",
@@ -483,6 +489,13 @@ public class InstanceServiceImpl implements
InstanceService {
}, true);
}
+ private static void validateJavaOpts(String opts) {
+ if (opts != null && !SAFE_JAVA_OPTS.matcher(opts).matches()) {
+ throw new IllegalArgumentException("Invalid javaOpts: only JVM
option characters are allowed "
+ + "(letters, digits, whitespace and _-+.,:=/@*)");
+ }
+ }
+
private void doStart(InstanceState instance, String name, String javaOpts)
throws IOException {
String opts = javaOpts;
if (opts == null || opts.length() == 0) {
@@ -491,6 +504,7 @@ public class InstanceServiceImpl implements InstanceService
{
if (opts == null || opts.length() == 0) {
opts = DEFAULT_JAVA_OPTS;
}
+ validateJavaOpts(opts);
// fallback and read karafOpts from KARAF_OPTS environment if no
System property present
String karafOptsEnv = System.getenv("KARAF_OPTS");
@@ -619,6 +633,7 @@ public class InstanceServiceImpl implements InstanceService
{
}
String current = System.getProperty("karaf.name");
if (name.equals(current)) {
+ validateJavaOpts(javaOpts);
String location = System.getProperty("karaf.home");
StringBuilder classpath = new StringBuilder();
addJar(classpath, "org.apache.karaf.instance",
"org.apache.karaf.instance.core");
diff --git
a/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.instance.cfg
b/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.instance.cfg
new file mode 100644
index 0000000000..c09da30087
--- /dev/null
+++
b/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.instance.cfg
@@ -0,0 +1,41 @@
+################################################################################
+#
+# 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.
+#
+################################################################################
+
+#
+# This configuration file defines the ACLs for commands in the instance
subshell
+#
+# For an explanation of the syntax of this file, see the file:
+# org.apache.karaf.command.acl.system.cfg
+#
+list = viewer
+status = viewer
+clone = admin
+connect = admin
+create = admin
+destroy = admin
+opts-change = admin
+package = admin
+rename = admin
+restart = admin
+rmi-registry-port-change = admin
+rmi-server-port-change = admin
+ssh-host-change = admin
+ssh-port-change = admin
+start = admin
+stop = admin
diff --git
a/itests/test/src/test/java/org/apache/karaf/itests/InstanceTest.java
b/itests/test/src/test/java/org/apache/karaf/itests/InstanceTest.java
index 90ce247862..ae49f00d23 100644
--- a/itests/test/src/test/java/org/apache/karaf/itests/InstanceTest.java
+++ b/itests/test/src/test/java/org/apache/karaf/itests/InstanceTest.java
@@ -13,6 +13,7 @@
*/
package org.apache.karaf.itests;
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -39,6 +40,9 @@ import static org.junit.Assert.assertTrue;
@ExamReactorStrategy(PerClass.class)
public class InstanceTest extends BaseTest {
+ private static final RolePrincipal ADMIN_ROLE = new RolePrincipal("admin");
+ private static final RolePrincipal VIEWER_ROLE = new
RolePrincipal("viewer");
+
private String generateRandomInstanceName() {
return "instance-" + UUID.randomUUID();
}
@@ -46,10 +50,10 @@ public class InstanceTest extends BaseTest {
@Test
public void createDestroyCommand() throws Exception {
String instanceName = generateRandomInstanceName();
- System.out.println(executeCommand("instance:create " + instanceName));
- assertContains(instanceName, executeCommand("instance:list"));
- System.out.println(executeCommand("instance:destroy " + instanceName));
- assertContainsNot(instanceName, executeCommand("instance:list"));
+ System.out.println(executeCommand("instance:create " + instanceName,
ADMIN_ROLE));
+ assertContains(instanceName, executeCommand("instance:list",
VIEWER_ROLE));
+ System.out.println(executeCommand("instance:destroy " + instanceName,
ADMIN_ROLE));
+ assertContainsNot(instanceName, executeCommand("instance:list",
VIEWER_ROLE));
}
@Test
@@ -68,10 +72,10 @@ public class InstanceTest extends BaseTest {
@Test
public void createStartStopDestroyCommand() throws Exception {
String instanceName = generateRandomInstanceName();
- System.out.println(executeCommand("instance:create " + instanceName));
- assertContains(instanceName, executeCommand("instance:list"));
- System.out.println(executeCommand("instance:start " + instanceName));
- String output = executeCommand("instance:status " + instanceName);
+ System.out.println(executeCommand("instance:create " + instanceName,
ADMIN_ROLE));
+ assertContains(instanceName, executeCommand("instance:list",
VIEWER_ROLE));
+ System.out.println(executeCommand("instance:start " + instanceName,
ADMIN_ROLE));
+ String output = executeCommand("instance:status " + instanceName,
VIEWER_ROLE);
int i = 0;
while (!output.contains("Started")) {
if (i >= 10) {
@@ -79,12 +83,12 @@ public class InstanceTest extends BaseTest {
}
i = i + 1;
Thread.sleep(5000);
- output = executeCommand("instance:status " + instanceName);
+ output = executeCommand("instance:status " + instanceName,
VIEWER_ROLE);
}
System.out.println("itest instance status: " + output);
assertContains("Started", output);
- System.out.println(executeCommand("instance:stop " + instanceName));
- output = executeCommand("instance:status " + instanceName);
+ System.out.println(executeCommand("instance:stop " + instanceName,
ADMIN_ROLE));
+ output = executeCommand("instance:status " + instanceName,
VIEWER_ROLE);
i = 0;
while (!output.contains("Stopped")) {
if (i >= 10) {
@@ -92,18 +96,18 @@ public class InstanceTest extends BaseTest {
}
i = i + 1;
Thread.sleep(5000);
- output = executeCommand("instance:status " + instanceName);
+ output = executeCommand("instance:status " + instanceName,
VIEWER_ROLE);
}
System.out.println("itest instance status: " + output);
assertContains("Stopped", output);
- executeCommand("instance:destroy " + instanceName);
+ executeCommand("instance:destroy " + instanceName, ADMIN_ROLE);
}
@Test
public void packageCommand() throws Exception {
String instanceName = generateRandomInstanceName();
- executeCommand("instance:create " + instanceName);
- executeCommand("instance:package " + instanceName + " archive.zip");
+ executeCommand("instance:create " + instanceName, ADMIN_ROLE);
+ executeCommand("instance:package " + instanceName + " archive.zip",
ADMIN_ROLE);
String zipPath = Paths.get(System.getProperty("karaf.home"),
"archive.zip").toString();
ZipFile zipFile = new ZipFile(zipPath);
@@ -112,7 +116,7 @@ public class InstanceTest extends BaseTest {
assertTrue(entries.stream().anyMatch(e ->
e.getName().equals("bin/karaf")));
assertTrue(entries.stream().anyMatch(e ->
e.getName().equals("etc/system.properties")));
- executeCommand("instance:destroy " + instanceName);
+ executeCommand("instance:destroy " + instanceName, ADMIN_ROLE);
}
private int getInstancesNum(MBeanServerConnection connection, ObjectName
name) throws Exception {
@@ -123,8 +127,8 @@ public class InstanceTest extends BaseTest {
@Test
public void cloneCommand() throws Exception {
String instanceName = generateRandomInstanceName();
- System.out.println(executeCommand("instance:clone root " +
instanceName));
- assertContains(instanceName, executeCommand("instance:list"));
+ System.out.println(executeCommand("instance:clone root " +
instanceName, ADMIN_ROLE));
+ assertContains(instanceName, executeCommand("instance:list",
VIEWER_ROLE));
}
@Test
@@ -141,9 +145,9 @@ public class InstanceTest extends BaseTest {
@Test
public void renameCommand() throws Exception {
String instanceName = generateRandomInstanceName();
- executeCommand("instance:create " + instanceName);
- executeCommand("instance:rename " + instanceName + " new_" +
instanceName);
- assertContains("new_" + instanceName, executeCommand("instance:list"));
+ executeCommand("instance:create " + instanceName, ADMIN_ROLE);
+ executeCommand("instance:rename " + instanceName + " new_" +
instanceName, ADMIN_ROLE);
+ assertContains("new_" + instanceName, executeCommand("instance:list",
VIEWER_ROLE));
}
@Test
