Add SCRAM-SHA-256 support #842
Conversation
|
Looks like checkstyle is complaining quite a bit ;) |
|
I think you can use the jdk6,jdk7 profiles to exclude compilation of sasl/ScramAuthenticator.java then preprocessing to remove the import statements |
|
|
||
| public void processServerMechanismsAndInit() throws IOException, PSQLException { | ||
| String scramAuthenticationMechanism = pgStream.receiveString(); | ||
| pgStream.receiveChar(); // '0' terminating the String |
There was a problem hiding this comment.
This should assert that the zero byte is indeed a zero byte. At worst it's an extra "if" and, particularly in development, may catch protocol errors.
There was a problem hiding this comment.
This is exactly what assertions are made for, so 100% agreed with an assertion. For development, developers should run with -ea ;)
| public void processServerMechanismsAndInit() throws IOException, PSQLException { | ||
| String scramAuthenticationMechanism = pgStream.receiveString(); | ||
| pgStream.receiveChar(); // '0' terminating the String | ||
| if (!EXPECTED_SERVER_ADVERTISED_METHODS.equals(scramAuthenticationMechanism)) { |
There was a problem hiding this comment.
Currently the server only supports a single SCRAM auth mechanism but the protocol spec supports a list of values. This code will break if more than one auth mechanism is sent as the second and onward will not be read (and thus mess up whatever is next on the wire).
We'll need to keep reading strings until you get to one that is of length zero (i.e. the \0 all by itself) to indicate the end of the list. The strings themselves should be added to a list so that the priority order can be maintained.
There was a problem hiding this comment.
Yes, you are right. I believe when I initially wrote this code, there was only one SCRAM mechanism defined, but after some discussion on -hackers it was turned into a list. Now changed to parse a list. Fortunately change is trivial, as the SCRAM library was already prepared to process a list of SCRAM mechanisms and pick "the best one" and throw IAE if bad things are passed to it :)
| @@ -133,7 +138,8 @@ public QueryExecutor openConnectionImpl(HostSpec[] hostSpecs, String user, Strin | |||
| Iterator<HostSpec> hostIter = hostChooser.iterator(); | |||
| while (hostIter.hasNext()) { | |||
| HostSpec hostSpec = hostIter.next(); | |||
| LOGGER.log(Level.FINE, "Trying to establish a protocol version 3 connection to {0}", hostSpec); | |||
| LOGGER.log(Level.FINE, "Trying to establish a protocol version 3 connection to {0}", | |||
There was a problem hiding this comment.
Are those logging changes just formatting? Would you please refrain from introducing a feature and altering formats at the same time?
There was a problem hiding this comment.
I did so because of the (strict) checkstyle requirements, which I only noticed after Travis run its job. I could resubmit the commits, squashing them, but I believe that is not what the history of the feature was, so I think it is more appropriate to leave it as is. Please let me know otherwise
There was a problem hiding this comment.
I did so because of the (strict) checkstyle requirements, which I only noticed after Travis run its job.
What I mean is: there were no checkstyle violations before, and the line LOGGER.log(Level.FINE, "Trying to establish a protocol version 3 connection to {0}", hostSpec); was perfectly fine. I don't follow you on "noticed after Travis run its job".
Here's the log for your first commit: https://travis-ci.org/pgjdbc/pgjdbc/jobs/241824426
There are just "wrong import order", "unused import order", and "whitespace around -" warnings regarding ConnectionFactoryImpl.java. It did not warn on "wrong formatting of LOGGER.log calls".
So this particular change (and other changes to LOGGER.log kind of lines) seem to be unrelated to the PR itself.
PS. I do agree checkstyle warnings are not that human-friendly, however that helps us to keep codebase sane and uniform.
There was a problem hiding this comment.
I'm not questioning checkstyle, I agree it's necessary to keep code sanity. I don't like current checkstyle, that's it, and its strictness (order of imports and lines separating imports, which cost me like 10 mins of trial and error since it was the first case with an external dependency import. was not productive work that helped code quality IMHO).
Having said that, all I did when I saw the checkstyle errors was let the IDE handle them. It might have been stricter than necessary doing it, but honestly, I don't want to invest more time into this. Current version is checkstyle compliant and, I would rather prefer to devote my effort towards more value in the code (like going through the TODOs of the SCRAM library, which still needs a lot of love). If anyone fixes this with another commit or I should squash the commits, I'd be happy to do so ;) but otherwise I would not use a lot of time on this ^_^
There was a problem hiding this comment.
order of imports and lines separating imports, which cost me like 10 mins of trial and error since it was the first case with an external dependency import. was not productive work that helped code quality IMHO
There are ready to go IDEA/Eclipse configurations: https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#support-for-ides
There was a problem hiding this comment.
I did that, but still required manual fixing (?). Still the point is the same: checkstyle is necessary, but order of imports and while lines in between them is too much IMHO (I know it's the default, but that doesn't mean I have to agree with it anyway) :)
There was a problem hiding this comment.
I did that, but still required manual fixing (?)
Well, I fix 99% formatting issues by "autoformat code" shortcut. Imports included. IDEA arranges those automatically just fine.
PS. The strict order of imports reduces the number of potential merge-conflicts.
There was a problem hiding this comment.
Yepp, that I did. Intellij + autoformat code. And checkstyle was still crying out loud. I guess it doesn't understand the difference between "external imports" and "special imports". I didn't understand it either, so I did trial and error after running autoformat. That autoformat caused all the changes to formatting that you were referring to before.
There was a problem hiding this comment.
That autoformat caused all the changes to formatting that you were referring to before.
Well, you could confine autoformat to the selection.
Here's what I have:
There was a problem hiding this comment.
OK, I saw now what you meant. I left the original code as it was before the commit and made sure formatting only affects the new code. Now there's no other option but to squash my commits and submit them as a single commit with all changes so far. I will follow-up with that soon.
| @@ -604,6 +625,24 @@ private void doAuthentication(PGStream pgStream, String host, String user, Prope | |||
| sspiClient.continueSSPI(l_msgLen - 8); | |||
| break; | |||
|
|
|||
| case AUTH_REQ_SASL: | |||
| if (LOGGER.isLoggable(Level.FINEST)) { | |||
| LOGGER.log(Level.FINEST, " <=BE AuthenticationSASL"); | |||
There was a problem hiding this comment.
There's no need to verify isLoggable when arguments to LOGGER.log are trivial. In this case, message is just passed as is, and no arrays created, so isLoggable should be just dropped.
| private ScramSession.ClientFinalProcessor clientFinalProcessor; | ||
|
|
||
| private static void log(Level level, String message, Object... args) { | ||
| if (LOGGER.isLoggable(level)) { |
There was a problem hiding this comment.
LOGGER.isLoggable makes no sense here, and I'm not sure the method is justified. Well, it does shorten LOGGER.log( to just log(, however I would just call LOGGER field as log.
There was a problem hiding this comment.
It makes the code much more readable, because the if and then the very long lines end up requiring 4-5 lines to just log something. With the method, it is much more concise. And this is a method that should be inlined so why worry?
| private static void throwConnectionRejectedException(String message, Object... args) | ||
| throws PSQLException { | ||
| throw new PSQLException( | ||
| GT.tr(message, args), |
There was a problem hiding this comment.
I think this defeats the whole purpose of "message localization". As far as I know, gettext looks for GT.tr tokens and creates boilerplate.properties files with a set of messages to be translated.
It look like this usecase would defeat gettext, and automatic discovery of strings would not work.
Please inline the method to the call sites.
There was a problem hiding this comment.
Hmmm I don't know how that gettext automatic discovery works, but inline as suggested if that would break it. Still puzzled about this, code now is clunkier, but so be it...
|
|
||
| @FunctionalInterface | ||
| private interface BodySender { | ||
| void sendBody(PGStream pgStream) throws IOException; |
There was a problem hiding this comment.
Isn't Consumer<PGStream> just enough?
There was a problem hiding this comment.
Unfortunately not, because of the IOException thrown
| s.send(scramMechanismName.getBytes(StandardCharsets.UTF_8)); | ||
| s.sendChar(0); // List terminated in '\0' | ||
| s.sendInteger4(clientFirstMessage.length()); | ||
| s.send(clientFirstMessage.getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
This seems to assume getBytes(StandardCharsets.UTF_8) == clientFirstMessage.length().
Is it always the case? If so, please clarify (i.e. in comments) why.
Otherwise I'm inclined to use byte[].length instead of String.length here (and below)
There was a problem hiding this comment.
Good catch. Almost all the message parameters are base-64 encoded, and separators are ASCII, but at least the username, after string prep, may be >1byte/char. Changed to byte[].length
|
|
||
| authenticationMessageSender( | ||
| s -> s.send(clientFinalMessage.getBytes(StandardCharsets.UTF_8)), | ||
| clientFinalMessage.length() |
There was a problem hiding this comment.
byte vs char length issue possible.
| try { | ||
| clientFinalProcessor.receiveServerFinalMessage(serverFinalMessage); | ||
| } catch (ScramParseException e) { | ||
| throwConnectionRejectedException("Invalid server-final-message: {0}", serverFinalMessage); |
There was a problem hiding this comment.
Should the original exception be tied to the thrown one as a cause? The same for two other catch blocks below.
| break; | ||
|
|
||
| case AUTH_REQ_SASL_CONTINUE: | ||
| scramAuthenticator.processServerFirstMessage(l_msgLen - 4 - 4); |
There was a problem hiding this comment.
Check the message format. It is two ints. I hate this format, but the code is spread with this kind of calculations all the time. I wouldn't do it like this, but I don't want to refactor all this code here, so I just followed the pattern....
| try { | ||
| serverFirstProcessor = scramSession.receiveServerFirstMessage(serverFirstMessage); | ||
| } catch (ScramException e) { | ||
| throwConnectionRejectedException("Invalid server-first-message: {0}", serverFirstMessage); |
There was a problem hiding this comment.
Please propagate the source exception as a cause.
It would be extremely hard to debug the code in case there's ScramException for some reason.
|
BTW, comments here are invaluable. However, don't forget this is using an external dependency, https://github.com/ongres/scram/, which took more than 95% of the work to support this functionality. Even if it is still a WIP, if comments or PRs could be done there, I would greatly appreciate it. |
|
Sure will do. Was already planning on a deeper dive into the SCRAM lib itself. Figured I'd start off with the pgjdbc piece to get a feel for the API prior to getting into the SCRAM details. |
|
Submitted a refactored new commit, with all the comments above (except java6/7 compilation, that's TODO, commits/diffs welcome) fixed or commented. Please review. Thanks. |
|
Regarding "include the feature in JRE8 build only", the current pattern for For instance pgjdbc-jre7 excludes execution of However, we don't have such excludes for the
If that is the case, scram client helper classes should land to something like @davecramer , @sehrope what do you think? |
Codecov Report
@@ Coverage Diff @@
## master #842 +/- ##
============================================
- Coverage 65.96% 65.59% -0.37%
Complexity 3589 3589
============================================
Files 167 168 +1
Lines 15333 15419 +86
Branches 2484 2487 +3
============================================
Hits 10114 10114
- Misses 4042 4128 +86
Partials 1177 1177 |
|
Update: the PR is not yet ready for merge. Need to figure out the way dependency should be managed (optional dependency or shaded into the pgjdbc) |
|
@ahachete , would you please add |
75d33ec
to
4387eda
Compare
|
Rebased all 4 patches to current master |
4387eda
to
ad91518
Compare
| <dependency> | ||
| <groupId>com.ongres.scram</groupId> | ||
| <artifactId>client</artifactId> | ||
| <version>1.0.0-beta.2</version> |
There was a problem hiding this comment.
I was planning on bumping it to 1.0.0 when it is well tested and ready to be integrated. So if you feel this is OK, I will proceed and release 1.0.0 and update the PR.
There was a problem hiding this comment.
@ahachete well, it will never be tested if it's not included in pgjdbc, so is a chicken-egg problem, it's your code, so you better know if it is well tested.
I'm Ok if it's released as a Beta feature, mark it as such in release notes and if everything goes fine, bumping it to 1.0.0 and update it for the next release cycle.
|
+1 for keeping the same jar name |
|
OK, let me work on that bit.
I'm OK with it as nobody has really tested this. We may have to be the
first.
Dave Cramer
…On 12 October 2017 at 11:58, Vladimir Sitnikov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pgjdbc/pom.xml
<#842 (comment)>:
> @@ -36,6 +36,14 @@
<checkstyle.version>7.8.2</checkstyle.version>
</properties>
+ <dependencies>
+ <dependency>
+ <groupId>com.ongres.scram</groupId>
+ <artifactId>client</artifactId>
+ <version>1.0.0-beta.2</version>
That bothers me a bit as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#842 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9k8Bp-TPE7JCZhLJvIW7Tk4WL7YXks5srjckgaJpZM4N2hD5>
.
|
|
On 12 October 2017 at 15:05, Álvaro Hernández Tortosa < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pgjdbc/pom.xml
<#842 (comment)>:
> @@ -36,6 +36,14 @@
<checkstyle.version>7.8.2</checkstyle.version>
</properties>
+ <dependencies>
+ <dependency>
+ <groupId>com.ongres.scram</groupId>
+ <artifactId>client</artifactId>
+ <version>1.0.0-beta.2</version>
I was planning on bumping it to 1.0.0 when it is well tested and ready to
be integrated. So if you feel this is OK, I will proceed and release 1.0.0
and update the PR.
My suspicion is the only way to test it well is to put it in the JDBC jar
;) So I would say just release it.
Dave Cramer
|
544996a
to
1c4de0a
Compare
|
For the last three runs of Travis, due to this PR rebase or pushing new commits, like Dave Cramer's commit to shade the JAR, Travis is failing one test. But one different each time. And tests fail on non pgjdbc related stuff. You can see latest one, it falied when test when doing apt-get to install the oracle VM. In other words: the above message is wrong, patches are working but Travis is randomly and even I'd say consistently failing. Apart from having to "ignore" it, any suggestions are welcome.... or am I missing something here? Also @vlsi let me know if your requested changes are already implemented (should be) to close this request. Thanks. |
|
jdk9 is not being installed correctly. We have to wait for that package to
get fixed upstream.
Dave Cramer
…On 19 October 2017 at 09:57, Álvaro Hernández Tortosa < ***@***.***> wrote:
For the last three runs of Travis, due to this PR rebase or pushing new
commits, like Dave Cramer's commit to shade the JAR, Travis is failing one
test. But one different each time. And tests fail on non pgjdbc related
stuff. You can see latest one, it falied when test when doing apt-get to
install the oracle VM.
In other words: the above message is wrong, patches are working but Travis
is randomly and even I'd say consistently failing. Apart from having to
"ignore" it, any suggestions are welcome.... or am I missing something here?
Also @vlsi <https://github.com/vlsi> let me know if your requested
changes are already implemented (should be) to close this request. Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#842 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9g4aFVRBFZBH80ldWNwcEN4KZbYaks5st1VEgaJpZM4N2hD5>
.
|
|
Good to know @davecramer But on previous runs I saw one other test failing (one of the 9.4 variants IIRC). And previously, another one. In other words: Travis seems lately a bit unreliable.... |
Does that imply you know a reliable CI system? |
|
No, I mostly use Travis everywhere. I just wanted to raise the fact, as the checks seem to be failing, but they "are not". Please review if you can lift your requested changes, other than this I believe PR is ready to be merged... |
|
If you can review it @davecramer ... thanks |
|
unzip -l postgresql-42.1.5-SNAPSHOT.jar The purpose of shading is to alter the package name ( Would you please fix that? |
|
will do
Dave Cramer
…On 19 October 2017 at 10:22, Vladimir Sitnikov ***@***.***> wrote:
unzip -l postgresql-42.1.5-SNAPSHOT.jar
1626 10-19-2017 17:21 org/postgresql/xa/PGXADataSource.class
1669 10-19-2017 17:21 org/postgresql/xa/PGXADataSourceFactory.class
853 10-19-2017 17:21 org/postgresql/xa/PGXAException.class
2514 10-19-2017 17:21 org/postgresql/xa/RecoveredXid.class
0 10-19-2017 17:21 com/
0 10-19-2017 17:21 com/ongres/
0 10-19-2017 17:21 com/ongres/scram/
0 10-19-2017 17:21 com/ongres/scram/common/
0 10-19-2017 17:21 com/ongres/scram/common/exception/
586 10-19-2017 17:21 com/ongres/scram/common/exception/ScramException.class
1565 10-19-2017 17:21 com/ongres/scram/common/exception/
The purpose of shading is to alter the package name (com.ongres ->
org.postgresql.shaded.com.ongres) to avoid conflicts.
Would you please fix that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#842 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9skTVIl8NSm35CZsyL0pYKYaQh_wks5st1s8gaJpZM4N2hD5>
.
|
1c4de0a
to
48f8ed0
Compare
PostgreSQL 10 comes with SCRAM-SHA-256 support. This commit introduces support for it. Work is based on an external dependency, the SCRAM client library: https://github.com/ongres/scram/ which is now imported as a dependency. TODO: - SCRAM client library will be improved, on its own. Particularily, StringPrep support needs to be added. Final version of pgjdbc will depend on v1.0 - Testing - Probably macros to avoid this Java8-only code propagating to Java < 8 versions of the driver
Actually implemented via macros by JDBC < 4.2.
remove any extraneous classes picked up in the shade plugin
48f8ed0
to
a32357a
Compare
|
@ahachete what about bumping your client up to 1.0.0? Do you still think it needs more testing ? If so how ? |
|
@praiskup seems this is failing the fedora build phase due to maven not finding the shade plugin in offline mode? |
|
Development builds are published in Maven Central for successful builds. |
|
The beta SCRAM package is in Rawhide, let me please fix the CI then. |
|
@wibrt there should be a snapshots here https://oss.sonatype.org/content/repositories/snapshots/org/postgresql/postgresql/ |
|
CI should be fixed again, travis is still running https://travis-ci.org/pgjdbc/pgjdbc/jobs/308971928 .. Thanks to everyone for help! |
|
So do we have to go through this again when the scram client changes a
version ?
Dave Cramer
…On 29 November 2017 at 08:45, Pavel Raiskup ***@***.***> wrote:
CI should be fixed again, travis is still running
https://travis-ci.org/pgjdbc/pgjdbc/jobs/308971928 .. Thanks to everyone
for help!
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#842 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9oIQ_mr_NZqJFnLSYLuwRGvNDA4jks5s7WAJgaJpZM4N2hD5>
.
|
|
tnx for the replies, some feedback: i did some basic tests, but get a java error using background: the issued command: (the jar file is in that location, no typo; works with older drivers, but then the auth 10 error is thrown) the error when i'll check again when travis is finished |
|
On Wednesday, November 29, 2017 2:46:54 PM CET Dave Cramer wrote:
So do we have to go through this again when the scram client changes a
version ?
In Fedora will be scram beta version, until we update it... so it really
depends on pgjdbc source code whether the beta version will be enough for
the build. And if somebody pings us, we'll update to non-beta version.
Pavel
|
|
checked: the same error with version in the above context |
|
tnx for the work, the annoying thing is this context is that scram-sha-256 is a serverside setting applied at user creation or alter time. |
|
META-INF/MANIFEST.MF contains unshaded class names |
|
@ahachete: Good job! Linked to: |
PostgreSQL 10 comes with SCRAM-SHA-256 support. This commit introduces
support for it.
Work is based on an external dependency, the SCRAM client library:
which is now imported as a dependency.
TODO:
- SCRAM client library will be improved, on its own.
Particularily, StringPrep support needs to be added. Final
version of pgjdbc will depend on v1.0
- Testing
- Probably macros to avoid this Java8-only code propagating to
Java < 8 versions of the driver