Hi
Talking about wording. It would be lovely if the wording of this WARN
from pax could be improved as well.
eg this text
"This is deprecated & discouraged & just evil."
maybe tone that down a bit, for example to
"This is deprecated & discouraged. See more details at TODO link here"
This is from Karaf 2.3.3 with Camel.
2014-02-13 09:55:18,255 | WARN | l Console Thread |
MavenRepositoryURL | maven.commons.MavenRepositoryURL
116 | 1 - org.ops4j.pax.url.mvn - 1.3.6 | Repository spec
http://scriptengines.googlecode.com/svn/m2-repo/ does not contain an
identifier. This is deprecated & discouraged & just evil.
2014-02-13 09:55:22,551 | WARN | l Console Thread |
MavenRepositoryURL | maven.commons.MavenRepositoryURL
116 | 1 - org.ops4j.pax.url.mvn - 1.3.6 | Repository spec
http://maven.restlet.org/ does not contain an identifier. This is
deprecated & discouraged & just evil.
On Wed, Feb 12, 2014 at 6:50 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
--
Claus Ibsen
-----------------
Red Hat, Inc.
Email: [email protected]
Twitter: davsclaus
Blog: http://davsclaus.com
Author of Camel in Action: http://www.manning.com/ibsen
Make your Camel applications look hawt, try: http://hawt.io
