honeycombio / libhoney-java

Java library for sending data to Honeycomb
Apache License 2.0
9 stars 12 forks source link

Offer a thin (non-shaded) version of libhoney-java #33

Closed pete-woods closed 4 years ago

pete-woods commented 4 years ago

In our projects, when we create shaded versions of our applications that depend on libhoney, there is a "META-INF race" for which version of the shaded dependencies from this library get listed in the final shaded application Jar.

This means vulnerability scanners get (rightly) upset, as the application Jar contains old versions of dependencies (usually it's Jackson).

Could you offer a plain old Jar with no shading, so we can override the dependency versions, and not have to do tricks like excluding the META-INF from your library?

For prior art, see how launchdarkly offer a "thin" version of their library, that you can use by specifying a maven classifier of "thin" (e.g. in Clojure's leiningen: :classifier "thin"):

MikeGoldsmith commented 4 years ago

Hey @pete-woods - thanks for raising this issue and for the links to LaunchDarkly's solution. We'll look into how that might work for us.

MikeGoldsmith commented 4 years ago

After some research, I think we can do two things fairly quickly without much effort / risk of breaking things.

1) Apply the ServicesResourceTransformer transformer that should help prevent META-INF/services conflicts.

2) As we use the Apache Shade Plugin, we end up with a original-libhoney-java-${version}.jar after packaging. I believe this is the original jar without any shaded dependencies. If we were to rename it, as per your suggestion to libhoney-java-${version}-thin.jar and publish that alongside the shaded one, I think you could then use a classifier to grab that instead of the uber/fat jar.

If you think it'll help, I'd be happy to push those changes to a branch and generate a SNAPSHOT for you test with.

pete-woods commented 4 years ago

It'd be great to try out.

Currently I've had to put an exclusion in our vuln scanner for this particular thing (you're shipping an old Jackson with some vulns in), and for obvious reasons it can't stay that way for long.

pete-woods commented 4 years ago

My ideal result is a plain old jar with dependencies I can override to my needs. I realise this has the potential downside of using a version of your dependencies you've not tested with, but that's a risk I'm comfortable with.

MikeGoldsmith commented 4 years ago

Sure, I understand the want for a plain jar. I think the steps above are worth looking at to see if they work for you and decide on next actions after that. I don't want to fiddle with the dependency chain too much if we don't need to.

pete-woods commented 4 years ago

Isn't 2) exactly what I'm asking for? I'd be perfectly happy with that

MikeGoldsmith commented 4 years ago

I think it is, but I haven't tested it all out yet. The current setup is also a lot more verbose / magic with the maven plugins compared to what LaunchDarkly have done with their gradle build files.

I'll push my changes to a branch so I can share what it'll look like and we can do some testing.

pete-woods commented 4 years ago

Sounds great!

pete-woods commented 4 years ago

Hey @MikeGoldsmith! Did you manage to get that branch up yet? (apologies for the nag)

MikeGoldsmith commented 4 years ago

Hey @pete-woods - no, I was caught up with something else. I'll do it now - won't take long :+1:

MikeGoldsmith commented 4 years ago

I've pushed the change to mike/thin-jar branch. Let me know what you think / find 💯

pete-woods commented 4 years ago

Brilliant! Will have a look!

pete-woods commented 4 years ago

Hi! I built your branch to make a 1.3.0-SNAPSHOT, but it still seems to be just the same as the main branch.

On mike/thin-jar I can still see 2.9.9.2 in the META-INF:

ug -rz -F 2.9.9.2 /Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.3.0-SNAPSHOT/libhoney-java-1.3.0-SNAPSHOT.jar
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.3.0-SNAPSHOT/libhoney-java-1.3.0-SNAPSHOT.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.properties}:version=2.9.9.2
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.3.0-SNAPSHOT/libhoney-java-1.3.0-SNAPSHOT.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.xml}:  <version>2.9.9.2</version>
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.3.0-SNAPSHOT/libhoney-java-1.3.0-SNAPSHOT.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.xml}:    <tag>jackson-databind-2.9.9.2</tag>
Binary file /Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.3.0-SNAPSHOT/libhoney-java-1.3.0-SNAPSHOT.jar{io/honeycomb/libhoney/shaded/com/fasterxml/jackson/databind/cfg/PackageVersion.class} matches

just the same as a 1.2.0 build from main:


ug -rz -F 2.9.9.2 /Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.2.0/libhoney-java-1.2.0.jar                  
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.2.0/libhoney-java-1.2.0.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.properties}:version=2.9.9.2
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.2.0/libhoney-java-1.2.0.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.xml}:  <version>2.9.9.2</version>
/Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.2.0/libhoney-java-1.2.0.jar{META-INF/maven/com.fasterxml.jackson.core/jackson-databind/pom.xml}:    <tag>jackson-databind-2.9.9.2</tag>
Binary file /Users/pete/.m2/repository/io/honeycomb/libhoney/libhoney-java/1.2.0/libhoney-java-1.2.0.jar{io/honeycomb/libhoney/shaded/com/fasterxml/jackson/databind/cfg/PackageVersion.class} matches```
MikeGoldsmith commented 4 years ago

Hmm, okay - I guess that didn't work. We may need to specify classifiers for the build and opt-in dependencies to that. I had a couple of resources that talked though that process. I'll give it a go and report back.

Although, I think using ServiceResourceTransformer should help with META-INF/Services races as they rewrite the namespace into the built package. Would you be able to confirm that is the case?

pete-woods commented 4 years ago

The original jar does exist in libhoney/target:

% ll libhoney/target 
total 19960
drwxr-xr-x  25 pete  staff      800 Sep 15 10:09 apidocs
drwxr-xr-x   4 pete  staff      128 Sep 11 10:37 classes
drwxr-xr-x   3 pete  staff       96 Sep 11 10:37 generated-sources
drwxr-xr-x   3 pete  staff       96 Sep 11 10:37 generated-test-sources
drwxr-xr-x   4 pete  staff      128 Sep 15 10:09 javadoc-bundle-options
-rw-r--r--   1 pete  staff   789073 Sep 15 10:00 libhoney-java-1.2.0-javadoc.jar
-rw-r--r--   1 pete  staff  3935143 Sep 15 10:00 libhoney-java-1.2.0.jar
-rw-r--r--   1 pete  staff   790322 Sep 15 10:09 libhoney-java-1.3.0-SNAPSHOT-javadoc.jar
-rw-r--r--   1 pete  staff  3935342 Sep 15 10:09 libhoney-java-1.3.0-SNAPSHOT.jar
drwxr-xr-x   3 pete  staff       96 Sep 11 10:37 maven-archiver
drwxr-xr-x   3 pete  staff       96 Sep 11 10:37 maven-status
-rw-r--r--   1 pete  staff    86482 Sep 15 10:00 original-libhoney-java-1.2.0.jar
-rw-r--r--   1 pete  staff    86512 Sep 15 10:09 original-libhoney-java-1.3.0-SNAPSHOT.jar
drwxr-xr-x   2 pete  staff       64 Sep 15 10:02 surefire
drwxr-xr-x  43 pete  staff     1376 Sep 15 10:02 surefire-reports
drwxr-xr-x   3 pete  staff       96 Sep 11 10:37 test-classes

but Maven doesn't seem to install it anywhere.

pete-woods commented 4 years ago

Hmm, okay - I guess that didn't work. We may need to specify classifiers for the build and opt-in dependencies to that. I had a couple of resources that talked though that process. I'll give it a go and report back.

Although, I think using ServiceResourceTransformer should help with META-INF/Services races as they rewrite the namespace into the built package. Would you be able to confirm that is the case?

While it might help with the races, what I need is libhoney's versions to not be present at all in the thin jar - as the vuln scanner will still get upset if it sees old versions. 🤔

MikeGoldsmith commented 4 years ago

Sure, I understand. I think setting up project classifiers will allow maven to install both versions of the jar (uber & thin), and then also publish to Sonatype / maven central.

I'll look at that and let you know how I get on.

pete-woods commented 4 years ago

Sure, I understand. I think setting up project classifiers will allow maven to install both versions of the jar (uber & thin), and then also publish to Sonatype / maven central.

I'll look at that and let you know how I get on.

Awesome, thanks!

pete-woods commented 4 years ago

Hey @MikeGoldsmith! If you can outline your plan for how to do this, I can have a go at making PR myself, if that would help?

MikeGoldsmith commented 4 years ago

Hey @pete-woods - I'm sorry for the delay. I've been getting tied up with other things.

We'd very much appreciate your help on working on this. I was looking at the maven assembly plugin which can use classifiers. I didn't get too far with it though.

Thanks!

pete-woods commented 4 years ago

@MikeGoldsmith so the easiest thing to do from a technical perspective is #34. The shade plugin is designed to allow you to additionally offer a shaded jar with a different classifier, so this is pretty easy. However I suspect that is undesirable to you, as it might count as a breaking change?

I'm having a look now to see if we can use a different plugin to attach the original jar to the build with a classifier.

pete-woods commented 4 years ago

Okay, I think I've managed that in #35

pete-woods commented 4 years ago

Let me know what you think of each approach!

pete-woods commented 4 years ago

@MikeGoldsmith any views on #35 ?

MikeGoldsmith commented 4 years ago

Hey @pete-woods - thanks for your super speedy work on submitting these two solutions! I haven't had time to properly review yet, I'm in the middle of some other work. I should get time either later today or tomorrow morning.

Libhoney & Beeline are scheduled for release this week so once we decide on a path we can move fairly quickly in providing an official 'thin' jar.

MikeGoldsmith commented 4 years ago

Hey @pete-woods - I think #35 looks good. It's much simpler change than I first anticipated.

Have you been able to test with the -thin jar and see if it fixes your vulnerability failures?

pete-woods commented 4 years ago

It seems to work (I've not run the app in production yet) and I've upgrep-ed the Jar for any references to 2.9.9.2, and there are none, so it looks good from my end 👍

pete-woods commented 4 years ago

@MikeGoldsmith I've marked my PR as ready for review now, btw :)

MikeGoldsmith commented 4 years ago

Thanks @pete-woods - I'll be working on releasing libhoney & beeline later today 👍

MikeGoldsmith commented 4 years ago

@pete-woods v1.3.0 (both uber and thin) has now been published and we're waiting for Maven Central to index https://oss.sonatype.org/#nexus-search;gav~io.honeycomb.libhoney~libhoney-java~~~~kw,versionexpand

Thanks again for your contribution!