-
Notifications
You must be signed in to change notification settings - Fork 828
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
fix: Use SASLprep normalization for SCRAM authentication #2052
Conversation
d45419c
to
024ad74
Compare
looks good to me |
@jorsol , could you please update https://github.com/pgjdbc/pgjdbc/blob/master/CHANGELOG.md to reflect the change? I wonder if this change can break something. It if can break things, then we probably need to make the preparation configurable. |
024ad74
to
6e7425d
Compare
This will not break anything:
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... |
6e7425d
to
452bcc2
Compare
The test looks great, thanks.
AFAIK Travis is not enabled in pgjdbc :-/ |
c655738
to
bf29ef0
Compare
Using NO_PREPARATION the handling of spaces was incorrect, the driver should use SASL_PREPARATION. Signed-off-by: Jorge Solórzano <jorsol@gmail.com>
bf29ef0
to
f3994ab
Compare
Well, it looks like it's not enforced but it still runs: https://travis-ci.com/github/pgjdbc/pgjdbc 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. |
@odubaj , this change reveals that Fedora includes an outdated Failure log: https://travis-ci.com/github/pgjdbc/pgjdbc/jobs/481929347#L857-L861
|
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! |
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. |
@odubaj can the stringprep/saslprep dependency be also packaged in fedora-rawhide? |
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. |
One of the key policies for Fedora is |
Makes sense! :) |
@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 ? |
@odubaj I'm not familiar with the packaging on fedora of java dependencies but it should be roughly like this
And the end result will be:
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):
But again, I'm not familiar with Fedora packaging and the potential issues that this approach might have. |
@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. |
Thanks @odubaj, yes, stringprep doesn't require any other dependency, so it should be enough to have pgjdbc working in fedora. |
@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 ? |
@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 , do you need it sooner or later? I guess we need to release 42.2.19 this week. |
@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. |
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. |
Using NO_PREPARATION the handling of spaces was incorrect, the driver should use SASL_PREPARATION.
Fixes: #1970
Fixes: #2000