karaf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jon Anstey <jans...@gmail.com>
Subject Re: git commit: [KARAF-2753] Logging for override mechanism. Added additional logging and unit test to trigger log events
Date Wed, 12 Feb 2014 19:14:27 GMT
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 <janstey@gmail.com> 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. <jamie.goodyear@gmail.com>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 <janstey@gmail.com> 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. <jamie.goodyear@gmail.com>
>> > 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 <gnodet@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.goodyear@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 <
>> gnodet@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 <
>> bcanhome@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 <gnodet@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 <jgoodyear@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 <jgoodyear@apache.org>
>> > > > > > > > > Authored: Wed Feb 12 10:29:10 2014 -0330
>> > > > > > > > > Committer: jgoodyear <jgoodyear@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/>
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> >
>> >
>> > --
>> > 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
>> >
>>
>
>
>
> --
> 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
>



-- 
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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message