Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bouncy Castle dependency is not shaded in Java 9+ #1193

Closed
wesleyhillyext opened this issue Nov 17, 2022 · 3 comments · Fixed by #1197
Closed

Bouncy Castle dependency is not shaded in Java 9+ #1193

wesleyhillyext opened this issue Nov 17, 2022 · 3 comments · Fixed by #1197

Comments

@wesleyhillyext
Copy link

It appears that starting with version 3.13.19, the org.bouncycastle dependency bundled inside the Snowflake JDBC driver .jar is no longer being shaded.

This is causing problems for our applications which have a dependency on both the Snowflake JDBC driver and a direct dependency on Bouncy Castle, as now those applications are including two copies of Bouncy Castle. Our build pipeline enforces the one version rule and our applications will not build while this conflict exists, so we're blocked from upgrading beyond 3.13.18.

We've been down this road before, when the Snowflake JDBC driver didn't shade a JNI library (I forget which one exactly, it was a while ago), which also blocked our upgrade for a while until that dependency was removed.

I am aware that this ticket is in opposition to #608 (JDBC driver should not be shade/relocate Bouncycastle), and that ticket may be why Bouncy Castle stopped being shaded! But even the requestor of that ticket acknowledged that unshaded dependencies cause their own problems.

Personally, I think Snowflake has to pick a lane here. Either provide a fat jar and shade every single dependency, or provide a skinny jar which allows clients to manage dependencies like regular Java libraries do (what #174 calls for). Or provide both. Trying to pick a middle lane where only a fat-jar is provided, but which doesn't shade all dependencies, just causes problems.

@wesleyhillyext
Copy link
Author

After Snowflake Support indicated that snowflake-jdbc should indeed be shading bouncycastle, we looked into this more.

It turns out that the snowflake-jdbc JAR is a multi-release JAR. If you're using Java 8 or lower, then bouncycastle is indeed relocated. But for anyone using Java 9+ (we are using 15), the JAR includes an additional copy of bouncycastle which is not shaded!

To use the ASN1BitString class from Bouncy Castle as an example, all the following .class files exist in the snowflake-jdbc JAR since version 3.13.19:

  1. net/snowflake/client/jdbc/internal/org/bouncycastle/asn1/ASN1BitString.class
  2. META-INF/versions/9/org/bouncycastle/asn1/ASN1BitString.class
  3. META-INF/versions/11/org/bouncycastle/asn1/ASN1BitString.class
  4. META-INF/versions/15/org/bouncycastle/asn1/ASN1BitString.class

So that causes two problems in Java 9+:

  1. The non-shaded copy of Bouncy Castle causes conflicts with the real Bouncy Castle JAR.
  2. Unless the Snowflake driver is using reflection or similar to access that non-shaded copy in Java 9+, then the driver is using the Java 8 version of Bouncy Castle (at net.snowflake.client.jdbc.internal.org.bouncycastle) when it should presumably be using the versions for later Java versions.

Basically, the shading just seems to be incomplete as it hasn't taken in to account the multi-release nature of Bouncy castle.

@wesleyhillyext wesleyhillyext changed the title Bouncy Castle dependency is not shaded Bouncy Castle dependency is not shaded in Java 9+ Nov 18, 2022
@devurandom
Copy link

We see the following error with https://mvnrepository.com/artifact/net.snowflake/snowflake-jdbc/3.14.5 (using docker.io/library/eclipse-temurin:11.0.21_9-jre-alpine and building our project using Clojure 1.11.1.1413):

Exception in thread "main" java.lang.NoClassDefFoundError: net/snowflake/client/jdbc/internal/org/bouncycastle/crypto/Mac (wrong name: org/bouncycastle/crypto/Mac)

Could this be a regression or is this more likely caused by how we build our project?

@sfc-gh-dprzybysz
Copy link
Collaborator

We have merged PR #1621 fixing the relocation of classes in META-INF and it will be included in the February 2024 release. If you want to test it now you can build the driver from source as described in README

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants