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

Update SCRAM dependency to 3.0 and support channel binding #3188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorsol
Copy link
Member

@jorsol jorsol commented Apr 3, 2024

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does ./gradlew styleCheck pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

@Neustradamus
Copy link

@jorsol: Good job!

Linked to:

@jorsol jorsol force-pushed the scram-3.0 branch 2 times, most recently from 2f5845b to f980fc1 Compare April 4, 2024 16:11
@jorsol jorsol marked this pull request as ready for review April 4, 2024 16:41
@jorsol
Copy link
Member Author

jorsol commented Apr 4, 2024

@davecramer this is passing tests, obviously the RPM build will fail until the scram 3.0 and stringprep 2.1 libraries are packaged for which I don't know who's in charge of that.

I see that the CI tests scram with and without SSL which should cover the channel binding, maybe a specific test is needed but I don't really have more time for this.

Also lacks documentation, maybe can be added later, in general "it should just work".

Not sure how this behave with pgbouncer or the like and there's no kill switch right now.

Another point to consider is the version to use, this could fit a minor version like 42.7.4 but at the same time I see more appropriate something like 42.8.0.

Please review and test when you can.

Comment on lines +191 to 196
overrideLicense("com.ongres.scram:scram-common") {
licenseFiles = "scram"
}
overrideLicense("com.ongres.scram:client") {
overrideLicense("com.ongres.scram:scram-client") {
licenseFiles = "scram"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If scram-* bundles the license file, we could probably remove customizations here, and remove /licenses/* files.
WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it bundle the license now, not sure what exact does Gradle here, so if it's not needed to do anything else here, it could be probably removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configuration here sets to use license files from https://github.com/pgjdbc/pgjdbc/tree/master/licenses/scram for the scram library. Ideally, libraries should bundle their licenses (and the licenses of the shaded libraries as well), so in the ideal world, overrides should not be needed. However, previous scram versions missed the license files so we had to keep them in pgjdbc source tree

Copy link
Member Author

@jorsol jorsol Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch!, I just checked that the stringprep dependency does not have bundled the LICENSE file, was completely an oversight, so we might need to keep the overrides a bit longer.

I will need to add a test to ensure this never happens in the future.

pgjdbc/build.gradle.kts Outdated Show resolved Hide resolved
pgjdbc/build.gradle.kts Outdated Show resolved Hide resolved
@jorsol jorsol force-pushed the scram-3.0 branch 3 times, most recently from 287e54f to 4410c01 Compare April 8, 2024 10:23
Signed-off-by: Jorge Solórzano <jorsol@gmail.com>
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.

None yet

3 participants