karaf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Tran <dant...@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 21:08:39 GMT
Could you put this in FAQ how to turn off this logger?

-D


On Wed, Feb 12, 2014 at 12:36 PM, Guillaume Nodet <gnodet@apache.org> wrote:

> 2014-02-12 21:02 GMT+01:00 Jamie G. <jamie.goodyear@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 <gnodet@apache.org>
> > wrote:
> >
> > > 2014-02-12 17:35 GMT+01:00 Jamie G. <jamie.goodyear@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 <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/>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

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