search

impossibl / pgjdbc-ng

A new JDBC driver for PostgreSQL aimed at supporting the advanced features of JDBC and Postgres
https://impossibl.github.io/pgjdbc-ng
Other
596 stars 108 forks source link

Version bumps to fix build and resolve CVE-2021-37136 and CVE-2021-37137 #578

Closed davidwheeler123 closed 1 month ago

davidwheeler123 commented 1 year ago

Bump some versions to get it to build and resolve CVEs CVE-2021-37136 and CVE-2021-37137

urieli commented 1 year ago

@davidwheeler123 I was going to make this pull request when I saw you had done it already. The CI logs are no longer available. Have you any idea why CI was failing?

davidwheeler123 commented 1 year ago

It looked completely unrelated to the change, that's all I remember

davidwheeler123 commented 1 year ago

I get the impression this project has become inactive 😟

kdubb commented 8 months ago

@davidwheeler123 Please push a new (or empty) commit to restart actions.

davidwheeler123 commented 8 months ago 
java.lang.AssertionError: Unexpected Exception Message: Connection Error: SSL Error: Received fatal alert: certificate_expired
expected: Connection Error: SSL Error: Hostname .* could not be verified
from
com.impossibl.postgres.jdbc.PGSQLSimpleException: Connection Error: SSL Error: Received fatal alert: certificate_expired

@kdubb Any direction on how to resolve this?

kdubb commented 8 months ago

@davidwheeler123 Glad to see you got this working! I had it open and was starting to look at it when I got pulled away

davidwheeler123 commented 8 months ago

@kdubb Not sure it's working yet - I just (force) pushed some changes that reduced the number of failures on my local - it seemed to fix all the SSL test failures, but I'm still getting 3 failures

image

The timezone ones are probably because I'm in a different tz to you (Australia), but I can't explain the Float test 🤔

davidwheeler123 commented 8 months ago

Oooh looks like it worked on Github!

@kdubb I've disabled the Java 8 builds because there was some dependency not available in java 8 class format, but github still seems to be waiting for them. Is there some github setting you need to update to correspond? Would you like me to enable another java version also?

kdubb commented 8 months ago

I think the Java 8 thing was what I was seeing. Can you post an error message from the Java 8 build?

davidwheeler123 commented 8 months ago

I think the Java 8 thing was what I was seeing. Can you post an error message from the Java 8 build?

You can see it here: https://github.com/impossibl/pgjdbc-ng/actions/runs/6758368037/job/18369978501

Error: A problem occurred configuring project ':documentation'.
> Could not resolve all artifacts for configuration ':documentation:classpath'.
   > Could not resolve org.ajoberstar.grgit:grgit-core:latest.release.
     Required by:
         project :documentation > org.ajoberstar.git-publish:org.ajoberstar.git-publish.gradle.plugin:3.0.0 > org.ajoberstar:gradle-git-publish:3.0.0
      > No matching variant of org.ajoberstar.grgit:grgit-core:5.2.1 was found. The consumer was configured to find a runtime of a library compatible with Java 8, packaged as a jar, and its dependencies declared externally, as well as attribute 'org.gradle.plugin.api-version' with value '7.0.2' but:
          - Variant 'apiElements' capability org.ajoberstar.grgit:grgit-core:5.2.1 declares a library, packaged as a jar, and its dependencies declared externally:
              - Incompatible because this component declares an API of a component compatible with Java 11 and the consumer needed a runtime of a component compatible with Java 8
              - Other compatible attribute:
                  - Doesn't say anything about org.gradle.plugin.api-version (required '7.0.2')
          - Variant 'javadocElements' capability org.ajoberstar.grgit:grgit-core:5.2.1 declares a runtime of a component, and its dependencies declared externally:
              - Incompatible because this component declares documentation and the consumer needed a library
              - Other compatible attributes:
                  - Doesn't say anything about its target Java version (required compatibility with Java 8)
                  - Doesn't say anything about its elements (required them packaged as a jar)
                  - Doesn't say anything about org.gradle.plugin.api-version (required '7.0.2')
          - Variant 'runtimeElements' capability org.ajoberstar.grgit:grgit-core:5.2.1 declares a runtime of a library, packaged as a jar, and its dependencies declared externally:
              - Incompatible because this component declares a component compatible with Java 11 and the consumer needed a component compatible with Java 8
              - Other compatible attribute:
                  - Doesn't say anything about org.gradle.plugin.api-version (required '7.0.2')
          - Variant 'sourcesElements' capability org.ajoberstar.grgit:grgit-core:5.2.1 declares a runtime of a component, and its dependencies declared externally:
              - Incompatible because this component declares documentation and the consumer needed a library
              - Other compatible attributes:
                  - Doesn't say anything about its target Java version (required compatibility with Java 8)
                  - Doesn't say anything about its elements (required them packaged as a jar)
                  - Doesn't say anything about org.gradle.plugin.api-version (required '7.0.2')
kdubb commented 8 months ago

Yeah that looks familiar...

Not sure how dependencies are disappearing from Central but it's a plugin for documentation generation (which shouldn't care about what Java version is used to build it). We could increase the Java version to 11 for that module to get it working again.

kdubb commented 8 months ago

I'm all for dropping Version 8. I had planned to move the minimum to 11 quite a long time ago but it should be done in a planned way and a major release.

davidwheeler123 commented 8 months ago

We could increase the Java version to 11 for that module to get it working again.

That's a good idea. We still wouldn't be able to run that build in a java 8 JDK though, even if we produce java 8 compatible results

kdubb commented 8 months ago

Nevermind that wouldn't work the actual java-version is set in the action workflow. I thought it was being selected via Gradle toolchains.

kdubb commented 8 months ago

You could try using toolchains in the Gradle build to select the JDK (it should download the required JDK if setup correctly). This might slow down the build a bit but that can be mititagated by using the Gradle Build Action from Gradle which enables caching fairly easily.

kdubb commented 8 months ago

Then change the workflow to always use Java 17 or something.

davidwheeler123 commented 8 months ago

Nevermind that wouldn't work the actual java-version is set in the action workflow. I thought it was being selected via Gradle toolchains.

I think it's using both actually. I've just set target vers back to 1.8 and it seems to be working on my local (building with jdk 17)

kdubb commented 8 months ago

Ahh I missed your change to the minimum Java version. Yeah that needs to be Java 11 until we really remove Java 8.

davidwheeler123 commented 8 months ago

I'm gonna sign off for a while, I think this is ready for a review. LMK what you think

kdubb commented 8 months ago

Did you add Java 8 back to the build matrix?

davidwheeler123 commented 8 months ago

Nope. Do you think I should? It's still producing Java 8 compatible output, so I don't see why we need to build with Java 8

davidwheeler123 commented 7 months ago

@kdubb i have some time today. Anything else you'd like me to do on this pr?

kdubb commented 7 months ago

@davidwheeler123 Sorry, I thought I responded to this 🤦‍♂️

It may be a bit pedantic but I think we should stick to building with Java 8 until we update to a newer LTS release (at least 11, if not 17).

davidwheeler123 commented 7 months ago

I can't work out how to do it.

The issue at hand is that grgit-core (used by git-publish plugin) only has java 11+ versions available. AFAICS we can't use gradle toolchains for this, because it's not compiling, running or javadoc-ing, so there's no way to set the java version for executing the task (or for selecting the right grgit-core dependency)

davidwheeler123 commented 7 months ago

I suppose one way to do it would be to have separate github jobs to publish the maven artifacts vs the documentation. Does that sound ok to you?

davidwheeler123 commented 7 months ago

@kdubb Build is now passing, but it's not super tidy because I actually have to exclude the documentation module when using java 8 so it can configure properly. LMK if you have a better idea of how to handle that. We can undo this later once we retire java8 support.

davidwheeler123 commented 7 months ago

BTW I tried running the tests against postgres 14-16, but the tests are failing. I'll tackle that as a separate PR once this one gets merged

davecramer commented 7 months ago

you should be able to build on java 17 and release on java 8 if that helps

davidwheeler123 commented 6 months ago

you should be able to build on java 17 and release on java 8 if that helps

Yeah I know, that's how I had it but @kdubb preferred to build with Java 8.

Any thoughts on how it looks now @kdubb ?

vlsi commented 6 months ago

It may be a bit pedantic but I think we should stick to building with Java 8 until we update to a newer LTS release (at least 11, if not 17).

@kdubb , javac 8 has known issues and it might easily produce ill bytecode. Here's a recent case: https://lists.apache.org/thread/o736wz4qnr4l285bj5gv073cy0qll9t0

I would suggest using Java 17 as Gradle toolchain and use release=1.8 (see https://github.com/pgjdbc/pgjdbc/pull/3026/files#diff-f39b6bdb833169aaabf15859994fe2128c02890ec30d7a3b73448312e1eb9b24R57 ) + Kotlin's -Xjdk-release=... (see https://github.com/pgjdbc/pgjdbc/pull/3026/files#diff-0e7e7fe3fcd04387e25c869097729ddf9b007102df318012a16e4e44947dfdd9R29 ) to ensure they produce workable Java 8 bytecode.

Here's a sample: https://github.com/pgjdbc/pgjdbc/pull/3026

Of course, it might require slightly more changes, however, I am sure the best approach is to use Java 17 toolchain for build (both running Gradle and use javac 17), and then target whatever you need (e.g. 8).

kdubb commented 5 months ago

@davidwheeler123 My apologies for my absence. I'm fine with checking this in as is but now that @davecramer and @vlsi have weighed in it seems your original idea might be the correct course.

It's up to you at this point. Let me know what you want to do.. revert to your original plan or check it in as is.

davidwheeler123 commented 5 months ago

I'll revert back to how it was I think, it's a bit simpler to maintain

davidwheeler123 commented 4 months ago

@kdubb I've reverted to how it was before: building with JDK 11 with target set to java 8. I can see the resulting jar has produced files compatible with java 8

➜  driver git:(develop) ✗ file build/classes/java/main/com/impossibl/postgres/api/data/Tid.class
build/classes/java/main/com/impossibl/postgres/api/data/Tid.class: compiled Java class data, version 52.0 (Java 1.8)

Happy with that?

Apologies for taking so long with this, it's been a busy few months.

davidwheeler123 commented 4 months ago

Oh I think there must be some github settings to say wait for the JDK 8 builds to pass before allowing merge. Are you able to correct that?

davidwheeler123 commented 3 months ago

@kdubb Just checking you saw that this PR is ready.

kdubb commented 1 month ago

@davidwheeler123 I removed the 8 build requirements so this is passing now.

kdubb commented 1 month ago

@davidwheeler123 https://github.com/impossibl/pgjdbc-ng/actions/runs/9310946886 The test certificate that passed for this PR appears to be out of date now.

davidwheeler123 commented 1 month ago

Oh that's weird I thought I made 10yr certs. I'll have a look when I can

kdubb commented 1 month ago

Actually it can't load it but the error is

"/var/lib/postgresql/server.crt": ee key too small

davidwheeler123 commented 1 month ago

Ahh, I reckon we might need to regenerate the certs with rsa:2048 rather than rsa:1024 https://github.com/impossibl/pgjdbc-ng/blob/1ef3e9c7ec4bd8ec831fc63c65e07e707216ca7c/driver/src/test/resources/certdir/README#L24-L38 https://stackoverflow.com/a/67754773

crowmagnumb commented 1 month ago

Can we bump the netty to 4.1.109.Final which is now the latest and what all my other 3rd-party dependencies are looking for?

kdubb commented 1 month ago

@crowmagnumb I don't see why not.

kdubb commented 1 month ago

Need a PR to clear up this cert issue first