fix: Use SASLprep normalization for SCRAM authentication

[jorsol]

Using NO_PREPARATION the handling of spaces was incorrect, the driver should use SASL_PREPARATION.

Fixes: #1970
Fixes: #2000

[davecramer]

looks good to me

[vlsi]

@jorsol , could you please update https://github.com/pgjdbc/pgjdbc/blob/master/CHANGELOG.md to reflect the change?
It looks like the change should be mentioned in the "notable changes" release section.

I wonder if this change can break something. It if can break things, then we probably need to make the preparation configurable.

[jorsol]

I wonder if this change can break something. It if can break things, then we probably need to make the preparation configurable.

This will not break anything:

Note that implementations MUST either implement SASLprep or disallow use of non US-ASCII Unicode codepoints in "str".

Previously the non US-ASCII codepoints were disallowed, this actually limits the use of passwords with other characters that are non US-ASCII, and now it does a proper normalization based on the RFC (PostgreSQL also uses SASLprep).

To give some context, this fix doesn't update any dependency right now, but when version 2.1 of scram was included in the driver, the SASLprep normalization was not activated (the version 2.x was the first version to include SASLprep).

This patch is just enabling the use of SASLprep on the driver (it took me more time to write the test than to fix the issue), the test verifies the two cases reported so this should work properly, anyway, we should ask the reporters of the issues to test the -SNAPSHOT and report any drawback.

NOTE: I'm still working on the travis configuration...

[vlsi]

the test verifies the two cases reported so this should work properly

The test looks great, thanks.

NOTE: I'm still working on the travis configuration...

AFAIK Travis is not enabled in pgjdbc :-/
At some point, Travis became too slow. I'm not sure if it is better now.

[jorsol]

AFAIK Travis is not enabled in pgjdbc :-/
At some point, Travis became too slow. I'm not sure if it is better now.

Well, it looks like it's not enforced but it still runs: https://travis-ci.com/github/pgjdbc/pgjdbc
(Actually, it's enforced for pull requests to master, this is for 42.2, and for some reason, it doesn't trigger travis)

I should definitely start looking at GitHub Actions then (for later improvements).

I have tested locally and the test work as expected, the trouble right now is that CI is mostly configured with trust and could give a false sense of correctness, I plan to check GitHub Actions in the future. Other than this, it could be merged, but anyone is welcome to make a local test before.

[vlsi]

@odubaj , this change reveals that Fedora includes an outdated ongres-scram library.
Would you please bump https://src.fedoraproject.org/rpms/ongres-scram/blob/rawhide/f/ongres-scram.spec to use https://gitlab.com/ongresinc/scram v2.1?

Failure log: https://travis-ci.com/github/pgjdbc/pgjdbc/jobs/481929347#L857-L861

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /builddir/build/BUILD/postgresql-jdbc-42.3.0/src/main/java/org/postgresql/jre7/sasl/ScramAuthenticator.java:[77,48] cannot find symbol
  symbol:   variable SASL_PREPARATION
  location: class com.ongres.scram.common.stringprep.StringPreparations

[odubaj]

I am not maintainer of ongres-scram, so I do not have any knowledge about this component, but I will try to communicate it with the maintainer and see what we can do.

Thank you!

[odubaj]

This might be a problem, as ongres-scram v2.1 requires mvn(com.ongres.stringprep:saslprep), which is not provided in fedora-rawhide. I will investigate the possibilities, how this problem can be resolved.

[jorsol]

@odubaj can the stringprep/saslprep dependency be also packaged in fedora-rawhide?

[odubaj]

That's not a simple thing to do, currently we are trying to minimize java dependencies in fedora, so we have to discuss this step with other maintainers and see if it is necessary, or we can make a workaround or bundle the needed jar

[jorsol]

That's not a simple thing to do, currently we are trying to minimize java dependencies in fedora, so we have to discuss this step with other maintainers and see if it is necessary, or we can make a workaround or bundle the needed jar

Well, the previous policy was to not provide shaded/bundle dependencies, but if now you are trying to minimize java dependencies in fedora, pgjdbc already shades the scram/saslprep dependencies, I guess it now could be an option if that works for fedora.

Thanks for keeping us updated about what next steps need to be done.

[vlsi]

pgjdbc already shades the scram/saslprep dependencies

One of the key policies for Fedora is all builds must work in offline, so scram/saslprep has to be taken from somewhere :)

[jorsol]

pgjdbc already shades the scram/saslprep dependencies

One of the key policies for Fedora is all builds must work in offline, so scram/saslprep has to be taken from somewhere :)

Makes sense! :)

[odubaj]

@jorsol If I understand it correctly, there is a need to package ongres/stringprep (provides mvn(com.ongres.stringprep:saslprep)) project to be able to build ongres/scram in Fedora. Are there any other necessary dependencies, which we should explicitely take care grom ongres/* project ?

[jorsol]

@odubaj I'm not familiar with the packaging on fedora of java dependencies but it should be roughly like this

ongres-scram 2.1 (https://gitlab.com/ongresinc/scram/-/tags/2.1)
  |-- ongres-scram
       |--- Provides mvn(com.ongres.scram:common)
       |--- Requires mvn(com.ongres.stringprep:saslprep)
  |-- ongres-scram-client
       |--- Provides mvn(com.ongres.scram:client)
       |--- Requires mvn(com.ongres.scram:common)

ongres-stringprep 1.1 (https://gitlab.com/ongresinc/stringprep/-/tags/1.1)
  |-- ongres-stringprep
       |--- Provides mvn(com.ongres.stringprep:stringprep)
  |-- ongres-saslprep
       |--- Provides mvn(com.ongres.stringprep:saslprep)
       |--- Requires mvn(com.ongres.stringprep:stringprep)

And the end result will be:

mvn(org.postgresql:postgresql) (42.2.19)
  |--  Requires mvn(com.ongres.scram:client) (2.1)
    |--  Requires mvn(com.ongres.scram:common) (2.1)
      |--  Requires mvn(com.ongres.stringprep:saslprep) (1.1)
        |--  Requires mvn(com.ongres.stringprep:stringprep) (1.1)

With Maven this is pretty much common as the dependencies are pulled online by maven itself and following a modular approach, but maybe if Fedora wants to reduce java dependencies, it might be possible to provide RPMs that contains two (or more) jars (this is what makes more sense to me):

  |-- ongres-scram 2.1 (rpm) (includes both jars)
      |--  Provides mvn(com.ongres.scram:common)
      |--  Provides mvn(com.ongres.scram:client)
  |-- ongres-stringprep 1.1 (rpm) (includes both jars)
      |--  Provides mvn(com.ongres.stringprep:stringprep)
      |--  Provides mvn(com.ongres.stringprep:saslprep)

But again, I'm not familiar with Fedora packaging and the potential issues that this approach might have.

[odubaj]

@jorsol so that would mean packaging ongres-stringprep in fedora might resolve the ongres-scram dependencies. This could be an option, as ongres-stringprep does not seem to require any other unpackaged dependencies. I'll start the packaging process and see if any other problems occur and what the community will say about it. We actually need to have pgjdbc in fedora, so it is a valid requirement to package/bundle its dependencies.

[jorsol]

Thanks @odubaj, yes, stringprep doesn't require any other dependency, so it should be enough to have pgjdbc working in fedora.

[odubaj]

@jorsol Just a note, you mentioned ongres-scram v2.1 working with ongres-stringprep v1.1

Is there a reason why shouldn't we use ongres-stringprep v2.0 ?

[jorsol]

@odubaj ongres-stringprep v2.0 is a major release and has some breaking changes that are not compatible with ongres-scram 2.x

There are some planned updates to ongres-scram (that might become v3.x) which will make use of ongres-stringprep v2.x, and will be included on a future release of pgjdbc, but for the moment by packaging ongres-stringprep v1.1 and ongres-scram v2.1 should be enough for working with postgresql-jdbc v42.2.19.

[odubaj]

@jorsol thanks for info. Both packages are ready and the packaging process for ongres-stringprep is already launched, so hopefully everything will go as planned.

@vlsi do you have an estimate date for releasing pgjdbc v42.2.19 ?

[vlsi]

@odubaj , do you need it sooner or later? I guess we need to release 42.2.19 this week.

[odubaj]

@vlsi depends on how to packaging process will go, but we can package ongres-stringprep to fedora and wait with ongres-scram rebase together with pgjdbc-42.2.19, so technically it is not a problem. Need it just for informative reasons, as some of the reviewers might ask about the estimated date of completion.

[odubaj]

onsgres-scram v2.1 and ongres-stringprep v1.1 successfully released in fedora-rawhide. The plan is to have pgjdbc v42.2.19 in fedora-rawhide as well in short time.