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 <[email protected]> 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. <[email protected]>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 <[email protected]> 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. <[email protected]> >> > 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 <[email protected]> >> > > 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. <[email protected]>: >> > > > >> > > > > 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 < >> [email protected] >> > > >> > > > > 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 < >> [email protected] >> > >: >> > > > > > >> > > > > > > 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 <[email protected] >> >: >> > > > > > > >> > > > > > > > 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 >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > -- >> > > > > > > >> > > > > > > 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: [email protected] >> > 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: [email protected] > 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: [email protected] Web: http://redhat.com Twitter: jon_anstey Blog: http://janstey.blogspot.com Author of Camel in Action: http://manning.com/ibsen
