I'm tempted to -1 this change. What kind of problems are you trying to solve here ? Imho, such code is unnecessary because there are many other ways to introduce so called "malicious" code. If one wants to be safe, there is already an existing way to solve the problem which is signed bundles.
Now, an example on how to introduce "malicious" code : if such a bundle is installed first, the features service will think the "correct" bundle is already installed and will not install the "safe" bundle. This can be done by manually installing the bundle before installing features, or by adding it to the etc/startup.properties. Another option is just to hack the features file manually and change the url of the bundle, it will have exactly the same effect. In addition, checking the vendor is not a guarantee, as if someone wanted to "fake" a bundle, setting that header is not more difficult than changing the symbolic name or version. I've had a use case where the user wanted to make sure that no "malicious" code is introduced or used. In such a case, there is already an existing solution which is fully supported by OSGi (and Karaf) which is signed bundles. It works well and it's secured. Well, secured to the point that you control the file system. In all cases, if you don't trust the file system, there's no possible way to secure the OSGi framework (just because classes are read from the file system). Last, there is no possible misuse of the overrides really. If you add random bundles, it will most of the case have no effects, or at least, not more than if you had installed them manually before. We don't add any checks in the bundle:update command, so I don't really see why we'd add those here. On a side note, I was wondering about starting a slightly broader discussion about patching, which is related to this particular feature and I hope to do so this week or the next. 2014-02-12 15:00 GMT+01:00 <[email protected]>: > Updated Branches: > refs/heads/master d2af093dd -> 36808c560 > > > [KARAF-2753] Logging for override mechanism. Added additional logging and > unit test to trigger log events > > > Project: http://git-wip-us.apache.org/repos/asf/karaf/repo > Commit: http://git-wip-us.apache.org/repos/asf/karaf/commit/36808c56 > Tree: http://git-wip-us.apache.org/repos/asf/karaf/tree/36808c56 > Diff: http://git-wip-us.apache.org/repos/asf/karaf/diff/36808c56 > > Branch: refs/heads/master > Commit: 36808c5607d3fc0de40861146775e10b7c248e59 > Parents: d2af093 > Author: jgoodyear <[email protected]> > Authored: Wed Feb 12 10:29:10 2014 -0330 > Committer: jgoodyear <[email protected]> > Committed: Wed Feb 12 10:29:10 2014 -0330 > > ---------------------------------------------------------------------- > .../karaf/features/internal/Overrides.java | 25 ++++++++++- > .../karaf/features/internal/OverridesTest.java | 47 ++++++++++++++++++++ > 2 files changed, 71 insertions(+), 1 deletion(-) > ---------------------------------------------------------------------- > > > > http://git-wip-us.apache.org/repos/asf/karaf/blob/36808c56/features/core/src/main/java/org/apache/karaf/features/internal/Overrides.java > ---------------------------------------------------------------------- > diff --git > a/features/core/src/main/java/org/apache/karaf/features/internal/Overrides.java > b/features/core/src/main/java/org/apache/karaf/features/internal/Overrides.java > index 655dfea..8397222 100644 > --- > a/features/core/src/main/java/org/apache/karaf/features/internal/Overrides.java > +++ > b/features/core/src/main/java/org/apache/karaf/features/internal/Overrides.java > @@ -48,6 +48,7 @@ public class Overrides { > private static final Logger LOGGER = > LoggerFactory.getLogger(Overrides.class); > > private static final String OVERRIDE_RANGE = "range"; > + private static final String VENDOR_WARNING = "Malicious code possibly > introduced by patch override, see log for details"; > > /** > * Compute a list of bundles to install, taking into account > overrides. > @@ -86,6 +87,7 @@ public class Overrides { > if (manifest != null) { > String bsn = getBundleSymbolicName(manifest); > Version ver = getBundleVersion(manifest); > + String ven = getBundleVendor(manifest); > String url = info.getLocation(); > for (Clause override : overrides) { > Manifest overMan = > manifests.get(override.getName()); > @@ -111,10 +113,26 @@ public class Overrides { > range = VersionRange.parseVersionRange(vr); > } > > + String vendor = getBundleVendor(overMan); > > + // Before we do a replace, lets check if vendors > change > + if (ven == null) { > + if (vendor != null) { > + LOGGER.warn(VENDOR_WARNING); > + } > + } else { > + if (vendor == null) { > + LOGGER.warn(VENDOR_WARNING); > + } else { > + if (!vendor.equals(ven)) { > + LOGGER.warn(VENDOR_WARNING); > + } > + } > + } > // The resource matches, so replace it with the > overridden resource > // if the override is actually a newer version > than what we currently have > if (range.contains(ver) && ver.compareTo(oVer) < > 0) { > + LOGGER.info("Overriding original bundle " + > url + " to " + override.getName()); > ver = oVer; > url = override.getName(); > } > @@ -178,6 +196,11 @@ public class Overrides { > return bsn; > } > > + private static String getBundleVendor(Manifest manifest) { > + String ven = > manifest.getMainAttributes().getValue(Constants.BUNDLE_VENDOR); > + return ven; > + } > + > private static Manifest getManifest(String url) throws IOException { > InputStream is = new URL(url).openStream(); > try { > @@ -205,4 +228,4 @@ public class Overrides { > } > return cs[0].getName(); > } > -} > \ No newline at end of file > +} > > > http://git-wip-us.apache.org/repos/asf/karaf/blob/36808c56/features/core/src/test/java/org/apache/karaf/features/internal/OverridesTest.java > ---------------------------------------------------------------------- > diff --git > a/features/core/src/test/java/org/apache/karaf/features/internal/OverridesTest.java > b/features/core/src/test/java/org/apache/karaf/features/internal/OverridesTest.java > index 46d163a..79e2015 100644 > --- > a/features/core/src/test/java/org/apache/karaf/features/internal/OverridesTest.java > +++ > b/features/core/src/test/java/org/apache/karaf/features/internal/OverridesTest.java > @@ -42,6 +42,9 @@ public class OverridesTest { > private File b101; > private File b102; > private File b110; > + private File c100; > + private File c101; > + private File c110; > > @Before > public void setUp() throws IOException { > @@ -72,6 +75,50 @@ public class OverridesTest { > .set("Bundle-Version", "1.1.0") > .build(), > new FileOutputStream(b110)); > + > + c100 = File.createTempFile("karafc", "-100.jar"); > + copy(TinyBundles.bundle() > + .set("Bundle-SymbolicName", bsn) > + .set("Bundle-Version", "1.0.0") > + .set("Bundle-Vendor", "Apache") > + .build(), > + new FileOutputStream(c100)); > + > + c101 = File.createTempFile("karafc", "-101.jar"); > + copy(TinyBundles.bundle() > + .set("Bundle-SymbolicName", bsn) > + .set("Bundle-Version", "1.0.1") > + .set("Bundle-Vendor", "NotApache") > + .build(), > + new FileOutputStream(c101)); > + > + c110 = File.createTempFile("karafc", "-110.jar"); > + copy(TinyBundles.bundle() > + .set("Bundle-SymbolicName", bsn) > + .set("Bundle-Version", "1.1.0") > + .set("Bundle-Vendor", "NotApache") > + .build(), > + new FileOutputStream(c110)); > + } > + > + @Test > + public void testDifferentVendors() throws IOException { > + File props = File.createTempFile("karaf", "properties"); > + Writer w = new FileWriter(props); > + w.write(c101.toURI().toString()); > + w.write("\n"); > + w.write(c110.toURI().toString()); > + w.write("\n"); > + w.close(); > + > + List<BundleInfo> res = Overrides.override( > + Arrays.<BundleInfo>asList(new > Bundle(c100.toURI().toString())), > + props.toURI().toString()); > + assertNotNull(res); > + assertEquals(1, res.size()); > + BundleInfo out = res.get(0); > + assertNotNull(out); > + assertEquals(c101.toURI().toString(), out.getLocation()); > } > > @Test > >
