Add BouncyCastle providers to generated AutoFeature #23527
Conversation
|
Hi @sberyozkin, It looks like GraalVM is not able to detect the So to resolve this we will need to move the What's interesting is why it works for the other tests, since we follow the same approach there...
|
|
Hi @zakkak Thanks for this analysis,
But I wonder how it can be done without tying that code to
Let me debug that a bit later today - I thought I noticed a different code branch was used in JVM mode. |
The AutoFeature is generated every time we perform a native compilation, so (in theory at least) we can still invoke |
|
As a proof of concept I tested your branch with: diff --git a/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java b/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java
index 9e7a72807f..03275e4f0a 100644
--- a/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java
+++ b/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java
@@ -1,5 +1,6 @@
package io.quarkus.deployment.steps;
+import static io.quarkus.gizmo.MethodDescriptor.ofConstructor;
import static io.quarkus.gizmo.MethodDescriptor.ofMethod;
import java.io.ObjectStreamClass;
@@ -10,6 +11,8 @@ import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.nio.charset.StandardCharsets;
+import java.security.Provider;
+import java.security.Security;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
@@ -207,6 +210,9 @@ public class NativeImageAutoFeatureStep {
}
}
+ ResultHandle bcProvider = overallCatch.newInstance(ofConstructor("org.bouncycastle.jce.provider.BouncyCastleProvider"));
+ overallCatch.invokeStaticMethod(ofMethod(Security.class, "addProvider", int.class, Provider.class), bcProvider);
+
if (!proxies.isEmpty()) {
ResultHandle proxySupportClass = overallCatch.loadClassFromTCCL(DYNAMIC_PROXY_REGISTRY);
ResultHandle proxySupport = overallCatch.invokeStaticMethod(and it makes it work. |
|
@zakkak Cool, let me experiment with the idea of a build item, might take a bit of time but I'm on it :-), thanks for the help |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
7d5651c to
b39c0ae
Compare
|
Hi @zakkak I've experimented with adding I have a question though, you suggested earlier:
So, now we add it in |
The current approach seems fine to me. What happens if you
Yes, looking at your patch I was thinking the same. It looks OK to me but I'll also ask on GraalVM's slack. |
I haven't tried yet but will do next :-) |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
b39c0ae to
c0dc625
Compare
sberyozkin
changed the title
|
@zakkak Hi, it actually works now for BC JSSE as well :-), so #17046 will get fixed - I added the code which inserts the BC JSSE (alongside BC) provider in the generated AutoFeature - the fact that the same It does look like, given the code which has to be generated to support BC JSSE that this callback to Let me try if may be Thanks |
|
Great work @sberyozkin . While at it, it might be worth testing whether we can now get rid of In my local tests after registrering the provider with the |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
c0dc625 to
ec4d371
Compare
Good point, indeed, I was able to remove it for |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
ec4d371 to
0dfc9ca
Compare
|
Hey @zakkak, unfortunately adding BC JSSE FIPS to the feature does not work yet; well, it may work in the end, but I can't verify it since I'm getting the error at #14139, so for now I'm not adding the BC JSSE FIPS specific registration code (would be easy to do - nearly identical to the BC JSSE case). |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 0dfc9ca
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖ ⚙️ JVM Tests - JDK 17 #- Failing: integration-tests/container-image/maven-invoker-way
📦 integration-tests/container-image/maven-invoker-way✖ 📦 integration-tests/container-image/maven-invoker-way/target/it/container-build-with-keycloak-default-realm✖ ✖ |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
0dfc9ca to
80bd519
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 80bd519
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖ ⚙️ JVM Tests - JDK 17 #- Failing: extensions/reactive-routes/deployment
! Skipped: extensions/agroal/deployment extensions/elytron-security-jdbc/deployment extensions/flyway/deployment and 164 more📦 extensions/reactive-routes/deployment✖ |
|
Test failures are not related |
There was a problem hiding this comment.
LGTM thanks @sberyozkin
I'll leave the decision about whether the "generic" AutoFeatureAugmentor is welcome or not to the Quarkus team :)
|
Thanks @zakkak, yeah, I'll wait for Guillaume and others to comment |
Regarding this issue, Aleksandar Gradinac had a good idea: """ |
|
Hi @zakkak Thanks, so we indeed do the built time static initialization (via
But not sure about this one - as whatever is done in the
|
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
39d3923 to
b245996
Compare
...ployment/src/main/java/io/quarkus/deployment/builditem/nativeimage/AutoFeatureAugmentor.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
b245996 to
dd7be08
Compare
|
I'd like @stuartwdouglas to have a look at this one before we backport it. |
...ployment/src/main/java/io/quarkus/deployment/builditem/nativeimage/AutoFeatureAugmentor.java
Outdated
Show resolved
Hide resolved
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
dd7be08 to
2a3668f
Compare
|
Hi @stuartwdouglas @zakkak @gsmet Now a BC specific auto feature is optionally generated. Have a look please and let me know what you think |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
2a3668f to
af17d0e
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building af17d0e
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/vertx-http/deployment
! Skipped: core/test-extension/deployment extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 308 more📦 extensions/vertx-http/deployment✖ |
| ClassCreator file = createBouncyCastleFeatureCreator(nativeImageClass); | ||
| MethodCreator beforeAn = createBeforeAnalysisMethod(file); | ||
| TryBlock overallCatch = beforeAn.tryBlock(); |
There was a problem hiding this comment.
This is common code in both if and else blocks, I would move it before the if-statement to avoid code duplication.
This would also remove the need for defining the createBeforeAnalysisMethod method.
There was a problem hiding this comment.
Hi @zakkak In both cases (here and in the other one you comment at below) it is conditional,
Optional<BouncyCastleJsseProviderBuildItem> bouncyCastleJsseProvider = getOne(bouncyCastleJsseProviders);
if (bouncyCastleJsseProvider.isPresent()) {
ClassCreator file = createBouncyCastleFeatureCreator(nativeImageClass);
MethodCreator beforeAn = createBeforeAnalysisMethod(file);
TryBlock overallCatch = beforeAn.tryBlock();
if (!bouncyCastleJsseProvider.get().isInFipsMode()) {
//...
} else {
//...
}
completeBouncyCastleFeature(file, beforeAn, overallCatch);
} else {
Optional<BouncyCastleProviderBuildItem> bouncyCastleProvider = getOne(bouncyCastleProviders);
if (bouncyCastleProvider.isPresent()) {
ClassCreator file = createBouncyCastleFeatureCreator(nativeImageClass);
MethodCreator beforeAn = createBeforeAnalysisMethod(file);
TryBlock overallCatch = beforeAn.tryBlock();
//...
completeBouncyCastleFeature(file, beforeAn, overallCatch);
}
}
See, I can't move it the class creator code above the top if (and closing it outside it).
Am I missing something ?
Thanks
There was a problem hiding this comment.
I.e, there is a branch which allows for no BC or BC JSSE providers found at all
There was a problem hiding this comment.
@zakkak I'll collapse both ifs
There was a problem hiding this comment.
@zakkak I've done a small refactoring, hope it looks cleaner now, thanks
There was a problem hiding this comment.
You are right, I missed the if-statement in the else body... Sorry for the noise
There was a problem hiding this comment.
@zakkak Np at all, I still followed up with a minor refactoring, to avoid nested if inside the outer else which was not too cool, so thanks for your suggestion.
...ions/security/deployment/src/main/java/io/quarkus/security/deployment/SecurityProcessor.java
Outdated
Show resolved
Hide resolved
|
@sberyozkin if you want this one in 2.7.4.Final, I need it merged today. Thanks. |
sberyozkin
force-pushed
the
bc_keypair_ecdsa_xdh
branch
from
af17d0e to
8f86882
Compare
|
@gsmet Thanks, I think I have addressed Foivos's comments, so if one more approval is added then it will make it |
Failing Jobs - Building 8f86882
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖ ✖ ✖ ✖ |
Fixes #17046
Related to #23967
Hi @zakkak
The following trace is produced when
BouncyCastleEndpoint.checkAesCbcPKCS7PaddingCipheris run in native:But we do register it at build time as the additional security provider. It is verified OK in all other native tests in the integration test. Is it possible to verify somehow the BC provider is registered in Native, I recall you did some manual updates to GraalVM to trace it.
I wonder if something might've changed in the latest GraalVM, as the user confirms it worked in Quarkus 1.x