This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/main by this push:
new 511df4e0fc Restrict feature:repo-add -i and feature:repo-remove -u to
admin role (#2883)
511df4e0fc is described below
commit 511df4e0fcdc66139dbcf2b00586801a2fece7ac
Author: JB Onofré <[email protected]>
AuthorDate: Sat Sep 12 13:07:01 2026 +0200
Restrict feature:repo-add -i and feature:repo-remove -u to admin role
(#2883)
* Restrict feature:repo-add -i and feature:repo-remove -u to admin role
feature:repo-add -i and feature:repo-remove -u call
FeaturesServiceImpl.addRepository()/removeRepository() with the
install/uninstall flag set, which installs or uninstalls every
feature in the repository. Neither command had an ACL entry, so
any SSH user with just the viewer role could reach this and have
Karaf install and start an arbitrary attacker-supplied feature
repository, bypassing the admin-only ACL on feature:install and
feature:uninstall.
Add option-specific ACL rules (same pattern already used for
bundle:refresh -f and friends) so only -i/-u require admin, while
plain repo-add/repo-remove stay open as before.
* Fix CI: add open fallback ACL rule for plain feature:repo-add/repo-remove
The regex-conditioned ACL rules (repo-add[/.*[-][i].*/],
repo-remove[/.*[-][u].*/])
had no fallback entry for the plain command. Karaf's ACL guard treats a
command
name that has at least one ACL entry but no rule matching the current
invocation
as "no roles allowed" (empty role list), which denies everyone -- including
admin -- rather than leaving it unrestricted. This broke feature:repo-add/
feature:repo-remove without -i/-u for all callers, causing FeatureTest and
the
new FeatureSshCommandSecurityTest to fail in CI with
CommandNotFoundException.
Add an explicit "repo-add = viewer" / "repo-remove = viewer" fallback,
mirroring
the existing pattern used for bundle:refresh -f, so the plain commands
remain
open while -i/-u stay admin-only.
* Revert incorrect open fallback for feature:repo-add/repo-remove
The previous "Fix CI" commit added a bare `repo-add = viewer` /
`repo-remove = viewer` entry, on the theory that a command with a
regex-specific ACL rule but no matching entry falls back to "deny
all, including admin". That's not what happens: ACLConfigurationParser
returns Specificity.NO_MATCH for a non-matching invocation, and
checkSecurity() treats NO_MATCH as unrestricted access.
What the bare `= viewer` entry actually did was make the *plain*
command name itself require the viewer role for visibility
(SecuredSessionFactoryImpl.isVisible(), which checks the bare command
name regardless of arguments). Callers with no roles at all -- like
the pax-exam-based FeatureTest itests, which invoke commands directly
via executeCommand() without any RolePrincipal -- were no longer able
to even see feature:repo-add/repo-remove, failing with
CommandNotFoundException instead of running the command.
Dropping the bare fallback restores the pre-existing behavior: the
plain commands stay open to everyone (no ACL entry at all for the
non-`-i`/`-u` case), while the regex-specific rules keep requiring
admin only when `-i`/`-u` is present.
* Restore viewer access to plain feature:repo-add/repo-remove
The regex-conditioned ACL rules added for -i/-u (repo-add[/.*[-][i].*/],
repo-remove[/.*[-][u].*/]) had no plain fallback entry. Karaf's shell
guard decides command *visibility* before it knows the actual arguments
(SecuredSessionFactoryImpl.isVisible() calls getRolesForInvocation with
params=null), and a command with only a regex/exact-arg ACL entry is
treated as requiring that entry's role for every invocation at that
stage - so feature:repo-add/feature:repo-remove became invisible
("Command not found") to any non-admin session, even without -i/-u.
This broke the PR's own FeatureSshCommandSecurityTest (which expects a
viewer to run the plain command) and the pre-existing FeatureTest cases.
Add a "= viewer" fallback for both commands, mirroring the existing
bundle:refresh/-f pattern in org.apache.karaf.command.acl.bundle.cfg,
so the plain form stays open to viewer/manager/admin while -i/-u keeps
requiring admin.
The three pre-existing FeatureTest cases that called repo-add/repo-remove
with no role at all (previously fine since the commands had no ACL entry)
now pass an explicit viewer RolePrincipal, since introducing any ACL entry
for a command means invoking it requires holding a role.
---
.../features/standard/src/main/feature/feature.xml | 11 ++++++++
.../etc/org.apache.karaf.command.acl.feature.cfg | 11 ++++++++
.../src/test/filtered-resources/etc/feature.xml | 4 +++
.../java/org/apache/karaf/itests/FeatureTest.java | 10 ++++----
.../itests/ssh/FeatureSshCommandSecurityTest.java | 30 ++++++++++++++++++++++
manual/src/main/asciidoc/user-guide/security.adoc | 5 ++--
6 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/assemblies/features/standard/src/main/feature/feature.xml
b/assemblies/features/standard/src/main/feature/feature.xml
index 3f7d1c7bff..06bb632a51 100644
--- a/assemblies/features/standard/src/main/feature/feature.xml
+++ b/assemblies/features/standard/src/main/feature/feature.xml
@@ -147,6 +147,17 @@ install = admin
uninstall = admin
start = admin
stop = admin
+#
+# repo-add/repo-remove stay open to the viewer role (the plain form only
reads/lists
+# a repository), but installing/uninstalling every feature in it via -i/-u
requires
+# admin. The bare "= viewer" fallback is required: a command with a regex-only
ACL
+# entry and no fallback is treated as admin-only for *every* invocation,
because the
+# command guard decides visibility before it knows the actual arguments.
+#
+repo-add[/.*[-][i].*/] = admin
+repo-add = viewer
+repo-remove[/.*[-][u].*/] = admin
+repo-remove = viewer
</config>
<bundle
start-level="30">mvn:org.apache.karaf.features/org.apache.karaf.features.command/${project.version}</bundle>
</conditional>
diff --git
a/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.feature.cfg
b/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.feature.cfg
index 823fe3e2cc..cefaa7a7fc 100644
---
a/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.feature.cfg
+++
b/instance/src/main/resources/org/apache/karaf/instance/resources/etc/org.apache.karaf.command.acl.feature.cfg
@@ -24,3 +24,14 @@ install = admin
uninstall = admin
start = admin
stop = admin
+#
+# repo-add/repo-remove stay open to the viewer role (the plain form only
reads/lists
+# a repository), but installing/uninstalling every feature in it via -i/-u
requires
+# admin. The bare "= viewer" fallback is required: a command with a regex-only
ACL
+# entry and no fallback is treated as admin-only for *every* invocation,
because the
+# command guard decides visibility before it knows the actual arguments.
+#
+repo-add[/.*[-][i].*/] = admin
+repo-add = viewer
+repo-remove[/.*[-][u].*/] = admin
+repo-remove = viewer
diff --git a/itests/test/src/test/filtered-resources/etc/feature.xml
b/itests/test/src/test/filtered-resources/etc/feature.xml
index ec36a8c8d6..b4c139af3d 100644
--- a/itests/test/src/test/filtered-resources/etc/feature.xml
+++ b/itests/test/src/test/filtered-resources/etc/feature.xml
@@ -107,6 +107,10 @@
uninstall = admin
start = admin
stop = admin
+ repo-add[/.*[-][i].*/] = admin
+ repo-add = viewer
+ repo-remove[/.*[-][u].*/] = admin
+ repo-remove = viewer
</config>
<bundle
start-level="30">mvn:org.apache.karaf.features/org.apache.karaf.features.command/${project.version}</bundle>
</conditional>
diff --git a/itests/test/src/test/java/org/apache/karaf/itests/FeatureTest.java
b/itests/test/src/test/java/org/apache/karaf/itests/FeatureTest.java
index 18c3a839ef..cd28549e7f 100644
--- a/itests/test/src/test/java/org/apache/karaf/itests/FeatureTest.java
+++ b/itests/test/src/test/java/org/apache/karaf/itests/FeatureTest.java
@@ -76,7 +76,7 @@ public class FeatureTest extends BaseTest {
@Test
public void listCommandFromRepository() {
- executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features");
+ executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features", new
RolePrincipal("viewer"));
String repositoryName = "karaf-cellar-3.0.0";
String featureListOutput = executeCommand("feature:list --repository "
+ repositoryName);
assertFalse(featureListOutput.isEmpty());
@@ -150,17 +150,17 @@ public class FeatureTest extends BaseTest {
@Test
public void repoAddRemoveCommand() throws Exception {
- System.out.println(executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features"));
+ System.out.println(executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features", new
RolePrincipal("viewer")));
assertContains("apache-karaf-cellar",
executeCommand("feature:repo-list"));
- System.out.println(executeCommand("feature:repo-remove
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features"));
+ System.out.println(executeCommand("feature:repo-remove
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features", new
RolePrincipal("viewer")));
assertContainsNot("apache-karaf-cellar",
executeCommand("feature:repo-list"));
}
@Test
public void repoAddRemoveCommandWithRegex() throws Exception {
- System.out.println(executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features"));
+ System.out.println(executeCommand("feature:repo-add
mvn:org.apache.karaf.cellar/apache-karaf-cellar/3.0.0/xml/features", new
RolePrincipal("viewer")));
assertContains("apache-karaf-cellar",
executeCommand("feature:repo-list"));
- System.out.println(executeCommand("feature:repo-remove
'.*apache-karaf-cellar.*'"));
+ System.out.println(executeCommand("feature:repo-remove
'.*apache-karaf-cellar.*'", new RolePrincipal("viewer")));
assertContainsNot("apache-karaf-cellar",
executeCommand("feature:repo-list"));
}
diff --git
a/itests/test/src/test/java/org/apache/karaf/itests/ssh/FeatureSshCommandSecurityTest.java
b/itests/test/src/test/java/org/apache/karaf/itests/ssh/FeatureSshCommandSecurityTest.java
index 0ec55c3d72..288a085026 100644
---
a/itests/test/src/test/java/org/apache/karaf/itests/ssh/FeatureSshCommandSecurityTest.java
+++
b/itests/test/src/test/java/org/apache/karaf/itests/ssh/FeatureSshCommandSecurityTest.java
@@ -64,4 +64,34 @@ public class FeatureSshCommandSecurityTest extends
SshCommandTestBase {
Assert.assertFalse(feature + " feature should have been uninstalled",
r5.contains(feature));
}
+
+ @Test
+ public void testFeatureRepoCommandSecurityViaSsh() throws Exception {
+ // Skip on Windows where PTY output can be garbled,
+ // when upgrading to Junit5, this can be replaced with
@DisabledOnOs(OS.WINDOWS)
+ // TODO: remove this once we have a better solution for PTY output on
Windows
+ Assume.assumeFalse(System.getProperty("os.name",
"").toLowerCase().contains("win"));
+
+ String vieweruser = "viewer" + System.nanoTime() + "_repos";
+ // Deliberately non-existent so repo-add/repo-remove never actually
add, install, remove
+ // or uninstall anything; this test only cares about the ACL decision,
not the outcome of
+ // the underlying operation (same approach as bundle:refresh -f 999 in
BundleSshCommandSecurityTest).
+ // Note: the ACL regex matches against the whole argument list, so
this URL must not
+ // itself contain "-i" or "-u" or it would coincidentally match the
option-specific rules.
+ String bogusUrl = "file:///nonexistent/karaf-test-repo-features.xml";
+
+ addViewer(vieweruser);
+
+ // repo-add/repo-remove without options are not gated by any role,
same as before this ACL
+ // was introduced: only the -i/-u options (which install/uninstall
every feature in the
+ // repository) must require admin.
+ assertCommand(vieweruser, "feature:repo-add " + bogusUrl, Result.OK);
+ assertCommand(vieweruser, "feature:repo-remove " + bogusUrl,
Result.OK);
+
+ assertCommand(vieweruser, "feature:repo-add -i " + bogusUrl,
Result.NO_CREDENTIALS);
+ assertCommand(vieweruser, "feature:repo-remove -u " + bogusUrl,
Result.NO_CREDENTIALS);
+
+ assertCommand("karaf", "feature:repo-add -i " + bogusUrl, Result.OK);
+ assertCommand("karaf", "feature:repo-remove -u " + bogusUrl,
Result.OK);
+ }
}
diff --git a/manual/src/main/asciidoc/user-guide/security.adoc
b/manual/src/main/asciidoc/user-guide/security.adoc
index 7a736b02a1..fdc6817bbe 100644
--- a/manual/src/main/asciidoc/user-guide/security.adoc
+++ b/manual/src/main/asciidoc/user-guide/security.adoc
@@ -492,8 +492,9 @@ By default, Apache Karaf defines the following commands
ACLs:
the users with the `manager` role can execute `config:*` commands. As
`config:install` writes an arbitrary file into
the `etc` folder, it is restricted to the users with the `admin` role.
* `etc/org.apache.karaf.command.acl.feature.cfg` configuration file defines
the ACL for `feature:*` commands.
- Only the users with `admin` role can execute `feature:install`,
`feature:uninstall`,`feature:start`, `feature:stop` and `feature:update`
commands. The other `feature:*`
- commands can be executed by any user.
+ Only the users with `admin` role can execute `feature:install`,
`feature:uninstall`,`feature:start`, `feature:stop` and `feature:update`
commands. `feature:repo-add`
+ and `feature:repo-remove` require the `admin` role only when used with the
`-i`/`--install` or `-u`/`--uninstall` option, which installs or uninstalls
every
+ feature in the repository; otherwise they only require the `viewer` role. The
other `feature:*` commands can be executed by any user.
* `etc/org.apache.karaf.command.acl.jaas.cfg` configuration file defines the
ACL for `jaas:*` commands.
Only the users with `admin` role can execute `jaas:update` command. The other
`jaas:*` commands can be executed by any
user.
