tcurdt / jdeb

This library provides an Ant task and a Maven plugin to create Debian packages from Java builds in a truly cross platform manner.
https://github.com/tcurdt/jdeb
Apache License 2.0
536 stars 322 forks source link

Shade Plugin is eliminating most BouncyCastle algorithms (?) #481

Closed christianAppl closed 8 months ago

christianAppl commented 2 years ago

We are using version 1.7 of this fine plugin and have ran into issues.

  1. This could be a simple usage error and possibly we simply missed some parameter or similar.
  2. This could be a known issue and I simply did not find the existing ticket.

In either case: My apologies!

The issue: When using the plugin from our project the BouncyCastle provider can be found and indeed does provide algorithms, but the aliases for SHA256 seem to be missing and unknown. (AlgorithmNotFound)

Finding the cause: To find the cause of our issue we checked out the jdeb project and had a look at the source code.

When running the jdeb Unit Tests BouncyCastle is providing: allAlgorithms.txt

When running the plugin from our application - it is providing: ShadedAlgorithms.txt

It is obvious, that the second set is missing entries, that we would require (30 vs. 2944 known algorithms/aliases). When setting "minimizeJar" to "false" for the maven shade plugin, everything is working fine and the issue seems to be resolved.

Assumptions: As most BouncyCastle algorithms won't be referenced directly by jdeb, the shade plugin possibly won't recognize those as required. As such algorithms/aliases are not whitelisted, and hence can be expected to be removed, when minimizing the dependency.

Questions: What are we missing? Is this actually a jdeb issue? How could it be resolved? Is this working as expected?

tcurdt commented 2 years ago

Hey @christianAppl, thanks for the report and digging through the details. Good find! As not all algorithms are referenced this might indeed happen.

I see 3 possible options 1) reference all algorithms that make sense in the context of jdeb statically and by that convince the minijar algorithm not to remove the classes 2) exclude the bouncy castle dependency from the shade plugin configuration 3) turn off the jar minimization

That's also the order of preference as this will increases the size of the resulting artifact.

I wanted to prepare a new release soon anyway. So that's a perfect time to sneak that fix in.

Do you think you could look into a creating a PR for this? So we are sure the next release works for you?

christianAppl commented 2 years ago

Thank you very much for the quick response!

We have already done that! :) We already created a private repository and took the easy way out by disabling the minimization - it works perfectly fine then and has been integrated. As soon as you let me know a fix is available, we are more than happy to test it, give feedback and drop the private repo!

Thank you very much and keep up the good work! jdeb is great! :)

tcurdt commented 2 years ago

Well, the very least I'd need to know what algorithms you are after.

christianAppl commented 2 years ago

The issue occurred when signing the resulting packages using SHA256.

"org.vafer.jdeb.DebMaker" method "sha1Hash" threw a NoSuchAlgorithm exception for the given field "digest" using it´s default value "SHA256": grafik grafik

Currently I assume, that we ourself don't require support for any further algorithms/aliases on our end, but I assume that including the SHA256 aliases and algorithms (possibly just the alias for the spelling "SHA256") should resolve this issue.

tcurdt commented 2 years ago

Hm. That's a bit odd though. That's java.security.MessageDigest and the SHA256 should not come from BouncyCastle but from the JDK.

tcurdt commented 2 years ago

Wait - isn't it "SHA-256"?

christianAppl commented 2 years ago

Also possibly a misspelling - both spellings exist. BouncyCastle does know aliases for both spellings. :)

I should give the following two caveats for my answer above:

  1. I wrote that answer from memory - would have to reproduce the issue to make claims with greater certainty.
  2. A setter for said field "digest" exists... I did not check the usage of that setter and can´t say whether a user would be able to influence that value - possibly yielding different errors concerning missing algorithms. :)
tcurdt commented 2 years ago

OK - that seems to be combination of problems.

The digest field was meant as a mapping to the bouncy castle algorithm for signing.

    static int getDigestCode(String digestName) throws PackagingException {
        if ("SHA1".equals(digestName)) {
            return HashAlgorithmTags.SHA1;
        } else if ("MD2".equals(digestName)) {
            return HashAlgorithmTags.MD2;
        } else if ("MD5".equals(digestName)) {
            return HashAlgorithmTags.MD5;
        } else if ("RIPEMD160".equals(digestName)) {
            return HashAlgorithmTags.RIPEMD160;
        } else if ("SHA256".equals(digestName)) {
            return HashAlgorithmTags.SHA256;
        } else if ("SHA384".equals(digestName)) {
            return HashAlgorithmTags.SHA384;
        } else if ("SHA512".equals(digestName)) {
            return HashAlgorithmTags.SHA512;
        } else if ("SHA224".equals(digestName)) {
            return HashAlgorithmTags.SHA224;
        } else {
            throw new PackagingException("unknown hash algorithm tag in digestName: " + digestName);
        }
    }

When we accepted a PR we introduced changing the algorithm (which before was just SHA1). The PR used the digest string that was meant for bouncy for the java MessageDigest factory. This ain't good.

  1. I think we should reference all BC algorithms we map to via HashAlgorithmTags
  2. Maybe we can untangle the sha1 method to always use the SHA-256. Not sure it makes sense to have that configurable.
tcurdt commented 8 months ago

It's been a while. Is this still a problem?

christianAppl commented 8 months ago

Hi there, I am afraid I won´t find the time to validate this any time soon. I would asume this issue to be fixed, when I come back to this, I can reopen another issue, should this not be the case. I assume we possibly would try to update to the current version in our next iteration. Thank you very much for having a look into it and keep up the good work. :)