openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.08k stars 312 forks source link

Do not eagerly resolve Maven repositories, to prevent `SSLHandshakeException` stacktrace from `maven.java.net` #4101

Open gsmet opened 5 months ago

gsmet commented 5 months ago

I have noticed several warnings lately when updating various projects related to repositories not being available or not up to date.

As far as I can see, it's usually dependencies coming with exotic repositories declared and it seems that OpenRewrite resolves all of them (not sure if it's new but I noticed it only lately).

It generates big warnings in the output such as:

(This is just an example, I had another issue with some dependency in the classpath registering central.maven.org as a repository...)

[WARNING] Failed to access maven repository https://maven.java.net/content/repositories/snapshots
[WARNING] javax.net.ssl.SSLHandshakeException: PKIX path validation failed: java.security.cert.CertPathValidatorException: validity check failed
[WARNING]     at sun.security.ssl.Alert.createSSLException (Alert.java:131)
[WARNING]     at sun.security.ssl.TransportContext.fatal (TransportContext.java:378)
[WARNING]     at sun.security.ssl.TransportContext.fatal (TransportContext.java:321)
[WARNING]     at sun.security.ssl.TransportContext.fatal (TransportContext.java:316)
[WARNING]     at sun.security.ssl.CertificateMessage$T12CertificateConsumer.checkServerCerts (CertificateMessage.java:654)
[WARNING]     at sun.security.ssl.CertificateMessage$T12CertificateConsumer.onCertificate (CertificateMessage.java:473)
[WARNING]     at sun.security.ssl.CertificateMessage$T12CertificateConsumer.consume (CertificateMessage.java:369)
[WARNING]     at sun.security.ssl.SSLHandshake.consume (SSLHandshake.java:396)
[WARNING]     at sun.security.ssl.HandshakeContext.dispatch (HandshakeContext.java:480)
[WARNING]     at sun.security.ssl.HandshakeContext.dispatch (HandshakeContext.java:458)
[WARNING]     at sun.security.ssl.TransportContext.dispatch (TransportContext.java:201)
[WARNING]     at sun.security.ssl.SSLTransport.decode (SSLTransport.java:172)
[WARNING]     at sun.security.ssl.SSLSocketImpl.decode (SSLSocketImpl.java:1510)
[WARNING]     at sun.security.ssl.SSLSocketImpl.readHandshakeRecord (SSLSocketImpl.java:1425)
[WARNING]     at sun.security.ssl.SSLSocketImpl.startHandshake (SSLSocketImpl.java:455)
[WARNING]     at sun.security.ssl.SSLSocketImpl.startHandshake (SSLSocketImpl.java:426)
[WARNING]     at sun.net.www.protocol.https.HttpsClient.afterConnect (HttpsClient.java:589)
[WARNING]     at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect (AbstractDelegateHttpsURLConnection.java:187)
[WARNING]     at sun.net.www.protocol.http.HttpURLConnection.getInputStream0 (HttpURLConnection.java:1665)
[WARNING]     at sun.net.www.protocol.http.HttpURLConnection.getInputStream (HttpURLConnection.java:1589)
[WARNING]     at java.net.HttpURLConnection.getResponseCode (HttpURLConnection.java:529)
[WARNING]     at sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode (HttpsURLConnectionImpl.java:308)
[WARNING]     at org.openrewrite.ipc.http.HttpUrlConnectionSender.send (HttpUrlConnectionSender.java:97)
[WARNING]     at org.openrewrite.maven.internal.MavenPomDownloader.lambda$sendRequest$1 (MavenPomDownloader.java:142)
[WARNING]     at dev.failsafe.Functions.lambda$toCtxSupplier$11 (Functions.java:243)
[WARNING]     at dev.failsafe.Functions.lambda$get$0 (Functions.java:46)
[WARNING]     at dev.failsafe.internal.RetryPolicyExecutor.lambda$apply$0 (RetryPolicyExecutor.java:74)
[WARNING]     at dev.failsafe.SyncExecutionImpl.executeSync (SyncExecutionImpl.java:187)
[WARNING]     at dev.failsafe.FailsafeExecutor.call (FailsafeExecutor.java:376)
[WARNING]     at dev.failsafe.FailsafeExecutor.get (FailsafeExecutor.java:112)
[WARNING]     at org.openrewrite.maven.internal.MavenPomDownloader.sendRequest (MavenPomDownloader.java:141)
[WARNING]     at org.openrewrite.maven.internal.MavenPomDownloader.normalizeRepository (MavenPomDownloader.java:748)
[WARNING]     at org.openrewrite.maven.internal.MavenPomDownloader.distinctNormalizedRepositories (MavenPomDownloader.java:670)
[WARNING]     at org.openrewrite.maven.internal.MavenPomDownloader.download (MavenPomDownloader.java:507)
[WARNING]     at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentPom (ResolvedPom.java:492)
[WARNING]     at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentDependenciesRecursively (ResolvedPom.java:427)
[WARNING]     at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentsRecursively (ResolvedPom.java:375)
[WARNING]     at org.openrewrite.maven.tree.ResolvedPom.resolveDependencies (ResolvedPom.java:838)
[WARNING]     at org.openrewrite.maven.tree.ResolvedPom.resolveDependencies (ResolvedPom.java:754)
[WARNING]     at org.openrewrite.maven.tree.MavenResolutionResult.resolveDependencies (MavenResolutionResult.java:174)

Given there's nothing we can do about it as they come from external dependencies, I wonder if this should be debug logging instead? At least for repositories coming from dependencies and not from the current project.

I have seen this lately in two different projects (and two different errors) so I suspect this might be more common than we think.

Thoughts?

shanman190 commented 5 months ago

Just noting it here also, but there's a thread in Slack mentioning the same issue.

https://rewriteoss.slack.com/archives/C01A843MWG5/p1711020348961109?thread_ts=1711020348.961109&cid=C01A843MWG5

Could potentially catch the exception from an OpenRewrite standpoint, but I'm also guessing that this likely appears directly from Maven as well for the same reasons.

gsmet commented 5 months ago

@shanman190 no, I don't see anything from Maven. Only OpenRewrite is complaining.

shanman190 commented 5 months ago

That's interesting.

@sambsnyd, do you have any opinions on how you'd like to see this handled?

gsmet commented 5 months ago

I wonder if OpenRewrite selects the repositories the same way as Maven. I wasn't able to find where these repositories came from exactly but I wouldn't expect them to be used if things are in Maven Central.

sambsnyd commented 5 months ago

I think it could be appropriate in rewrite-maven-plugin to check some of the common SSL-skipping parameters used in Maven builds and provide an appropriate http client in the execution context

gsmet commented 5 months ago

I’m not sure the problem is due to that really. For the other project, it was contacting another server that wasn’t responding at all.

I think the problem is that you shouldn’t contact these servers at all in the first place.

gsmet commented 5 months ago

To further clarify: I'm not sure adding the Maven Central repository last is actually what Maven does by default. At least, not compared to repositories declared in dependencies of the project.

https://github.com/openrewrite/rewrite/blob/main/rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java#L683-L688

Another option could be to not report any error when it's impossible to download the artifact. I mean I could see how you could maybe pile up the download errors and only log them if in the end we can't download the artifact from any repository?

But I think getting a clear picture of the correct ordering is important, because in the case I had where some artifact somewhere was referencing a repository that was non-existent and timing out, it would make my build a lot slower if we try to resolve all artifacts with this repository first.

javanegmond commented 5 months ago

I think the problem is that you shouldn’t contact these servers at all in the first place.

I want to reiterate this. IIRC, java.net should not be used for anything at all anymore, really, see the notice at[1]. I assume they're keeping up their Nexus instance so as not to break anyone's build if you depend on artifacts from projects that for some reason haven't moved on to other maven repositories (like central). Unfortunately their SSL certificate has simply expired. I have personally emailed both the domain owner as well as the mail-address mentioned on [1]. Perhaps it would be for the better if they just opted to shut down said Nexus.

[1] https://www.java.net

gsmet commented 5 months ago

It is used because some artifacts somewhere in the dependency tree has it. You can't drop it from existing artifacts. And I know that when resolving the dependencies of artifact1, if artifact1 has java.net as a repository, you might contact it... but my understanding was that we would hit Maven Central first and not last.

What needs fixing is that we should either not contact it at all if in Maven Central as I think Maven resolves Maven Central by default first or we should ignore the error if we can find it in a later repositories.

shanman190 commented 5 months ago

https://maven.apache.org/guides/mini/guide-multiple-repositories.html#repository-order

It's possible that internal Maven code is out of sync with this, but the documentation kinda makes it seem like if Maven settings aren't used and some repository that contains the artifact isn't explicitly defined somewhere prior, then it should hit java.net (undesirably) by the order of the rules there.

It also makes it seem like the default super pom comes after any local or parent poms, but before dependency poms. So it does seem like there may be some incorrect ordering within rewrite-maven as well.

raman777 commented 5 months ago

It is NOT ONLY a warning: It stops all processing when this error occures and nothing is transformed.

Unusable stuff at all.

javanegmond commented 5 months ago

It is used because some artifacts somewhere in the dependency tree has it. You can't drop it from existing artifacts. And I know that when resolving the dependencies of artifact1, if artifact1 has java.net as a repository, you might contact it... but my understanding was that we would hit Maven Central first and not last.

I think you may have misunderstood. What I meant is that a conscious effort should be made to update dependencies such that we don't get any kind of (not even transitive) reference to the old maven.java.net repository. If we can't do that (because a dependency is not replaceable for instance), the effort should go into assisting the dependency to move on etc. Since the whole java.net domain shouldn't have been used for a while it's pretty likely that somewhere along the dependency tree where it's referenced some kind of (transitive) dependency is also no longer under maintenance. Depending on unmaintained libraries is also a clear risk that should be avoided as much as possible.

What needs fixing is that we should either not contact it at all if in Maven Central as I think Maven resolves Maven Central by default first or we should ignore the error if we can find it in a later repositories.

Ignoring the error if it's found in later repositories is what maven does I believe. It's an acceptable quick solution I think. If the logic used here comes from the maven code, perhaps the effort should be put into improving maven.

FWIW: one of the things that has annoyed me in maven is that if in a dependency a specific repo is mentioned, maven contacts it for all dependencies it tries to resolve after, including local dependencies in multi-module projects and therefore leaking miniscule bits of information of your project to some repository. Ignore this aside as it's not relevant for this issue.

jkschneider commented 5 months ago

@timtebeek and @sambsnyd this one feels pretty important. Do we simply need a carve out exclusion to never contact java.net? On the other point, I am pretty confident Maven Central is added last deep in the Maven code.

gsmet commented 5 months ago

That's not only java.net. I had the issues with some other repositories that were declared in some dependencies.

And I don't recall having seen failures of artifacts being downloaded from these repositories first when using regular Maven so I'm not entirely sure the logic in OpenRewrite is right.

delanym commented 5 months ago

I've had numerous issue with openrewrite because its clearly not doing things by the book. Pls get a Maven maintainer to look at how this project is working with Maven.

gsmet commented 5 months ago

@delanym FWIW, that is not my personal experience. Anyway, there are more pleasant ways to convey this message.

gsmet commented 5 months ago

@jkschneider

I tried again on two of my projects to gather a bit more information.

For https://github.com/quarkusio/quarkus-github-bot, I get the warning twice:

[WARNING] Failed to access maven repository http://snapshots.repository.codehaus.org [WARNING] Failed to access maven repository https://maven.java.net/content/repositories/snapshots

Unfortunately, there are no details about the requests in the warnings but what I find odd is that both are for snapshots repositories and the only snapshot version in this project is the version of the project itself (and I wouldn't expect it to try to download the modules from the project).

I built the projects (regular mvn clean install) with an empty repo to see if these repositories were contacted but I can only see:

[INFO] Downloading from central [INFO] Downloading from sonatype-snapshots

as the two contacted repositories.

From what I can see the sonatype-snapshots is only called when resolving some Arquillian/Shrinkwrap stuff so it's not globally taken into account. My understanding is that the scope of the repositories declared in a dependency is very localized to resolving the dependencies of this particular artifact (not sure if it's transitive or not). It is contacted first, then as the dependencies are not snapshots, they can't be resolved and the dependencies is resolved from central in a second time. Other than that, central is contacted first, even after this sonatype-snapshots was detected.

I have the same experience with https://github.com/quarkiverse/quarkus-github-app/ but I only have one warning there:

[WARNING] Failed to access maven repository https://maven.java.net/content/repositories/snapshots

I remember having seen another project with central.maven.org but I couldn't find it back.

I'm sorry, I don't have the time to dig more at the moment but I hope this could be helpful.

delanym commented 5 months ago

Repositories are defined in the poms included in a JAR, like woodstox-core-6.5.1/META-INF/maven/com.sun.xml.bind.jaxb/isorelax/pom.xml

      <repositories>
          <repository>
              <id>releases.java.net</id>
              <url>https://maven.java.net/content/repositories/releases/</url>
              <layout>default</layout>
          </repository>
          <repository>
              <id>jvnet-nexus-staging</id>
              <url>https://maven.java.net/content/repositories/staging/</url>
              <layout>default</layout>
          </repository>
          <repository>
              <id>eclipse</id>
              <url>http://download.eclipse.org/rt/eclipselink/maven.repo</url>
              <layout>default</layout>
          </repository>
     </repositories>

Why openrewrite would read this I don't know I'm not a Maven expert, but neither should you have to be. There's no shame in asking for help. We cant all be experts in every field, and Maven is a complex beast all by itself. Its the kind of issue that is so obviously wrong it suggests this project is not going through the right channels.

gsmet commented 5 months ago

@delanym I'm not part of this project so I only speak for myself but really, again, you can say the same things without being borderline insulting. Please keep things civil, provide feedback, I personally don't like to work in a hostile environment and I'm pretty sure other people around here don't either.

Feedback is useful but there's really no need to be hostile.

delanym commented 5 months ago

No hostility intended

iksaif commented 4 months ago

👋 I have the same issue as well. Is there a known workaround? This is blocking rewrite:run to complete

Dzeri96 commented 4 months ago

I have the same issue. It leads to a java.lang.IllegalStateException: Illegal state while comparing versions, or at least I assume that's the cause

timtebeek commented 4 months ago

So we just did a new release yesterday that folks here might want to try out: https://github.com/openrewrite/rewrite-recipe-bom/releases/tag/v2.10.0

I've done a quick recipe run locally using this latest version and I had no issue getting recipe results from OrderImports against the above mentioned https://github.com/quarkiverse/quarkus-github-app/ just now

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.activeRecipes=org.openrewrite.java.OrderImports

This despite this warning that we surface, but does not break running recipes

[INFO] --- rewrite:5.29.0:run (default-cli) @ quarkus-github-app-docs ---
[INFO] Using active recipe(s) [org.openrewrite.java.OrderImports]
[INFO] Using active styles(s) []
[INFO] Validating active recipes...
[INFO] Project [Quarkus - GitHub App - Parent] Resolving Poms...
[WARNING] Failed to access maven repository https://maven.java.net/content/repositories/snapshots
javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
    at sun.security.ssl.Alert.createSSLException (Alert.java:130)
    at sun.security.ssl.TransportContext.fatal (TransportContext.java:378)
...
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:206)
    at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:103)
    at java.lang.reflect.Method.invoke (Method.java:580)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:283)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:226)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:407)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:348)
[INFO] Project [Quarkus - GitHub App - Parent] Parsing source files
[WARNING] There were problems parsing some source files, run with --errors to see full stack traces
[WARNING] There were problems parsing .github/workflows/build.yml
[WARNING] There were problems parsing .github/workflows/downstream.yml
[INFO] Project [Quarkus - GitHub App - Events] Parsing source files
[INFO] Project [Quarkus - GitHub App - Runtime] Parsing source files
[INFO] Project [Quarkus - GitHub App - Deployment] Parsing source files
[INFO] Project [Quarkus - GitHub App - Command Airline] Parsing source files
[INFO] Project [Quarkus - GitHub App - Command Airline - Runtime] Parsing source files
[INFO] Project [Quarkus - GitHub App - Command Airline - Deployment] Parsing source files
[INFO] Project [Quarkus - GitHub App - Testing] Parsing source files
[INFO] Project [Quarkus - GitHub App - Integration Tests] Parsing source files
[INFO] Project [Quarkus - GitHub App - Integration Tests - Testing Framework] Parsing source files
[INFO] Project [Quarkus - GitHub App - Integration Tests - App] Parsing source files
[INFO] Project [Quarkus - GitHub App - Integration Tests - Command Airline] Parsing source files
[INFO] Project [Quarkus - GitHub App - Documentation] Parsing source files
[INFO] Running recipe(s)...
[WARNING] Changes have been made to events/src/main/java/io/quarkiverse/githubapp/event/Discussion.java by:
[WARNING]     org.openrewrite.java.OrderImports
[WARNING] Changes have been made to events/src/main/java/io/quarkiverse/githubapp/event/WorkflowRun.java by:
[WARNING]     org.openrewrite.java.OrderImports
...
[WARNING] Please review and commit the results.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Quarkus - GitHub App - Parent 999-SNAPSHOT:
[INFO] 
[INFO] Quarkus - GitHub App - Parent ...................... SUCCESS [  1.806 s]
[INFO] Quarkus - GitHub App - Events ...................... SUCCESS [  4.856 s]
[INFO] Quarkus - GitHub App - Runtime ..................... SUCCESS [  9.224 s]
[INFO] Quarkus - GitHub App - Deployment .................. SUCCESS [  3.832 s]
[INFO] Quarkus - GitHub App - Command Airline ............. SUCCESS [  0.007 s]
[INFO] Quarkus - GitHub App - Command Airline - Runtime ... SUCCESS [  0.728 s]
[INFO] Quarkus - GitHub App - Command Airline - Deployment  SUCCESS [  0.271 s]
[INFO] Quarkus - GitHub App - Testing ..................... SUCCESS [  0.489 s]
[INFO] Quarkus - GitHub App - Integration Tests ........... SUCCESS [  0.006 s]
[INFO] Quarkus - GitHub App - Integration Tests - Testing Framework SUCCESS [  0.364 s]
[INFO] Quarkus - GitHub App - Integration Tests - App ..... SUCCESS [  0.167 s]
[INFO] Quarkus - GitHub App - Integration Tests - Command Airline SUCCESS [  0.321 s]
[INFO] Quarkus - GitHub App - Documentation ............... SUCCESS [ 24.865 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  47.627 s
[INFO] Finished at: 2024-04-25T19:58:57+02:00
[INFO] ------------------------------------------------------------------------
Dzeri96 commented 4 months ago

You are right. The execution-breaking error I referred to comes from dependencies using the LATEST version.

timtebeek commented 4 months ago

Thanks for chiming in with that insight; good that recipe runs are at least not affected. That brings us back to the message Guillaume started with: that it's probably best to not show the full stacktrace if this is not affecting recipe runs in practice.

With this PR we already have some work in progress that touches upon some of the same areas

I propose we wait for the above PR to be merged and then reevaluate if we still have to change the reported stacktrace here.

gsmet commented 4 months ago

@timtebeek I think the problem is more complex than just having a warning.

See this comment: https://github.com/openrewrite/rewrite/issues/4101#issuecomment-2039520890

I think there is a problem in how the repositories are collected and their scopes. As these servers shouldn't even be contacted (and are not in a standard Maven run).

FWIW, from the beginning in my case I had the warning but the recipes were applied.

domariscampos-hotmart commented 4 months ago

👋 I have the same issue as well. Is there a known workaround? But is not blocking rewrite:run

timtebeek commented 4 months ago

👋 I have the same issue as well. Is there a known workaround? But is not blocking rewrite:run

Sorry to hear; would you be able to provide a minimal reproducer project? Despite the number of folks chiming I've not yet been able to reproduce the behavior where rewrite:run is blocked. Would help towards getting this resolved, as we certainly wouldn't want anyone stuck on older versions.

Edit: I now see you've edited your earlier remark: So runs are not blocked then? Just the call-out attempt that you'd like to see removed?

delanym commented 4 months ago

Maybe a complication for issues like this is that OpenRewrite takes a long time to run a recipe on a large codebase. If there's any kind of error and then no activity it looks like the build is stuck. There's also no disk writes until the last moment. Is there some way to make the (Maven) plugin output more verbose or get it to write files immediately after processing them?

hannesgeodan commented 3 months ago

What is the status of this issue? I have the same problems. About being blocking or stuck: When maven tells you "BUILD SUCCESS" but there are no files changed, that is a clear indication of a finished job which didn't do anything. Can I help with stacktraces?

delanym commented 3 months ago

Maven 3.9.7 added the --ignore-transitive-repositories switch. You might try that as a workaround.

zhetao1116 commented 2 months ago

I am facing the same issue. What is the latest solution here? Do I have to upgrade maven to 3.9.7 to run rewrite cmd?

timtebeek commented 2 months ago

We're added two changes recently that affect which dependencies are looked up

The first one is already in openrewrite/rewrite releases v8.28.1, the second is available from our snapshot versions.

Can anyone share a reliable reproducer for this issue, and your experiences using the latest snapshot versions?

jonesbusy commented 2 months ago

Thanks @timtebeek Found this issue also when running recipes on some jenkins plugins

hquevedorccl commented 2 months ago

hi @timtebeek , is going to be a fix? it is not helping me at all with this error. ty

timtebeek commented 2 months ago

I would love it if someone could provide me with an ideally minimized reproducer; something like

1) checkout this minimal project, (ideally not a 10 module project) 2) run this command 3) expected to see: xyz 4) actually saw: stack trace & no changes.

So far I've heard from folks affected, but lack clear steps to reproduce the issue. I've also heard from folks who saw the stack trace, but nonetheless saw their changes applied. That's making this hard for me to follow up on. Any help is then appreciated.

jonesbusy commented 2 months ago

I have one example where changes are applied but the stacktrace displayed

git clone git@github.com:jenkinsci/ansible-plugin.git
cd ansible-plugin
mvn -U org.openrewrite.maven:rewrite-maven-plugin:5.34.1:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-jenkins:0.8.1 -Drewrite.activeRecipes=org.openrewrite.jenkins.ModernizeJenkinsfile

Environment

Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)
Java version: 21.0.2, vendor: Eclipse Adoptium, runtime: /opt/temurin-21/jdk-21.0.2+13
OS name: "linux", version: "5.15.133.1-microsoft-standard-wsl2", arch: "amd64", family: "unix"

Adding the --ignore-transitive-repositories doesn't change anything

mvn --ignore-transitive-repositories -U org.openrewrite.maven:rewrite-maven-plugin:5.34.1:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-jenkins:0.8.1 -Drewrite.activeRecipes=org.openrewrite.jenkins.ModernizeJenkinsfile
vsevel commented 2 months ago

is there any way to work around the issue in the meantime?

timtebeek commented 2 months ago

is there any way to work around the issue in the meantime?

Do you have a reproducer? That's been the main hold up to work on this.

vsevel commented 2 months ago

I have been working on an update to the update task of the quarkus plugin. something like this applied to a quarkus app would show the stacktrace: mvn io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:update -N -DrewriteDryRun=true and I was mistaken to think this is blocking. the process continues normally after printing the stack trace as a warning.

gsmet commented 2 months ago

Sorry, time is scarce on my side and I wasn't able to provide feedback here earlier.

Here is a simple reproducer with a simple project, a simple recipe and using the OpenRewrite Maven plugin directly:

git clone https://github.com/quarkusio/quarkus-github-bot
cd quarkus-github-bot
cat <<EOT > rewrite.yml
type: specs.openrewrite.org/v1beta/recipe
name: io.quarkus.AddHibernateValidator
recipeList:
  - org.openrewrite.maven.AddDependency:
      groupId: io.quarkus
      artifactId: quarkus-hibernate-validator
      version: 3.12.0
EOT
./mvnw -U org.openrewrite.maven:rewrite-maven-plugin:run  -Drewrite.activeRecipes=io.quarkus.AddHibernateValidator

You will get this warning during the execution:

[WARNING] Failed to access maven repository https://maven.java.net/content/repositories/snapshots
javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

When building the project with Maven, even with an empty Maven repo, you don't get this warning so, as I explained a bit earlier, there's something incorrect in how the repositories are resolved.

timtebeek commented 2 months ago

Thanks for the clear steps @gsmet ! Indeed I see that warning logged before making the correct changes with that recipe.

In my debugger I see that quarkusio/quarkus-github-bot depends on: https://repo1.maven.org/maven2/io/quarkiverse/githubapp/quarkus-github-app-testing/2.6.0/quarkus-github-app-testing-2.6.0.pom

Which itself has the following dependencies (among others).

[INFO] +- io.quarkus:quarkus-junit5:jar:3.11.3:compile
[INFO] |  +- io.quarkus:quarkus-bootstrap-core:jar:3.11.3:compile
[INFO] |  |  +- io.quarkus:quarkus-classloader-commons:jar:3.11.3:compile
[INFO] |  |  +- io.quarkus:quarkus-bootstrap-app-model:jar:3.11.3:compile
[INFO] |  |  \- io.smallrye.common:smallrye-common-io:jar:2.3.0:compile
[INFO] |  +- org.eclipse.sisu:org.eclipse.sisu.inject:jar:0.9.0.M2:compile
[INFO] |  +- io.quarkus:quarkus-test-common:jar:3.11.3:compile
[INFO] |  |  +- io.quarkus:quarkus-core-deployment:jar:3.11.3:compile
[INFO] |  |  |  +- org.aesh:readline:jar:2.4:compile
[INFO] |  |  |  |  \- org.fusesource.jansi:jansi:jar:2.4.0:compile
[INFO] |  |  |  +- org.aesh:aesh:jar:2.7:compile
[INFO] |  |  |  +- io.quarkus.gizmo:gizmo:jar:1.8.0:compile
[INFO] |  |  |  |  \- org.ow2.asm:asm-util:jar:9.7:compile
[INFO] |  |  |  |     \- org.ow2.asm:asm-analysis:jar:9.7:compile
[INFO] |  |  |  +- org.ow2.asm:asm:jar:9.7:compile
[INFO] |  |  |  +- org.ow2.asm:asm-commons:jar:9.7:compile
[INFO] |  |  |  |  \- org.ow2.asm:asm-tree:jar:9.7:compile
[INFO] |  |  |  +- io.quarkus:quarkus-hibernate-validator-spi:jar:3.11.3:compile
[INFO] |  |  |  +- io.quarkus:quarkus-class-change-agent:jar:3.11.3:compile
[INFO] |  |  |  +- io.quarkus:quarkus-devtools-utilities:jar:3.11.3:compile
[INFO] |  |  |  +- io.quarkus:quarkus-builder:jar:3.11.3:compile
[INFO] |  |  |  \- org.junit.platform:junit-platform-launcher:jar:1.10.2:compile
[INFO] |  |  +- io.quarkus:quarkus-bootstrap-maven-resolver:jar:3.11.3:compile
[INFO] |  |  |  +- io.smallrye.beanbag:smallrye-beanbag-maven:jar:1.5.2:compile
[INFO] |  |  |  |  +- io.smallrye.beanbag:smallrye-beanbag-sisu:jar:1.5.2:compile
[INFO] |  |  |  |  |  \- io.smallrye.beanbag:smallrye-beanbag:jar:1.5.2:compile
[INFO] |  |  |  |  +- commons-codec:commons-codec:jar:1.17.0:compile
[INFO] |  |  |  |  +- javax.inject:javax.inject:jar:1:compile
[INFO] |  |  |  |  +- org.apache.httpcomponents:httpclient:jar:4.5.14:compile
[INFO] |  |  |  |  +- org.apache.httpcomponents:httpcore:jar:4.4.16:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-artifact:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-builder-support:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-model:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-model-builder:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-repository-metadata:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-settings:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven.resolver:maven-resolver-api:jar:1.9.18:compile
[INFO] |  |  |  |  +- org.apache.maven.resolver:maven-resolver-impl:jar:1.9.18:compile
[INFO] |  |  |  |  |  \- org.apache.maven.resolver:maven-resolver-named-locks:jar:1.9.18:compile
[INFO] |  |  |  |  +- org.apache.maven.resolver:maven-resolver-spi:jar:1.9.18:compile
[INFO] |  |  |  |  +- org.apache.maven.resolver:maven-resolver-util:jar:1.9.18:compile
[INFO] |  |  |  |  +- org.apache.maven.resolver:maven-resolver-transport-http:jar:1.9.20:compile
[INFO] |  |  |  |  +- org.apache.maven.wagon:wagon-provider-api:jar:3.5.3:compile
[INFO] |  |  |  |  +- org.apache.maven.wagon:wagon-http-shared:jar:3.5.3:compile
[INFO] |  |  |  |  +- org.codehaus.plexus:plexus-interpolation:jar:1.26:compile
[INFO] |  |  |  |  +- org.codehaus.plexus:plexus-utils:jar:3.5.1:compile
[INFO] |  |  |  |  +- org.codehaus.plexus:plexus-xml:jar:4.0.1:compile
[INFO] |  |  |  |  |  \- org.apache.maven:maven-xml-impl:jar:4.0.0-alpha-5:compile
[INFO] |  |  |  |  |     \- org.apache.maven:maven-api-xml:jar:4.0.0-alpha-5:compile
[INFO] |  |  |  |  |        \- org.apache.maven:maven-api-meta:jar:4.0.0-alpha-5:compile
[INFO] |  |  |  |  +- org.codehaus.plexus:plexus-cipher:jar:2.0:compile
[INFO] |  |  |  |  \- org.codehaus.plexus:plexus-sec-dispatcher:jar:2.0:compile
[INFO] |  |  |  +- org.apache.maven:maven-embedder:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-core:jar:3.9.6:compile
[INFO] |  |  |  |  |  \- org.codehaus.plexus:plexus-component-annotations:jar:2.1.0:compile
[INFO] |  |  |  |  +- org.apache.maven:maven-plugin-api:jar:3.9.6:compile
[INFO] |  |  |  |  +- org.apache.maven.shared:maven-shared-utils:jar:3.3.4:compile
[INFO] |  |  |  |  +- com.google.inject:guice:jar:5.1.0:compile
[INFO] |  |  |  |  |  \- aopalliance:aopalliance:jar:1.0:compile
[INFO] |  |  |  |  +- com.google.guava:guava:jar:33.2.0-jre:compile
[INFO] |  |  |  |  +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] |  |  |  |  +- javax.annotation:javax.annotation-api:jar:1.3.2:compile
[INFO] |  |  |  |  +- org.codehaus.plexus:plexus-classworlds:jar:2.6.0:compile
[INFO] |  |  |  |  \- commons-cli:commons-cli:jar:1.5.0:compile

Zooming on on that javax.annotation:javax.annotation-api:jar:1.3.2:compile: https://repo1.maven.org/maven2/javax/annotation/javax.annotation-api/1.3.2/javax.annotation-api-1.3.2.pom

We can see that has a parent of net.java:jvnet-parent:3 https://repo1.maven.org/maven2/net/java/jvnet-parent/3/jvnet-parent-3.pom which contains a Maven repository https://maven.java.net/content/repositories/snapshots/ that throws an exception our way

java.io.UncheckedIOException: javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

<repositories>
<repository>
<id>jvnet-nexus-snapshots</id>
<name>Java.net Nexus Snapshots Repository</name>
<url>https://maven.java.net/content/repositories/snapshots</url>
<releases>
<enabled>false</enabled>
<updatePolicy>never</updatePolicy>
</releases>
<snapshots>
<enabled>true</enabled>
<updatePolicy>never</updatePolicy>
</snapshots>
</repository>
</repositories>

That at least gives us a path of where the look up is coming from; next up a way to either prevent that call, or silence the reported issue somewhat.

gsmet commented 2 months ago

Yeah, FWIW, it's not the only problematic artifact. Old legacy artifacts often point to a repository that doesn't exist anymore.

In Quarkus, we are using jakarta.annotation but... we rely on the Maven artifacts here that require the javax. annotation and thus we had to use the old javax.annotation artifact to avoid conflicts.

But as you can see from the various reports you had, it happens quite a lot in the wild as not all the reports here are related to Quarkus.

timtebeek commented 2 months ago

Hmm; Closes this a little early through my choice of words on https://github.com/openrewrite/rewrite-maven-plugin/pull/811 , but in short what I've done there hide the stack trace by default, as so far no one has come up with a reproducer that actually blocks recipes from making changes. This message only started to appear for folks after we added the logging (only) in the plugin:

I'll continue to explore if we can prevent contacting that repository where it's not needed to hopefully fully get rid of the warning message as well.

timtebeek commented 2 months ago

Folks looking to explore this issue in more depth can do so using this unit test.

@Test
@Issue("https://github.com/openrewrite/rewrite/issues/4101")
void mavenJavaNetFailedSSLNeedNotBeContactedForJavaxAnnotationApi() throws MavenDownloadingException {
    InMemoryExecutionContext ctx = new InMemoryExecutionContext();
    AtomicBoolean attempted = new AtomicBoolean(false);
    MavenExecutionContextView.view(ctx).setResolutionListener(new ResolutionEventListener() {
        @Override
        public void repositoryAccessFailed(String uri, Throwable e) {
            attempted.set(true);
        }
    });
    MavenPomDownloader downloader = new MavenPomDownloader(ctx);
    // This has a parent of `net.java:jvnet-parent:3`, which itself has a snapshot repository for maven.java.net
    GroupArtifactVersion annotationGav = new GroupArtifactVersion("javax.annotation", "javax.annotation-api", "1.3.2");
    Pom downloadedPom = downloader.download(annotationGav, null, null, singletonList(MAVEN_CENTRAL));
    ResolvedPom resolvedPom = downloadedPom.resolve(List.of(), downloader, ctx);
    assertThat(resolvedPom).isNotNull();
    assertThat(attempted.get())
      .withFailMessage("maven.java.net was contacted, but without an immediate need to do so")
      .isFalse();
}

You'll notice we log the SSLHandshakeException here, even when there's perhaps no need to contact maven.java.net just yet, as neither javax.annotation-api nor it's jvnet-parent have any dependencies to resolve from there. https://github.com/openrewrite/rewrite/blob/56986647f0c8b8a162d548d52c70572b939f1d0a/rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java#L804-L807

michael-o commented 1 month ago

Total crap, Oracle is too stupid to configure the certificate chain sent to the client. Just hit this as well:

$ grep -r maven.java.net /tmp/r2/
/tmp/r2/net/java/jvnet-parent/3/jvnet-parent-3.pom:                     <url>https://maven.java.net/content/repositories/snapshots</url>
/tmp/r2/net/java/jvnet-parent/3/jvnet-parent-3.pom:                     <url>https://maven.java.net/service/local/staging/deploy/maven2/</url>
/tmp/r2/net/java/jvnet-parent/3/jvnet-parent-3.pom:             <jvnetDistMgmtSnapshotsUrl>https://maven.java.net/content/repositories/snapshots/</jvnetDistMgmtSnapshotsUrl>

Similar: https://bugzilla.mozilla.org/show_bug.cgi?id=1763708

Here is the breakdown:

$ openssl s_client -connect maven.java.net:443
CONNECTED(00000003)
depth=0 C = US, ST = California, L = Redwood City, O = Oracle Corporation, CN = maven.java.net
verify error:num=20:unable to get local issuer certificate
verify return:1
depth=0 C = US, ST = California, L = Redwood City, O = Oracle Corporation, CN = maven.java.net
verify error:num=21:unable to verify the first certificate
verify return:1
depth=0 C = US, ST = California, L = Redwood City, O = Oracle Corporation, CN = maven.java.net
verify return:1
---
Certificate chain
 0 s:C = US, ST = California, L = Redwood City, O = Oracle Corporation, CN = maven.java.net
   i:C = US, O = DigiCert Inc, CN = DigiCert TLS RSA SHA256 2020 CA1
 1 s:C = US, ST = California, L = Redwood City, O = Oracle Corporation, CN = maven.java.net
   i:C = US, O = DigiCert Inc, CN = DigiCert TLS RSA SHA256 2020 CA1
---

"DigiCert TLS RSA SHA256 2020 CA1" is an intermediate, but neither part of Mozilla NSS, nor OpenJDK cacerts. The missed to send the root certificate DN as well: https://tls-observatory.services.mozilla.com/static/certsplainer.html?id=188407168

Quoting from https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslcertificatefile:

The files may also include intermediate CA certificates, sorted from leaf to root. This is supported with version 2.4.8 and later,
and obsoletes [SSLCertificateChainFile](https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslcertificatechainfile).
When running with OpenSSL 1.0.2 or later, this allows to configure the intermediate CA chain on a per-certificate basis.

Ugly solution:

timtebeek commented 1 month ago

Thanks for chiming in here with those details @michael-o ! Good to know it's not just us, and caused by a certificate issue that we warn about, but continue in spite of to make the required changes.

gsmet commented 1 month ago

Total crap, Oracle is too stupid to configure the certificate chain sent to the client. Just hit this as well:

Hmmm. Really that's not the root of the issue.

We had this particular issue with:

Some very old dependencies registers repositories that are not really useful and are not actually used anymore.

From my experiments, in a normal Maven run, these repositories are not even contacted. You have nothing in your logs about them, even if you drop your .m2/repository.

To fix the issue, OpenRewrite would need to resolve the repositories more like Maven does (easier said than done, I agree).

michael-o commented 1 month ago

Total crap, Oracle is too stupid to configure the certificate chain sent to the client. Just hit this as well:

Hmmm. Really that's not the root of the issue.

We had this particular issue with:

* repositories that don't exist anymore

* repositorites that are outdated (I think that's the case with maven.java.net) so people don't update their certificates and at some point they will die

Some very old dependencies registers repositories that are not really useful and are not actually used anymore.

From my experiments, in a normal Maven run, these repositories are not even contacted. You have nothing in your logs about them, even if you drop your .m2/repository.

To fix the issue, OpenRewrite would need to resolve the repositories more like Maven does (easier said than done, I agree).

Maybe true, but I don't know what maven.java.net carries, but still there is no justification to make TLS setup right. You see how much trouble it has caused.

cstamas commented 1 month ago

Would like to draw attention to following things that were added to Maven not so long time ago:

Basically, in ideal case:

In Maven, that repo is not even touched, as can be seen:

[cstamas@urnebes openrewrite]$ mvn eu.maveniverse.maven.plugins:toolbox:0.1.31:gav-list-repositories -Dgav=io.quarkiverse.githubapp:quarkus-github-app-testing:2.6.0 -Dmaven.repo.local=local
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------< org.apache.maven:standalone-pom >-------------------
[INFO] Building Maven Stub Project (No POM) 1
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- toolbox:0.1.31:gav-list-repositories (default-cli) @ standalone-pom ---
[INFO]  * central (https://repo.maven.apache.org/maven2/, default, releases)
[INFO]    First introduced on root
[INFO]  * jboss-public-repository-group (https://repository.jboss.org/nexus/content/groups/public, default, releases)
[INFO]    First introduced on jakarta.enterprise:jakarta.enterprise.cdi-api:jar:4.1.0
[INFO]  * sonatype-snapshots (https://s01.oss.sonatype.org/content/repositories/snapshots, default, releases+snapshots)
[INFO]    First introduced on io.smallrye.reactive:mutiny:jar:2.6.0
[INFO]  * microprofile-snapshots (https://repo.eclipse.org/content/repositories/microprofile-snapshots, default, snapshots)
[INFO]    First introduced on io.smallrye.reactive:mutiny:jar:2.6.0
[INFO]  * sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots, default, snapshots)
[INFO]    First introduced on io.netty:netty-codec:jar:4.1.108.Final
[INFO]  * vertx-snapshots-repository (https://s01.oss.sonatype.org/content/repositories/snapshots/, default, snapshots)
[INFO]    First introduced on io.vertx:vertx-core:jar:4.5.7
[INFO]  * repo.jenkins-ci.org (https://repo.jenkins-ci.org/public/, default, releases+snapshots)
[INFO]    First introduced on org.kohsuke:github-api:jar:1.321
[INFO]  * ow2-snapshot (https://repository.ow2.org/nexus/content/repositories/snapshots, default, snapshots)
[INFO]    First introduced on org.ow2.asm:asm-util:jar:9.7
[INFO]  * apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)
[INFO]    First introduced on org.apache.maven.resolver:maven-resolver-impl:jar:1.9.18
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.106 s
[INFO] Finished at: 2024-07-11T13:42:15+02:00
[INFO] ------------------------------------------------------------------------
[cstamas@urnebes openrewrite]$

Reason is simple: