That would be acceptable to me - gives users a head's up that something more than a simple minor bump has occurred, and spurs them to action.
--Jamie On Wed, Feb 12, 2014 at 3:44 PM, Jon Anstey <jans...@gmail.com> wrote: > How about "WARNING: Bundle Vendor for X has changed, please check if this > is intentional." where X is the bundle name? > > > On Wed, Feb 12, 2014 at 3:39 PM, Jon Anstey <jans...@gmail.com> wrote: > > > Yeah, I get that it only pops up when the vendor changes. I was just > > concerned about the "malicious" code implication as that would cause > alarm > > to admins in most deployments. > > > > BTW its not a problem in the custom Karaf distro that I work on ;-) but I > > know of other Karaf users that may have this problem... > > > > > > On Wed, Feb 12, 2014 at 3:14 PM, Jamie G. <jamie.goody...@gmail.com > >wrote: > > > >> To be fare that only happens when vendors switch. Perhaps "WARNING: > Bundle > >> Vendor has changed, please review your feature, unexpected behaviours > may > >> occur". Using the car part analogy if my BMW's alternator belt was > >> replaced > >> with a FIAT part then I'd expect to be told by the mechanic - I have an > >> expected behaviour from the brand. Note, this does not prevent the > >> installation and use of the part, it just makes sure the user is aware > of > >> the switch. > >> > >> --Jamie > >> > >> > >> On Wed, Feb 12, 2014 at 2:20 PM, Jon Anstey <jans...@gmail.com> wrote: > >> > >> > No need to revert this completely IMO. The wording is too strong > >> though. I > >> > know of many companies (can't say names here) that have rebranded > >> > customized versions of Karaf that would not be able to ship with a > >> message > >> > like that in the logs. Or they would just not be able to use this > >> feature. > >> > Looks really bad if your product always spits out that it may have > >> > malicious code even if you know you put it there :-) > >> > > >> > > >> > On Wed, Feb 12, 2014 at 1:05 PM, Jamie G. <jamie.goody...@gmail.com> > >> > wrote: > >> > > >> > > Changing vendors to me would be something i'd like to be warned > >> about. I > >> > > have Apache Camel installed, with XYZ under the hood - lets me know > >> its a > >> > > franken-build. That being said, if i was going to fork and build my > >> own > >> > > camel jar to fix a local issue, why would i then need to use the > >> > override, > >> > > i'd just deploy the library, refresh, and carry on (different work > >> flows > >> > > for different folks - I do get that that's simplifying things - > >> generally > >> > > we'd end up with a large list of bundles needing changing and the > >> > override > >> > > would simplify managing that recipe update). > >> > > > >> > > Regardless, I'm open to amending how vendors are handled, if we want > >> to > >> > > change the message or scrap it all together. Personally i think > >> something > >> > > should be noted since things are changing (i'd like to know I'm > going > >> > from > >> > > Land Rover parts to something made by Ford in my Range Rover). > >> > > > >> > > As to a global on/off switch for the mechanism that would be a nice > >> > > addition. > >> > > > >> > > --Jamie > >> > > > >> > > > >> > > On Wed, Feb 12, 2014 at 12:23 PM, Guillaume Nodet < > gno...@apache.org> > >> > > wrote: > >> > > > >> > > > I just think the check is worth nothing. If someone build a > >> > customized > >> > > > version of a bundle (let's say camel), he will usually build by > >> forking > >> > > > from camel, in which case the vendor would still be the same. And > >> if > >> > the > >> > > > user wants to make things cleaner and actually change the vendor > to > >> > > reflect > >> > > > the fact that it does not come from Apache, then we throw at him a > >> > > WARNING > >> > > > log. > >> > > > Again, I don't think we should assume the user does not know what > he > >> > > does, > >> > > > I'd rather add a global flag to disable overrides if you think > it's > >> > > safer, > >> > > > but the file does not even exist by default, which means the user > >> > > actually > >> > > > know what he is doing... > >> > > > > >> > > > > >> > > > 2014-02-12 16:42 GMT+01:00 Jamie G. <jamie.goody...@gmail.com>: > >> > > > > >> > > > > 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/> > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > Cheers, > >> > Jon > >> > --------------- > >> > Red Hat, Inc. > >> > Email: jans...@redhat.com > >> > Web: http://redhat.com > >> > Twitter: jon_anstey > >> > Blog: http://janstey.blogspot.com > >> > Author of Camel in Action: http://manning.com/ibsen > >> > > >> > > > > > > > > -- > > Cheers, > > Jon > > --------------- > > Red Hat, Inc. > > Email: jans...@redhat.com > > Web: http://redhat.com > > Twitter: jon_anstey > > Blog: http://janstey.blogspot.com > > Author of Camel in Action: http://manning.com/ibsen > > > > > > -- > Cheers, > Jon > --------------- > Red Hat, Inc. > Email: jans...@redhat.com > Web: http://redhat.com > Twitter: jon_anstey > Blog: http://janstey.blogspot.com > Author of Camel in Action: http://manning.com/ibsen >
