Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add PasswordUtil for encrypting passwords client side #3082
Add PasswordUtil for encrypting passwords client side #3082
Changes from all commits
330ed0e
219f1ed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding empty comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why show up with feedback on lines that have not changed since the PR was first opened after the PR is merged?
Are you just looking for something to nitpick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not fix javadocs (see #3082 (comment)), and I thought it would be easier and faster to just wait for you to merge the PR and then fix the style issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the method is not reliable from the users' perspective.
What does
driver's default
mean?What if somebody uses the method, and then they upgrade the driver some time later. Is the driver allowed to change the default encoding method?
Apparently, for backward compatibility, we can't change the method. In that regard,
encodePassword
duplicatesencodeScramSha256
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means the driver encodes the password with whatever the latest version of password encoding the driver wants, using the defaults built into the driver.
Yes. That's exactly the point. So that code targeting that method uses the latest, most recommended method of encoding passwords without being connected to a specific server.
Compatibility with what? We haven't haven't released anything yet. Are you suggesting changing the signature?
It doesn't duplicate the SCRAM-SHA-256 function, it it delegates to it because that's the current driver default.
If in the future if the SCRAM-SHA-256 default is replaced with SCRAM-SHA-512 or something else entirely, we'd change that delegation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever make such a change, then we effectively break backward compatibility. That means we can't easily make such a change.
So, please suggest what is the use case for having "driver's default"
encodePassword
method. Why add the method assuming there's not a single use case for it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't break anything because the definition of that method is encoding the password with whatever the driver considers to be the most secure and recommended approach. The user is delegating to this driver, as the de facto Java driver for PosgreSQL, to make a determination of how the user should be encoding passwords.
If a user wants to use a specific algo or parameters then there's other overloads to use instead.
It's in the original PR description:
A user can leverage that to generate their own SQL that involves encoding passwords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is that if the server uses SCRAM-512 for the latest version and previous versions use SCRAM-256 using the latest driver would fail on older versions of the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean if the driver is bumped to SCRAM-512 and the server does not yet support it, then it would fail.
Yes, that's expected because the output of that method is not for a particular server. It's for generating literals for the encoded password using the latest recommended method per the driver. I'd see it being used by something that is generating it's own SQL, potentially for future execution out of band. The tie in to the driver is that the driver, as the de factor Java driver for PostgreSQL, is aware of the recommended password encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, what I mean is server version 17 comes out with SCRAM-512. The driver uses SCRAM-512 as the
default
and now the default only works for server version 17.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow ... isn't that the same situation I described in my previous comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is, but we can't break backward compatibility. If a user upgrades the driver their code should continue to work.
Generally if we do a major version upgrade we would mention that we have breaking changes but a breaking change to a
default
seems wrong somehowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't see it as a break as the functional meaning of the method ("encode my password using the recommended...) has not changed. I see it like the following changing over server versions:
It'd be md5 encoded in <=10 and some variant SCRAM after that. Though that's not as user facing so maybe not the best example.
The original goal was to have a method that users could rely on when making things like SQL script generating tooling that they know will always be the latest recommendation. That way when SCRAM-SHA-256 is replaced with SCRAM-SHA-512 or something else entirely, a user that bumps their driver to the latest pgjdbc would automatically get the newer recommendation.
If that doesn't make sense as valid use case or if you foresee misuse of it causing complication, then let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coulo you add the meaning of
null
to the docs?