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

Register com.jcraft.jsch.jbcrypt.JBCrypt for reflection #106

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

erik-wramner
Copy link
Contributor

Fix issue #105 (hopefully, not tested) by registering JBCrypt for reflection.

@gastaldi
Copy link
Member

gastaldi commented Jul 31, 2023

Can you create a test using the snippet you provided in the issue description?

var keyPair = KeyPair.load(new JSch(), privateKeyBytes, null);
keyPair.decrypt(passphrase.getBytes(StandardCharsets.UTF_8)):

@erik-wramner
Copy link
Contributor Author

I'll try. Problem is that code runs in Quarkus and only fails in native mode, not Java mode. Do you have any existing tests that run in native mode, that I can use as a starting point? (I'll look around).

@gastaldi
Copy link
Member

@erik-wramner I have a test that invokes that snippet of yours, let me push to your PR

@erik-wramner
Copy link
Contributor Author

I also added a test, but your test is nicer. Shall I revert mine?

I haven't been able to actually test that it works. I had Visual Studio installed when this computer crashed and since then I can't reinstall it, so I can't compile native code without doing it in Docker. Can't find a way to run the integration tests in Docker, unfortunately.

I have verified with the real Quarkus release that adding com.jcraft.jsch.jbcrypt.JBCrypt to reflection-config.json helps.

@gastaldi
Copy link
Member

I also added a test, but your test is nicer. Shall I revert mine?

Sure, I think it's better if we don't store any RSA keys in the repository :)

I haven't been able to actually test that it works. I had Visual Studio installed when this computer crashed and since then I can't reinstall it, so I can't compile native code without doing it in Docker. Can't find a way to run the integration tests in Docker, unfortunately.

I have verified with the real Quarkus release that adding com.jcraft.jsch.jbcrypt.JBCrypt to reflection-config.json helps.

That's great to hear. For some reason my test passes even without your fix, but I'll investigate what's going on

@erik-wramner
Copy link
Contributor Author

Reverted. As for the RSA key, it is harmless, it was created for these tests, but it is still nicer to do without it.

@gastaldi gastaldi linked an issue Jul 31, 2023 that may be closed by this pull request
@gastaldi gastaldi merged commit 611b422 into quarkiverse:main Jul 31, 2023
@gastaldi gastaldi changed the title #105 register JBCrypt for reflection Register com.jcraft.jsch.jbcrypt.JBCrypt for reflection Jul 31, 2023
@gastaldi
Copy link
Member

PR Merged. Thanks! Can you build and try this extension locally and see if that resolves your problem?

@erik-wramner
Copy link
Contributor Author

Yes, I'll give it a try.

@gastaldi
Copy link
Member

I found out how to reproduce the issue in the test, it happens with OpenSSL keys only. I'll update it

@gastaldi
Copy link
Member

@all-contributors add @erik-wramner for code

@allcontributors
Copy link
Contributor

@gastaldi

I've put up a pull request to add @erik-wramner! 🎉

@erik-wramner
Copy link
Contributor Author

@gastaldi I can confirm that the new version solves the issue when I build locally.

I have a question though. To make it work, I need to depend on quarkus-jsch-deployment. However, if I follow the installation docs and run mvnw quarkus:add-extension -Dextensions="io.quarkiverse.jsch:quarkus-jsch" the dependency becomes quarkus-jsch. That does not work (even with 999-SNAPSHOT).

Are the installation instructions wrong, so that the correct dependency is quarkus-jsch-deployment, or have I missed something?

@erik-wramner erik-wramner deleted the 105_JBCrypt_reflection branch July 31, 2023 16:47
@gastaldi
Copy link
Member

The quarkus-jsch-deployment is resolved via the Quarkus extension Maven plugin and you don't need to explicitly add it (unless you're developing an extension)

@erik-wramner
Copy link
Contributor Author

Hm, I thought so. Weird - but then perhaps the Maven plugin finds the released version, so I needed to depend on the deployment artifact explicitly to get 999-SNAPSHOT?

@gastaldi
Copy link
Member

No, the resolver should pick from the local repository first. That sounds like a bug, we've been doing some changes in the Maven integration recently and I wonder if this is a new bug.

Can you open an issue in https://github.com/quarkusio/quarkus/issues with a small reproducer project?

@erik-wramner
Copy link
Contributor Author

I'll see if it is reasonably easy to reproduce. My real project is complex with multiple Maven projects. Stripping it to a small reproducer is not trivial, but if the problem appears in a single Maven project I'll create an issue for it.

@erik-wramner
Copy link
Contributor Author

@gastaldi I created a single-project reproducer, but it didn't have the problem. It (correctly) uses the local version without having to drag in the deployment project. It could be one of the many other dependencies that creates a conflict or it may be something with the multiple Maven projects. I could keep adding things, trying to make the example project even more like the real one, but given how long time a single build takes and how many variables there are to tweak I'm afraid I don't have the time.

@gastaldi
Copy link
Member

gastaldi commented Aug 1, 2023

@erik-wramner I've been trying to reproduce your use case and haven't seen it fail. Perhaps you could output the logs with mvn clean package -X? That would give you the -deployment artifacts being used by the Quarkus plugin

@erik-wramner
Copy link
Contributor Author

I'd prefer not to upload the full trace to the world as it contains a bit too much that may be sensitive. However, I think this is where it goes wrong:

[DEBUG]    com.icecoresoft.connect:filetransfer-lib:jar:2.2-SNAPSHOT:compile
[DEBUG]       io.quarkiverse.jsch:quarkus-jsch:jar:3.0.1:compile (version managed from 999-SNAPSHOT)

For some reason it decides that the version from Quarkus BOM is better than the one in the pom file. I'll move things around a bit to see if I can find out why.

@gastaldi
Copy link
Member

gastaldi commented Aug 1, 2023

Funny, because this extension is not listed in the Quarkus BOM (none of the io.quarkiverse.* groupId are).

@erik-wramner
Copy link
Contributor Author

Indeed, plus further down in the trace I get:

[DEBUG] [io.quarkus.bootstrap.resolver.maven.ApplicationDependencyTreeResolver] Injecting deployment dependency io.quarkiverse.jsch:quarkus-jsch-deployment:jar:999-SNAPSHOT
[DEBUG] [io.quarkus.bootstrap.classloading.QuarkusClassLoader] Adding elements io.quarkus.bootstrap.classloading.PathTreeClassPathElement[io.quarkiverse.jsch:quarkus-jsch-deployment / runtime=false resources=null] to QuarkusClassLoader Augmentation Class Loader: PROD

That looks correct. I'm building again with a few changes, perhaps Maven gets confused as the library is pulled in from two sub-projects.

@erik-wramner
Copy link
Contributor Author

Actually, if you check the bom for 3.2.1 you'll find:

<dependency>
  <groupId>io.quarkiverse.jsch</groupId>
  <artifactId>quarkus-jsch-deployment</artifactId>
  <version>3.0.1</version>
</dependency>

@gastaldi
Copy link
Member

gastaldi commented Aug 1, 2023

Perhaps you already know, but it's important to mention that Maven resolves to the version that is nearest to the resolved dependency tree. Better use dependencyManagement in the parent pom and declare the version there just in case (although not required, declaring the -deployment there helps)

@gastaldi
Copy link
Member

gastaldi commented Aug 1, 2023

Actually, if you check the bom for 3.2.1 you'll find:

<dependency>
  <groupId>io.quarkiverse.jsch</groupId>
  <artifactId>quarkus-jsch-deployment</artifactId>
  <version>3.0.1</version>
</dependency>

Oh that's news to me, I was looking at https://github.com/quarkusio/quarkus/blob/main/bom/application/pom.xml but it looks like some preprocessing is run before the artifact is deployed

@erik-wramner
Copy link
Contributor Author

Well, since the dependency is in the Quarkus BOM, it is already present in the parent's dependencyManagement section. If that is supposed to win, we have found the problem?

@gastaldi
Copy link
Member

gastaldi commented Aug 1, 2023

Right, so declaring the extension AND the deployment with the new version BEFORE the quarkus-bom should fix the problem.

You don't need to declare the -deployment as a dependency in your application though, only in dependencyManagement

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 this pull request may close these issues.

Missing reflection configuration for com.jcraft.jsch.jbcrypt.JBCrypt
2 participants