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
