Return-Path: X-Original-To: apmail-karaf-dev-archive@minotaur.apache.org Delivered-To: apmail-karaf-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id DDDA910AE8 for ; Wed, 12 Feb 2014 17:50:49 +0000 (UTC) Received: (qmail 37076 invoked by uid 500); 12 Feb 2014 17:50:49 -0000 Delivered-To: apmail-karaf-dev-archive@karaf.apache.org Received: (qmail 37039 invoked by uid 500); 12 Feb 2014 17:50:48 -0000 Mailing-List: contact dev-help@karaf.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@karaf.apache.org Delivered-To: mailing list dev@karaf.apache.org Received: (qmail 37028 invoked by uid 99); 12 Feb 2014 17:50:48 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 12 Feb 2014 17:50:47 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of janstey@gmail.com designates 209.85.160.178 as permitted sender) Received: from [209.85.160.178] (HELO mail-yk0-f178.google.com) (209.85.160.178) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 12 Feb 2014 17:50:41 +0000 Received: by mail-yk0-f178.google.com with SMTP id 79so16117212ykr.9 for ; Wed, 12 Feb 2014 09:50:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=fljcCAwEGLpyeZbpMxZBkpV+Z4wwbQptoCf+ISCJYDw=; b=aRM9Wv9oZdIqpwv0INL377XkpMU8lcll/2ogzg44sto/SI/7rlRa7f5LytITcFRjgR zhOeezWWizxF9EYxB8ebsWMMNIX/gMWpkF2Pzc4c/nre4YwIIbe+oA3NFtK+j6iiHvlV go86DMwBNybkBP3y3mk15ijVzPWK/2UPgesveDe0AIlPqt3OK0LH/Ac/baMSDyfsg25D b13QKQRPxnlm6S6ad/NDlG2HDxE+QNT8PbT+1Szov2BvsPGoK3w36We4FaYT+rbsb9Fo G13Yst4lNTVrrzV2DY0XgdCESONp6i2+qJP41Kj7DRxXI+AUomndXJ7Vmre09purLv1D wvJA== MIME-Version: 1.0 X-Received: by 10.236.37.105 with SMTP id x69mr40306286yha.15.1392227419791; Wed, 12 Feb 2014 09:50:19 -0800 (PST) Received: by 10.170.45.10 with HTTP; Wed, 12 Feb 2014 09:50:19 -0800 (PST) In-Reply-To: References: Date: Wed, 12 Feb 2014 14:20:19 -0330 Message-ID: Subject: Re: git commit: [KARAF-2753] Logging for override mechanism. Added additional logging and unit test to trigger log events From: Jon Anstey To: dev@karaf.apache.org Content-Type: multipart/alternative; boundary=089e01229766cda79f04f239340b X-Virus-Checked: Checked by ClamAV on apache.org --089e01229766cda79f04f239340b Content-Type: text/plain; charset=ISO-8859-1 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. 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 > 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. : > > > > > 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 > > > 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 : > > > > > > > > > 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 : > > > > > > > > > > > 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 : > > > > > > > > > > > > > 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 > > > > > > > Authored: Wed Feb 12 10:29:10 2014 -0330 > > > > > > > Committer: jgoodyear > > > > > > > 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 res = Overrides.override( > > > > > > > + Arrays.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 Committer & PMC > > > > > OPS4J Pax Web > > > Committer > > > > & > > > > > Project Lead > > > > > OPS4J Pax for Vaadin < > > > http://team.ops4j.org/wiki/display/PAXVAADIN/Home> > > > > > Commiter & Project Lead > > > > > blog > > > > > > > > > > > > > > > -- Cheers, Jon --------------- Red Hat, Inc. Email: janstey@redhat.com Web: http://redhat.com Twitter: jon_anstey Blog: http://janstey.blogspot.com Author of Camel in Action: http://manning.com/ibsen --089e01229766cda79f04f239340b--