karaf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guillaume Nodet <gno...@apache.org>
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 15:37:41 GMT
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