My interpretation is that a bundle is being updated by its maintainer, if a different group is providing the replacement bundle then Karaf should be making some noise about it as its masquerading as being what was originally intended by the feature provider. I'm up for different wordings however. What would you suggest?
On Wed, Feb 12, 2014 at 12:07 PM, Guillaume Nodet <gno...@apache.org> wrote: > Yes, I was going to add that I had no problems saying a bundle has been > overridden (though not sure if it has to be with a WARNING level). > It's really the vendor check which I don't get and the log of "Malicious > code possibly introduced by patch override, see log for details". > > > 2014-02-12 16:30 GMT+01:00 Achim Nierbeck <bcanh...@googlemail.com>: > > > 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/> > > >
