I would agree if that assumption you wrote when installing features were true.
Let's say I have two features which both references the same bundle but with a slightly different version. After installing both features, you will usually (unless you use OBR resolver + flag the bundle as a dpendency). However, the fact that both bundles are installed does not mean that each feature will use its "intended" bundle. A simple refresh will wire the two features to the higher version of the bundle. In the same idea, if a bundle is pre-installed with a slightly higher version (compatible), it will be used even if the older one will be installed by the features service. There is no isolation (unless you use regions, and in such a case, we may want to improve the override feature to let user specify more information about the regions). So installing a bundle can actually affect other bundles if the container is restarted or refreshed. So when you say feature ABC has been tested with bundle M 1.0.1 but feature XYZ does not support M 1.0.2, if that's the case, the override won't change anything on that side, because the only way to overcome this problem is to change XYZ bundles (either reducing the range, or upgrading the code to support the newer bundle) or not go to 1.0.2. Now, I assume the user also test his changes. If we don't assume that, we also need to warn when installing new feature repositories or "untrusted" repositories or features. That's not what we do, we usually trust the user (we sometimes ask for a confirmation, but that's only when in interactive mode). 2014-02-12 16:37 GMT+01:00 Jamie G. <jamie.goody...@gmail.com>: > It's been my experience that users rarely use signed jars, until that is > standard practice I think we should be providing some cursory checks. As > such my additional logging was intended to provide some sort of safety net > to catch changes in the system. > > I under stand that a patch would be used for feature XYZ overriding, but > similar bundles may not require changes in feature ABC, or DEF - so noting > to the user that things are changing there too would be appropriate. > > As to there being no or little effects of these overrides to a feature's > bundle set, I'd strongly disagree -- feature ABC may have been tested with > bundle M 1.0.1 becoming 1.0.2 but feature XYZ may get borked at this stage. > I think the warning on XYZ's feature is requried. > > --Jamie > > > > On Wed, Feb 12, 2014 at 12:00 PM, Achim Nierbeck <bcanh...@googlemail.com > >wrote: > > > 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/> > > >
