2014-02-12 21:02 GMT+01:00 Jamie G. <jamie.goody...@gmail.com>:
Those updates are performed as a user console session command - one at a
time. An override file could contain many substitutions (bulk operation),
as such Karaf here is alerting the user to a change they may not realize
has happened.
Well, automation is usually not a bad idea, it's usually faster, more
reproductible, and safer. But again, you're assuming that if someone makes
a decision to put a bundle url in that override file, it's different than
making the decision to update a bundle with the url of that file. I just
don't get it, but I'll stop arguing and loosing time on a log statement,
it's not worth it.
Having a switch that may invoke a signed bundle installation only Karaf
could be interesting.
--jamie
On Wed, Feb 12, 2014 at 4:08 PM, Guillaume Nodet <gno...@apache.org>
wrote:
2014-02-12 17:35 GMT+01:00 Jamie G. <jamie.goody...@gmail.com>:
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).
It all depends on the workflow, the number of containers to modify, how
often features are deployed or undeployed, wether the one installing
features is the one that validates them, etc...
At some point, manual intervention can be very painful. So that's
right,
it's not the usual workflow we've supported so far, but it does not
mean
it's less secured In all cases, things have to be tested and verified
before put into production.
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).
Or it could be like changing the radio in your car .... ;-)
What I don't get is why that would be the only place for such a check ?
If we consider that changing the vendor of a bundle is risky, we need
to
put that check in bundle:update, file install, web console, etc...
You know that you can update camel-core with asm4 by using
bundle:update,
right ? We don't have any checks here, and that's much more risky than
when you already ensured the symbolic names are the same and version
expected to be compatible.
If security is really an issue, even if not going as far as using
signed
bundles, one possible way would be to restrict bundle installation to
trusted bundles. By that, I mean adding a setting which would lead to
only
accept externally signed bundles (the *.asc file uploaded to maven
repo)
and verify them against a trusted key store. I think this would be a
good
way to actually address the problem, if we think there's a problem.
Guillaume
As to a global on/off switch for the mechanism that would be a nice
addition.
Yeah, I can add that, though it's not as if this feature was triggered
automatically, as you have to create this known file, so there's
always a
conscious decision made at some point.
Guillaume
--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/>
