[karaf-4.4.x] Restrict feature:repo-add -i and feature:repo-remove -u to admin role - #2884
Open
jbonofre wants to merge 2 commits into
Open
[karaf-4.4.x] Restrict feature:repo-add -i and feature:repo-remove -u to admin role#2884jbonofre wants to merge 2 commits into
jbonofre wants to merge 2 commits into
Conversation
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. Backport of apache#2883 to karaf-4.4.x.
Test Results 656 files ±0 656 suites ±0 1h 4m 4s ⏱️ - 2m 20s For more details on these errors, see this check. Results for commit bc3cb21. ± Comparison against base commit 47efa80. ♻️ This comment has been updated with latest results. |
…move Same issue as apache#2883: 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backport of #2883 to
karaf-4.4.x.feature:repo-add -i <url>andfeature:repo-remove -u <url>callFeaturesServiceImpl.addRepository()/removeRepository()with the install/uninstall flag set, which installs (and starts) or uninstalls every feature in the given repository.repo-addnorrepo-removehad an entry in thefeaturescope's command ACL, so any SSH user holding only theviewerrole could reach them and have Karaf install/start an arbitrary attacker-supplied features repository — bypassing the admin-only ACL already enforced onfeature:install/feature:uninstall.bundle:refresh -fand similar bundle-scope commands: only the-i/-uoption variants now requireadmin; plainrepo-add/repo-remove(which don't install/uninstall anything) remain open as before, so there's no behavior change beyond closing the escalation path.featureACL is defined on this branch: the standard assemblyfeature.xml, theinstanceetctemplate, and the itest config fixture.FeatureSshCommandSecurityTest#testFeatureRepoCommandSecurityViaSsh) exercising the ACL boundary for both a viewer and an admin user, using a non-existent repository URL so nothing is actually installed/removed.Test plan
mvn -pl itests/test test-compilepassesitests/test) run in CI