Well, I hope you didn't get distracted by my comment. Though as far as I can see the change only introduced some logging to let the user know something changed due to adding another feature, I think this is a viable solution, especially when looking for failures or unintended changes. No?
2014-02-12 16:15 GMT+01:00 Guillaume Nodet <gno...@apache.org>: > 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 <jgoody...@apache.org>: > > > 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 <jgoody...@apache.org> > > Authored: Wed Feb 12 10:29:10 2014 -0330 > > Committer: jgoodyear <jgoody...@apache.org> > > 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 > > > > > -- Apache Karaf <http://karaf.apache.org/> Committer & PMC OPS4J Pax Web <http://wiki.ops4j.org/display/paxweb/Pax+Web/> Committer & Project Lead OPS4J Pax for Vaadin <http://team.ops4j.org/wiki/display/PAXVAADIN/Home> Commiter & Project Lead blog <http://notizblog.nierbeck.de/>
