search

luisgoncalves / xades4j

A Java library for XAdES signature services
GNU Lesser General Public License v3.0
111 stars 66 forks source link

Add Compatibility to Java17 #239

Closed stianrincon closed 2 years ago

stianrincon commented 3 years ago

Since Java 16 reflective access was turn off by default, but we still can do this:

--illegal-access=permit

The problem with Java 17 is reflective access was removed completely, meaning --illegal-access=permit cannot be used any more.

It would be nice if we can use xades4j with java 17.

luisgoncalves commented 3 years ago

I'll investigate where the deps on reflective access come from and how to circumvent them.

luisgoncalves commented 2 years ago

@stianrincon Can you provide a stack trace of one of the errors in Java 17? Is it when using the PKCS11 key provider? Tried running the tests in Java 16 and 17 and the build passes (https://github.com/luisgoncalves/xades4j/pull/240). However, those build do not run the tests that use the PKCS11 provider.

Btw, what version of xades4j are you using?

EDIT: I investigated a bit more and PKCS11KeyStoreKeyingDataProvider doesn't work with Java 9+, due to a change on how security providers are configured. A "no such method" exception is thrown, but I didn't run into illegal accesses yet.

stianrincon commented 2 years ago

Hi @luisgoncalves Thank you so much for such as fast response,

Answering your questions:

  1. xades4j version 1.6.0
  2. I'm signing a document using XadesEpesSigningProfile
  3. this is the error trace:
        xades4j.utils.XadesProfileResolutionException: java.lang.ExceptionInInitializerError
        at xades4j.utils.XadesProfileCore.getInstance(XadesProfileCore.java:223)
        at xades4j.production.XadesSigningProfile.newSigner(XadesSigningProfile.java:113)

Once again thank you so much, if you guide me I can help you create a PR.

luisgoncalves commented 2 years ago

Can you get the nested exceptions (or at least the message) of that ExceptionInInitializerError? That would help understand the root cause.

How did you find that there were problems with reflective accesses?

luisgoncalves commented 2 years ago

I investigated a bit more and I'd expect you to get the "no such method" exception I mentioned above when running in Java 16 or 17. This is due to xades4j looking up a constructor of SunPKCS11 that no longer exists.

I was able to reproduce the illegal reflective access if I try to use the available constructor of sun.security.pkcs11.SunPKCS11 via reflection, but it should never get to that point due to the error I mentioned above.

Are you using PKCS11KeyStoreKeyingDataProvider or did you hit the illegal access in another scenario?

A possible workaround for the illegal accesses is adding --add-exports jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED (tested with both OpenJDK 16 and 17), but that doesn't fix the problem of using a constructor that was removed.

Fixing this for good requires using new APIs in Java's Provider class, which requires Java 9+, which is a breaking change (currently only Java 8 is required to run xades4j). It's probably about time that's done, but I need to think a bit more and eventually come up with a plan for a new major release (which can include other long due cleanups).

luisgoncalves commented 2 years ago

@stianrincon did you have a chance to see my comments above? Which error are you hitting?

stianrincon commented 2 years ago

Hi @luisgoncalves I made a debug and... the error is thrown in this exact line https://github.com/google/guice/blob/2a45130f8962e21a3df075c42d1c4c5c61ec5bba/src/com/google/inject/BindingProcessor.java#L229

Now, this is what I have in my code:

        var xadesEpesSigningProfile = new XadesEpesSigningProfile(getKeyingDataProvider(), signaturePolicyInfoProvider);
        xadesEpesSigningProfile.withSignaturePropertiesProvider(signaturePropertiesProvider);
        xadesEpesSigningProfile.withBasicSignatureOptions(new BasicSignatureOptions().signKeyInfo(true).includePublicKey(true)
                .includeSigningCertificate(SigningCertificateMode.SIGNING_CERTIFICATE));
        xadesEpesSigningProfile..newSigner()

and the error is thrown when calling newSigner()

I use java16 and I have to include this --illegal-access=permit when executing the program

And finally, thank you so much!!!

if you want, we can have a quick meeting and I can show you the code, just lemme know :)

luisgoncalves commented 2 years ago

Hi @stianrincon,

Earlier you mentioned the following error:

xades4j.utils.XadesProfileResolutionException: java.lang.ExceptionInInitializerError
        at xades4j.utils.XadesProfileCore.getInstance(XadesProfileCore.java:223)
        at xades4j.production.XadesSigningProfile.newSigner(XadesSigningProfile.java:113)

Can you get the causes of that exception? ExceptionInInitializerError is thrown when there's an error in a static constructor/class initializer, so I'd like to know what the original exception is.

Also:

stianrincon commented 2 years ago

As mentioned before the exception ExceptionInInitializerError is thrown in this method https://github.com/google/guice/blob/2a45130f8962e21a3df075c42d1c4c5c61ec5bba/src/com/google/inject/BindingProcessor.java#L229

An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.ReflectUtils$2 (/.gradle/caches/modules-2/files-2.1/com.google.inject/guice/2.0/a4c67006178262122e93121e94fff306fcf0cda1/guice-2.0.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.ReflectUtils$2
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
luisgoncalves commented 2 years ago

Ah, the illegal reflective access is coming from Guice itself. When you first pointed to Guice I thought the exception could be surfacing there but be caused by something in xades4j.

Anyway, a few days ago xades4j 1.7.0 was released, which depends on a more recent version of Guice that shouldn't have those issues. The Guice upgrade was done some time ago, but I hadn't released a new version of xades4j yet.

The builds I did on Java 16 and 17 passed, and those were based on the master branch, which included the aforementioned changes. Can you try the latest xades4j version?

stianrincon commented 2 years ago

Thank you so so much, with the new version 1.7.0 and java16 the command --illegal-access=permit is not needed. I'll try with java17 and I'll let you know.

Thanks man!!!

luisgoncalves commented 2 years ago

Great. I'm confident it works well.

I'll close the issue. Re-open if needed.

stianrincon commented 2 years ago

@luisgoncalves tested it with java17 and works like a charm.... THANK YOU!!!!