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 |
sehrope
Jun 13, 2017
Contributor
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.
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.
ahachete
Jun 13, 2017
•
Author
Contributor
This is exactly what assertions are made for, so 100% agreed with an assertion. For development, developers should run with -ea ;)
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)) { |
sehrope
Jun 13, 2017
Contributor
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.
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.
ahachete
Jun 13, 2017
Author
Contributor
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 :)
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}", |
vlsi
Jun 13, 2017
Member
Are those logging changes just formatting? Would you please refrain from introducing a feature and altering formats at the same time?
Are those logging changes just formatting? Would you please refrain from introducing a feature and altering formats at the same time?
ahachete
Jun 13, 2017
Author
Contributor
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
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
vlsi
Jun 13, 2017
Member
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.
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.
ahachete
Jun 13, 2017
Author
Contributor
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 ^_^
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 ^_^
vlsi
Jun 13, 2017
Member
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
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
ahachete
Jun 13, 2017
Author
Contributor
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) :)
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) :)
vlsi
Jun 13, 2017
Member
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.
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.
ahachete
Jun 13, 2017
Author
Contributor
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.
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.
ahachete
Jun 14, 2017
Author
Contributor
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.
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"); |
vlsi
Jun 13, 2017
Member
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.
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.
ahachete
Jun 14, 2017
Author
Contributor
Agreed.
Agreed.
private ScramSession.ClientFinalProcessor clientFinalProcessor; | ||
|
||
private static void log(Level level, String message, Object... args) { | ||
if (LOGGER.isLoggable(level)) { |
vlsi
Jun 13, 2017
Member
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
.
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
.
ahachete
Jun 14, 2017
Author
Contributor
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?
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), |
vlsi
Jun 13, 2017
Member
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.
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.
ahachete
Jun 14, 2017
Author
Contributor
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...
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; |
vlsi
Jun 13, 2017
Member
Isn't Consumer<PGStream>
just enough?
Isn't Consumer<PGStream>
just enough?
ahachete
Jun 14, 2017
Author
Contributor
Unfortunately not, because of the IOException thrown
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)); |
vlsi
Jun 13, 2017
Member
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)
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)
ahachete
Jun 14, 2017
Author
Contributor
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
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() |
vlsi
Jun 13, 2017
Member
byte
vs char
length issue possible.
byte
vs char
length issue possible.
ahachete
Jun 14, 2017
Author
Contributor
Done
Done
try { | ||
clientFinalProcessor.receiveServerFinalMessage(serverFinalMessage); | ||
} catch (ScramParseException e) { | ||
throwConnectionRejectedException("Invalid server-final-message: {0}", serverFinalMessage); |
vlsi
Jun 13, 2017
Member
Should the original exception be tied to the thrown one as a cause
? The same for two other catch
blocks below.
Should the original exception be tied to the thrown one as a cause
? The same for two other catch
blocks below.
ahachete
Jun 14, 2017
Author
Contributor
Agreed.
Agreed.
break; | ||
|
||
case AUTH_REQ_SASL_CONTINUE: | ||
scramAuthenticator.processServerFirstMessage(l_msgLen - 4 - 4); |
vlsi
Jun 13, 2017
Member
What -4-4
means here?
What -4-4
means here?
ahachete
Jun 14, 2017
Author
Contributor
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....
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); |
vlsi
Jun 13, 2017
Member
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.
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.
ahachete
Jun 14, 2017
Author
Contributor
Done
Done
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 |
Rebased all 4 patches to current master |
<dependency> | ||
<groupId>com.ongres.scram</groupId> | ||
<artifactId>client</artifactId> | ||
<version>1.0.0-beta.2</version> |
jorsol
Oct 12, 2017
Member
Will this be a beta
dependency?
Will this be a beta
dependency?
vlsi
Oct 12, 2017
Member
That bothers me a bit as well.
That bothers me a bit as well.
ahachete
Oct 12, 2017
Author
Contributor
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.
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.
jorsol
Oct 12, 2017
Member
@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.
@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
|
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>
.
|
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
@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
|
* Add SCRAM-SHA-256 support 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. Particularly, 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 * Prepare Java8-only code to be excluded for jre<=8 Actually implemented via macros by JDBC < 4.2. * On HEAD all testing connections will use SCRAM * Better error message for drivers w/o SCRAM support * added configuration for shaded jar remove any extraneous classes picked up in the shade plugin
* Add SCRAM-SHA-256 support 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. Particularly, 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 * Prepare Java8-only code to be excluded for jre<=8 Actually implemented via macros by JDBC < 4.2. * On HEAD all testing connections will use SCRAM * Better error message for drivers w/o SCRAM support * added configuration for shaded jar remove any extraneous classes picked up in the shade plugin
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